From 831f48e719edfdfd40adf5f6127863e292de0c87 Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Tue, 4 May 2010 15:43:20 -0500 Subject: [PATCH] Fix the macLib buffer overflow issue, Bug 551909. This is mostly Eric's patches, without the valend name change. --- src/as/asLibRoutines.c | 2 +- src/dbStatic/dbLexRoutines.c | 2 +- src/libCom/macLib/macCore.c | 41 ++++++++++++++++++------------------ src/libCom/macLib/macLib.h | 12 +++++------ src/libCom/test/macLibTest.c | 26 ++++++++++++++++++++++- 5 files changed, 53 insertions(+), 30 deletions(-) diff --git a/src/as/asLibRoutines.c b/src/as/asLibRoutines.c index e2d0ac41d..bf1f59213 100644 --- a/src/as/asLibRoutines.c +++ b/src/as/asLibRoutines.c @@ -201,7 +201,7 @@ static int myInputFunction(char *buf, int max_size) fgetsRtn = fgets(mac_input_buffer,BUF_SIZE,stream); if(fgetsRtn) { n = macExpandString(macHandle,mac_input_buffer, - my_buffer,BUF_SIZE-1); + my_buffer,BUF_SIZE); if(n<0) { epicsPrintf("access security: macExpandString failed\n" "input line: %s\n",mac_input_buffer); diff --git a/src/dbStatic/dbLexRoutines.c b/src/dbStatic/dbLexRoutines.c index f2e6cf57a..575470e55 100644 --- a/src/dbStatic/dbLexRoutines.c +++ b/src/dbStatic/dbLexRoutines.c @@ -314,7 +314,7 @@ static int db_yyinput(char *buf, int max_size) pinputFileNow->fp); if(fgetsRtn) { n = macExpandString(macHandle,mac_input_buffer, - my_buffer,MY_BUFFER_SIZE-1); + my_buffer,MY_BUFFER_SIZE); if(n<0) { errPrintf(0,__FILE__, __LINE__, "macExpandString failed for file %s", diff --git a/src/libCom/macLib/macCore.c b/src/libCom/macLib/macCore.c index bb2407483..9eb206e4f 100644 --- a/src/libCom/macLib/macCore.c +++ b/src/libCom/macLib/macCore.c @@ -171,7 +171,7 @@ epicsShareAPI macSuppressWarning( * This is a very basic and powerful routine. It's basically a wrapper * around the the translation "engine" trans() */ -long /* #chars copied, <0 if any macros are */ +long /* strlen(dest), <0 if any macros are */ /* undefined */ epicsShareAPI macExpandString( MAC_HANDLE *handle, /* opaque handle */ @@ -180,8 +180,7 @@ epicsShareAPI macExpandString( char *dest, /* destination string */ - long maxlen ) /* maximum number of characters to copy */ - /* to destination string */ + long capacity ) /* capacity of destination buffer (dest) */ { MAC_ENTRY entry; const char *s; @@ -196,7 +195,11 @@ epicsShareAPI macExpandString( /* debug output */ if ( handle->debug & 1 ) - printf( "macExpandString( %s, maxlen = %ld )\n", src, maxlen ); + printf( "macExpandString( %s, capacity = %ld )\n", src, capacity ); + + /* Check size */ + if (capacity <= 1) + return -1; /* expand raw values if necessary */ if ( expand( handle ) < 0 ) @@ -211,7 +214,7 @@ epicsShareAPI macExpandString( s = src; d = dest; *d = '\0'; - trans( handle, &entry, 0, "", &s, &d, d + maxlen ); + trans( handle, &entry, 0, "", &s, &d, d + capacity - 1 ); /* return +/- #chars copied depending on successful expansion */ length = d - dest; @@ -228,7 +231,7 @@ epicsShareAPI macExpandString( * Define the value of a macro. A NULL value deletes the macro if it * already existed */ -long /* length of value */ +long /* strlen(value) */ epicsShareAPI macPutValue( MAC_HANDLE *handle, /* opaque handle */ @@ -286,7 +289,7 @@ epicsShareAPI macPutValue( /* * Return the value of a macro */ -long /* #chars copied (<0 if undefined) */ +long /* strlen(value), <0 if undefined */ epicsShareAPI macGetValue( MAC_HANDLE *handle, /* opaque handle */ @@ -295,8 +298,7 @@ epicsShareAPI macGetValue( char *value, /* string to receive macro value or name */ /* argument if macro is undefined */ - long maxlen ) /* maximum number of characters to copy */ - /* to value */ + long capacity ) /* capacity of destination buffer (value) */ { MAC_ENTRY *entry; /* pointer to this macro's entry structure */ long length; /* number of characters returned */ @@ -314,31 +316,30 @@ epicsShareAPI macGetValue( /* look up macro name */ entry = lookup( handle, name, FALSE ); - /* if maxlen <= 0 or VALUE == NULL just return -1 / 0 for undefined / + /* if capacity <= 1 or VALUE == NULL just return -1 / 0 for undefined / defined macro */ - if ( maxlen <= 0 || value == NULL ) { + if ( capacity <= 1 || value == NULL ) { return ( entry == NULL ) ? -1 : 0; } /* if not found, copy name to value and return minus #chars copied */ if ( entry == NULL ) { - strncpy( value, name, maxlen ); - return ( value[maxlen-1] == '\0' ) ? - (long) strlen( name ) : -maxlen; + strncpy( value, name, capacity ); + return ( value[capacity-1] == '\0' ) ? - (long) strlen( name ) : -capacity; } /* expand raw values if necessary; if fail (can only fail because of memory allocation failure), return same as if not found */ if ( expand( handle ) < 0 ) { errlogPrintf( "macGetValue: failed to expand raw values\n" ); - strncpy( value, name, maxlen ); - return ( value[maxlen-1] == '\0' ) ? - (long) strlen( name ) : -maxlen; + strncpy( value, name, capacity ); + return ( value[capacity-1] == '\0' ) ? - (long) strlen( name ) : -capacity; } /* copy value and return +/- #chars copied depending on successful expansion */ -/* FIXME: nul-terminator */ - strncpy( value, entry->value, maxlen ); - length = ( value[maxlen-1] == '\0' ) ? entry->length : maxlen; + strncpy( value, entry->value, capacity ); + length = ( value[capacity-1] == '\0' ) ? entry->length : capacity; return ( entry->error ) ? -length : length; } @@ -694,7 +695,7 @@ static void trans( MAC_HANDLE *handle, MAC_ENTRY *entry, int level, /* debug output */ if ( handle->debug & 2 ) - printf( "trans-> entry = %p, level = %d, maxlen = %u, discard = %s, " + printf( "trans-> entry = %p, level = %d, capacity = %u, discard = %s, " "rawval = %s\n", entry, level, (unsigned int)(valend - *value), discard ? "T" : "F", *rawval ); /* initially not in quotes */ @@ -777,7 +778,7 @@ static void refer ( MAC_HANDLE *handle, MAC_ENTRY *entry, int level, /* debug output */ if ( handle->debug & 2 ) - printf( "refer-> entry = %p, level = %d, maxlen = %u, rawval = %s\n", + printf( "refer-> entry = %p, level = %d, capacity = %u, rawval = %s\n", entry, level, (unsigned int)(valend - *value), *rawval ); /* step over '$(' or '${' */ diff --git a/src/libCom/macLib/macLib.h b/src/libCom/macLib/macLib.h index 79f505f82..b637687cb 100644 --- a/src/libCom/macLib/macLib.h +++ b/src/libCom/macLib/macLib.h @@ -65,7 +65,7 @@ epicsShareAPI macSuppressWarning( int falseTrue /*0 means issue, 1 means suppress*/ ); -epicsShareFunc long /* #chars copied, <0 if any macros are */ +epicsShareFunc long /* strlen(dest), <0 if any macros are */ /* undefined */ epicsShareAPI macExpandString( MAC_HANDLE *handle, /* opaque handle */ @@ -74,12 +74,11 @@ epicsShareAPI macExpandString( char *dest, /* destination string */ - long maxlen /* maximum number of characters to copy */ - /* to destination string */ + long capacity /* capacity of destination buffer (dest) */ ); -epicsShareFunc long /* length of value */ +epicsShareFunc long /* strlen(value) */ epicsShareAPI macPutValue( MAC_HANDLE *handle, /* opaque handle */ @@ -88,7 +87,7 @@ epicsShareAPI macPutValue( const char *value /* macro value */ ); -epicsShareFunc long /* #chars copied (<0 if undefined) */ +epicsShareFunc long /* strlen(value), <0 if undefined */ epicsShareAPI macGetValue( MAC_HANDLE *handle, /* opaque handle */ @@ -97,8 +96,7 @@ epicsShareAPI macGetValue( char *value, /* string to receive macro value or name */ /* argument if macro is undefined */ - long maxlen /* maximum number of characters to copy */ - /* to value */ + long capacity /* capacity of destination buffer (value) */ ); epicsShareFunc long /* 0 = OK; <0 = ERROR */ diff --git a/src/libCom/test/macLibTest.c b/src/libCom/test/macLibTest.c index d2997e4e9..35dca6fe9 100644 --- a/src/libCom/test/macLibTest.c +++ b/src/libCom/test/macLibTest.c @@ -44,9 +44,31 @@ static void check(const char *str, const char *expect) } } +static void ovcheck(void) +{ + char output[54]; + long status; + + macPutValue(h, "OVVAR","abcdefghijklmnopqrstuvwxyz"); + + memset(output, '~', sizeof output); + status = macExpandString(h, "abcdefghijklmnopqrstuvwxyz$(OVVAR)", output, 52); + testOk(status == 51, "expansion returned %ld, expected 51", status); + testOk(output[50] == 'y', "final character %x, expect 79 (y)", output[50]); + testOk(output[51] == '\0', "terminator character %x, expect 0", output[51]); + testOk(output[52] == '~', "sentinel character %x, expect 7e, (~)", output[52]); + + memset(output, '~', sizeof output); + status = macExpandString(h, "abcdefghijklmnopqrstuvwxyz$(OVVAR)", output, 53); + testOk(status == 52, "expansion returned %ld, expected 52", status); + testOk(output[51] == 'z', "final character %x, expect 7a (z)", output[51]); + testOk(output[52] == '\0', "terminator character %x, expect 0", output[52]); + testOk(output[53] == '~', "sentinel character %x, expect 7e, (~)", output[53]); +} + MAIN(macLibTest) { - testPlan(81); + testPlan(89); if (macCreateHandle(&h, NULL)) testAbort("macCreateHandle() failed"); @@ -193,5 +215,7 @@ MAIN(macLibTest) check("${FOO,FOO=${FOO}}", "!$(FOO,recursive)"); check("${FOO=GRIBBLE,FOO=${FOO}}", "!$(FOO,recursive)"); + ovcheck(); + return testDone(); }