[Opensrf-commits] r1039 - in branches/new-json2: . src/srfsh

[svn at svn.open-ils.org]

Author: erickson
Date: 2007-07-16 09:08:55 -0400 (Mon, 16 Jul 2007)
New Revision: 1039

Modified:
   branches/new-json2/
   branches/new-json2/src/srfsh/srfsh.c
Log:
Merged revisions 1036-1038 via svnmerge from 
svn://svn.open-ils.org/OpenSRF/trunk

........
  r1038 | erickson | 2007-07-16 09:03:48 -0400 (Mon, 16 Jul 2007) | 25 lines
  
  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/
........



Property changes on: branches/new-json2
___________________________________________________________________
Name: svnmerge-integrated
   - /trunk:1-1035
   + /trunk:1-1038

Modified: branches/new-json2/src/srfsh/srfsh.c
===================================================================
--- branches/new-json2/src/srfsh/srfsh.c	2007-07-16 13:03:48 UTC (rev 1038)
+++ branches/new-json2/src/srfsh/srfsh.c	2007-07-16 13:08:55 UTC (rev 1039)
@@ -324,25 +324,30 @@
 
 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;
 }