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.

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

crypto/hd: make DerivePrivateKeyForPath error and not panic on trailing slashes #8607

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

odeke-em
Copy link
Collaborator

Detected during my audit, right before fuzzing, the code that
checked for presence of hyphens per path segment assumed that
the part would always be non-empty. However, with paths such as:

  • m/4/
  • /44/
  • m/4///

it'd panic with a runtime slice out of bounds.

With this new change, we now:

  • firstly strip the right trailing slash
  • on finding any empty segments of a path return an error

Fixes #8557


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

…ng slashes

Detected during my audit, right before fuzzing, the code that
checked for presence of hyphens per path segment assumed that
the part would always be non-empty. However, with paths such as:
* m/4/
* /44/
* m/4///

it'd panic with a runtime slice out of bounds.

With this new change, we now:
* firstly strip the right trailing slash
* on finding any empty segments of a path return an error

Fixes #8557
@odeke-em odeke-em force-pushed the crypto-hd-make-DerivePrivateKeyForPath-not-panic branch from b757615 to a962bac Compare February 17, 2021 10:17
@odeke-em
Copy link
Collaborator Author

This PR fixes a security issue, so kindly also backport :-)

Copy link
Contributor

@jgimeno jgimeno left a comment

Choose a reason for hiding this comment

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

Good one!

@alessio alessio added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 17, 2021
@mergify mergify bot merged commit f970056 into master Feb 17, 2021
@mergify mergify bot deleted the crypto-hd-make-DerivePrivateKeyForPath-not-panic branch February 17, 2021 10:30
@amaury1093 amaury1093 mentioned this pull request Feb 17, 2021
23 tasks
@alessio
Copy link
Contributor

alessio commented Feb 17, 2021

@Mergifyio backport release/v0.41.x

@mergify
Copy link
Contributor

mergify bot commented Feb 17, 2021

Command backport release/v0.41.x: success

Backports have been created

mergify bot pushed a commit that referenced this pull request Feb 17, 2021
…ng slashes (#8607)

Detected during my audit, right before fuzzing, the code that
checked for presence of hyphens per path segment assumed that
the part would always be non-empty. However, with paths such as:
* m/4/
* /44/
* m/4///

it'd panic with a runtime slice out of bounds.

With this new change, we now:
* firstly strip the right trailing slash
* on finding any empty segments of a path return an error

Fixes #8557

(cherry picked from commit f970056)
alessio pushed a commit that referenced this pull request Feb 17, 2021
…ng slashes (#8607) (#8608)

Detected during my audit, right before fuzzing, the code that
checked for presence of hyphens per path segment assumed that
the part would always be non-empty. However, with paths such as:
* m/4/
* /44/
* m/4///

it'd panic with a runtime slice out of bounds.

With this new change, we now:
* firstly strip the right trailing slash
* on finding any empty segments of a path return an error

Fixes #8557

(cherry picked from commit f970056)

Co-authored-by: Emmanuel T Odeke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
3 participants