Astute Explorer (GCHQ Challenge 5 – 10)

Continuation for http://www.malwaretech.com/2014/09/astute-explorer-gchq-challenge-1-5.htmlVulnerability
On line 26 the function fails if exactly BLOCK_SIZE is not read, this means if there is data available but less than BLOCK_SIZE is present, or the read fails, the function will return NULL. On failure the function does not free szBuffer so there’s a pretty serious memory leak.

Solution
If the read operation fails, the function should free(szBuf) before returning null, it should also be worth considering handling the event that the read function returns less than BLOCK_SIZE.

Vulnerability
I have a hard time understanding the point of this function (it reads the data to a local buffer, which is then disregarded on return), but I’m sure making the function useful is not part of the assignment. The problem exists on line 1009; assuming GetFromInput can read more than 1 byte at a time, it can still exceed MAX_RECEIVE. For example: if MAX_RECEIVE is 10 and GetFromInput reads 20 bytes, siBytesReceived is going to be 20, the loop will exit but the data will have already been written and siBytesRecieved will already have exeeded the limit. There’s also the problem that if GetFromInput can fail or return less than MAX_RECEIVE, the loop has no way of checking this and will continue looping (possibly infinitely).

Solution
The best idea would be to implement a parameter in GetFromInput that allows the user to specify the maximum amount of data to read in a single call. The function can then calculate how much data is left before MAX_RECEIVE is hit and specify a limit to prevent more than that from being read.

Vulnerability
I wasn’t able to find any vulnerabilities here, unless the user supplies invalid pointers to the function (it should be their job to check they’re valid, not the functions). On line 45 the loop will decrement len until it’s 0 then exit, as a result it will always return 0.

Solution
Make a copy of len and then decrement the copy.

Vulnerability
If the aim of this code was to use obscure nested if/else statements to make code auditing almost impossible, then the programmer (who probably works on the security team at oracle) did a great job. I’m really not sure what’s going on with the code or what err is and where it’s set. Assuming err is an actual variable and not pseudo-code a runtime library function like malloc wouldn’t set it; if malloc returns NULL the application is going to try and use that null pointer. There’s also the issue of the use-after-free on line 52 (On error ptr is freed and abrt is set, which means logError will always be passed ptr after it has been freed).

Solution
Stop hiring college kids.

Vulnerability
This piece of code should be instantly recognizable as the apple SSL bug from February (it was all over the news and security sites for months). The extra goto fail; on line 408 means the application will always skip to the cleanup code without setting err, as a result the client doesn’t verify that server owns the private key matching the certificate, which opens the client up to MITM attacks.

Solution
Remove the extra goto fail;

Conclusion

Although the challenges are fun, they are very confusing at times. Most of the code has no context given, leaving the player making massive assumptions about how the code works (I can’t think of a real world scenario where you would have to find vulnerabilities in tiny snippet of code without knowing what they do or how the application uses them). Almost all of the vulnerabilities in this article could be non existent if the application performs check prior to calling the snippets, or if assumptions made about the imaginary functions they call are wrong.