1

I'm new to secure code review. I know that strlen will calculates the length until it finds a null character. This is a part of a larger code.

char* executeMount(char* password, char* path, int unmountOrMount)
 {
 char* catString = new char[(strlen("echo|set /p password=") + strlen(password) + strlen(" | runas /user:administrator \"mountvol ") + strlen(path) + 1)];
 strcpy(catString, "echo|set /p password=");
 strcat(catString, password);
 strcat(catString, " | runas /user:administrator \"mountvol ");
 strcat(catString, path);
 //Equivalent command: system("echo|set /p password=" + adminPassword + " | runas /user:administrator mountvol" + path);
 return catString;
 }

Will return catString lead to a buffer overflow?

Strlen will calculate all the strings until it reaches a null terminator. So catString is the total length of all the strings + 1 (for null terminator). strcpy will copy echo|set /p password=" + null terminator. Following strcat will remove the null terminator and concatenate "password" but the subsequent strcat will not remove the null terminator leading to buffer overflow? Is my thinking correct? Is this code vulnerable to any other threat?

4
  • @Rocelle - If you have access to the source code could you modify your post to include it as preformatted text instead of an image?
    – Hector
    Jan 8, 2018 at 11:58
  • @Rocelle - I may be missing something here. Why is the strlen incorrect?
    – Hector
    Jan 8, 2018 at 12:43
  • 1
    @Rocelle - The "+ 1" at the end of the catString = new... seems to add space for the null terminator. Saying this there is a risk that the input (password or path) is not null terminated - but that will lead to copying arbitrary data and/or a segfault from attempting to access data it shouldn't as opposed to a buffer overflow.
    – Hector
    Jan 8, 2018 at 12:51
  • i've updated my answer.
    – Hector
    Jan 8, 2018 at 13:03

2 Answers 2

2

You're using C++ (because you couldn't use new char if you weren't); you should really be using std::string here, including as the return value. You can then dereference .c_str to get a const char * pointer if you need that in your upstream code.

You don't want to just return the std::string.c_str from your function however; the std::string will go out of scope and you'll be invoking undefined behavior.

That avoids having to deal with any of this character-counting stuff.

1
  • 2
    @Joe - maybe reclarify. Whilst you have stated to return a std::string its probably best to make it explicitly clear that you can't use std::string and then return .c_str.
    – Hector
    Jan 8, 2018 at 13:05
1

Will return catString lead to a buffer overflow due to the incorrect strlen?

From a quick skim the string length looks OK. The only risk seems to be if the parameters are not null terminated - there it would lead to whatever follows password/path being dumped into the command line.

Are there any other threats in this code?

There are a lot of problems with this code - but buffer overflow due to the catString allocation length isn't one of them.. What happens when the user enters in a valid path followed by "; rmdir C:\Windows /s"? Or any other command.

As @Joe points out this should probably use std::string rather than character arrays (including returning one). If you are to use character arrays the pointers should be marked as const to enforce not modifying them within the function and allow compiler optimisations. Password & path need to be properly quoted and escaped to stop command injection (and allow passwords with spaces and special characters). I also think you should be using permissions to allow the application to call mountvol directly and not having to pass through an administrator account password to it.

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.