From cf1433d151d0d749b2295648a51de69996a078f1 Mon Sep 17 00:00:00 2001 From: erickson Date: Mon, 16 Jul 2007 13:03:48 +0000 Subject: [PATCH] Patch from Scott McKellar to repair srfsh buffer overflow: The potential overflow occurs in handle_introspection(), which in the existing code uses sprintf() to insert some user-supplied input into a fixed length buffer, without checking the length of the user-supplied input. There's always more than one way to fix this sort of thing. For example I could have formatted into a growing_buffer. Instead, I chose to calculate the necessary buffer size and then declare a variable-length buffer. That way I avoid two mallocs and two frees. I tested this change by inserting some temporary printfs to capture the commands being sent to parse_request(). The outputs before and after the change were identical. There's at least one more such potential overflow that I haven't addressed yet. It's near the top of handle_login(), where we use sprintf() to insert a username into a fixed length buffer. There may be others that I haven't noticed yet. Scott McKellar http://home.swbell.net/mck9/ct/ git-svn-id: svn://svn.open-ils.org/OpenSRF/trunk@1038 9efc2488-bf62-4759-914b-345cdb29e865 --- src/srfsh/srfsh.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/srfsh/srfsh.c b/src/srfsh/srfsh.c index 37a767b..6c4cb54 100644 --- a/src/srfsh/srfsh.c +++ b/src/srfsh/srfsh.c @@ -324,25 +324,30 @@ static int parse_request( char* request ) { static int handle_introspect(char* words[]) { - if(words[1] && words[2]) { - fprintf(stderr, "--> %s\n", words[1]); - char buf[256]; - memset(buf,0,256); - sprintf( buf, "request %s opensrf.system.method %s", words[1], words[2] ); + if( ! words[1] ) + return 0; + + fprintf(stderr, "--> %s\n", words[1]); + + // Build a command in a suitably-sized + // buffer and then parse it + + size_t len; + if( words[2] ) { + static const char text[] = "request %s opensrf.system.method %s"; + len = sizeof( text ) + strlen( words[1] ) + strlen( words[2] ); + char buf[len]; + sprintf( buf, text, words[1], words[2] ); return parse_request( buf ); } else { - - if(words[1]) { - fprintf(stderr, "--> %s\n", words[1]); - char buf[256]; - memset(buf,0,256); - sprintf( buf, "request %s opensrf.system.method.all", words[1] ); - return parse_request( buf ); - } - } + static const char text[] = "request %s opensrf.system.method.all"; + len = sizeof( text ) + strlen( words[1] ); + char buf[len]; + sprintf( buf, text, words[1] ); + return parse_request( buf ); - return 0; + } } -- 2.43.2