OpenSSL undefined pointer arithmetic
Posted on 30 November -0001
<HTML><HEAD><TITLE>OpenSSL undefined pointer arithmetic</TITLE><META http-equiv="Content-Type" content="text/html; charset=utf-8"></HEAD><BODY>Hi, More off Twitter: <guidovranken> @solardiz Here is another very recent OpenSSL CVE that hasnt been released officially yet https://github.com/openssl/openssl/commit/a004e72b95835136d3f1ea90517f706c24c03da7 | Avoid some undefined pointer arithmetic | | A common idiom in the codebase is: | | if (p + len > limit) | { | return; /* Too long */ | } | | Where "p" points to some malloc'd data of SIZE bytes and | limit == p + SIZE | | "len" here could be from some externally supplied data (e.g. from a TLS | message). | | The rules of C pointer arithmetic are such that "p + len" is only well | defined where len <= SIZE. Therefore the above idiom is actually | undefined behaviour. | | For example this could cause problems if some malloc implementation | provides an address for "p" such that "p + len" actually overflows for | values of len that are too big and therefore p + len < limit! | | Issue reported by Guido Vranken. | | CVE-2016-2177 The commit message above gives pointer wraparound as an example of when and how this UB could manifest itself, but I think even more likely is that an optimizing C compiler would remove the check because it can't be reliably true (it can be either false or UB). A valid pointer is at most one element beyond the end of an object, so in the example given above "p + len > limit" is never reliably true. In the actual code being patched, there are different instances of the problem, and some of the checks look like they're effectively ">= limit" rather than "> limit". Those ">=" checks are more lucky, as they can't be completely removed (but can still misbehave if the pointer advances further). In fact, there are so many instances that someone should re-review the patch and possibly look for even more instances of the problem (maybe in an automated way different from what might have been used so far). The commit has "Reviewed-by: Rich Salz", which is great, but I think it needs more eyes than the committer's and one other person's. Alexander </BODY></HTML>
