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

DOMDeserializer: setExpandEntityReferences(false) may not prevent external entity expansion in all cases [CVE-2020-25649] #2589

Closed
baranowb opened this issue Jan 9, 2020 · 22 comments
Labels
CVE Issues related to public CVEs (security vuln reports)
Milestone

Comments

@baranowb
Copy link

baranowb commented Jan 9, 2020

As per description:
https://github.com/FasterXML/jackson-databind/blob/master/src/main/java/com/fasterxml/jackson/databind/ext/DOMDeserializer.java#L30
and
https://github.com/FasterXML/jackson-databind/blob/master/src/main/java/com/fasterxml/jackson/databind/ext/DOMDeserializer.java#L33

is not enough to stop expansion of entities. Depending on provider(xerces) being used it might work with current DOMDeserializer or not. If JDK default is used(at least one that I used at time of test), it wont allow to expand entities, however, if other provider from classpath is used it might, for instance, xerces-2.12.... does allow( iirc) expansion.

Reference: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#jaxp-documentbuilderfactory-saxparserfactory-and-dom4j

I tinkered a bit with databind classes and I had something like:

factory.setValidating(true);
factory.setExpandEntityReferences(false);
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
@cowtowncoder
Copy link
Member

First of all, thank you for reporting this security concern.

One quick note/question: you should not enable validation, since that would basically require expansion -- you can not do that, and it is not something Jackson itself sets.
I wonder if that is a requirement for problem to occur?

Aside from that, I'll see if adding missing settings from the list of 3 mentioned is safe wrt JDK baselines Jackson expects.

@cowtowncoder
Copy link
Member

Ok so specifically while other 2 settings are already applied, this could be added:

factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);

It is not entirely risk-free change, actually, since it does not only block entity expansion that we want, but also outlaws use of DOCTYPE declaration which theoretically could cause issues for someone.
For that reason this will need to go in 2.11, not 2.10 patch.

One more setting that could be considered, from the linked-to page, would be:

factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);

which I'll probably add too.

@cowtowncoder cowtowncoder added 2.11 and removed 2.10 labels Jan 9, 2020
@cowtowncoder cowtowncoder changed the title #1279 - setExpandEntityReferences(false); is not enough to stop expansion #1279 - setExpandEntityReferences(false) may not prevent external entity expansion in all cases Jan 9, 2020
@cowtowncoder cowtowncoder added this to the 2.11.0 milestone Jan 10, 2020
@cowtowncoder cowtowncoder changed the title #1279 - setExpandEntityReferences(false) may not prevent external entity expansion in all cases DOMDeserializer: setExpandEntityReferences(false) may not prevent external entity expansion in all cases Jan 10, 2020
@cowtowncoder cowtowncoder removed the 2.11 label Apr 12, 2020
@cowtowncoder cowtowncoder added the CVE Issues related to public CVEs (security vuln reports) label Oct 13, 2020
@cowtowncoder
Copy link
Member

This problem was assigned following vuln id: CVE-2020-25649

@cowtowncoder cowtowncoder changed the title DOMDeserializer: setExpandEntityReferences(false) may not prevent external entity expansion in all cases DOMDeserializer: setExpandEntityReferences(false) may not prevent external entity expansion in all cases [CVE-2020-25649] Oct 13, 2020
jgallimore pushed a commit to jgallimore/jackson-databind that referenced this issue Oct 22, 2020
cowtowncoder added a commit that referenced this issue Oct 23, 2020
Co-authored-by: Tatu Saloranta <[email protected]>
@cowtowncoder
Copy link
Member

cowtowncoder commented Oct 23, 2020

Update: backported in potential future versions:

  • 2.6.7.4 (released 25-Oct-2020)
  • 2.9.10.7 (released 02-Dec-2020)
  • 2.10.5.1 (released 02-Dec-2020)

(in addition to being included in 2.11.0 and later)

@nloke
Copy link

nloke commented Nov 3, 2020

@cowtowncoder
I saw the code being backported to 2.10.6 and we needed that for this CVE. Do you know when will 2.10.6 be released?

@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 3, 2020

@nloke No immediate plan; it might be that no full 2.10.6 will be released but 2.10.5.1 for databind. And if so, would probably want to do around same time as 2.9.10.7 (the last micro-patch for 2.9, as 2.9 goes EOL by end of 2020).
So depending a bit on all around progress, either towards end of November or in December.

@sourabhsparkala
Copy link

@nloke No immediate plan; it might be that no full 2.10.6 will be released but 2.10.5.1 for databind. And if so, would probably want to do around same time as 2.9.10.7 (the last micro-patch for 2.9, as 2.9 goes EOL by end of 2020).
So depending a bit on all around progress, either towards end of November or in December.

Hello, is there a planned date for 2.10.6 or 2.10.5.1 with the above fix?

@cowtowncoder
Copy link
Member

@sourabhsparkala due to variability in time I have for development and all related tasks, I typically do not set out dates for various releases. This is relatively low priority thing for me because it's just a single fix and while there's cve id registered it seems unlikely to be exploitable by anybody.

But I will go ahead and create a separate issue for that micro-patch so I won't forget. Maybe I'll get patch out by November -- main limitation is that I will not want to release any more versions of 2.10.x and after this patch will probably close 2.10 branch for good. 2.11.x has been out for a while, and 2.12.0 should be out in November.
I think 2.10.5.1 will be released right after 2.12.0.

@radarsh
Copy link

radarsh commented Nov 30, 2020

@cowtowncoder now that 2.12.0 is out, when can we expect 2.10.5.1? Apologies if this is already being considered.

@cowtowncoder
Copy link
Member

Yes, #2920 is on my todo list after blogging about 2.12.0 release.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 2, 2020

Updated notes above: now fix included in:

  • 2.6.7.4
  • 2.9.10.7
  • 2.10.5.1
  • 2.11.0 and later

@cowtowncoder cowtowncoder modified the milestones: 2.11.0, 2.9.10.7 Dec 2, 2020
@melloware
Copy link

I am confused is this fixed in 2.11.0 or 2.12.0? Because Sonatype is reporting this as a security vulnerability in 2.11.0 still?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CVE Issues related to public CVEs (security vuln reports)
Projects
None yet
Development

No branches or pull requests

10 participants