"the reduction function is called with num set to the bit size, where it should be number of BN_ULONG elements (which are always 8 bytes large, because that is the size of an unsigned long on x64 systems, which is the only architecture which can have AVX512 support). So with the input sizes being 1024 bits, 8192 bytes are accessed (read from or written to) instead of 128."
Really unfortunate that a performance optimization like this introduced RCE. Feels like something you would hope would be caught via the use of something like asan/msan or valgrind, at least it was caught relatively quickly after release via fuzzing.
Tracking units of measurement is so important for security (bits vs bytes vs BN_ULONGs). There was a bug a few years ago with Bouncy Castle using 16 bit vs 16 byte keys, IIRC, due to a similar mixup. Ideally units are captured in the type system, but I've found that even naming methods/fields appropriately (e.g. setTimeoutInSeconds(long) vs setTimeout(long)) goes a long way to reducing these kinds of bugs.
Math and crypto code are different from business logic. Under some circumstances, it's acceptable to use single-letter variable names. This often happens when the code implements a mathematical operation according to a standard formula.
For example, the following SHA-256 code comes from OpenBSD - a Unix derivative known for its focus on security and correctness.
do {
/* Rounds 0 to 15 (unrolled): */
ROUND256_0_TO_15(a,b,c,d,e,f,g,h);
ROUND256_0_TO_15(h,a,b,c,d,e,f,g);
ROUND256_0_TO_15(g,h,a,b,c,d,e,f);
ROUND256_0_TO_15(f,g,h,a,b,c,d,e);
ROUND256_0_TO_15(e,f,g,h,a,b,c,d);
ROUND256_0_TO_15(d,e,f,g,h,a,b,c);
ROUND256_0_TO_15(c,d,e,f,g,h,a,b);
ROUND256_0_TO_15(b,c,d,e,f,g,h,a);
} while (j < 16);
In this particular case, the code is a direct translation of math, following the paper's notations is preferable. The state variables are named a, b, c, d, e, f, g because they're what the authors of SHA-256 chose to call them. When writing this particular fragment of code, renaming the state variables to something else is not constructive and increases the chance of mistakes.
I haven't read the OpenSSL code in question so I cannot comment whether the uses of single-letter variables in your comment are appropriate. To make an educated guess, a, b, m, r are probably reasonable. The uses of carry, mask, num, tmp are potentially questionable, though.
In crypto code you often have conventions to name a certain value with a letter. E.g. RSA public keys consist of the values N, e, private keys also have d, p, q etc.
One may argue whether a, b and m are as "commonly known" for modular exponentiation. Still: There are definitely valid cases to use single letter variable names if they are the most well known "names" of certain values.
this comes from a culture of obscurity and cryptic writings.. thirty character variable names? no way ; single character variable names? terse no context variable names ? variables used in differerent local blocks with the same names? really, clueless ..
As they mentioned, it's unlikely to be hit by fuzzers when the flow first has to hit a specific math condition. Similar thing happens when you try to fuzz something protected by checksums, but at least there is known that you should patch them out first. Here it's much more tricky.
On Twitter they said they would have found it before release with fuzzing if they had access to an AVX-512 system. I'm not sure why they don't have one. In another place they said they got $10k in funding for fuzzing and systems with AVX-512 are 4¢/hour on EC2, and NUCs with the feature are < $500. Certainly if companies are going to fund this research it makes sense to make sure the fuzzing runs on the most common platform, and OpenSSL on server platforms with AVX-512 is an extremely common use case, as the feature has been universal on Intel server CPUs for over 5 years.
The post says "the vulnerability has only existed for a week." Many Linux systems don't yet have a 3.0.4 package available, but apparently Homebrew is already installing the affected version:
And on the other hand, "many Linux systems" (hello, all the containers with Ubuntu 16.04 as a base image!) continue to use 1.0.2g to this day. In practice, the main reason why these things get forcefully updated is because they stop working with the newly minted HTTPS keys/certs.
I've been replacing my OpenSSL installations with LibreSSL [1] where possible. I manage a lot of machines and it's just way nicer to not have to do patching whenever these things come up, with the peace of mind that there's not some little-used internal script or feature which ends in privilege escalation or whatever when it's supposed to be a SECURITY product first and foremost. Besides this bug I subscribe to Debian's security announcements list and CVE-2022-2068 was something that came up yesterday for patching [2]
I've heard some people moan that "it's not exactly a drop-in replacement" but I've yet to find in my own work a case where this is true.
The only off-putting thing to me is the fact that these OpenBSD projects love giving puffy lips for some reason.
"the reduction function is called with num set to the bit size, where it should be number of BN_ULONG elements (which are always 8 bytes large, because that is the size of an unsigned long on x64 systems, which is the only architecture which can have AVX512 support). So with the input sizes being 1024 bits, 8192 bytes are accessed (read from or written to) instead of 128."
Really unfortunate that a performance optimization like this introduced RCE. Feels like something you would hope would be caught via the use of something like asan/msan or valgrind, at least it was caught relatively quickly after release via fuzzing.
A good bit of news is that since this requires AVX512 many CPUs won't hit it, including new Intel chips: https://www.pcgamer.com/intel-kills-alder-lake-avx-512-suppo...