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

fix: verify length and hashes of fetched bytes before parsing #325

Merged
merged 7 commits into from Jun 23, 2022

Conversation

joshuagl
Copy link
Member

@joshuagl joshuagl commented Jun 21, 2022

Fixes #233
Release Notes:
Check hashes and length of downloaded snapshot and targets metadata before parsing.

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of the changes being introduced by the pull request:
Better match the specification by performing checks against length and hashes on the downloaded bytes before parsing the bytes into a FileMeta.

Please verify and check that the pull request fulfills the following requirements:

  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@joshuagl joshuagl changed the title Verify length and hashes of fetched bytes before parsing fix: verify length and hashes of fetched bytes before parsing Jun 21, 2022
@joshuagl joshuagl requested review from rdimitrov, znewman01 and a team June 21, 2022 11:26
util/util.go Outdated Show resolved Hide resolved
mnm678
mnm678 previously approved these changes Jun 22, 2022
znewman01
znewman01 previously approved these changes Jun 22, 2022
util/util.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

I'd suggest we add a few simple unit tests for BytesMatchLenAndHashes and VersionEqual in util_tests.go, so we know in case they break at some point 👍 Other than that, it's great 👍 🙏

util/util.go Outdated Show resolved Hide resolved
Add BytesMatchLenAndHashes() to verify that fetched bytes match the
expected length and hashes.
This is useful to check data before parsing.

Signed-off-by: Joshua Lock <jlock@vmware.com>
Signed-off-by: Joshua Lock <jlock@vmware.com>
Signed-off-by: Joshua Lock <jlock@vmware.com>
Signed-off-by: Joshua Lock <jlock@vmware.com>
We check the length and hashes of the fetched bytes before parsing them,
therefore once the data are parsed into a FileMeta we only need to check
against the expected version. The length and hashes checks are no longer
required at this point, as they have already been done.

Signed-off-by: Joshua Lock <jlock@vmware.com>
Signed-off-by: Joshua Lock <jlock@vmware.com>
Add tests for newly exported functions in the util package:
* VersionEqual() - simple test of version number comparison
* BytesMatchLenAndHashes() - check function returns appropriate errors
  when incorrect length, hashes, or both are passed

Signed-off-by: Joshua Lock <jlock@vmware.com>
@joshuagl
Copy link
Member Author

I've add some tests in af46f5d and removed the unnecessary len() check in 39b9b0f. I've also rebased on the latest master branch and force pushed.

PTAL @mnm678 @znewman01 @rdimitrov 🙏

Copy link
Contributor

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

Thanks! 👍 LGTM! 💯

@znewman01 znewman01 merged commit e3efe98 into theupdateframework:master Jun 23, 2022
@joshuagl joshuagl deleted the joshuagl/issue-233 branch June 24, 2022 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

metadata hashes used in a way that negates security benefits
4 participants