Backport 2.7: Fix buffer overflow in mbedtls_mpi_sub_abs negative case #4077
Conversation
gilles-peskine-arm
added
bug
needs-review
tests/suites/test_suite_mpi.data
Outdated
| mbedtls_mpi_sub_abs:10:"5":10:"-7":10:"0":MBEDTLS_ERR_MPI_NEGATIVE_VALUE | ||
|
|
||
| Base test mbedtls_mpi_sub_abs #1 (|B| >> |A|) |
There was a problem hiding this comment.
Like above, I find spelling it out more helpful (eg. "Test with much larger second input" or "Second input is at least a limb longer").
There was a problem hiding this comment.
I think having "limb" in the test description would be useful, as this is really what the test is about. For example (|B| > |A| with more limbs), or something like that.
There was a problem hiding this comment.
I changed the description to (|B| >> |A| with more limbs). I kept >> because it's relevant that B has more limbs not just because of leading zeros.
There was a problem hiding this comment.
Looks good to me - I think the test description would be improved but is good enough as it is.
In terms of commit structure, I like that you introduce the test first, which makes it easier for reviewers to check that it's failing before the fix. However, for the benefit of people who might be bisecting this in the future, in those cases it would probably be better to mention explicitly in the commit message that the tests are expected to fail. (Not requesting a history rewrite here unless you feel like it, more of a general note for the future.)
Add test cases for mbedtls_mpi_sub_abs() where the second operand has more limbs than the first operand (which, if the extra limbs are not all zero, implies that the function returns MBEDTLS_ERR_MPI_NEGATIVE_VALUE). This exposes a buffer overflow (reported in Mbed-TLS#4042). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Fix a buffer overflow in mbedtls_mpi_sub_abs() when calculating |A| - |B| where |B| is larger than |A| and has more limbs (so the function should return MBEDTLS_ERR_MPI_NEGATIVE_VALUE). Fix Mbed-TLS#4042 Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
force-pushed
the
mpi_sub_abs-buffer_overflow-2.7
branch
from
ccbb758 to
54c3046
Compare
|
I rewrote the history to amend the commit message and the test descriptions in the first commit. I'm not convinced that a commit that introduces failing tests should explain how and why the tests might fail. A commit that adds new tests expands the test coverage, and if that reveals a bug (known or not to the developer), it's up to subsequent commits to fix that bug. But ok, I added a reference to the bug report in the commit that introduces the tests. |
gilles-peskine-arm
removed
the
needs-backports
In that case, the fact that the test failed was not only known but even expected: actually, that's one of the things I checked as a reviewer (and I suppose that's one of the things you checked after writing the tests). So I think recording that in the commit message is simply a way to record information that we had when adding the test and making sure that information remains conveniently available in the future. |
mpg
added
approved
Fix a buffer overflow in
mbedtls_mpi_sub_abs()when calculating |A| - |B| where |B| is larger than |A| and has more limbs (so the function should returnMBEDTLS_ERR_MPI_NEGATIVE_VALUE).This is a regression introduced in Mbed TLS 2.7.18. Fix #4042.
Backport of #4096