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

mbedtls_debug_print_mpi crashes on 0 #4608

Closed
gilles-peskine-arm opened this issue Jun 2, 2021 · 1 comment · Fixed by #4604
Closed

mbedtls_debug_print_mpi crashes on 0 #4608

gilles-peskine-arm opened this issue Jun 2, 2021 · 1 comment · Fixed by #4604
Assignees
Labels
bug component-tls size-s Estimated task size: small (~2d)

Comments

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jun 2, 2021

This bug was originally reported by @lhuang04 in #4578. I'm creating an issue to facilitate tracking.

Steps to reproduce (confirmed at 72dcd4e, but I think the TLS/x25519 part has existed ever since TLS started supporting x52219 and the debug_mpi part since PolarSSL):

make
programs/ssl/ssl_server2 debug_level=3 &
programs/ssl/ssl_client2 curves=x25519

Expected behavior: a TLS connection happens normally, using x25519 for the key exchange.

Actual behavior:

…
ssl_srv.c:3665: |2| => write server key exchange
ssl_srv.c:3443: |2| ECDHE curve: x25519
ssl_srv.c:3468: |3| value of 'ECDH: Q(X)' (254 bits) is:
ssl_srv.c:3468: |3|  3d 3f 0e fc 6e 35 c8 53 53 d8 b7 59 20 95 b2 5a
ssl_srv.c:3468: |3|  60 1c 16 ea b7 9a cb ff dd 04 98 46 b6 55 8b 86
[1]    2345374 segmentation fault (core dumped)  programs/ssl/ssl_server2 debug_level=3

An UBSan or Msan build gives a backtrace:

source/library/debug.c:236:13: runtime error: applying non-zero offset 18446744073709551608 to null pointer
    #0 0x4d449d in mbedtls_debug_print_mpi library/debug.c:236:13
    #1 0x4d3b1f in mbedtls_debug_print_ecp library/debug.c:213:5
    #2 0x4f2beb in ssl_prepare_server_key_exchange library/ssl_srv.c:3468:9
    #3 0x4f2beb in ssl_write_server_key_exchange library/ssl_srv.c:3703:15
    #4 0x4f2beb in mbedtls_ssl_handshake_server_step library/ssl_srv.c:4736:19
    #5 0x511461 in mbedtls_ssl_handshake library/ssl_tls.c:5815:15
    #6 0x4ca075 in main programs/ssl/ssl_server2.c:3155:20
    #7 0x7fb5c1ebc0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #8 0x41c47d in _start (/home/gilpes01/z/dev/mbedtls/main/build-asan/programs/ssl/ssl_server2+0x41c47d)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior source/library/debug.c:236:13 in 

As analyzed by @lhuang04, mbedtls_debug_print_mpi does not correctly handle the case of an MPI such that X->n == 0, which is a valid representation of 0.

In addition, mbedtls_debug_print_ecp shouldn't be printing 0 here, but that's more of a cosmetic issue.

@gilles-peskine-arm gilles-peskine-arm self-assigned this Jun 2, 2021
@gilles-peskine-arm gilles-peskine-arm added the size-s Estimated task size: small (~2d) label Jun 2, 2021
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Jun 2, 2021
Rewrite mbedtls_debug_print_mpi to be simpler and smaller. Leverage
mbedtls_mpi_bitlen() instead of manually looking for the leading
zeros.

Fix Mbed-TLS#4608: the old code made an invalid memory dereference when
X->n==0 (freshly initialized bignum with the value 0).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Copy link
Contributor Author

Fix posted in #4604. I'll make backports once it's been reviewed.

gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Jun 7, 2021
Rewrite mbedtls_debug_print_mpi to be simpler and smaller. Leverage
mbedtls_mpi_bitlen() instead of manually looking for the leading
zeros.

Fix Mbed-TLS#4608: the old code made an invalid memory dereference when
X->n==0 (freshly initialized bignum with the value 0).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Jun 17, 2021
Rewrite mbedtls_debug_print_mpi to be simpler and smaller. Leverage
mbedtls_mpi_bitlen() instead of manually looking for the leading
zeros.

Fix Mbed-TLS#4608: the old code made an invalid memory dereference when
X->n==0 (freshly initialized bignum with the value 0).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this issue Jun 21, 2021
Rewrite mbedtls_debug_print_mpi to be simpler and smaller. Leverage
mbedtls_mpi_bitlen() instead of manually looking for the leading
zeros.

Fix Mbed-TLS#4608: the old code made an invalid memory dereference when
X->n==0 (freshly initialized bignum with the value 0).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@mpg mpg closed this as completed in #4604 Jun 22, 2021
jepaan added a commit to LogosDesignAS/mbedtls that referenced this issue Jun 24, 2021
Rewrite mbedtls_debug_print_mpi to be simpler and smaller. Leverage
mbedtls_mpi_bitlen() instead of manually looking for the leading
zeros.

Fix Mbed-TLS#4608: the old code made an invalid memory dereference when
X->n==0 (freshly initialized bignum with the value 0).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
(cherry picked from commit 2ee0bb3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-tls size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant