Skip to content

apply allocation bounds check to EmfExtCreatePen style entries#1113

Merged
pjfanning merged 1 commit into
apache:trunkfrom
metsw24-max:extcreatepen-style-entries-bound
Jun 4, 2026
Merged

apply allocation bounds check to EmfExtCreatePen style entries#1113
pjfanning merged 1 commit into
apache:trunkfrom
metsw24-max:extcreatepen-style-entries-bound

Conversation

@metsw24-max
Copy link
Copy Markdown
Contributor

Unbounded dash array in EmfExtCreatePen

NumStyleEntries is read straight from the EMR_EXTCREATEPEN record and used as the length of the dash-pattern array with no cap, so a crafted metafile allocates an arbitrary float array, and a high-bit value goes negative once narrowed by the (int) cast. I added the same safelyAllocateCheck the sibling pen and poly decoders already use, so an oversize count raises RecordFormatException like the rest of the record.

// If the pen type in the PenStyle field is PS_GEOMETRIC, the lengths are specified in logical
// units; otherwise, the lengths are specified in device units.

IOUtils.safelyAllocateCheck(numStyleEntries, HwmfPicture.getMaxRecordLength());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this seems bad - why use use hwmf limit in hemf?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch, that was reaching into the hwmf limit by mistake. now that #1114 has landed the hemf-specific limit i've rebased and switched this to HemfPicture.safelyAllocateCheck, same as the polyline decoder in HemfDraw.

@pjfanning
Copy link
Copy Markdown
Member

I must admit that I'm getting very bored of PRs that apply these limits.
POI is over 20 years old and has a lot of code. It was not originally written with security in mind. It is a free convenience library.
This new code - all it does is switch from one type of exception to a different type of exception. So unprotected code might throw an Array Index exception or even OutOfMemoryException. Parse enough documents in parallel and you can still get OutOfMemoryExceptions.
https://poi.apache.org/security.html warns that you should never parse untrusted documents with POI.
We are introducing new problems with this code.
The limits might be too low for some reasonable files. We now have dozens of different limits and the exceptions raised don't tell you what method to use to raise the limit.
Even if we add some new tests, we are not getting 100% test coverage or even close to that number.

I'm thinking of raising a general discussion among the POI volunteers about just giving up on this sort of work.

@metsw24-max metsw24-max force-pushed the extcreatepen-style-entries-bound branch from ecc78a3 to c5fb2f1 Compare June 4, 2026 12:38
@metsw24-max
Copy link
Copy Markdown
Contributor Author

that's fair, and i understand the fatigue. you're right that this mostly swaps one exception for another and the security page is clear that untrusted input isn't supported, so i won't keep pushing more of these. happy to leave the broader call to you and the other volunteers. i've rebased this one to use the new hemf limit from #1114 so at least it's consistent with the rest of the package, but no objection if you'd rather close it.

@metsw24-max metsw24-max force-pushed the extcreatepen-style-entries-bound branch from c5fb2f1 to f03f4d9 Compare June 4, 2026 18:30
@pjfanning pjfanning merged commit 69f8eb6 into apache:trunk Jun 4, 2026
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