Skip to content
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

Loading zero-padded negative number results in positive number #4295

Closed
guidovranken opened this issue Apr 2, 2021 · 0 comments · Fixed by #4297
Closed

Loading zero-padded negative number results in positive number #4295

guidovranken opened this issue Apr 2, 2021 · 0 comments · Fixed by #4297
Assignees
Labels
bug component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d)

Comments

@guidovranken
Copy link
Contributor

E.g. using mbedtls_mpi_read_string to load "-1" actually loads -1, but loading "-01" loads 1.

This appears to be new behavior. It was detected by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=32828

The revision range: e93095f...d520037

#include <mbedtls/bignum.h>

int main(void)
{
    mbedtls_mpi mpi;
    mbedtls_mpi_init(&mpi);
    if ( mbedtls_mpi_read_string(&mpi, 10, "-01") != 0 ) {
        goto error;
    }
    char out[1024];
    size_t outlen = sizeof(out);
    if ( mbedtls_mpi_write_string(&mpi, 10, out, outlen, &outlen) != 0 ) {
        goto error;
    }
    printf("%s\n", out);
error:
    return 0;
}
@gilles-peskine-arm gilles-peskine-arm self-assigned this Apr 3, 2021
@gilles-peskine-arm gilles-peskine-arm added bug component-crypto Crypto primitives and low-level interfaces Product Backlog labels Apr 3, 2021
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Apr 3, 2021
Move the handling of the sign out of the base-specific loops. This
both simplifies the code, and corrects an edge case: the code in the
non-hexadecimal case depended on mbedtls_mpi_mul_int() preserving the
sign bit when multiplying a "negative zero" MPI by an integer, which
used to be the case but stopped with PR Mbed-TLS#2512.

Fix Mbed-TLS#4295. Thanks to Guido Vranken for reporting the bug.
Credit to OSS-Fuzz.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Apr 3, 2021
Move the handling of the sign out of the base-specific loops. This
both simplifies the code, and corrects an edge case: the code in the
non-hexadecimal case depended on mbedtls_mpi_mul_int() preserving the
sign bit when multiplying a "negative zero" MPI by an integer, which
used to be the case but stopped with PR Mbed-TLS#2512.

Fix Mbed-TLS#4295. Thanks to Guido Vranken for analyzing the cause of the bug.
Credit to OSS-Fuzz.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Apr 9, 2021
Move the handling of the sign out of the base-specific loops. This
both simplifies the code, and corrects an edge case: the code in the
non-hexadecimal case depended on mbedtls_mpi_mul_int() preserving the
sign bit when multiplying a "negative zero" MPI by an integer, which
used to be the case but stopped with PR Mbed-TLS#2512.

Fix Mbed-TLS#4295. Thanks to Guido Vranken for analyzing the cause of the bug.
Credit to OSS-Fuzz.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
daverodgman pushed a commit that referenced this issue Apr 23, 2021
Move the handling of the sign out of the base-specific loops. This
both simplifies the code, and corrects an edge case: the code in the
non-hexadecimal case depended on mbedtls_mpi_mul_int() preserving the
sign bit when multiplying a "negative zero" MPI by an integer, which
used to be the case but stopped with PR #2512.

Fix #4295. Thanks to Guido Vranken for analyzing the cause of the bug.
Credit to OSS-Fuzz.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added the size-s Estimated task size: small (~2d) label May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants