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.