Buffer overflow in epicsStrnRawFromEscaped

[Bruce Hill]

Error found when Chris Ford, a member of our DAQ group ran this valgrind of caput to a UINT8 waveform
Note the missing -S, as it works with -S.

valgrind caput TST:EDT:ORCA1:FileName 0123456789012345678901234567890123456789012
==9708== Memcheck, a memory error detector
==9708== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==9708== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info
==9708== Command: caput TST:EDT:ORCA1:FileName 0123456789012345678901234567890123456789012
==9708==
==9708== Invalid write of size 1
==9708== at 0x43A226: epicsStrnRawFromEscaped (epicsString.c:103)
==9708== by 0x409890: main (caput.c:503)
==9708== Address 0x601aab8 is 0 bytes after a block of size 40 alloc'd
==9708== at 0x4C20140: calloc (vg_replace_malloc.c:418)
==9708== by 0x409807: main (caput.c:427)
==9708==
Old : TST:EDT:ORCA1:FileName 256 -1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
New : TST:EDT:ORCA1:FileName 256 -1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
==9708==
==9708== HEAP SUMMARY:
==9708== in use at exit: 20,909 bytes in 225 blocks
==9708== total heap usage: 376 allocs, 151 frees, 1,054,201 bytes allocated
==9708==
==9708== LEAK SUMMARY:
==9708== definitely lost: 430 bytes in 5 blocks
==9708== indirectly lost: 271 bytes in 1 blocks
==9708== possibly lost: 0 bytes in 0 blocks
==9708== still reachable: 20,208 bytes in 219 blocks
==9708== suppressed: 0 bytes in 0 blocks
==9708== Rerun with --leak-check=full to see details of leaked memory
==9708==
==9708== For counts of detected and suppressed errors, rerun with: -v
==9708== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4)

Here's a patch to epicsStringTest.c that tests the bug:
--- src/libCom/test/epicsStringTest.c (revision 16400)
+++ src/libCom/test/epicsStringTest.c (working copy)
@@ -43,14 +43,16 @@
     const char * const empty = "";
     const char * const space = " ";
     const char * const A = "A";
+ const char * const ABC = "ABC";
     const char * const ABCD = "ABCD";
     const char * const ABCDE = "ABCDE";
     const char * const a = "a";
     const char * const abcd = "abcd";
     const char * const abcde = "abcde";
+ char result[20];
     char *s;

- testPlan(281);
+ testPlan(285);

     testChars();

@@ -85,5 +87,17 @@
     testOk1(epicsStrHash(abcd, 0) == epicsMemHash(abcde, 4, 0));
     testOk1(epicsStrHash(abcd, 0) != epicsMemHash("abcd\0", 5, 0));

+ /* Test for buffer overflow and NULL termination on epicsStrnEscapedFromRaw */
+ epicsStrnEscapedFromRaw( result, sizeof(result), ABCDE, strlen(ABCDE) );
+ epicsStrnEscapedFromRaw( result, strlen(ABCD), ABCD, strlen(ABCD) );
+ testOk( result[strlen(ABCDE)-1] == 'E', "epicsStrnEscapedFromRaw buffer overflow");
+ testOk( strcmp(result, ABC) == 0, "epicsStrnEscapedFromRaw NULL termination");
+
+ /* Test for buffer overflow and NULL termination on epicsStrnRawFromEscaped */
+ epicsStrnRawFromEscaped( result, sizeof(result), ABCDE, strlen(ABCDE) );
+ epicsStrnRawFromEscaped( result, strlen(ABCD), ABCD, strlen(ABCD) );
+ testOk( result[strlen(ABCDE)-1] == 'E', "epicsStrnRawFromEscaped buffer overflow");
+ testOk( strcmp(result, ABC) == 0, "epicsStrnRawFromEscaped NULL termination");
+
     return testDone();
 }

Possible patch to fix it based on 3.14.12.4 epicsString.c
--- src/libCom/misc/epicsString.c (revision 16400)
+++ src/libCom/misc/epicsString.c (working copy)
@@ -40,6 +40,7 @@
 {
     const char *pfrom = from;
     char *pto = to;
+ char *pend = to + outsize - 1;
     char c;
     int nto = 0, nfrom = 0;

@@ -100,7 +101,10 @@
             *pto++ = c; nto++;
         }
     }
- *pto = '\0'; /* NOTE that nto does not have to be incremented */
+ /* Guard against writing past the end of the buffer */
+ if( pto > pend )
+ pto = pend;
+ *pto = '\0'; /* NOTE that nto does not have to be incremented */
     return nto;
 }

[Bruce Hill (bhill)]

Also in 3.15 series

[Andrew Johnson (anj)]

These two epicsStrn*() routines are documented in the AppDevGuide at http://www.aps.anl.gov/epics/base/R3-14/12-docs/AppDevGuide/node20.html#SECTION0020176000000000000000

I have to admit that given their names I would expect them to behave like strncpy() and not guarantee to nil-terminate the output string if its too short, but that doesn't seem to be the case. Under some (rare, possibly non-existent) circumstances it appears that epicsStrnEscapedFromRaw() can return without nil-terminating its output string, but it does return -1 when that happens marking an error (although most callers will probably ignore the return value).

I found the test code proposed somewhat confusing so I simplified it and added more tests, and I do now agree that there is a bug in epicsStrnRawFromEscaped() which can write a Nil terminator beyond the end of the output buffer (only when the input string is longer than the output buffer though, so the dbTranslateEscape() case is safe).

My fix avoids adding another variable:

--- src/libCom/misc/epicsString.c 2014-03-11 22:12:41 +0000
+++ src/libCom/misc/epicsString.c 2014-11-03 23:38:02 +0000
@@ -43,6 +43,9 @@
     char c;
     int nto = 0, nfrom = 0;

+ if (outsize == 0)
+ return 0;
+
     while ((c = *pfrom++) && nto < outsize && nfrom < inlen) {
         nfrom++;
         if (c == '\\') {
@@ -100,7 +103,10 @@
             *pto++ = c; nto++;
         }
     }
- *pto = '\0'; /* NOTE that nto does not have to be incremented */
+ if (nto < outsize)
+ *pto = '\0';
+ else
+ pto[-1] = '\0';
     return nto;
 }

Feedback on this would be appreciated.

[Bruce Hill (bhill)]

Hi Andrew,
I agree with your adding a check for a zero output buffer.
I'm not sure why you object to adding another variable.
I prefer testing against a ptr to the end of the output buffer
instead of relying on nto and pto always being kept in sync
by the loop. However, either solution should work and would
prevent the buffer overflow.

Regards,
- Bruce

On 11/3/2014 3:42 PM, Andrew Johnson wrote:
> These two epicsStrn*() routines are documented in the AppDevGuide at
> http://www.aps.anl.gov/epics/base/R3-14/12-docs/AppDevGuide/node20.html#SECTION0020176000000000000000
>
> I have to admit that given their names I would expect them to behave
> like strncpy() and not guarantee to nil-terminate the output string if
> its too short, but that doesn't seem to be the case. Under some (rare,
> possibly non-existent) circumstances it appears that
> epicsStrnEscapedFromRaw() can return without nil-terminating its output
> string, but it does return -1 when that happens marking an error
> (although most callers will probably ignore the return value).
>
> I found the test code proposed somewhat confusing so I simplified it and
> added more tests, and I do now agree that there is a bug in
> epicsStrnRawFromEscaped() which can write a Nil terminator beyond the
> end of the output buffer (only when the input string is longer than the
> output buffer though, so the dbTranslateEscape() case is safe).
>
> My fix avoids adding another variable:
>
> --- src/libCom/misc/epicsString.c 2014-03-11 22:12:41 +0000
> +++ src/libCom/misc/epicsString.c 2014-11-03 23:38:02 +0000
> @@ -43,6 +43,9 @@
> char c;
> int nto = 0, nfrom = 0;
>
> + if (outsize == 0)
> + return 0;
> +
> while ((c = *pfrom++) && nto < outsize && nfrom < inlen) {
> nfrom++;
> if (c == '\\') {
> @@ -100,7 +103,10 @@
> *pto++ = c; nto++;
> }
> }
> - *pto = '\0'; /* NOTE that nto does not have to be incremented */
> + if (nto < outsize)
> + *pto = '\0';
> + else
> + pto[-1] = '\0';
> return nto;
> }
>
> Feedback on this would be appreciated.
>

[Andrew Johnson (anj)]

Slightly shorter fix committed.