Jonathan Heusser wrote:
Hello list,
While testing out my new code analyer, I found some problems in the
functions rdconvertbufftostr,rdconvertbufftobinstr and
rddecryptpass in the RADIUS dissector and in the function
iaconvertbufftostr in the IAPP dissector.
Consider the following lines:
packet-radius.c:3397:
(1) static void rdconvertbufftostr(gchar *dest, tvbuff_t *tvb, int
offset, int length) {
guint32 i;
..
(2) for (i=0; i < (guint32)length; i++) {
if( isprint((int)pd[i])) {
(3) dest[totlen]=(gchar)pd[i];
totlen++;
} else {
(4) sprintf(&(dest[totlen]), "\\%03o", pd[i]);
totlen=totlen+strlen(&(dest[totlen]));
}
}
..
..
If the fourth argument of this function (1) is set to a negative value
If the fourth argument to "rdconvertbufftostr()" is negative, there's a
bug in whatever code is calling it.
In many cases, it's not ever going to be negative;
"dissect_attribute_value_pairs()" checks for an AVP length < 2, and
refuses to dissect the AVP if it's < 2, because the length includes the
2 bytes of type and length. Therefore, in those cases, avp_length-2 is
>= 0, and that's what's passed to "rdconvertbufftostr()".
There is at least one case (tagged strings) where it doesn't check that
the length is >= 3 - and, in fact, there are other non-string parameters
that also need length checks; "rd_value_to_str()" should be doing those
checks.
A simple fix would be to bail out when 'length' is negative.
"Bailing out" should be done with "g_assert()", because, as indicated,
the length should never *be* negative.
Alternatively, the argument should perhaps be made unsigned.
However, the *real* fix to "rdconvertbufftostr()" involves either
supplying a bytes-remaining-in-the-destination argument to it, and doing
length checking against that, or having it grow the buffer as needed, as
the buffer could be overflowed by a string that's just too long.
