I've been working on a pass_persist based extension, and while I was
puzzling out the exact encoding/decoding of octets and strings, I ran
across something that looks like a potential buffer overflow.
The bin2asc() function is used in both pass.c and pass_persist.c to
encode strings with non-printable characters so that they may be safely
transmitted to the pass(_persist) extension program.
In pass.c that ships with net-snmp-5.4.2.1, bin2asc() is at line 124 of
pass.c:
/*
* This is also called from pass_persist.c
*/
int
bin2asc(char *p, size_t n)
{
int i, flag = 0;
char buffer[SNMP_MAXBUF];
/* prevent buffer overflow */
if ((int)n > (sizeof(buffer) - 1))
n = sizeof(buffer) - 1;
for (i = 0; i < (int) n; i++) {
buffer[i] = p[i];
if (!isprint(p[i]))
flag = 1;
}
if (flag == 0) {
p[n] = 0;
return n;
}
for (i = 0; i < (int) n; i++) {
sprintf(p, "%02x ", (unsigned char) (buffer[i] & 0xff));
p += 3;
}
*--p = 0;
return 3 * n - 1;
}
Note that in the final for loop, bin2asc() modifies *p in place. If n =
SNMP_MAXBUF, and *p is encoded, *p will grow to be 3n-1 bytes, as is
shown in the return value.
Now look at lines 498 and 499 of pass.c:
else if (bin2asc(buf2, var_val_len) == (int) var_val_len)
snprintf(buf, sizeof(buf), "string \"%s\"\n", buf2);
bin2asc() is called on buf2, which is declared at line 443:
char buf[SNMP_MAXBUF], buf2[SNMP_MAXBUF];
So if buf2 is a string of SNMP_MAXBUF-1 characters in length, with at
least one non-printing characters, buf2 will overflow when line 498 is
evaluated.
Further, the data in buf2 will be truncated when snprintf is called in
line 499.
The exact same problem can be found at lines 460 and 461 of pass_persist.c.
I'm not sure what the best solution is for fixing these issues. Here
are some ideas:
1. Change bin2asc() to limit the size of the encoded string to
SNMP_MAXBUF/3 character.
2. Change the allocation for buf and buf2 in setPassPersis and setPass
so that they can accommodate the required number of characters.
3. Change bin2asc()'s API and rewrite passSet() and passPersistSet() to
use the new API.
Option 1 is the simplest approach, and will require the smallest change
to codebase, but it just papers over the problem and allows the
potential data corruption to continue.
Option 2 is potentially much more complex. I'm not sure how resizing
those buffers will imapact other parts of the code.
Option 3 is pretty much wide open, it is definitely the hardest approach
and may be the only desirable approach or it may be a waste of time.
I don't know the net-snmp codebase well enough to assess these options
or provide a patch.
I'd also like to suggest that bin2asc() be given doxygen docs, something
like this:
/**
* Checks a buffer for non-printable characters (as determined by
isprint()).
* If non-printable characters are found, encodes the buffer as a string
of space separated,
* hexadecimal ASCII values. The encoded data is written into the buffer.
*
* For example, "123\r" will be encoded as "31 32 33 0a".
*
* @warning Encoding a buffer will cause its size to increase by a
factor of 3.
* Be sure enough space is allocated to allow for this size
increase.
*
* Note: This function is also used by pass_persist.c
*
* @returns The number of characters in the buffer.
**/
Thanks,
--Mark Swayne
------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge
This is your chance to win up to $100,000 in prizes! For a limited time,
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize
details at: http://p.sf.net/sfu/Challenge
_______________________________________________
Net-snmp-users mailing list
Net-snmp-users@lists.sourceforge.net
Please see the following page to unsubscribe or change other options:
https://lists.sourceforge.net/lists/listinfo/net-snmp-users
