Thread: Making Vulnerable Program Not Vulnerable

I suggest avoiding the use of magic numbers can often make the code less likely to have buffer over runs.

Tim S.

Originally Posted by TheCMan

It appears that I need to use strncpy instead of strcpy in order to put an upper bound on the characters that can be copied..?

No, you don't. The reason why we would use strcpy or strncpy (and other related alternatives) is that we have an original string that is not to be modified, and yet we want to modify it, so we copy the string to a buffer and modify the buffer instead. In this case, the original string argv[1] isn't modified, and even if it were modified, there's no issue modifying it, hence you don't need to copy in the first place.

Another thing: you need to check that the user did in fact supply a command line argument, otherwise argv[1] would likely be a null pointer instead of a string.

Okay. So I tell you how to avoid the vulnerability (don't copy in the first place), and your response is to tell me that the use of strcpy is a vulnerability in this case when I told you that in post #6. *shrugs*

I already told you how to fix it: don't copy in the first place! Directly use argv[1]. You cannot have a buffer overflow vulnerability with strcpy when you never call strcpy at all.

You could fix the vulnerability by changing to strncpy, that's true. But it comes with two disadvantages: you would be making an unnecessary assumption concerning the maximum length of user input, and you would need to get the strncpy call correct (not hard, but mistakes can and do happen).

Originally Posted by stahta01

I suggest avoiding the use of magic numbers can often make the code less likely to have buffer over runs.

Tim S.

I can not believe laserlight missed the hidden buffer overrun.

Tim S.

Originally Posted by TheCMan

I guess I'm not understanding what you mean when you say use argv[1]. Use it where?

Use it instead of buffer.

Originally Posted by stahta01

I can not believe laserlight missed the hidden buffer overrun.

No, I didn't; I just didn't bother mentioning it in post #6. Did you read my post #8?

Originally Posted by TheCMan

So can anyone help me get the code to not be vulnerable?

Aren't we helping you? What is your current code?

Also, I note that you didn't state what the code was actually supposed to do, so we can only guess based on the code itself. This leads to the curious situation where it looks like you want to reinterpret the bytes to print an unsigned int, yet you use a long* and have a comment about "four-byte word". It makes me wonder if you should just stick to unsigned int or unsigned long with sizeof.

Originally Posted by TheCMan

The current code is posted at the beginning.

Which means that you have done nothing to improve it despite suggestions?

Originally Posted by TheCMan

And I said what the code was supposed to do- to not be vulnerable to stack buffer overflow attack.

That's not what the code is supposed to do in the sense of functional requirements. That's a non-functional requirement, as important as it may be. If it were a functional requirement, then we could change the code to:

Code:

int main(void) { return 0; }

and it would be correct... which I think you would agree is absurd.

Originally Posted by TheCMan

Does anyone want to post a solution, using the current code I have so that I can test it?

Nope, not yet. Improve your code based on suggestions and post your updated code first. I actually wrote it for fun yesterday, but I decided not to post it partly because I noticed the long vs unsigned int situation.

Originally Posted by TheCMan

No, it means your suggestions haven't been helpful. I don't need an explanation on what is and isn't a functional requirement. The question isn't about the function of the code, it's about the vulnerability of the code, which I guess you are yet to understand. The question is how to prevent the method from being vulnerable to stack buffer overflow attacks. I don't need suggestions, I need a solution and to understand why the solution is not vulnerable.

No, I understand this perfectly fine. Since you consider yourself too stupid to make use of suggestions, and I'm getting tired of arguing, here's your solution:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(int argc, char **argv)
{
    if (argc <= 1)
    {
        fprintf(stderr, "Error: please provide one command line argument.\n");
        return EXIT_FAILURE;
    }

    size_t len = strlen(argv[1]);
    if (len % sizeof(unsigned int) != 0)
    {
        fprintf(stderr,
                "Error: the input must be a multiple of %zu characters.\n",
                sizeof(unsigned int));
        return EXIT_FAILURE;
    }

    unsigned int *addr_ptr = (unsigned int*)argv[1];
    for (size_t i = 0; i < len / sizeof(unsigned int); ++i)
    {
        printf("%02zu:%p:%08x\n", i, (void*)addr_ptr, *addr_ptr);
        addr_ptr++;
    }

    return 0;
}