Skip to content

Fix incrementing pointer instead of value #354

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 1 commit into from
Jan 28, 2020
Merged

Fix incrementing pointer instead of value #354

merged 1 commit into from
Jan 28, 2020

Conversation

mpg
Copy link
Contributor

@mpg mpg commented Jan 24, 2020 •
edited
Loading

This was introduced by a hasty search-and-replace that embarrassingly didn't account for C's
operator precedence when changing those variables to pointer types.

Backports

ChangeLog entry

### Security

* Fix potential memory overread when performing an ECDSA signature
  operation. The overread only happens with cryptographically low
  probability (of the order of 2^-n where n is the bitsize of the curve)
  unless the RNG is broken, and could result in information disclosure or
  denial of service (application crash or extra resource consumption).
  Found by Auke Zeilstra and Peter Schwabe, using static analysis.

@mpg
Fix incrementing pointer instead of value
ee4ba54 
This was introduced by a hasty search-and-replace that didn't account for C's
operator precedence when changing those variables to pointer types.
@mpg mpg added bug Something isn't working needs: review The pull request is ready for review. This generally means that it has no known issues. needs: ci Needs a passing full CI run needs: backports Needs backports to Mbed TLS branches security Security expertise requested in review labels Jan 24, 2020
@mpg mpg mentioned this pull request Jan 24, 2020
Merged
1 task
yanesca
yanesca approved these changes Jan 24, 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

@mpg
Copy link
Contributor Author

mpg commented Jan 27, 2020

The CI passes all tests except the ones involving Mbed OS, which is a known issue independent from this PR, so it's as good as a pass as far as merging this PR is concerned.

@mpg mpg removed the needs: ci Needs a passing full CI run label Jan 27, 2020
@gilles-peskine-arm gilles-peskine-arm added ready for merge Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only. and removed 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 27, 2020
@mpg mpg merged commit 358462d into ARMmbed:development Jan 28, 2020
gilles-peskine-arm added a commit to gilles-peskine-arm/mbed-crypto that referenced this pull request Feb 3, 2020
@gilles-peskine-arm
Update crypto submodule
81d3100 
Previously in d875285:
* ARMmbed#333: Streamline PSA key type encodings: prepare
* ARMmbed#323: Initialise return values to an error

Previously in dbcb442:
* ARMmbed#291: Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY
* ARMmbed#334: Fix some pylint warnings

Previously in ceceedb:
* ARMmbed#348: Bump version to Mbed TLS 2.20.0 and crypto SO version to 4
* ARMmbed#354: Fix incrementing pointer instead of value

In this commit:
* ARMmbed#349: Fix minor defects found by Coverity
* ARMmbed#179: Add option to build SHA-512 without SHA-384
* ARMmbed#327: Implement psa_hash_compute and psa_hash_compare
* ARMmbed#330: Streamline PSA key type and curve encodings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanesca yanesca yanesca approved these changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

Assignees
No one assigned
Labels
bug Something isn't working ready for merge Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only. security Security expertise requested in review
Projects
None yet
Milestone
No milestone
Development

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