Fix buffer overflow in hash_pbkdf2() with a large output length - #23380
Fix buffer overflow in hash_pbkdf2() with a large output length#23380lazerg wants to merge 1 commit into
Conversation
514c90e to
95e52bd
Compare
|
#23377 was closed since the odd-length case turned out fine. This PR fixes something else though, a float-precision overflow that causes a real out-of-bounds write for large output lengths. Reproduced it under ASAN. Still worth a review. @LamentXU123, mind taking a look when you get a chance? |
| } | ||
|
|
||
| loops = (zend_long) ceil((float) digest_length / (float) ops->digest_size); | ||
| loops = (digest_length + ops->digest_size - 1) / ops->digest_size; |
There was a problem hiding this comment.
Nit: this gives a compiler warning (if I enable all warnings):
ext/hash/hash.c:1048:17: warning: conversion to ‘zend_long’ {aka ‘long int’} from ‘long unsigned int’ may change the sign of the result [-Wsign-conversion]
1048 | loops = (digest_length + ops->digest_size - 1) / ops->digest_size;
| ^
It is not an actual problem, so I think it's fine.
There was a problem hiding this comment.
Agreed, it cannot bite here. digest_length is always at least 1 at that point: a negative length is rejected earlier, and 0 is replaced by the digest size. So the unsigned arithmetic cannot wrap, and the quotient always fits back into a zend_long. php-src does not build with -Wsign-conversion, so I left the line as it is. I can add an explicit cast if you would rather not see the warning.
|
Looks good to me. Thanks for looking into the issue I made. |
hash_pbkdf2()sizes the digest buffer withceil((float) length / 2.0)and the block count withceil((float) digest_length / (float) ops->digest_size). Afloatcarries 24 bits of mantissa, so past 2^24 the conversion rounds and both counts come out wrong: rounding up makeszend_bin2hex()write past the end of the return string, rounding down leaves the tail of the string uninitialized.hash_pbkdf2('md5', 'password', 'salt', 1, 268435473)writes 8 bytes out of bounds under ASAN.Both counts are exact in integer arithmetic, so this replaces them with round-up divisions.
<math.h>had no other user in the file.This came out of #23377. The odd
lengthcase reported there is fine on its own, as Sjord noted before closing it:zend_string_alloc(length)reserveslength + 1bytes and2 * ceil(length / 2) == length + 1, so the extra hexit lands on the terminator byte that gets overwritten a line later. The float conversion on those same two lines is a real overflow though.