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

Improve memory allocation and validation #401

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

Conversation

Marcono1234
Copy link
Contributor

This is a general proposal for how the commons-imaging code could better handle allocations to protect against malicious image data causing high memory consumption. The changes do not show any specific exploit or similar.

Even if you don't agree with the exact code changes here, maybe the general concept is still useful? Or feel free to pick what you like, and adjust it.


The general concept here is that allocations go through methods of the Allocator class to make it explicit whether allocations are assumed to be trusted (e.g. when writing image data) or untrusted (when reading image data). Of course this is not completely foolproof, but at least it makes the intentions of the developers more clear.
The methods for untrusted allocations can perform additional validations then, such as restricting the maximum size or when pre-sizing for example an ArrayList to clamp the initial capacity to a reasonable maximum to then see if the (potentially malicious) image data actually provides that many elements.

The other part is the new KnownSizeByteArrayBuilder class which allows building an byte[] with a known size, but without eagerly allocating it to make sure the input data actually contains that many bytes and has not been maliciously crafted. It also avoids the overhead of boxing which a List<Byte> would have, and also due to starting with a known expected size it avoids creating too large temporary arrays in the process, which general "byte array builder" classes might do.
However, possibly that KnownSizeByteArrayBuilder class can be replaced with similar Commons IO classes and methods if you prefer that.

These changes might not cover every aspect where memory allocation could possibly be exploited for a denial of service, but I think they cover at least some. I have not tested the effect this has on performance though.


Originally I provided these changes privately to the commons-imaging maintainers, but apparently did not got any response at all. So I hope at least this way here this adds some value, even if it just serves as inspiration for other changes.

I know that this merge request has conflicts, but I am not planning to resolve them unless there is actually strong interest in these changes. I was already not very happy about getting no feedback at all (despite being asked to "contribute to the solution of this issue") when I originally invested quite some time into this.

@garydgregory
Copy link
Member

@Marcono1234
Thank you for your PR but it conflicts with the current state of the master branch.
As a general process issue, you should start with a smaller PR, 51 files is too much to review for a delicate and likely complex topic.

@Marcono1234
Copy link
Contributor Author

Marcono1234 commented May 18, 2024

@garydgregory, yes I know that it has conflicts, and I can try to split this. My main goal was to show the general idea I had. The main changes are in Allocator and KnownSizeByteArrayBuilder, the other changes then show how this would be applied (and also fix some other small related issues).

However, quoting from the description of this pull request:

I know that this merge request has conflicts, but I am not planning to resolve them unless there is actually strong interest in these changes. I was already not very happy about getting no feedback at all (despite being asked to "contribute to the solution of this issue") when I originally invested quite some time into this.

Thanks anyway for at least having a short look at this already.

@fizzzzzz
Copy link

1

1 similar comment
@fizzzzzz
Copy link

1

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.

3 participants