fix(sdk): Fuzz testing and protocol fixes #214
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
jentfoo
changed the title
jentfoo
changed the title
pflynn-virtru
approved these changes
Dec 2, 2024
mkleene
reviewed
Dec 3, 2024
This change fixes a couple possible protocol attacks: * Possible NullPointerExceptions from `readLong/Int/Short` when a full value could not be read. This is assumed to be unexpected, and may not be handled by users. Instead `InvalidZipException` is now thrown. * A permutation of the above is also possible when reading the Signature. The behavior was changed to defer to the existing signature validation failure (logging and existing InvalidZipException). * Loops where a partial read is handled (see example reading in the filename) could result in a tight loop thread if content is shorter than the defined length. The logic now expects a blocking input which will only return a zero value if the content has reached the end. A premature end will result in an `EOFException`.
When the list size is known initializing at that value reduces the minor memory overhead of expanding and copying the underline array.
This mocked class is not needed due to TDF only being used with auto configure set to `false`. To simplify this test the mock was removed and instead null is passed in to validate it's not used.
…ions This commit validates the fields were read from the Manifest before the TDF is read. This results in convering previous NullPointerExceptions into `IllegalArgumentExceptions` with a message that indicates what field had an unexpected state.
This matches the protections introduced in the go SDK PR: opentdf/platform#1536
This fuzz testing and seed corpus helped validate for protocol flaws in decoding TDF's. This testing is time consuming, and Jazzer sometimes has some weird IO blocking behavior that is not actually indicative of a flaw. For that reason this is not part of CI, and instead is run through `fuzz.sh` when needed.
This change was applied easily in go, but there are issues with integration and other existing test payloads. Because this is low risk, I believe it's ok to remove this protection in the Java SDK, but leave commented so it's known to be explict. Alternatively we could update test payloads.
|
mkleene
approved these changes
Dec 3, 2024
mkleene
pushed a commit
that referenced
this pull request
Feb 6, 2025
🤖 I have created a release *beep* *boop* --- <details><summary>0.7.6</summary> ## [0.7.6](v0.7.5...v0.7.6) (2025-02-06) ### Features * Add assertion verification ([#216](#216)) ([e0f8caf](e0f8caf)) * **cmdline:** assertions cli support ([#204](#204)) ([3325114](3325114)) * **sdk:** Add and expose tamper error types ([#187](#187)) ([b4f95e6](b4f95e6)) * **sdk:** adds Collections API ([#212](#212)) ([1ee1367](1ee1367)) ### Bug Fixes * Correct null assertions when deserializing ([#211](#211)) ([b075194](b075194)) * incorrect isStreamable serialized name ([#210](#210)) ([32825b0](32825b0)) * NanoTDF secure key from debug logging and iv conflict risk ([#208](#208)) ([6301d32](6301d32)) * **sdk:** deserialize object statement values correctly ([#219](#219)) ([c513e8c](c513e8c)) * **sdk:** Fuzz testing and protocol fixes ([#214](#214)) ([cf6f932](cf6f932)) * **sdk:** group splits with empty/missing split IDs together ([#217](#217)) ([0f47702](0f47702)) * **sdk:** remove hex encoding ([#213](#213)) ([e076d11](e076d11)) * **sdk:** uses offset for ByteBuffer array offset ([#209](#209)) ([0d6e761](0d6e761)) * Use reusable start-additional-kas workflow ([#215](#215)) ([cb6f757](cb6f757)) </details> --- 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>
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 PR includes a variety of fixes found with fuzz testing. This PR is likely easiest to consume by reviewing commit by commit. Here is a highlevel of the changes:
NullPointerExceptionshave been converted to other exception types. Please provde feedback on if other exception types should be used for these cases. InFuzzing.javanow also serves to define in testing what exception types are acceptable. Thecatchspecifically lists the types of exceptions that were discovered for each API call, andthrowsare checked exceptions that are not expected to be possible. As a future improvement we may want to refine this list further and better document what exceptions happen under what conditions. For now I thought it was best to start with just theNullPointerExceptioncases. Since these cases were numerous, these changes span multiple commits, with each commit focused on a specific area of the protocol.Fuzzing.javaexecuted throughsdk/fuzz.sh. This script is long running, and there are occasional Jazzer failures which are not believed to be real deficiencies (timeouts when.position()is called on the stream). For that reason this testing needs to be done manually, and not expected to be included in CII look forward to questions and recommendations, thank you!
PR closes #168