-1

I'm preparing for a computer security exam, and I found this question in previous exams

To create an authentication program, we write a check_auth function as follows:

int check_auth(char *password) { 
    int auth_flag = 0; 
    size_t n = 0;
    n = strlen(password);
    char pass[n+1];
    strcpy(pass, password);
    if (strcmp(pass, "abc123") == 0) 
        auth_flag = 1;
    return auth_flag;
}

The function strlen(str) returns the length of str (not including the terminating \0 character) and strcpy(dst, src) copies the string pointed to by src into dst, including the terminating \0 character. Is this code secure, or does it have memory safety issues?

I’m not sure about my opinion, can someone help me out?

4
  • Good Lord, that mess of a code is completely unfit for production. From a security point of view, but also just plain best practices. Is it supposed to be C or C++? Also, who has created that pointer? Is it guaranteed to be a null-terminated string by the surrounding application, or do we have to suspect that it is just a random adress from Evil corp?
    – nvoigt
    Commented Aug 18, 2021 at 17:45
  • I've tried to properly format the code and correct some obvious errors, like missing semicolon or "3" instead of new line - with these errors it would not even compile. Please check if this is really the intended code in question. Commented Aug 18, 2021 at 17:50
  • I think the code is in C. I think it is a random address from Evil corp. @nvoigt
    – Bella
    Commented Aug 18, 2021 at 18:01
  • I think the main problem they are asking about is the buffer overflow. but I'm not sure if the code is secure
    – Bella
    Commented Aug 18, 2021 at 18:03

1 Answer 1

1

If the pointer password is untrusted, you obviously run the risk of reading off the end of the string while trying to find the length, which can crash the process (segfault/access violation on read).

Worse, if it's potentially controlled by another thread/process, you have a Time-Of-Check-to-Time-Of-Use (TOCTOU) vulnerability, where the attacker can have the pointed-to string initially be short and then modify it (e.g. by overwriting a null with a non-null) between the calls to strlen and strcpy, resulting in an arbitrary-length buffer overflow with arbitrary data. However, this is extremely unlikely in real-world cases (it would require that password point to memory mapped into multiple process' address space, or similar). If it's another thread in the same process and the check is in user-mode, there's a reliability risk but not really a security risk (the other thread could just directly do anything that might be achieved via memory corruption in this function). However, if the check is in kernel mode but the pointer is in user mode, that's once again vulnerable as it exposes the risk of user-to-kernel EoP (this is one of several reasons it's very dangerous for kernel-mode code to ever directly process user-mode pointers).

If password is attacker-sourced but otherwise verified to be valid (e.g. it was read from a file but verified to be null-terminated and not modifiable after reading) then neither of the above threats apply, but there's an (unlikely) possibility of integer overflow. If the attacker can make password have a length of SIZE_MAX, then n+1 will wrap around to zero, and thus pass will have zero length and the strcpy will cause a buffer overflow. This is unlikely because it's usually not possible for a string to have a length of SIZE_MAX on any platform.

However, there's another risk. pass is a variable-length array (VLA) that gets allocated on the stack. Unlike heap-allocated arrays (using malloc or new), I don't believe allocation of such an array can ever fail... instead, if the array is too long, the program simply crashes when it tries to access an address below the bottom of the stack. Stacks are usually fairly small - one megabyte is common - and you can easily supply a string such that it just won't fit on the stack. Because this code tries anyhow, you'll get a (slightly unusual sort of) stack overflow. Usually there's a guard page (a page of memory not allocated to a physical address) at the bottom of the stack (remember, the stack grows downward, toward lower addresses) and if you try to write any data to that guard page the process will immediately segfault/AV with a stack overflow. If the buffer is huge enough, it might actually extend the array all the way down into other in-use memory (such as the heap) where it could mess with some other data structures before hitting an unallocated address; in the worst case this could overwrite the exception/signal handler in a way that causes the eventual SIGSEGV/AV to result in arbitrary code execution. Here's an example of a VLA overflowing the stack.


I think that's it for actual memory safety vulns. However, there are definitely other problems here. For example, hardcoded passwords are always a bad idea.

Furthermore, strcmp is an early-exit function - it'll return immediately upon detecting a mismatch in the input strings - which means if you have very fine timing measurements (or the ability to run the test many times) you can detect how many bytes were compared successfully. Thus you can brute-force the password one byte at a time, making the total effort linear rather than exponential to the length of the string. This is only relevant if you can try many times (and measure timing pretty sensitively) though.

Beyond that, it's just bad code. While a good optimizing compiler would reduce this to a fraction of its total "lines" (e.g. since auth_flag is only used to store the return value, there's no point allocating and initializing memory for it; just return !strcmp(...), it's weird and harder to read than is really justified.

3
  • Last but not least, VLA are only an optional feature. If an implementation chooses not to support them, the pass array will have a 0 size. Commented Aug 19, 2021 at 14:07
  • @SergeBallesta Are they allowed to compile without error in that case? I'd expect that, if the feature isn't supported, either the compiler is old enough to treat a non-constant length as a syntax error, or to raise an error about using an unsupported/disabled feature.
    – CBHacking
    Commented Aug 20, 2021 at 6:08
  • I can remember tons of errors caused by trying to use VLA arrays on systems that did not support them... And after reading again the standard, I could not find any hint that a diagnostic is required. Commented Aug 20, 2021 at 8:09

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .