-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mbedtls_mpi_cmp_int undefined behavior if parameter is LLONG_MIN #6597
Comments
I thought this had been raised in the past, but it might have been a private discussion only. Several other The Expecting a cryptographic library to cope with negative integers is dubious in the first place; Mbed TLS currently uses them internally, but we're currently rewriting the bignum module to separate out these uses of negative integers, and it's likely that the next major version of Mbed TLS (if not, the one after that) will not support negative bignums anymore. So we're likely to keep this as a documented bug, and add a note to the documentation that -2^(biL-1) is not an acceptable input to these functions. |
OK, I've worked around the issue in my fuzzer, so it won't be detected on OSS-Fuzz anymore. |
Some bignum functions take a value of type mbedtls_mpi_sint as an argument. This was mostly intended for constants in the source code. There isn't much use for supporting the whole _signed_ range correctly (supporting the whole unsigned range would be another matter). Supporting the most negative value V of a two's complement signed type is hard: -V is an integer overflow. Since it's both hard and not useful, don't bother. Just document that it is not supported. This does not affect the use of these functions inside the library and is unlikely to affect real applications. Resolves Mbed-TLS#6597. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Some bignum functions take a value of type mbedtls_mpi_sint as an argument. This was mostly intended for constants in the source code. There isn't much use for supporting the whole _signed_ range correctly (supporting the whole unsigned range would be another matter). Supporting the most negative value V of a two's complement signed type is hard: -V is an integer overflow. Since it's both hard and not useful, don't bother. Just document that it has undefined behavior. This does not affect the use of these functions inside the library and is unlikely to affect real applications. Resolves Mbed-TLS#6597. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Actually, looking closely, a cast is enough to make the behavior well-defined. So here's a proper fix. |
Fixed by #6609 and backport. |
Summary
mbedtls_mpi_cmp_int
negates the input value. If the input value if the lowest possible value formbedtls_mpi_sint
, then negating that value is undefined behavior, because the negationLLONG_MIN
(which is-9223372036854775808
) is9223372036854775808
, but that value cannot be expressed in anmbedtls_mpi_sint
.System information
Mbed TLS version (number or commit id):
Operating system and version: Linux x64
Configuration (if not default, please attach
mbedtls_config.h
):Compiler and options (if you used a pre-built binary, please indicate how you obtained it):
git clone --depth 1 -b development https://github.com/ARMmbed/mbedtls.git
Additional environment information:
Expected behavior
No undefined behavior
Actual behavior
Undefined behavior:
Steps to reproduce
Additional information
OSS-Fuzz #53367 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=53367
The text was updated successfully, but these errors were encountered: