Backport 2.x: Fix mbedtls_debug_print_mpi crash on 0 #4623
Conversation
Reorder test cases and make their descriptions more explicit. No change in test data. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
There was already a test case for 0 but with a non-empty representation (X->n == 1). Add a test case with X->n == 0 (freshly initialized mpi). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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>
MPI sizes do fit in int. Let MSVC know this conversion is deliberate. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
added
bug
needs-review
|
I am not sure to follow. Both this PR and #4621 target development_2.x. Is that as intended ? |
|
@ronald-cron-arm This PR and #4621 are both partial backports of #4604. I discovered two independent issues (one bug in debug, and some documentation issues) while working on #4604, and I made separate PR to backport them. I had made some mistakes in the links in the description of this PR which I've now fixed. |
I think backporting the rewrite is fine. Since the new code is simpler and cleaner, the risk of introducing unwanted changed seems relatively low, and considering it's in the debug module, I think it's acceptable for 2.16. I'll note, though, that it's not clear to me that this bug could be triggered in 2.16: the ChangeLog says this can happen when using a Montgomery curve for key exchange, which 2.16 does not support. But we don't know if it could happen in other circumstances as well, so I think it's safer to backport the fix. |
Good point, I'll remove this sentence form the changelog. I haven't investigated what other ways there could be to trigger the bug in 2.16. Given that the MPI read functions don't create 0-limb MPIs, it's harder to trigger in 2.16, but there may well be debug code that prints out an MPI that remained in its initial state because it's unused with a particular combination of parameters, or because there was an error before it was filled. So I can't rule out TLS exploitability in 2.16. |
ronald-cron-arm
removed
the
needs-reviewer
There was a problem hiding this comment.
I've been through the cherry-picking of all the commits of #4604 related to mbedls_debug.c and its testing and only those and ended-up with the same tree. This is thus a fair partial backport of the work on mbedtls_debug.c in #4604. There is just the change log that needs some adjustment (as per #4623 (comment)) and then I can approve.
ronald-cron-arm
added
the
approved
mpg
removed
the
needs-review
Fix #4608. A direct backport of the relevant part of #4604.
Other parts of #4604 that needed backporting are in #4621.
Question about the 2.16 backport: should I backport this rewrite of `mbedtls_debug_print_mpi (which I find a lot more readable, and reduces the code size, but means more risk of an accidental behavior change), or should I find a way to fix the bug with as little deviation from the original code as possible?