fix(sdk): 🔒 During read, limits TDF Manifest to 10MB #1385
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes GHSA-9m35-5v9f-8x2j by limiting TDF Manifest sizes to 10MB or smaller. Prior to this fix, a protocol attack is possible to define a large manifest size which will result in a large memory allocation (even if the manifest transmitted is substantially smaller). This commit also includes the fuzz testing that was used to discover this flaw. In `reader` the Manifest load uses the function `ReadAllFileData`. This function is documented to only be used on small files, however to accomplish that a call to `ReadFileSize` beforehand would be necessary. Because the call to check the defined size is likely to be missed, this commit updates `ReadAllFileData` to instead accept the maximum size as an argument, which is validated before allocating the byte slice.
jentfoo
previously approved these changes
Aug 19, 2024
dmihalcik-virtru
force-pushed
the
feature/fuzzzzzzzzzz
branch
from
987e2e4 to
d3734ee
Compare
- still doesn't assert much, just that either error exists or doesn't really, which isn't very helpful
dmihalcik-virtru
force-pushed
the
feature/fuzzzzzzzzzz
branch
from
e47b521 to
c4b1107
Compare
dmihalcik-virtru
changed the title
dmihalcik-virtru
changed the title
dmihalcik-virtru
changed the title
Member
Author
|
@jentfoo Can you take a look and make sure I haven't introduced any issues with my fixups? Thanks! |
jentfoo
reviewed
Aug 19, 2024
- don't block on fuzz testing; instead we should run this on a schedule - don't have complex 'valid value' checks on good output; not very useful, hard to maintain, and often slower than just running the test more
jentfoo
approved these changes
Aug 21, 2024
Contributor
|
Thank you for the help @dmihalcik-virtru 🙌 |
strantalis
approved these changes
Aug 21, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 21, 2024
🤖 I have created a release *beep* *boop* --- ## [0.3.10](sdk/v0.3.9...sdk/v0.3.10) (2024-08-21) ### Bug Fixes * **sdk:** 🔒 During read, limits TDF Manifest to 10MB ([#1385](#1385)) ([cfeebce](cfeebce)) * **sdk:** let value grants override attr grants ([#1318](#1318)) ([77f1e11](77f1e11)) * **sdk:** well-known warning logs and public client id error ([#1415](#1415)) ([e6e76bf](e6e76bf)), closes [#1414](#1414) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 4, 2024
This PR includes additional fuzzing and security testing that was not covered in prior PR's (#1472 and #1385). Those PR's include the relevant tests for their fixes, where this provides additional testing that was explored but did not show issues. In addition this PR includes some minor changes that were found while reviewing the code base: * Spelling fixes * Token issue time and expiration based off the same time sample (reduced kernel overheads and also ensures no time skew risks) PR closes #1341
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit limits TDF Manifest sizes to 10MB or smaller. Prior to this fix, a protocol attack is possible to define a large manifest size which will result in a large memory allocation (even if the manifest transmitted is substantially smaller).
This commit also includes the fuzz testing that was used to discover this flaw.
In
readerthe Manifest load uses the functionReadAllFileData. This function is documented to only be used on small files, however to accomplish that a call toReadFileSizebeforehand would be necessary.Because the call to check the defined size is likely to be missed, this commit updates
ReadAllFileDatato instead accept the maximum size as an argument, which is validated before allocating the byte slice.Fixes #1391