Fix undefined behaviour in e_aes_cbc_hmac_sha256.c and e_aes_cbc_hmac… #3832
Conversation
…_sha1.c
In TLS mode of operation the padding value "pad" is obtained along with the
maximum possible padding value "maxpad". If pad > maxpad then the data is
invalid. However we must continue anyway because this is constant time code.
We calculate the payload length like this:
inp_len = len - (SHA_DIGEST_LENGTH + pad + 1);
However if pad is invalid then inp_len ends up -ve (actually large +ve
because it is a size_t).
Later we do this:
/* verify HMAC */
out += inp_len;
len -= inp_len;
This ends up with "out" pointing before the buffer which is undefined
behaviour. Next we calculate "p" like this:
unsigned char *p =
out + len - 1 - maxpad - SHA256_DIGEST_LENGTH;
Because of the "out + len" term the -ve inp_len value is cancelled out
so "p" points to valid memory (although technically the pointer arithmetic
is undefined behaviour again).
We only ever then dereference "p" and never "out" directly so there is
never an invalid read based on the bad pointer - so there is no security
issue.
This commit fixes the undefined behaviour by ensuring we use maxpad in
place of pad, if the supplied pad is invalid.
With thanks to Brian Carpenter for reporting this issue.
mattcaswell
added
branch: 1.0.2
richsalz
added
the
approval: done
Question is if this gives away information. I mean let's say you corrupt short packet. Then by noting that it took longer time you might be able to judge how it was corrupted... I think I'd like to ask for some time to ponder over this... |
|
This specific commit is constant time in both paths if I am reading it correctly. Where do you see a non-constant time error path @dot-asm ? |
|
All paths are constant time. I think the concern is the change in length of data processed altering the execution time. |
My understanding of the existing code is that it is constant time no matter what the value of the padding. It always loops over maxpad bytes to check the padding regardless of what the actual value of pad is. If you corrupt a packet you will get a different value for pad than the original valid packet. But the padding check should still take exactly the same amount of time. So corrupting the packet should give you no advantage, and changing an invalid pad value to be maxpad in this code should also make no difference to the total execution time. |
|
I know what code supposed to do (I couldn't not know), but sometimes you have to look from grander perspective, i.e. beyond specifically affected lines. That's what I'd like to and am doing... |
|
What originally triggered my mind was not being sure if it doesn't effectively make payload longer, thus tricking it into processing more data [and consequently increasing processing time]. Another trigger is that 'inp_len' is cryptographically significant in sense that it's taken into consideration when calculating MAC. 'pad' is also used later on to verify padding... Commit message reads 'inp_len = len - (SHA_DIGEST_LENGTH + pad + 1);' and then 'out += inp_len;'. But there is extra step after inp_len assignment... I'm not saying that any of these triggers are valid, these are just things I'd like to and am double-checking... |
crypto/evp/e_aes_cbc_hmac_sha1.c
Outdated
| * we'll use the maxpad value instead of the supplied pad to make | ||
| * sure we perform well defined pointer arithmetic. | ||
| */ | ||
| pad = constant_time_select(mask, pad, maxpad); | ||
|
|
||
| inp_len = len - (SHA_DIGEST_LENGTH + pad + 1); | ||
| mask = (0 - ((inp_len - len) >> (sizeof(inp_len) * 8 - 1))); |
There was a problem hiding this comment.
In the nutshell the root to this and[!] previous problem (yes, the CVE-2016-2107 thing) is that this line doesn't serve the intended purpose of checking if previous operation carried. Simply put outcome of this operation is constant and is all ones. It's possible to a) remove this and following two lines [with rationale that above three statements/expressions do the job]; b) fix this check [e.g. by simply removing - len exploiting the fact that len is known to be small, more specifically limited by TLS record size] instead of suggested modification [in which case it would also be possible to remove even preceding ret &= thing].
There was a problem hiding this comment.
To summarize, all of the previously mentioned concerns turned to be imaginary, but it wouldn't be inappropriate to not limit ourselves to one-liner.
There was a problem hiding this comment.
There's too many negatives in that sentence for me to parse it "would NOT be INappropriate to NOT.."! Are you saying that the one liner is correct but there might be a better fix? If so would you like to create a PR for it?
There was a problem hiding this comment.
b) fix this check [e.g. by simply removing - len exploiting the fact that len is known to be small, more specifically limited by TLS record size]
My bad! This is not the right way to fix it!
There was a problem hiding this comment.
Are you saying that the one liner is correct but there might be a better fix?
I'm saying that it wouldn't be inappropriate to either a) remove some dead code in the process or b) solve it alternatively. By too many negatives I tried to say that I didn't put value, i.e. better or worse, into either option [at that point]...
The intention of the removed code was to check if the previous operation carried. However this does not work. The "mask" value always ends up being a constant and is all ones - thus it has no effect. This check is no longer required because of the previous commit.
|
New commit added to remove the dead code. Ping @dot-asm. |
…_sha1.c
In TLS mode of operation the padding value "pad" is obtained along with the
maximum possible padding value "maxpad". If pad > maxpad then the data is
invalid. However we must continue anyway because this is constant time code.
We calculate the payload length like this:
inp_len = len - (SHA_DIGEST_LENGTH + pad + 1);
However if pad is invalid then inp_len ends up -ve (actually large +ve
because it is a size_t).
Later we do this:
/* verify HMAC */
out += inp_len;
len -= inp_len;
This ends up with "out" pointing before the buffer which is undefined
behaviour. Next we calculate "p" like this:
unsigned char *p =
out + len - 1 - maxpad - SHA256_DIGEST_LENGTH;
Because of the "out + len" term the -ve inp_len value is cancelled out
so "p" points to valid memory (although technically the pointer arithmetic
is undefined behaviour again).
We only ever then dereference "p" and never "out" directly so there is
never an invalid read based on the bad pointer - so there is no security
issue.
This commit fixes the undefined behaviour by ensuring we use maxpad in
place of pad, if the supplied pad is invalid.
With thanks to Brian Carpenter for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #3832)
The intention of the removed code was to check if the previous operation carried. However this does not work. The "mask" value always ends up being a constant and is all ones - thus it has no effect. This check is no longer required because of the previous commit. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3832)
…_sha1.c
In TLS mode of operation the padding value "pad" is obtained along with the
maximum possible padding value "maxpad". If pad > maxpad then the data is
invalid. However we must continue anyway because this is constant time code.
We calculate the payload length like this:
inp_len = len - (SHA_DIGEST_LENGTH + pad + 1);
However if pad is invalid then inp_len ends up -ve (actually large +ve
because it is a size_t).
Later we do this:
/* verify HMAC */
out += inp_len;
len -= inp_len;
This ends up with "out" pointing before the buffer which is undefined
behaviour. Next we calculate "p" like this:
unsigned char *p =
out + len - 1 - maxpad - SHA256_DIGEST_LENGTH;
Because of the "out + len" term the -ve inp_len value is cancelled out
so "p" points to valid memory (although technically the pointer arithmetic
is undefined behaviour again).
We only ever then dereference "p" and never "out" directly so there is
never an invalid read based on the bad pointer - so there is no security
issue.
This commit fixes the undefined behaviour by ensuring we use maxpad in
place of pad, if the supplied pad is invalid.
With thanks to Brian Carpenter for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #3832)
(cherry picked from commit 335d0a4)
The intention of the removed code was to check if the previous operation carried. However this does not work. The "mask" value always ends up being a constant and is all ones - thus it has no effect. This check is no longer required because of the previous commit. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3832) (cherry picked from commit d5475e3)
…_sha1.c
In TLS mode of operation the padding value "pad" is obtained along with the
maximum possible padding value "maxpad". If pad > maxpad then the data is
invalid. However we must continue anyway because this is constant time code.
We calculate the payload length like this:
inp_len = len - (SHA_DIGEST_LENGTH + pad + 1);
However if pad is invalid then inp_len ends up -ve (actually large +ve
because it is a size_t).
Later we do this:
/* verify HMAC */
out += inp_len;
len -= inp_len;
This ends up with "out" pointing before the buffer which is undefined
behaviour. Next we calculate "p" like this:
unsigned char *p =
out + len - 1 - maxpad - SHA256_DIGEST_LENGTH;
Because of the "out + len" term the -ve inp_len value is cancelled out
so "p" points to valid memory (although technically the pointer arithmetic
is undefined behaviour again).
We only ever then dereference "p" and never "out" directly so there is
never an invalid read based on the bad pointer - so there is no security
issue.
This commit fixes the undefined behaviour by ensuring we use maxpad in
place of pad, if the supplied pad is invalid.
With thanks to Brian Carpenter for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #3832)
(cherry picked from commit 335d0a4)
The intention of the removed code was to check if the previous operation carried. However this does not work. The "mask" value always ends up being a constant and is all ones - thus it has no effect. This check is no longer required because of the previous commit. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #3832) (cherry picked from commit d5475e3)
|
Pushed. Thanks. |
…_sha1.c
In TLS mode of operation the padding value "pad" is obtained along with the
maximum possible padding value "maxpad". If pad > maxpad then the data is
invalid. However we must continue anyway because this is constant time code.
We calculate the payload length like this:
inp_len = len - (SHA_DIGEST_LENGTH + pad + 1);
However if pad is invalid then inp_len ends up -ve (actually large +ve
because it is a size_t).
Later we do this:
/* verify HMAC */
out += inp_len;
len -= inp_len;
This ends up with "out" pointing before the buffer which is undefined
behaviour. Next we calculate "p" like this:
unsigned char *p =
out + len - 1 - maxpad - SHA256_DIGEST_LENGTH;
Because of the "out + len" term the -ve inp_len value is cancelled out
so "p" points to valid memory (although technically the pointer arithmetic
is undefined behaviour again).
We only ever then dereference "p" and never "out" directly so there is
never an invalid read based on the bad pointer - so there is no security
issue.
This commit fixes the undefined behaviour by ensuring we use maxpad in
place of pad, if the supplied pad is invalid.
With thanks to Brian Carpenter for reporting this issue.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from openssl#3832)
(cherry picked from commit 335d0a4)
The intention of the removed code was to check if the previous operation carried. However this does not work. The "mask" value always ends up being a constant and is all ones - thus it has no effect. This check is no longer required because of the previous commit. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from openssl#3832) (cherry picked from commit d5475e3)
…_sha1.c
In TLS mode of operation the padding value "pad" is obtained along with the
maximum possible padding value "maxpad". If pad > maxpad then the data is
invalid. However we must continue anyway because this is constant time code.
We calculate the payload length like this:
However if pad is invalid then inp_len ends up -ve (actually large +ve
because it is a size_t).
Later we do this:
This ends up with "out" pointing before the buffer which is undefined
behaviour. Next we calculate "p" like this:
Because of the "out + len" term the -ve inp_len value is cancelled out
so "p" points to valid memory (although technically the pointer arithmetic
is undefined behaviour again).
We only ever then dereference "p" and never "out" directly so there is
never an invalid read based on the bad pointer - so there is no security
issue.
This commit fixes the undefined behaviour by ensuring we use maxpad in
place of pad, if the supplied pad is invalid.
With thanks to Brian Carpenter for reporting this issue.