ext/openssl: openssl_encrypt() zend mm heap overflow on AES-WRAP-PAD …#22187
ext/openssl: openssl_encrypt() zend mm heap overflow on AES-WRAP-PAD …#22187devnexen wants to merge 3 commits into
Conversation
|
Is this for 8.4 only? 8.5, 8.3 are also affected, |
|
it s the lowest branch (supported, 8.3 is EOL) it applies to, then we backport to upper branches |
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Fix seems good. Please see my question.
This likely has security impact so it needs a backport.
| const char *aad, size_t aad_len, int enc) /* {{{ */ | ||
| { | ||
| int i = 0; | ||
| size_t outlen = data_len + EVP_CIPHER_block_size(cipher_type); |
There was a problem hiding this comment.
Do we risk an overflow of the sum? I also don't like how the data_len is downcasted to an int due to OpenSSL's API.
It means we need to cap data_len to INT_MAX (if that's not already the case).
There was a problem hiding this comment.
No overflow risk. Both callers cap data_len to INT_MAX first (the PHP_OPENSSL_CHECK_SIZE_T_TO_INT_NULL_RETURN checks), so the worst case is INT_MAX + 2*block_size, which fits in size_t even on 32-bit. Same cap is what keeps the (int) casts safe.
There was a problem hiding this comment.
I also do not appreciate the downcasting much, but I guess that s the long (hi)story of openssl existence showing :)
3 participants
…mode.
Fix #22186