30 Apr
2013
30 Apr
'13
1:29 p.m.
Hello,
I have a kamailio 3.3.4 server running on x86-64 Linux, and I have a
script which looks something like this:
$var(gwruri) = $rU;
if ($(var(gwruri){s.substr,0,1}) == "+") {
$var(gwruri) = $(var(gwruri){s.substr,1,0});
}
When this script is run with say $rU = "+0000009724" (real number
removed). $var(gwruri) should contain the same but without the +, this
is not the case, $var(gwruri) ends up being "0000007724".
This smells memory corruption. When run under valgrind, valgrind
generates this error when that code is run:
==1206== Source and destination overlap in strncpy(0x55e3b2a, 0x55e3b2b, 10)
==1206== at 0x4C25ACF: strncpy (mc_replace_strmem.c:339)
==1206== by 0x2A28BB92: set_var_value (pv_svar.c:122)
==1206== by 0x2A2800D1: pv_set_scriptvar (pv_core.c:1683)
==1206== by 0x45E8F8: lval_assign (lvalue.c:353)
==1206== by 0x416A78: do_action (action.c:1524)
==1206== by 0x41E465: run_actions (action.c:1644)
==1206== by 0x4177FD: do_action (action.c:1136)
==1206== by 0x41E465: run_actions (action.c:1644)
==1206== by 0x4177FD: do_action (action.c:1136)
==1206== by 0x41E465: run_actions (action.c:1644)
==1206== by 0x419C67: do_action (action.c:1140)
==1206== by 0x41E465: run_actions (action.c:1644)
==1206==
I am not very familiar with the kamailio source code, but as far as I
can tell this happens because the strncpy() in set_var_value() in
pv_svar.c copies the string directly within the same overlapping memory
area. The same memory area is used because the tr_eval_string() function
and the TR_S_SUBSTR-code in pv_trans.c reuses the input buffer and just
increments the pointer and since the source pvar is the same as the
destination pvar the strncpy() ends up copying between overlapping
memory area.
I am not sure what the best fix would be for that, but I have attached a
patch which copies the string in the TR_S_SUBST code to _tr_buffer and
returns that buffer instead like a lot of the other transformations in
that function does.
If this fix is correct some other transformation functions probably
needs to be corrected as well.
--
Martin Mikkelsen, Zisson AS
30 Apr
30 Apr
1:31 p.m.
On Tue, Apr 30, 2013 at 01:29:21PM +0200, Martin Mikkelsen wrote:
I am not sure what the best fix would be for that, but
I have attached a
patch which copies the string in the TR_S_SUBST code to _tr_buffer and
returns that buffer instead like a lot of the other transformations in
that function does.
If this fix is correct some other transformation functions probably
needs to be corrected as well.
Forgot to attach the patch. Sorry.
--
Martin Mikkelsen, Zisson AS
2:42 p.m.
New subject: [PATCH] Memory corruption using s.substr transformation
Hi,
We've seen this behaviour as well and worked around it using avp_subst
with regex, as we didn't have the time yet to investigate further.
But basically I can confirm this issue.
Andreas
On 04/30/2013 01:31 PM, Martin Mikkelsen wrote:
On Tue, Apr 30, 2013 at 01:29:21PM +0200, Martin
Mikkelsen wrote:
I am not sure what the best fix would be for
that, but I have attached a
patch which copies the string in the TR_S_SUBST code to _tr_buffer and
returns that buffer instead like a lot of the other transformations in
that function does.
If this fix is correct some other transformation functions probably
needs to be corrected as well.
Forgot to attach the patch. Sorry.
_______________________________________________
SIP Express Router (SER) and Kamailio (OpenSER) - sr-users mailing list
sr-users(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-users
5:31 p.m.
On Tue, Apr 30, 2013 at 02:42:22PM +0200, Andreas Granig wrote:
Hi,
We've seen this behaviour as well and worked around it using
avp_subst with regex, as we didn't have the time yet to investigate
further.
I was also able to work around it with:
$var(tmp) = $(var(x){s.substr,1,0});
$var(x) = $(var(tmp));
But basically I can confirm this issue.
It seems that at least the s.substr, s.select, s.strip, s.striptail,
line.at and line.sw transformations are vulnerable to this issue since
they reuse the input buffer. I think that the URI-parsing
transformations are also vulnerable since they also reuse the existing
input as far as I can see.
I can probably write a patch to change the 6 string transformations to
use _tr_buffer, but I dont know if that is the best solution. It may be
better to fix the variable assignment functions to make a copy of the
rvalue if it overlaps the lvalue before the assignment, maybe someone
who is more knowledgable with the kamailio source code can take a look
at this.
--
Martin Mikkelsen, Zisson AS
5:39 p.m.
New subject: [PATCH] Memory corruption using s.substr transformation
Hello,
On 4/30/13 5:31 PM, Martin Mikkelsen wrote:
On Tue, Apr 30, 2013 at 02:42:22PM +0200, Andreas
Granig wrote:
please do the patch to store the new value in _tr_buffer and attach it
to mailing list or bug tracker. I haven't looked at code yet, but sounds
like there is indeed an issue. I will review the patch and apply it.
Cheers,
Daniel
--
Daniel-Constantin Mierla - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Training, San Francisco, USA - June 24-27, 2013
* http://asipto.com/u/katu *
Hi,
We've seen this behaviour as well and worked around it using
avp_subst with regex, as we didn't have the time yet to investigate
further.
I was also able to work around it with:
$var(tmp) = $(var(x){s.substr,1,0});
$var(x) = $(var(tmp));
But basically I can confirm this issue.
It
seems that at least the s.substr, s.select, s.strip, s.striptail,
line.at and line.sw transformations are vulnerable to this issue since
they reuse the input buffer. I think that the URI-parsing
transformations are also vulnerable since they also reuse the existing
input as far as I can see.
I can probably write a patch to change the 6 string transformations to
use _tr_buffer, but I dont know if that is the best solution. It may be
better to fix the variable assignment functions to make a copy of the
rvalue if it overlaps the lvalue before the assignment, maybe someone
who is more knowledgable with the kamailio source code can take a look
at this.
2 May
2 May
7:57 p.m.
New subject: [PATCH] pv: make set_var_value handle overlapping memory
set_var_value() may do memory corruption when a variable is set to the
substring of the same variable, this is atleast the case with the
s.substr, s.select, s.strip and s.striptail transformation functions
which just updates the pointer to its input buffer.
As an example, this would trigger a warning in valgrind and memory
corruption because the memory areas overlap:
$var(x) = $(var(x){s.substr,1,0});
Fix this by using memove() to do the copying instead of strncpy(), also
fix the add_var() function, there is no point in using strncpy when we
allready know the length of the variable so use memcpy() instead.
---
modules/pv/pv_svar.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/modules/pv/pv_svar.c b/modules/pv/pv_svar.c
index bb76a1d..7ea01d0 100644
--- a/modules/pv/pv_svar.c
+++ b/modules/pv/pv_svar.c
@@ -66,7 +66,7 @@ script_var_t* add_var(str *name)
return 0;
}
it->name.len = name->len;
- strncpy(it->name.s, name->s, name->len);
+ memcpy(it->name.s, name->s, name->len);
it->name.s[it->name.len] = '\0';
it->next = script_vars;
@@ -119,7 +119,10 @@ script_var_t* set_var_value(script_var_t* var, int_str *value, int
flags)
}
var->v.flags |= VAR_VAL_STR;
}
- strncpy(var->v.value.s.s, value->s.s, value->s.len);
+ if (var->v.value.s.s != value->s.s)
+ {
+ memmove(var->v.value.s.s, value->s.s, value->s.len);
+ }
var->v.value.s.len = value->s.len;
var->v.value.s.s[value->s.len] = '\0';
} else {
--
1.7.1
3 May
3 May
1:07 a.m.
New subject: [PATCH] pv: make set_var_value handle overlapping memory
Hello,
this patch is fixing only for $var(...), but there are other variables
that can be in the same situation. All PV-set functions have to be
updated as well as could be the case for config interpreter and some
core functions.
Anyhow, going for an possible unoptimized operation all the time is not
recommended. It should be fixed where it creates the problem, not
restrict everything to something that avoids the problem.
Cheers,
Daniel
On 5/2/13 7:57 PM, Martin Mikkelsen wrote:
set_var_value() may do memory corruption when a
variable is set to the
substring of the same variable, this is atleast the case with the
s.substr, s.select, s.strip and s.striptail transformation functions
which just updates the pointer to its input buffer.
As an example, this would trigger a warning in valgrind and memory
corruption because the memory areas overlap:
$var(x) = $(var(x){s.substr,1,0});
Fix this by using memove() to do the copying instead of strncpy(), also
fix the add_var() function, there is no point in using strncpy when we
allready know the length of the variable so use memcpy() instead.
---
modules/pv/pv_svar.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/modules/pv/pv_svar.c b/modules/pv/pv_svar.c
index bb76a1d..7ea01d0 100644
--- a/modules/pv/pv_svar.c
+++ b/modules/pv/pv_svar.c
@@ -66,7 +66,7 @@ script_var_t* add_var(str *name)
return 0;
}
it->name.len = name->len;
- strncpy(it->name.s, name->s, name->len);
+ memcpy(it->name.s, name->s, name->len);
it->name.s[it->name.len] = '\0';
it->next = script_vars;
@@ -119,7 +119,10 @@ script_var_t* set_var_value(script_var_t* var, int_str *value, int
flags)
}
var->v.flags |= VAR_VAL_STR;
}
- strncpy(var->v.value.s.s, value->s.s, value->s.len);
+ if (var->v.value.s.s != value->s.s)
+ {
+ memmove(var->v.value.s.s, value->s.s, value->s.len);
+ }
var->v.value.s.len = value->s.len;
var->v.value.s.s[value->s.len] = '\0';
} else {
--
Daniel-Constantin Mierla - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Training, San Francisco, USA - June 24-27, 2013
* http://asipto.com/u/katu *
16 May
16 May
10:06 a.m.
New subject: [PATCH] pv: make set_var_value handle overlapping memory
Hello,
I pushed a patch for fixing this issue on master branch -- can you test
and report if works fine for you now?
Cheers,
Daniel
On 5/3/13 1:07 AM, Daniel-Constantin Mierla wrote:
Hello,
this patch is fixing only for $var(...), but there are other variables
that can be in the same situation. All PV-set functions have to be
updated as well as could be the case for config interpreter and some
core functions.
Anyhow, going for an possible unoptimized operation all the time is
not recommended. It should be fixed where it creates the problem, not
restrict everything to something that avoids the problem.
Cheers,
Daniel
On 5/2/13 7:57 PM, Martin Mikkelsen wrote:
--
Daniel-Constantin Mierla - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Training, San Francisco, USA - June 24-27, 2013
* http://asipto.com/u/katu *
set_var_value() may do memory corruption when a
variable is set to the
substring of the same variable, this is atleast the case with the
s.substr, s.select, s.strip and s.striptail transformation functions
which just updates the pointer to its input buffer.
As an example, this would trigger a warning in valgrind and memory
corruption because the memory areas overlap:
$var(x) = $(var(x){s.substr,1,0});
Fix this by using memove() to do the copying instead of strncpy(), also
fix the add_var() function, there is no point in using strncpy when we
allready know the length of the variable so use memcpy() instead.
---
modules/pv/pv_svar.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/modules/pv/pv_svar.c b/modules/pv/pv_svar.c
index bb76a1d..7ea01d0 100644
--- a/modules/pv/pv_svar.c
+++ b/modules/pv/pv_svar.c
@@ -66,7 +66,7 @@ script_var_t* add_var(str *name)
return 0;
}
it->name.len = name->len;
- strncpy(it->name.s, name->s, name->len);
+ memcpy(it->name.s, name->s, name->len);
it->name.s[it->name.len] = '\0';
it->next = script_vars;
@@ -119,7 +119,10 @@ script_var_t* set_var_value(script_var_t* var,
int_str *value, int flags)
}
var->v.flags |= VAR_VAL_STR;
}
- strncpy(var->v.value.s.s, value->s.s, value->s.len);
+ if (var->v.value.s.s != value->s.s)
+ {
+ memmove(var->v.value.s.s, value->s.s, value->s.len);
+ }
var->v.value.s.len = value->s.len;
var->v.value.s.s[value->s.len] = '\0';
} else {
11 Jun
11 Jun
11:40 p.m.
New subject: [PATCH] pv: make set_var_value handle overlapping memory
On Thu, May 16, 2013 at 10:06:00AM +0200, Daniel-Constantin Mierla wrote:
I pushed a patch for fixing this issue on master
branch -- can you
test and report if works fine for you now?
(Sorry for the late reply)
I'm still on 3.4, but i tested the fe7e4a5152674aa9c81c09dd2fc9938d9e9e762e
commit backported to 3.4 and it works fine. Thank you.
--
Martin Mikkelsen (mamikk(a)mamikk.no)
http://www.mamikk.no/
12 Jun
12 Jun
9:30 a.m.
New subject: [PATCH] pv: make set_var_value handle overlapping memory
On 6/11/13 11:40 PM, Martin Mikkelsen wrote:
On Thu, May 16, 2013 at 10:06:00AM +0200,
Daniel-Constantin Mierla wrote:
ok, thanks for reporting back.
Cheers,
Daniel
--
Daniel-Constantin Mierla - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Training, San Francisco, USA - June 24-27, 2013
* http://asipto.com/u/katu *
I pushed a patch for fixing this issue on master
branch -- can you
test and report if works fine for you now?
(Sorry for the late reply)
I'm still on 3.4, but i tested the fe7e4a5152674aa9c81c09dd2fc9938d9e9e762e
commit backported to 3.4 and it works fine. Thank you.
1 May
1 May
11:04 a.m.
New subject: [PATCH] Memory corruption using s.substr transformation
On Tuesday 30 April 2013, Martin Mikkelsen wrote:
This smells memory corruption. When run under
valgrind, valgrind
generates this error when that code is run:
==1206== Source and destination overlap in strncpy(0x55e3b2a, 0x55e3b2b,
10) ==1206== at 0x4C25ACF: strncpy (mc_replace_strmem.c:339)
strncpy() must be replaced with memmove() when src and dst (may) overlap.
--
Alex Hermann
2 May
2 May
7:55 p.m.
On Wed, May 01, 2013 at 11:04:28AM +0200, Alex Hermann wrote:
strncpy() must be replaced with memmove() when src and
dst (may) overlap.
You are right, it is probably better to do the copying with memove(), then we
dont have to fixup all the transformation functions.
--
Martin Mikkelsen (mamikk(a)mamikk.no)
http://www.mamikk.no/
3 May
3 May
1:03 a.m.
New subject: [PATCH] Memory corruption using s.substr transformation
Hello,
On 5/2/13 7:55 PM, Martin Mikkelsen wrote:
On Wed, May 01, 2013 at 11:04:28AM +0200, Alex Hermann
wrote:
I don't think this is actually good. Most of operations are safe and
memcpy is optimized in all cases while memmove can imply more laborious
operations. Besides that, it is not a single place where to do this change.
Cheers,
Daniel
--
Daniel-Constantin Mierla - http://www.asipto.com
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Training, San Francisco, USA - June 24-27, 2013
* http://asipto.com/u/katu *
strncpy() must be replaced with memmove() when
src and dst (may) overlap.
You are right, it is probably better to do the copying
with memove(), then we
dont have to fixup all the transformation functions.
3976
days inactive
4019
days old
12 comments
5 participants
participants (5)
-
Alex Hermann -
Andreas Granig -
Daniel-Constantin Mierla -
Martin Mikkelsen -
Martin Mikkelsen