[apps/speed] Added checking for buflen overflow due to MAX_MISALIGNMENT. #17646
Conversation
apps/speed.c
Outdated
| @@ -1778,7 +1778,12 @@ int speed_main(int argc, char **argv) | |||
| buflen = lengths[size_num - 1]; | |||
| if (buflen < 36) /* size of random vector in RSA benchmark */ | |||
| buflen = 36; | |||
| buflen += MAX_MISALIGNMENT + 1; | |||
| if (0x7fffffff - (MAX_MISALIGNMENT + 1) < buflen) | |||
There was a problem hiding this comment.
This should be INT_MAX not 0x7fffffff.
There was a problem hiding this comment.
What about using buflen + MAX_MISALIGNMENT + 1 < buflen?
There was a problem hiding this comment.
@paulidale fixed. Also changed in a couple of other places where 0x7fffffff is used.
There was a problem hiding this comment.
@t8m C doesn't actually mandate signed integer overflow to be wraparound. I'm pretty sure the platforms OpenSSL supports will indeed wraparound, but I did this just to be safe.
There was a problem hiding this comment.
C specifies that signed integer overflow is undefined. We don't want to rely on undefined behaviour.
There was a problem hiding this comment.
[deleted duplicate comment to paulidale's point]
apps/speed.c
Outdated
| if (0x7fffffff - (MAX_MISALIGNMENT + 1) < buflen) | ||
| { | ||
| BIO_printf(bio_err, "Warning: ignoring -misalign option.\n"); | ||
| } else { |
There was a problem hiding this comment.
Because each branch of the if is but a single statement, our coding style would suggest omitting the {} on both.
I think better might be changing the warning to an error and failing. In which case, the {} should be retained.
There was a problem hiding this comment.
@paulidale Made it an error, also updated help with the maximum possible buffer size for clarity.
paulidale
added
approval: review pending
apps/speed.c
Outdated
| @@ -1778,7 +1778,13 @@ int speed_main(int argc, char **argv) | |||
| buflen = lengths[size_num - 1]; | |||
| if (buflen < 36) /* size of random vector in RSA benchmark */ | |||
| buflen = 36; | |||
| buflen += MAX_MISALIGNMENT + 1; | |||
| if (INT_MAX - (MAX_MISALIGNMENT + 1) < buflen) | |||
| { | |||
There was a problem hiding this comment.
Placement of { should be on the end of the preceding line
apps/speed.c
Outdated
| { | ||
| BIO_printf(bio_err, "Error: buffer size too large\n"); | ||
| goto end; | ||
| } else { |
There was a problem hiding this comment.
This could actually be not an "else" at all and just unconditional since the main branch ends in a goto so will never get here anyway.
There was a problem hiding this comment.
changed this as well.
t8m
added
triaged: bug
t8m
added
approval: done
|
Good for 3.0 too, I wasn't sure if it was a bug fix or not. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
|
Merged to master and 3.0 after end of line whitespace removal. |
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #17646)
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #17646) (cherry picked from commit 4b3777c9ad4a2058a9b87afb26289039ebf4a6c1)
If a large enough value is given to the "-bytes" option, a signed integer overflow occurs as MAX_ALIGNMENT + 1 is added to buflen. This negative value is interpreted as unsigned by malloc, which then attempts to allocate an extremely large buffer.
For now, I'm just ignoring the addition and printing a warning. I'm not sure about the semantics of the "-misalign" option, so if there is a better solution I will implement that instead.
This also might be a consideration for backporting to the 1.1.1 branch.