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

Security: restore previous default limits prior to commit 2374ade #1329

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

lovell
Copy link
Contributor

@lovell lovell commented Oct 10, 2024

Commit 2374ade allows security limits to be set per context but also removed/altered a couple of the original limits. Removing the max_iloc_items limit opens libheif to a denial of service attack. This PR restores the previous defaults.

@farindk
Copy link
Contributor

farindk commented Oct 10, 2024

You are right that we should have some limits there by default.

The reason for changing those numbers was that while experimenting with large tiled images, I often hit those limitations. Now I think that these limits should be computed differently. For example, the max_children_per_box should probably depend on the parent box.

Since setting the limits has been opened to the user, we might express them in simpler terms. It's probably not easy to see what a max_iloc_items does. If we can compute this from something like max_images_in_file it might be easier to understand and control by the user.

@lovell
Copy link
Contributor Author

lovell commented Oct 10, 2024

Thanks Dirk, I agree the defaults can be improved, this PR is attempting only to restore the previous defaults to reduce the noise levels we've been experiencing from libvips fuzz tests over the last couple of days.

@farindk farindk merged commit feef3c1 into strukturag:master Oct 10, 2024
35 checks passed
@lovell lovell deleted the security-limits-restore branch October 10, 2024 16:32
farindk added a commit that referenced this pull request Oct 14, 2024
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