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

Protect readLine() against DoS #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pixeebot[bot]
Copy link

@pixeebot pixeebot bot commented Dec 30, 2023

This change hardens all BufferedReader#readLine() operations against memory exhaustion.

There is no way to call readLine() safely since it is, by its nature, a read that must be terminated by the stream provider. Furthermore, a stream of data provided by an untrusted source could lead to a denial of service attack, as attackers can provide an infinite stream of bytes until the process runs out of memory.

Fixing it is straightforward using an API which limits the amount of expected characters to some sane limit. This is what our changes look like:

+ import io.github.pixee.security.BoundedLineReader;
  ...
  BufferedReader reader = getReader();
- String line = reader.readLine(); // unlimited read, can lead to DoS
+ String line = BoundedLineReader.readLine(reader, 5_000_000); // limited to 5MB
More reading

I have additional improvements ready for this repo! If you want to see them, leave the comment:

@pixeebot next

... and I will open a new PR right away!

Powered by: pixeebot (codemod ID: pixee:java/limit-readline)

@@ -72,7 +73,11 @@
<version>2.9.7</version>
</dependency -->

</dependencies>
Copy link
Author

Choose a reason for hiding this comment

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

This library holds security tools for protecting Java API calls.

License: MIT ✅ | Open source ✅ | More facts

Copy link
Author

pixeebot bot commented Jan 2, 2024

I'm confident in this change, but I'm not a maintainer of this project. Do you see any reason not to merge it?

If this change was not helpful, or you have suggestions for improvements, please let me know!

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.

2 participants