2

I performing the code analysis of my embedded C code with SonarQube and RATS (Rough Auditing Tool for Security).

Under Ubuntu Shell, I execute

rats --quiet --xml -w 1 . > ./rats_report.xml

to get the the report that will be imported into SonarQube.

I get some errors like this:

Extra care should be taken to ensure that character arrays that are allocated on the stack are used safely. They are prime targets for buffer overflow attacks.

This is a snippet code of the function that generates the error:

static char* GetQueryStringForValue( const char* valueLabel )
{
  static char queryString[QUERY_LEN + 1];

  memcpy( queryString, '\0', sizeof(queryString) );
  snprintf( queryString, sizeof(queryString), "{'%s'", valueLabel );

  return queryString;
}

I understand that the problem is related to the buffer allocated into the stack.

My question is: which is the best practice to prevent buffer overflow attacks?

Should I add particular controls?

Thanks for the help!

BR, Federico

G. Ann - SonarSource Team
  • 22,346
  • 4
  • 40
  • 76
Federico
  • 1,117
  • 6
  • 20
  • 37
  • 3
    `memcpy`? Are you thinking of `memset`? `memcpy` is expecting a pointer for the second argument, not the integer `'\0'`. – Weather Vane Aug 25 '17 at 12:39
  • 3
    Strangely `static char queryString[QUERY_LEN + 1];` *isn't* allocated on the stack. – Weather Vane Aug 25 '17 at 12:42
  • Before you worry about security and static analysis, make sure that the code actually compiles and gives the expected results... – Lundin Aug 25 '17 at 13:57

1 Answers1

4

That's a false positive, nothing is allocated "on the stack" here. With the static storage class specifier, queryString has static storage duration which means it exists during the entire execution time of your program. No implementation of C would place such an object on the stack.

But this function is still very wrong:

memcpy( queryString, '\0', sizeof(queryString) );

This is attempting to dereference a NULL pointer (the NUL character constant is implicitly converted to a NULL pointer). What you probably meant is

memset(queryString, 0, sizeof queryString);

That said, if you still receive this warning, take it as what it is: a warning. It warns you to take extra care. With the code fixed to use memset(), there's no way to overflow your queryString here.

Your code has something different to worry about: It's not thread safe, due to the use of a static variable. It would probably be better to let the caller provide the buffer for the queryString.

  • Thanks Felix for the explanation. So the best way to initialize a string is to use the memset and the '0' values. I used the memcpy because, I though, the in this way I always have a string with the terminator and if I use the strcpy my string in safe because I have already the terminator. – Federico Aug 25 '17 at 12:47
  • @Federico the cheapest way to initialise the empty string is to write `0` to its first byte. – Weather Vane Aug 25 '17 at 12:53
  • 2
    @Federico: `snprintf()` terminates the `char`-array for you, so just drop this `mem*()` thingy. – alk Aug 25 '17 at 12:53
  • Yes the best practice is use the snprintf. Is more secure. just for my understanding, if I write 0 on the first string byte and I use the strcpy, I can fall into an buffer overflow error. So the best practice is initialize the first byte to zero and use snprintf – Federico Aug 25 '17 at 12:57
  • 1
    @Federico *if I write 0 on the first string byte and I use the strcpy, I can fall into an buffer overflow error. So the best practice is initialize the first byte to zero and use snprintf* First, if you're using `strcpy()` or `snprintf()` to copy a string to a buffer, there's no need to initialize it with any type of zero as `strcpy()` and `snprintf()` both completely overwrite the buffers previous contents up to the length of the buffer or the terminating `\0` character in the copied string. Second, you can also use the C-standard function `strncpy()` to safely copy a string into a buffer. – Andrew Henle Aug 25 '17 at 13:01
  • 2
    @AndrewHenle be careful with `strncpy()`, it has some *surprising* semantics. The most problematic part is that there's no `NUL` terminator written to the destination if it isn't among the first `n` characters of the source. This is kind of a *poisonous* function. –  Aug 25 '17 at 13:03
  • 1
    @AndrewHenle `strncpy()` should never be used in mission-critical systems, if at all. It's a wide-spread, incorrect myth that it is some sort of safer version of `strcpy()`. See [Why are strlcpy and strlcat considered insecure?](https://stackoverflow.com/questions/2114896/why-are-strlcpy-and-strlcat-considered-insecure). – Lundin Aug 25 '17 at 13:59