Bogus usage of strncpy in unit tests

[skliper]

There are several places in OSAL where the Buffer Overflow protection
feature of the strncpy() function is used incorrectly, in a way that causes
it to not protect against buffer overflow.

Correct usage is to pass the size of the destination area as
the third argument, so strncpy() will stop before trying to write
past the end of the destination storage.

Incorrect usage observed is passing the length of the SOURCE string
as the limiting size. The resulting behavior is:

• Call strlen() to get length of source data.
• Call strncpy() to copy the string
• strncpy() copies bytes until it sees NUL or copies N bytes.
• in this case, it will always copy all data, and stop before the NUL.

The upshot of this is strncpy() always copies the whole source
string and never writes a terminating NUL.

Better usage would be to present the destination buffer size
as the limiting size in the 3rd argument (yes, strncpy() stops
writing after writing the NUL).

[skliper]

Imported from trac issue 35. Created by glimes on 2015-04-10T15:30:24, last modified: 2019-08-14T14:11:46

[skliper]

Trac comment by glimes on 2015-06-10 14:45:17:

This could very well go away with #40 (or similar) changes.
Documenting the bad strncpy usage, and placing this ticket
on ''hold'' until the #40 vs UT-Assert issues are resolved.

Confirming that as of [changeset:2c9c3451] ic-2015-0602-merge
the following incorrect usages of strncpy() still exist in
the OSAL Unit Test code:

ut_os_stubs.c -- UT_os_log_api() various lines:
{{{
strncpy(testInfo->name, testPtr->name, strlen(testPtr->name));
strncpy(testInfo->name, testPtr->name, strlen(testPtr->name));
strncpy(testInfo->result, testPtr->result, strlen(testPtr->result));
}}}
The 3rd argument is the length of the destination buffer,
so that strncpy will stop before it overflows. It already
knows to stop at the end of the source data (after moving
the zero termination), so the net result of the above is to
always copy all of the name to the buffer, even if it overflows,
and to never copy the terminating NUL.

Refraining from copying the NUL is an issue here, as the
structure contining the field which is being filled has been
pre-cleared to the NUL value, however, this usage actively
obscures the fact that the buffer overflow protection in strncpy
is actively being broken.

Here, the 3rd parameter should be sizeof (testInfo->name)
or sizeof (testInfo->result) as apporpriate. We know these
fields are arrays, not pointers, because the memset would have
nulled any pointers present.

ut_os_stubs.h -- various macros:
{{{
strncpy(VAR1.name, APINAME, strlen(APINAME));
strncpy(VAR1.tests[TSTIDX].name, TSTNAME, strlen(TSTNAME));
strncpy(VAR1.tests[TSTIDX].result, TSTRESULT, strlen(TSTRESULT));
}}}

Comments as above. 3rd arg needs to be ''SIZE OF DESTINATION BUFFER'',
not ''LENGTH OF THE SOURCE STRING''.

[skliper]

Trac comment by glimes on 2015-07-13 18:16:59:

When updating code using strncpy bear in mind the exact wording of the language specification:

{{{
#include <string.h>
char *strncpy(char * restrict s1,
const char * restrict s2,
size_t n);
}}}
The strncpy function copies not more than ''n'' characters (characters that follow a \0 character are not copied) from the array pointed to by ''s2'' to the array pointed to by ''s1''. If copying takes place between objects that overlap, the behavior is undefined.

If the array pointed to by ''s2'' is a string that is shorter than ''n'' characters, null characters are appended to the copy in the array pointed to by ''s1'', until ''n'' characters in all have been written.

That last paragraph was something I'd forgotten. It might be useful, and most assuredly obsoletes code that explicitly clears buffers before using strncpy to fill them. Just thought I would throw this into the ticket. I doubt that I'm the only person who forgets the fact that strncpy zero-fills the rest of the destination after short strings.

[skliper]

Trac comment by glimes on 2015-07-13 18:43:03:

Remaining misuses of strncpy in the most bleeding edge version of OSAL in Babelfish:

• a bunch of places in os/vxworks6 the size is 32
  when an appropriate OS_MAX_something macro should be used.

• rtems (and rtems-ng) osfilesys.c has
  {{{
  strncpy(LocalPath, VirtualPath, strlen(VirtualPath));
  LocalPath[strlen(VirtualPath)] = '\0';
  }}}

• ut_os_stubs.c, despite all the UT work recently, still has
  {{{
  memset(testInfo, 0x00, sizeof(UT_OsTestDesc_t));
  strncpy(testInfo->name, testPtr->name, strlen(testPtr->name));
  strncpy(testInfo->result, testPtr->result, strlen(testPtr->result));
  }}}

• ut_os_stubs.h also has this:
  {{{
  #define UT_OS_SET_TEST_RESULT_MACRO(VAR1, TSTIDX, TSTNAME, TSTRESULT)
  {
  memset(&(VAR1.tests[TSTIDX]), 0x00, sizeof(UT_OsTestDesc_t));
  strncpy(VAR1.tests[TSTIDX].name, TSTNAME, strlen(TSTNAME));
  strncpy(VAR1.tests[TSTIDX].result, TSTRESULT, strlen(TSTRESULT));
  (TSTIDX)++;
  }
  }}}

[skliper]

Trac comment by jphickey on 2018-05-14 16:17:14:

Confirmed that these strncpy issues are still present in the current code. These should be fixed in the next OSAL release.

Some of these exist inside macros -- which is wrong on several levels -- should not be hiding big chunks of code like this inside a macro to begin with. With the UT assert framework this can be much cleaner, but this UT code was never fixed up appropriately.

[skliper]

Trac comment by jhageman on 2019-07-03 13:34:40:

Moved open 4.2.2 tickets to 4.3.1

[skliper]

Trac comment by jhageman on 2019-07-26 15:13:16:

Milestone renamed

[skliper]

Note unit-test-coverage/vxworks6 goes away with #267.

Remaining issues:

unit-tests/shared/ut_os_stubs.h:    strncpy(VAR1.name, APINAME, strlen(APINAME));                      \
unit-tests/shared/ut_os_stubs.h:    strncpy(VAR1.tests[TSTIDX].name, TSTNAME, strlen(TSTNAME));        \
unit-tests/shared/ut_os_stubs.h:    strncpy(VAR1.tests[TSTIDX].result, TSTRESULT, strlen(TSTRESULT));  \
unit-tests/shared/ut_os_stubs.c:        strncpy(apiInfo->name, apiPtr->name, strlen(apiPtr->name));
unit-tests/shared/ut_os_stubs.c:            strncpy(testInfo->name,   testPtr->name,   strlen(testPtr->name));
unit-tests/shared/ut_os_stubs.c:            strncpy(testInfo->result, testPtr->result, strlen(testPtr->result));

[skliper]

Fixed in #318 - removed final offenders ut_os_stubs.h and ut_os_stubs.c

[skliper]

@dmknutsen can you confirm this is resolved?

[dmknutsen]

Yep. In order to identify any remaining issues, I audited all files in the main cFS directory that contain the word strncpy. The attached spreadsheet contains the results of the audit.
No issues remain.

strncpyAudit_v1.xlsx

[skliper]

Closing as duplicate (issue was resolved by other pull requests.)