Skip to content

Improve robustness and testing of mbedtls_mpi_copy #346

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 6, 2020
Merged

Improve robustness and testing of mbedtls_mpi_copy #346

merged 7 commits into from
Feb 6, 2020

Conversation

gilles-peskine-arm
Copy link
Collaborator

Also improve testing of mbedtls_mpi_swap.

The robustness improvement could plausibly fix a bug in some applications, so this should be backported.

@gilles-peskine-arm
Bignum copy/shrink: More precise test case descriptions
72d7609
@gilles-peskine-arm
Better coverage for copy and swap
7428b45 
Cover more cases: different signs, different zeronesses, repeated
argument.
@gilles-peskine-arm
mpi_copy: make the 0 case slightly more robust
db42062 
If Y was constructed through functions in this module, then Y->n == 0
iff Y->p == NULL. However we do not prevent filling mpi structures
manually, and zero may be represented with n=0 and p a valid pointer.
Most of the code can cope with such a representation, but for the
source of mbedtls_mpi_copy, this would cause an integer underflow.
Changing the test for zero from Y->p==NULL to Y->n==0 causes this case
to work at no extra cost.
@gilles-peskine-arm
Improve comments in mpi_shrink
e2f563e
@gilles-peskine-arm gilles-peskine-arm added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. needs: backports Needs backports to Mbed TLS branches labels Jan 20, 2020
mpg
mpg previously approved these changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I just left a suggestion for improving a comment, which you may choose to ignore as perhaps my initial confusion would be best fixed by me getting more sleep than by rewording the comment.

library/bignum.c Outdated
if( X->n <= nblimbs )
return( mbedtls_mpi_grow( X, nblimbs ) );
/* Now X->n > nblimbs >= 0. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: While we're improving comments, I'd like to suggest replacing "Now" with "If we reach this point, we know that". Initially I missed that there there was a return on the line above, so I started commenting that we only know X->n >= nblimbs here before realizing my mistake. I think wording like the one I suggested would have helped draw my attention to the return above.

@mpg
Copy link
Contributor

mpg commented Jan 21, 2020

I checked the CI results, and the only failures are in Mbed OS testing, which is expected as Mbed OS needs updating, so it's as good as a pass.

mpg
mpg previously approved these changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for improving the comment.

@mpg
Copy link
Contributor

mpg commented Jan 22, 2020

For some reason the CI had directly reported failure without actually running any test. I'm restarting it.

yanesca
yanesca requested changes Feb 3, 2020
View reviewed changes
Copy link
Collaborator

@yanesca yanesca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two minor suggestions for improvement.

tests/suites/test_suite_mpi.data Outdated
mbedtls_mpi_swap:0:1500

Test mbedtls_mpi_shrink #1
Shrink 2 in 2 to 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand this description, could you please make it slightly more verbose?

tests/suites/test_suite_mpi.function Outdated
mbedtls_mpi X, Y, X0;
mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &X0 );

TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our existing tests are using big endian input and I think it would be less confusing to have consistency in this matter. Why do you prefer little endian by the way?

@yanesca yanesca added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Feb 3, 2020
@gilles-peskine-arm
Move test functions from Lilliput to Blefuscu
9a6ecee 
We normally represent bignums in big-endian order and there is no
reason to deviate here.
@gilles-peskine-arm
shrink tests: clearer description
a9da093
@gilles-peskine-arm gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Feb 3, 2020
yanesca
yanesca approved these changes Feb 3, 2020
View reviewed changes
Copy link
Collaborator

@yanesca yanesca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

This was referenced Feb 3, 2020
Merged
Merged
mpg
mpg approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yanesca
Copy link
Collaborator

yanesca commented Feb 4, 2020

Checked the CI and it only fails on Mbed OS with known issues that are unrelated to this PR.

@yanesca yanesca removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Feb 4, 2020
@mpg mpg added ready for merge Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only. and removed needs: backports Needs backports to Mbed TLS branches labels Feb 6, 2020
@mpg
Copy link
Contributor

mpg commented Feb 6, 2020

The backport are now ready for merge, and so is this.

@mpg mpg merged commit 4d8c836 into ARMmbed:development Feb 6, 2020
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Feb 17, 2020
Merged
2 tasks
gilles-peskine-arm added a commit to gilles-peskine-arm/mbed-crypto that referenced this pull request Mar 23, 2020
@gilles-peskine-arm
Update crypto submodule
799ae77 
* ARMmbed#352: Parse RSA parameters DP, DQ and QP from PKCS1 private keys
* ARMmbed#263: Introduce ASN.1 SEQUENCE traversal API
* ARMmbed#345: Fix possible error code mangling in psa_mac_verify_finish
* ARMmbed#357: Update Mbed Crypto with latest Mbed TLS changes as of 2020-02-03
* ARMmbed#350: test_suite_asn1parse: improve testing of trailing garbage in parse_prefixes
* ARMmbed#346: Improve robustness and testing of mbedtls_mpi_copy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg approved these changes

@yanesca yanesca yanesca approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request ready for merge Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gilles-peskine-arm @mpg @yanesca