feat: add "limits:resolution" global attribute to cap per-dimension size#5297
Conversation
|
This new setting is confusing because the existing FWIW I've been finding that the existing check_open calls are conflating both the file format's "valid range" and our hopeful wish at detecting "invalid ranges" E.g. we got tripped up after release at one point due to an internal baking process wanting to produce a 1x4'194'304 EXR (it was a lookup data file really). But that's not permitted because of EXR's |
|
Yeah, I hear what you're saying. It is admittedly getting a little confusing. The checks are all done centrally by check_open(), but the values for the checks come from two places:
Essentially, the more stringent of those constraints is the operative one. I'm 100% open to hearing that I've chosen any of these values wrong. Often they were best guesses about "how big are we confident we will never see in real life for a legit file?" Do you think we shouldn't add the new linear resolution limit at all, and ONLY check the "legal" limits of each file format and the single overall "total memory" limit? Maybe as we finally plug the last of the holes related to int overflow, the linear limits will be less important? I was just trying to find another form of low-hanging fruit heuristics for what looks like an obviously bogus file, but an exr that is a 1x4M lookup table definitely is a counter-point to what I was trying to do. |
|
Having something like My feedback was just alluding to that if we get this new policy value, it means we should also adjust the |
|
Yes, I agree! Because we didn't have a configurable "limits:resolution" when we first set up check_open, there are some places where file formats have very permissive actual limits, and we kind of co-mingled common sense limits. Now we have a clean way to separate the two concerns. OpenEXR, in particular, can go back to a full uint32 range of spec-permitted resolution. (In your listing above, are you just showing the lines to remind me of their locations, or have you already adjusted each one to be what you think the format supports?) |
|
Just there as a reminder that those locations probably all need to change. They are all currently set to use 1<<20 at the moment. |
|
So updated. I added the following explanation to the commit description: Now then, with this in place, we can clean up some of the values we used in passing valid resolution ranges to check_open(). That was originally meant to express the true limits of what was valid in the file formats, but had in many cases been conflated with common sense "what value is so big that it's probably bogus". Now we can disentangle these ideas -- the value passed to check_open is the definitional limit of the file format itself, and the "limits:resolution" is the overrideable global policy for what we consider more likely to be malicious/corrupted than merely legit big. |
The existing "limits:imagesize_MB" rejects files whose total uncompressed size exceeds a cap, but it cannot catch an image that is tiny in one dimension yet absurdly large in another (e.g. an image claiming 32 x 50331680, which slips under the 32 GB default). Add a companion global attribute, "limits:resolution", that caps the maximum number of pixels along any single dimension. Like imagesize_MB, this is enforced only in ImageInput::check_open(), intended for validity checks when reading untrusted files. Applications may set the attribute larger or smaller than this default, depending on their need to deal with images larger than these limits, and/or willingness to risk mistaking a corrupted or maliciously crafted image for a legitimate one on this basis. Now then, with this in place, we can clean up some of the values we used in passing valid resolution ranges to check_open(). That was originally meant to express the true limits of what was valid in the file formats, but had in many cases been conflated with common sense "what value is so big that it's probably bogus". Now we can disentangle these ideas -- the value passed to check_open is the definitional limit of the file format itself, and the "limits:resolution" is the overrideable global policy for what we consider more likely to be malicious/corrupted than merely legit big. Assisted-by: Claude Code / Claude Opus 4.8 Signed-off-by: Larry Gritz <lg@larrygritz.com>
The existing "limits:imagesize_MB" rejects files whose total uncompressed size exceeds a cap, but it cannot catch an image that is tiny in one dimension yet absurdly large in another (e.g. an image claiming 32 x 50331680, which slips under the 32 GB default). Add a companion global attribute, "limits:resolution", that caps the maximum number of pixels along any single dimension.
Like imagesize_MB, this is enforced only in ImageInput::check_open(), intended for validity checks when reading untrusted files.
Applications may set the attribute larger or smaller than this default, depending on their need to deal with images larger than these limits, and/or willingness to risk mistaking a corrupted or maliciously crafted image for a legitimate one on this basis.
Now then, with this in place, we can clean up some of the values we used in passing valid resolution ranges to check_open(). That was originally meant to express the true limits of what was valid in the file formats, but had in many cases been conflated with common sense "what value is so big that it's probably bogus". Now we can disentangle these ideas -- the value passed to check_open is the definitional limit of the file format itself, and the "limits:resolution" is the overrideable global policy for what we consider more likely to be malicious/corrupted than merely legit big.
Assisted-by: Claude Code / Claude Opus 4.8