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

Add zip bomb tests #1300

Merged
merged 85 commits into from
Apr 6, 2023
Merged

Add zip bomb tests #1300

merged 85 commits into from
Apr 6, 2023

Conversation

StephenButtolph
Copy link
Contributor

@StephenButtolph StephenButtolph commented Apr 5, 2023

Why this should be merged

Verifies that our decompression is not vulnerable to a zip bomb attack.

How this works

Attempts to decompress pre-generated zip bombs. It verifies that decompressing the bombs doesn't crash and that it doesn't allocate significantly more memory than an expected message size.

How this was tested

It is a test

Base automatically changed from add-zstd-compression to dev April 5, 2023 23:47
@StephenButtolph StephenButtolph changed the base branch from dev to simplify-gzip-compression April 6, 2023 00:31
@StephenButtolph StephenButtolph self-assigned this Apr 6, 2023
@StephenButtolph StephenButtolph added the testing This primarily focuses on testing label Apr 6, 2023
@StephenButtolph StephenButtolph added this to the v1.10.0 (Cortina) milestone Apr 6, 2023
@StephenButtolph StephenButtolph changed the title Add zipbomb tests Add zip bomb tests Apr 6, 2023
@StephenButtolph StephenButtolph added the networking This involves networking label Apr 6, 2023
Base automatically changed from simplify-gzip-compression to dev April 6, 2023 01:12
// Make sure that we didn't allocate significantly more memory than
// the max message size.
bytesAllocatedDuringDecompression := afterDecompressionStats.TotalAlloc - beforeDecompressionStats.TotalAlloc
require.Less(t, bytesAllocatedDuringDecompression, uint64(10*maxMessageSize))
Copy link

Choose a reason for hiding this comment

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

Kinda interesting that you have to make this at least 6 to pass 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just because of how the underlying bytes buffer grows afaict.

@StephenButtolph StephenButtolph merged commit 87ba1ac into dev Apr 6, 2023
@StephenButtolph StephenButtolph deleted the add-zip-bomb-tests branch April 6, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking This involves networking testing This primarily focuses on testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants