Skip to content

Conversation

@pemensik
Copy link
Contributor

We checked fstrm library using static analysis and it found some minor issues. Mostly handling of error states with non-freed resources. Each fixed issue is mentioned in commit message.

libmy/argv.c:3212: var_deref_model: Passing null pointer "queue_list" to "do_list", which dereferences it
libmy/argv.c:3204: var_deref_model: Passing null pointer "queue_list" to "do_list", which dereferences it.

Workaround to possibility no arguments is received

Usually at least one arg is always passed in argv - program name. Do not
dereference null queue_list in unlikely case no parameter in argv.
invalid_type: Argument "conn->len_frame_total" to format specifier "%zd" was expected to have type "ssize_t"("long") but has type "unsigned int".
1. fstrm-0.6.0/libmy/argv.c:1782: addr_non_null: The address of an object "argv_types" is never null.
2. fstrm-0.6.0/libmy/argv.c:1782: assignment: Assigning: "type_p" = "argv_types".
3. fstrm-0.6.0/libmy/argv.c:1809: notnull: At condition "type_p == NULL", the value of "type_p" cannot be "NULL".
4. fstrm-0.6.0/libmy/argv.c:1809: dead_error_condition: The condition "type_p == NULL" cannot be true.
5. fstrm-0.6.0/libmy/argv.c:1810: dead_error_begin: Execution cannot reach this statement: "(void)fprintf(argv_error_st...".

40. fstrm-0.6.0/libmy/argv.c:2724: check_return: Calling "string_to_value" without checking return value (as is done elsewhere 6 out of 7 times).
34. fstrm-0.6.0/libmy/argv.c:2238: alloc_fn: Storage is returned from allocation function "realloc".
35. fstrm-0.6.0/libmy/argv.c:2238: var_assign: Assigning: "argv" = storage returned from "realloc(argv, 8UL * max)".
37. fstrm-0.6.0/libmy/argv.c:2254: var_assign: Assigning: "argv_p" = "argv".
47. fstrm-0.6.0/libmy/argv.c:2229: leaked_storage: Variable "argv_p" going out of scope leaks the storage it points to.
48. fstrm-0.6.0/libmy/argv.c:2229: leaked_storage: Variable "argv" going out of scope leaks the storage it points to.
libmy/argv.c:1352:7: warning[core.uninitialized.Assign]: The expression is an uninitialized value. The computed value will also be garbage
      (*(int *)var)++;
      ^~~~~~~~~~~~~
libmy/argv.c:1207:29: note: Assuming field 'at_value' is not equal to 0
  for (type_p = argv_types; type_p->at_value != 0; type_p++) {
                            ^~~~~~~~~~~~~~~~~~~~~
libmy/argv.c:1207:3: note: Loop condition is true.  Entering loop body
  for (type_p = argv_types; type_p->at_value != 0; type_p++) {
  ^
libmy/argv.c:1208:9: note: Assuming 'val_type' is equal to field 'at_value'
    if (type_p->at_value == val_type) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
libmy/argv.c:1208:5: note: Taking true branch
    if (type_p->at_value == val_type) {
    ^
libmy/argv.c:1210:7: note:  Execution continues on line 1214
      break;
      ^
libmy/argv.c:1214:15: note: Field 'at_value' is not equal to 0
  if (type_p->at_value == 0) {
              ^
libmy/argv.c:1214:3: note: Taking false branch
  if (type_p->at_value == 0) {
  ^
libmy/argv.c:1222:7: note: Assuming the condition is true
  if (type & ARGV_FLAG_ARRAY) {
      ^~~~~~~~~~~~~~~~~~~~~~
libmy/argv.c:1222:3: note: Taking true branch
  if (type & ARGV_FLAG_ARRAY) {
  ^
libmy/argv.c:1225:9: note: Assuming field 'aa_entry_n' is equal to 0
    if (arr_p->aa_entry_n == 0) {
        ^~~~~~~~~~~~~~~~~~~~~~
libmy/argv.c:1225:5: note: Taking true branch
    if (arr_p->aa_entry_n == 0) {
    ^
libmy/argv.c:1226:35: note: Storing uninitialized value
      arr_p->aa_entries = (char *)malloc(ARRAY_INCR *size);
                                  ^~~~~~~~~~~~~~~~~~~~~~~~
libmy/argv.c:1234:9: note: Assuming field 'aa_entries' is not equal to NULL
    if (arr_p->aa_entries == NULL) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~
libmy/argv.c:1234:5: note: Taking false branch
    if (arr_p->aa_entries == NULL) {
    ^
libmy/argv.c:1251:3: note: Control jumps to 'case 17:'  at line 1349
  switch (val_type) {
  ^
libmy/argv.c:1351:9: note: Assuming 'arg' is equal to NULL
    if (arg == NULL) {
        ^~~~~~~~~~~
libmy/argv.c:1351:5: note: Taking true branch
    if (arg == NULL) {
    ^
libmy/argv.c:1352:7: note: The expression is an uninitialized value. The computed value will also be garbage
      (*(int *)var)++;
      ^~~~~~~~~~~~~
@pemensik
Copy link
Contributor Author

There are still few left unfixed.

Error: CPPCHECK_WARNING (CWE-476): [#def1]
fstrm-0.6.0/fstrm/fstrm-private.h:76: error[ctunullpointer]: Null pointer dereference: elems
#   74|   } fs_buf;
#   75|   
#   76|-> VECTOR_GENERATE(fs_bufvec, fs_buf)
#   77|   
#   78|   /* buffer helpers */

Error: CPPCHECK_WARNING (CWE-476): [#def6]
fstrm-0.6.0/libmy/ubuf.h:37: error[ctunullpointer]: Null pointer dereference: elems
#   35|   #include "vector.h"
#   36|   
#   37|-> VECTOR_GENERATE(ubuf, uint8_t)
#   38|   
#   39|   static inline ubuf *

error repeats for elems, out, vec, vec1. I admit I don't see the error or how to fix it.

Error: DEADCODE (CWE-561): [#def5]
fstrm-0.6.0/fstrm/writer.c:360: assignment: Assigning: "iov_max" = "128".
fstrm-0.6.0/fstrm/writer.c:361: const: At condition "iov_max > 1024", the value of "iov_max" must be equal to 128.
fstrm-0.6.0/fstrm/writer.c:361: dead_error_condition: The condition "iov_max > 1024" cannot be true.
fstrm-0.6.0/fstrm/writer.c:362: dead_error_line: Execution cannot reach this statement: "iov_max = 1024;".
#  360|   	int iov_max = FSTRM__WRITER_IOVEC_SIZE / 2;
#  361|   	if (iov_max > IOV_MAX)
#  362|-> 		iov_max = IOV_MAX;
#  363|   	assert(iov_max > 0);
#  364|   

Is just compile-time check of header define range, no issue with that.

Error: TAINTED_SCALAR (CWE-20): [#def10]
fstrm-0.6.0/src/fstrm_capture.c:1227: tainted_data: Passing tainted expression "**argv" to "parse_args", which uses it as a loop boundary.
fstrm-0.6.0/src/fstrm_capture.c:1227: remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
# 1225|   {
# 1226|   	/* Parse arguments. */
# 1227|-> 	if (!parse_args(argc, argv, &g_program_ctx)) {
# 1228|   		usage(NULL);
# 1229|   		return EXIT_FAILURE;

Error: TAINTED_SCALAR (CWE-20): [#def11]
fstrm-0.6.0/src/fstrm_replay.c:273: tainted_data: Passing tainted expression "**argv" to "parse_args", which uses it as a loop boundary.
fstrm-0.6.0/src/fstrm_replay.c:273: remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
#  271|   	struct fstrm_writer *w = NULL;
#  272|   
#  273|-> 	if (!parse_args(argc, argv, &g_program_ctx))
#  274|   		usage(NULL);
#  275|   

Again, not sure how to fix this error. Does it check correctly argc?

@d-hat
Copy link

d-hat commented Mar 1, 2021

The Null pointer dereference warning appears to be in the macro expanded name##append, assuming the elems parameter may be null. I don't think this really needs fixing, as passing null would be an error on the caller's side .. and the impact would be a crash when memcpy tries to read page 0, while the best a fix could do is panic with a more detailed error message. To silence coverity a simple assert(elems != NULL) should do.

TAINTED_SCALAR warnings on argv also seem to be unimportant. Looking down the call chain, what it seems to be referring to is the loop at libmy/argv.c:3166 which loops from **argv until it finds a \0. I would consider this a false positive, since the contents of argv should always be 0-terminated strings.

@reedjc
Copy link
Contributor

reedjc commented Apr 2, 2021

I cherry-picked commit cdbebf0 (Fix unsorted printf args) to our next branch for impending release.

@cmikk
Copy link
Contributor

cmikk commented Apr 2, 2021

Thank you @pemensik for reporting these issues, and @d-hat for your analysis.

As @reedjc noted, we've cherry-picked the printf correction into our upcoming release, and will take a closer look at the argv.c issues.

@cmikk
Copy link
Contributor

cmikk commented Apr 2, 2021

As libmy has its own repository here (and is embedded in various other farsightsec projects), I've moved these commits over to farsightsec/libmy#4 and referenced this PR there for background.

Closing this PR, will work on changes in the core libmy and separately sync the embedded libmy copies.

@cmikk cmikk closed this Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants