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

Jaxb2RootElementHttpMessageConverter is susceptible to XXE vulnerability [SPR-11376] #16003

Closed
spring-issuemaster opened this issue Jan 31, 2014 · 7 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Jan 31, 2014

Spase Markovski opened SPR-11376 and commented

For background information, see XXE vulnerability.

This seems to not have been fixed in Jaxb2RootElementHttpMessageConverter when it was fixed in Jaxb2CollectionHttpMessageConverter. The way it is solved in Jaxb2CollectionHttpMessageConverter is by hard coding the property for resolving external entities to false. See #15432 and the attached patch.

By default the XML parser will parse and replace external entities. Also there is no way to configure how Jaxb2RootElementHttpMessageConverter handles external entities.


Affects: 3.2.5

Attachments:

Issue Links:

  • #15432 Fix potential security risk when using Spring OXM
  • #15704 Disable the processing of external entities in SourceHttpMessageConverter by default
  • #16359 AbstractMarshaller should avoid SAXSource workaround when processExternalEntities=true

Backported to: 3.2.8

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 3, 2014

Spase Markovski commented

Btw, I just tested it and it seems #15432 is still not safe... Setting a IS_REPLACING_ENTITY_REFERENCES to false is NOT enough, and will result in an entity being injected.

To trigger an Unmarshall exception in this scenario (which I expect to be the desired behavior) you would need to set the following properties. The default value for each of these is true, unless overridden.

Option1
  • IS_SUPPORTING_EXTERNAL_ENTITIES = true
  • IS_REPLACING_ENTITY_REFERENCES = true
  • SUPPORT_DTD = false
Option2
  • IS_SUPPORTING_EXTERNAL_ENTITIES = false
  • IS_REPLACING_ENTITY_REFERENCES = true
  • SUPPORT_DTD = false
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 3, 2014

Rossen Stoyanchev commented

Hi, thanks for the comment. I added a test to Jaxb2CollectionHttpMessageConverterTests that demonstrate allowing and not allowing external content. See this commit. I wonder what is different in the scenario you're testing with? Could take a look and compare. Thanks!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 3, 2014

Spase Markovski commented

Hello and no problem. I have a complete test case for Jaxb2RootElementHttpMessageConverter just not sure where to post it, due to the sensitive nature of the issue. In my test case the only property I set is the same one that is set in Jaxb2CollectionHttpMessageConverter (IS_REPLACING_ENTITY_REFERENCES=false), and the value still gets injected.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 3, 2014

Spase Markovski commented

Hmm, Spring test suites are failing for Jaxb2CollectionHttpMessageConverter at my side at (line 140), where you are asserting an empty string.

I now have failing test cases for both the Jaxb2CollectionHttpMessageConverter and Jaxb2RootHttpMessageConverter (when using only a IS_REPLACING_ENTITY_REFERENCES=false).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 3, 2014

Rossen Stoyanchev commented

Spring test suites are failing for Jaxb2CollectionHttpMessageConverter

Oops, indeed they passed inside the IDE but fail from the command line. I suspect the IDE classpath is picking up XML-related dependencies from a dependent project. In any case I've committed a fix for the failing test. According to this page the only StAX XmlInputReaderFactory property that needs to be set to false is "javax.xml.stream.isSupportingExternalEntities". That seemed to do it.

I am also going to attach a patch for the Jaxb2RootElementHttpMessageConverter. If you are able to apply it locally and confirm the fix in both converters, that would be very helpful.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 4, 2014

Spase Markovski commented

Setting IS_SUPPORTING_EXTERNAL_ENTITIES = false will result in an empty string, but I don't think this should be the default behavior. I would expect an unmarshalling exception by default. Setting both IS_SUPPORTING_EXTERNAL_ENTITIES = false and SUPPORT_DTD = false will result in an unmarshalling exception.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 4, 2014

Rossen Stoyanchev commented

Arjen Poutsma, did you have any comments on default behavior when external entity processing is disabled? I.e. an exception vs essentially ignoring any external entity references.

All fixes so far (Jaxb2Marshaller, Jaxb2CollectionHttpMessageConverter, SourceHttpMessageConverter) including DOM, SAX, StAX parsing result in ignoring external entities. So a default has already been picked I'm afraid and we can't change it without causing regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.