Fix (CWE-120, CWE-20) detected by Flawfinder

Question

I've been asked to analyze some C code with Flawfinder:

char * buffer;
size_t len;
// my_fd is a file descriptor
read(my_fd, &len, sizeof(len));
buffer = malloc(len + 1);
read(my_fd, buffer, len);
buffer[len] = '\0';

I get the following warnings on the 2 read:

test.c:xx:  [1] (buffer) read:
  Check buffer boundaries if used in a loop including recursive loops
  (CWE-120, CWE-20).
test.c:xx:  [1] (buffer) read:
  Check buffer boundaries if used in a loop including recursive loops
  (CWE-120, CWE-20).

I tried following this answer, modifying the function as follows:

char * buffer;
size_t len;
// my_fd is a file descriptor
ssize_t ret = read(my_fd, &len, sizeof(len));

if (ret == -1 || ret != sizeof len) {
     buffer = NULL;
} else {
     buffer = malloc(len + 1);
     ret = read(my_fd, buffer, len);
     buffer[ret] = '\0';
}
free(buffer);

But the vulnerabilities are still detected. What am I missing?

Update #1:

I updated the function according to @4386427 suggestions, checking both read() and malloc():

char * buffer = NULL;
size_t len;
ssize_t ret = read(my_fd, &len, sizeof(len));

if (ret == sizeof len)
{
     buffer = malloc(len + 1);

     if (buffer != NULL)
     {
          ret = read(my_fd, buffer, len);

          if (ret == len)
          {
               buffer[ret] = '\0';
          }
          free(buffer);
     }
}

But nothing has changed, how can i further improve security?

Update #2

Because Flawfinder only does pattern check, and because it seems that no more improvements can be applied; at this point I'm marking these errors as false positive.

Are you sure about the location of the warning? The message mentions usage in a loop which is not present in the code you show. — Gerhardh, Nov 04 '20 at 10:45
Yes, error points directly to the lines containing the `read()` call — LucaBonadia, Nov 04 '20 at 10:48
`ret = read(my_fd, buffer, len);` and what happens if this reads return -1 ? — Support Ukraine, Nov 04 '20 at 10:49
The second code example could be simplified a bit: `char * buffer = NULL; ... if (ret > 0) { /* Create buffer and read into it */ }` — Some programmer dude, Nov 04 '20 at 10:53
I'm assuming that a 2nd read should not generate errors if the 1st one is fine, am I wrong? — LucaBonadia, Nov 04 '20 at 10:54
@LucaBonadia And what makes you think it's safe to assume that? (yes, you are wrong..) — Support Ukraine, Nov 04 '20 at 10:56
After your update: `if (ret > 0)` is wrong! Must be `if (ret == sizeof len)` — Support Ukraine, Nov 04 '20 at 11:14
Sorry @4386427 I posted the wrong version of the function, but it does not solve the issue — LucaBonadia, Nov 04 '20 at 11:20
Yes but the errors are still detected by Flawfinder (this is why I haven't marked your answer as the correct one) — LucaBonadia, Nov 04 '20 at 11:26
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/224087/discussion-between-lucabonadia-and-4386427). — LucaBonadia, Nov 04 '20 at 11:28
Probably a "false positive". See https://stackoverflow.com/questions/59293533/a-flaw-reported-by-flawfinder-but-i-dont-think-it-makes-sense Anyway - you did get some bugs fixed ;-) — Support Ukraine, Nov 04 '20 at 11:40

Support Ukraine · Answer 1 · 2020-11-04T11:19:15.790

I see two places in your last code snippet where you don't handle return values correct. 1) You don't check the malloc 2) You don't check the read

Try:

char * buffer;
size_t len;
// my_fd is a file descriptor
ssize_t ret = read(my_fd, &len, sizeof(len));

if (ret != sizeof len) {
     buffer = NULL;
} else {
    buffer = malloc(len + 1);
    if (buffer != NULL)        // Check that malloc was ok
    {
         ret = read(my_fd, buffer, len);

         if (ret == -1)        // Check that read was ok
         {
             // error handling....
             //
             // for now just do:
             ret = 0;
         }
         else if (ret != len)
         {
             // Didn't get as much data as expected
             //
             // Add some error handling....
         }
         buffer[ret] = '\0';
    }
}
free(buffer);

Fix (CWE-120, CWE-20) detected by Flawfinder

1 Answers1