Fix crash(es) in RSA_padding_add_PKCS1_PSS_mgf1#2699
Fix crash(es) in RSA_padding_add_PKCS1_PSS_mgf1#2699guidovranken wants to merge 2 commits intoopenssl:masterfrom guidovranken:RSA_padding_add_PKCS1_PSS_mgf1-fix
Conversation
…o integer addition or subtraction results in overflows or OOB memory access
|
This affects both 1.0.2 and 1.1.0. |
richsalz
added
branch: 1.0.2
levitte
left a comment
levitte
left a comment
There was a problem hiding this comment.
Other than the nit pick, looks fine to me
crypto/rsa/rsa_pss.c
Outdated
| emLen < (hLen + sLen + 2)) { | ||
| RSAerr(RSA_F_RSA_PADDING_ADD_PKCS1_PSS_MGF1, | ||
| RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE); | ||
| RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE); |
There was a problem hiding this comment.
Nit pick: remove the extra indent
There was a problem hiding this comment.
Doesn't now look the RSA_padding_add_PKCS1_PSS_mgf1 look completely different than
RSA_verify_PKCS1_PSS_mgf1 ?
| if (RAND_bytes(salt, sLen) <= 0) | ||
| goto err; | ||
| } | ||
| if (hLen < 1 || hLen - 1 > emLen) |
There was a problem hiding this comment.
I don't understand the condition. Can you explain what you want to test here?
There was a problem hiding this comment.
The next line is emLen - hLen - 1.
So let's say that hLen is 0. Subtracting 1 from it makes it negative. This has to be avoided. This explains the first condition.
As for the second condition, let's say hLen - 1 resolves to a value larger than emLen. Then the computation emLen - hLen - 1 yields a negative value. Perhaps I should change the > in the second condition to >=.
Note: I didn't check whether any of my checks can be true in any case. For example I didn't check if hLen can actually be 0 at this point. I implemented the checks so that there can be no doubt over over/underflows.
There was a problem hiding this comment.
so you want to predict if "emLen - hLen - 1" will be negative.
but algebraically this is transforms like follows:
emLen - hLen - 1 < 0
emLen < hLen + 1
IOW hLen + 1 > emLen
There was a problem hiding this comment.
But we not concerned about possible overflow here, I guess?
If that is the case all the algebra here wins nothing, and it would be
far more easy to test the condition directly on the result.
| emLen--; | ||
| } | ||
| if (sLen == RSA_PSS_SALTLEN_MAX) { | ||
| if (hLen < 2 || hLen - 2 > emLen) |
There was a problem hiding this comment.
Here as well, maybe you meant hLen + 2 > emLen ?
| goto err; | ||
| sLen = emLen - hLen - 2; | ||
| } else if (emLen < (hLen + sLen + 2)) { | ||
| } else if (sLen > INT_MAX - 2 - hLen || |
There was a problem hiding this comment.
sLen needs to be much smaller, anything larger than emLen is obviously wrong.
| goto err; | ||
| } | ||
|
|
||
| /* At this point, sLen is >= 0 */ |
There was a problem hiding this comment.
Yes, that should be the case, but what happens if you try this:
openssl genrsa -out rsa.pem 512
openssl dgst -sign rsa.pem -sha512 -sigopt rsa_padding_mode:pss -hex < /dev/null
|
ping @levitte and @bernd-edlinger . status? |
|
Thanks for the info! Closing. |
4 participants
Checklist
Description of change
This patch adds a number of checks that ought to ensure that there is not a single addition or subtraction operation in
RSA_padding_add_PKCS1_PSS_mgf1that results in unwanted behavior.For example, before this fix
results in a crash if private.pem is a serialization of an RSA structure whose "N" parameter is "1".