Skip to content

fix setFeature bug and add test case#188

Merged
hunterhacker merged 1 commit into
hunterhacker:masterfrom
esti-burstein:SetFeatureBugFix
Jun 24, 2021
Merged

fix setFeature bug and add test case#188
hunterhacker merged 1 commit into
hunterhacker:masterfrom
esti-burstein:SetFeatureBugFix

Conversation

@esti-burstein

Copy link
Copy Markdown

No description provided.

@abergmann

Copy link
Copy Markdown

CVE-2021-33813 was assigned to this PR.

@sourabhsparkala

sourabhsparkala commented Jun 21, 2021

Copy link
Copy Markdown

Hello,

I deal with the security aspects in OSS libs within SAO.
I was wondering if there any way I can help you with the fix and preparing a release. Please let me know.

Thanks
Sourabh

@hunterhacker hunterhacker merged commit 2a25ee4 into hunterhacker:master Jun 24, 2021
@emilylin2798

Copy link
Copy Markdown

Is there any intention to incorporate this fix in a new version release?

@hunterhacker

Copy link
Copy Markdown
Owner

Yes, sure, at some point, but remember if you're worried about external entities being expanded, just call builder.setExpandEntities(false) and they won't be. That's what it's there for.

@rolfl

rolfl commented Jul 2, 2021

Copy link
Copy Markdown
Collaborator

This PR changes the order of precedence between setExpandEntities and user-supplied features. It's a relatively small risk, but this PR will change existing code behaviour in the case where existing code has set the "http://xml.org/sax/features/external-general-entities" feature to a value that conflicts with setExpendEntities.

Previously, setExpendEntities overruled user-supplied features, and now the opposite is true.

Note, JDOM tries to "isolate" users from needing to know the internal guts of what underlying parsers are used, so the setExpendEntities() makes it simpler to manage the feature than knowing exactly which parser is being used, and whether it supports the feature change.

Further, the way I read this, I think it's possible now to set conflicting features with "http://xml.org/sax/handlers/DeclHandler"

This may need to be tweaked.

@rolfl

rolfl commented Jul 2, 2021

Copy link
Copy Markdown
Collaborator

I think the better implementation for this fix would be to track whether setExpandEntities has been explicitly set, and if not, base it on whether the http://xml.org/sax/features/external-general-entities feature was explicitly set in the user overrides.

@rolfl

rolfl commented Jul 2, 2021

Copy link
Copy Markdown
Collaborator

Note, improved this regression with dd4f3c2

Synchronize setFeature and setExpandEntities so they can only work together.

@schlm3

schlm3 commented Jul 9, 2021

Copy link
Copy Markdown

The workaround of "builder.setExpandEntities(false)" can only be applied to "own" code, but not the the many usages of JDOM inside our many dependencies. So please come up with a fixed release soon.

@balgillo

Copy link
Copy Markdown

Can you publish CVE-2021-33813 as a security advisory on this project so that it shows up in GitHub Advisories?

Even if you're going to fix it in the next version, it's useful for it to be reported against the previous versions (and it will encourage your users to upgrade too).

@muthuIntern

Copy link
Copy Markdown

Is there any possibility to incorporate this security fix and make a new version release of JDOM ?

@stephanborn

Copy link
Copy Markdown

This fix is available and merged since June 2021. Is there any reason why this important fix is not released yet?

@hunterhacker

Copy link
Copy Markdown
Owner

Feel free to make a build for yourself, since the pull request has been merged in. If you want it in Maven, I'm working on proving identity to get credentials to push there. Rolf had them and he's not responding.

@ar

ar commented Dec 6, 2021

Copy link
Copy Markdown

Thank you Jason for your effort.

Hope that a link to this issue could be a good proof of identity for Sonatype to grant you access.

@hunterhacker

Copy link
Copy Markdown
Owner

Looks like being able to add TXT entries to the jdom.org DNS will suffice, which is something I can do.

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.