Daniel Stenberg wrote:
> > Yeah. Note that the last call will write outside s (buffer
> > overflow) if the size of s is defined to an even number.
> >
> > Once that is addressed (if neccessary) I want this fix to be
> > applied.
> >
> > Acked-by: Peter Stuge <pe...@stuge.se>
>
> Good catch! Both of them. Committed now with some cosmetic changes
> of mine.
Sorry, I should have been more clear about this. The overflow I had
in mind was when sprintf() wrote the final two hex digits followed by
\0 to &s[i*2] (but +1 was added to the temp buffer) but I was crying
wolf, because the screen number is always added after the cookie so
there should always be room for the \0 even if it gets overwritten.
Looking at this code a bit more I found another issue thought, it
will produce invalid packets if LIBSSH2_X11_RANDOM_COOKIE_LEN is odd.
Suggested patch attached.
//Peter
Index: src/channel.c
===================================================================
RCS file: /cvsroot/libssh2/libssh2/src/channel.c,v
retrieving revision 1.72
diff -u -r1.72 channel.c
--- src/channel.c 7 Mar 2009 22:08:05 -0000 1.72
+++ src/channel.c 8 Mar 2009 11:39:56 -0000
@@ -1093,16 +1093,28 @@
memcpy(s, auth_cookie, cookie_len);
} else {
int i;
- /* note: the extra +1 below is necessary since the sprintf()
- loop will always write 3 bytes so the last one will write
- the trailing zero at the LIBSSH2_X11_RANDOM_COOKIE_LEN/2
- border */
- unsigned char buffer[(LIBSSH2_X11_RANDOM_COOKIE_LEN / 2) +1];
+ unsigned char *buffer = malloc((cookie_len + 1) / 2);
+ if (!buffer) {
+ libssh2_error(session, LIBSSH2_ERROR_ALLOC,
+ "Unable to allocate memory for X11 cookie", 0);
+ return -1;
+ }
- libssh2_random(buffer, LIBSSH2_X11_RANDOM_COOKIE_LEN / 2);
- for(i = 0; i < (LIBSSH2_X11_RANDOM_COOKIE_LEN / 2); i++) {
+ libssh2_random(buffer, sizeof(buffer));
+
+ /* When cookie_len is an odd number, this will actually write
+ * cookie_len+1 hex digits into s. A little sloppy but still safe,
+ * because there is room in s also for the screen_number.
+ *
+ * cookie_len is not changed (and must not be, because it has
+ * already been written to the packet before coming here) so any
+ * superflous hex digits are overwritten with screen_number below.
+ */
+ for(i = 0; i < sizeof(buffer); i++) {
sprintf((char *)&s[i*2], "%02X", buffer[i]);
}
+ memset(buffer, 0, sizeof(buffer));
+ free(buffer);
}
s += cookie_len;
------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H
_______________________________________________
libssh2-devel mailing list
libssh2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libssh2-devel
