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.

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

Backport 2.7: Check for zero length and NULL buffer pointer #2794

Closed
wants to merge 2 commits into from
Closed

Backport 2.7: Check for zero length and NULL buffer pointer #2794

wants to merge 2 commits into from

Conversation

jainvikas8
Copy link
Contributor

In reference to issue ARMmbed/mbed-crypto#49

Backport of ARMmbed/mbed-crypto#222

@Patater
Copy link
Contributor

Patater commented Aug 16, 2019

Some trailing whitespace snuck in. Please remove.

@jainvikas8 jainvikas8 force-pushed the dev/mbedtls-2.7/zeroize-check branch from e24d969 to d64ae2c Compare August 16, 2019 09:53
@jainvikas8
Copy link
Contributor Author

Force-pushed to remove trailing whitespaces.

Patater
Patater previously approved these changes Aug 16, 2019
View reviewed changes
@RonEld RonEld added bug component-crypto Crypto primitives and low-level interfaces mbed TLS team needs-review Every commit must be reviewed by at least two team members, labels Aug 20, 2019
@k-stachowiak k-stachowiak self-requested a review August 23, 2019 07:39
k-stachowiak
k-stachowiak previously approved these changes Aug 23, 2019
View reviewed changes
Copy link
Contributor

@k-stachowiak k-stachowiak left a comment

Choose a reason for hiding this comment

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

LGTM

@Patater Patater added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 23, 2019
@Patater Patater changed the title Backport: Check for zero length and NULL buffer pointer - MbedTLS-2.7 Backport 2.7: Check for zero length and NULL buffer pointer Aug 23, 2019
@Patater Patater mentioned this pull request Aug 29, 2019
Merged
@Patater Patater removed the approved Design and code approved - may be waiting for CI or backports label Sep 3, 2019
@Patater
Copy link
Contributor

Patater commented Sep 3, 2019

Demoting from ready to merge state as we may want to resolve ARMmbed/mbed-crypto#222 (comment) first

@jainvikas8 jainvikas8 dismissed stale reviews from k-stachowiak and Patater via 2b340ed September 4, 2019 10:14
@jainvikas8 jainvikas8 force-pushed the dev/mbedtls-2.7/zeroize-check branch from d64ae2c to 2b340ed Compare September 4, 2019 10:14
@jainvikas8
Copy link
Contributor Author

Forced push to accommodate reviewer change request from ARMmbed/mbed-crypto#222 (comment).

@jainvikas8 jainvikas8 force-pushed the dev/mbedtls-2.7/zeroize-check branch from 2b340ed to a2fb154 Compare September 4, 2019 10:25
@jainvikas8
Copy link
Contributor Author

Forced push to changelog.

Patater
Patater reviewed Sep 4, 2019
View reviewed changes
library/aes.c
volatile unsigned char *p = (unsigned char*)v; while( n-- ) *p++ = 0;
static void mbedtls_zeroize( void *v, size_t n )
{
if( n > 0 )
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike the other implementations of zeroize that call memset, we probably don't need this if here as there is no undefined behavior. If n is 0, the loop is not executed. The local volatile pointer assignment won't do anything meaningful, since we don't use p at all when n is 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no bug to fix in 2.7, is there?

@jainvikas8
Copy link
Contributor Author

jainvikas8 commented Sep 4, 2019 •
edited
Loading

Yes, there is no bug to fix. Referred few functions and the NULL pointer is checked before the call. So no need to check for length. Therefore closing it.

@jainvikas8 jainvikas8 closed this Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Patater Patater Patater left review comments

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm left review comments

@k-stachowiak k-stachowiak k-stachowiak left review comments

Assignees
No one assigned
Labels
bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jainvikas8 @Patater @k-stachowiak @gilles-peskine-arm @RonEld