4

Fortify reported a buffer overflow vulnerability in below code citing following reason -

In this case we are primarily concerned with the case "Depends upon properties of the data that are enforced outside of the immediate scope of the code.", because we cannot verify the safety of the operation performed by memcpy() in abc.cpp

void create_dir(const char *sys_tmp_dir,  const char *base_name,
                size_t     base_name_len)
{
    char        *tmp_dir;
    size_t      sys_tmp_dir_len;

    sys_tmp_dir_len = strlen(sys_tmp_dir);

   tmp_dir = (char*) malloc(sys_tmp_dir_len + 1 + base_name_len + 1);
   if(NULL == tmp_dir) 
     return;

memcpy(tmp_dir, sys_tmp_dir, sys_tmp_dir_len);
tmp_dir[sys_tmp_dir_len] = FN_LIBCHAR;
memcpy(tmp_dir + sys_tmp_dir_len + 1, base_name, base_name_len);
tmp_dir[sys_tmp_dir_len + base_name_len + 1] = '\0';
..........
..........

}

It appears to me a false positive since we are getting the size of data first, allocating that much amount of space, then calling memcpy with size to copy. But I am looking for good reasons to convince fellow developer to get rid of current implementation and rather use c++ strings. This issue has been assigned to him. He just sees this a false positive so doesn't want to change anything.

Edit I see quick, valid criticism of the current code. Hopefully, I'll be able to convince him now. Otherwise, I'll hold the baton. :)

Improve this question
6
  • 2
    I guess the warning is because you're relying on base_name_len being truthful. As it says, that's not being enforced in the scope of this code. Using something more sensible like std::string would fix that. Commented Apr 16, 2015 at 15:25
  • 1
    Unless there's a reason to think that base_name is not a null-terminated string, you could use memcpy(tmp_dir + sys_tmp_dir_len + 1, base_name, base_name_len + 1); so memcpy() null terminates the string. If base_name is not accurate or is not a null-terminated string, then your existing code is safer. I don't see a problem with buffer overflow if you check the result of malloc() — but if you don't think you can trust the strings being passed, then you should check them for nullness. You can't check them for validity otherwise in a reliable, portable way. Commented Apr 16, 2015 at 15:26
  • 2
    You've got 8 lines of ugly code there and still need a free, and you can replace it with a std::string one-liner making it exception-safe in the process, plus you know it'll shut up a false positive in your tool - what more reason do you need? If you want to use C, asprintf() if it suits your portability needs... concise code can hide fewer bugs and obfuscate less meaningful logic. Commented Apr 16, 2015 at 15:26
  • 2
    It's funny that there are already 3 valid criticisms (Mike Syemour's comment on base_name_len, Adriano Repetti's comment on trusting sys_tmp_dir and Matt's comment on malloc failing, in just 20 minutes. Commented Apr 16, 2015 at 15:42
  • 1
    @MSalters Yeah! Sad part is developer who is assigned to fix the problem is not ready to re-factor the code. He simply sees this a false positive Commented Apr 16, 2015 at 15:45

2 Answers 2

Reset to default
7

Take a look to strlen(), it has input string but it has not an upper bound then it'll go on searching until it founds \0. It's a vulnerability because you'll perform memcpy() trusting its result (if it won't crash because of access violation while searching). Imagine:

create_dir((const char*)12345, baseDir, strlen(baseDir));

You tagged both C and C++...if you're using C++ then std::string will protect you from these issues.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

True, that's another vulnerability. Will that cause buffer overflow ? What will you call this new vulnerability ?
I fail to see how the strlen() can lead to a buffer overflow. It can return a very large number which could cause malloc() to fail and lead to a segmentation fault, but the size of the allocated buffer, if it does not fail, will always be large enough to accommodate the write. That said, if it is C++ code, I would definitely use std::string and if it is C code, I would use asfprintf(). As written, it makes my head hurt -- overly complex for the task. (I note that the original question has been edited to add a NULL test on malloc() now - I stand by the overly complex statement)
@kscottpiel suppose input string pointer for not point to a string but a code section. What will happen? What if pointer is just garbage (on purpose)? Result depends on OS (and deamons in your nose). Access violation, trap... Reading is not (much) more safe than writing because it may cause a write elsewhere. If this is done in drivers it's even worse.
4

It appears to me a false positive since we are getting the size of data first, allocating that much amount of space

This assumption is a problem that matches the warning/error. In your code, you're assuming that malloc successfully allocated the requested memory. If your system has no memory to spare, malloc will fail and return NULL. When you try to memcpy into tmp_dir, you'd be copying to NULL which would be bad news.

You should check to guarantee that the value returned by malloc is not NULL before considering it as a valid pointer.

Improve this answer

1 Comment

Thanks. That NULL check had already been added in code. I missed to add it. If memory allocation fails then simply returning for now.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.