Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-11376

Jaxb2RootElementHttpMessageConverter is susceptible to XXE vulnerability

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Complete
    • Affects Version/s: 3.2.5
    • Fix Version/s: 3.2.8, 4.0.2
    • Component/s: Web
    • Security Level: Public
    • Labels:
    • Last commented by a User:
      false

      Description

      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 SPR-10806 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.

        Issue Links

          Activity

          Hide
          berzerker Spase Markovski added a comment - - edited

          Btw, I just tested it and it seems SPR-10806 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
          Show
          berzerker Spase Markovski added a comment - - edited Btw, I just tested it and it seems SPR-10806 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
          Hide
          rstoya05-aop Rossen Stoyanchev added a comment -

          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!

          Show
          rstoya05-aop Rossen Stoyanchev added a comment - 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!
          Hide
          berzerker Spase Markovski added a comment -

          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.

          Show
          berzerker Spase Markovski added a comment - 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.
          Hide
          berzerker Spase Markovski added a comment - - edited

          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).

          Show
          berzerker Spase Markovski added a comment - - edited 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).
          Hide
          rstoya05-aop Rossen Stoyanchev added a comment -

          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.

          Show
          rstoya05-aop Rossen Stoyanchev added a comment - 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.
          Hide
          berzerker Spase Markovski added a comment -

          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.

          Show
          berzerker Spase Markovski added a comment - 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.
          Hide
          rstoya05-aop Rossen Stoyanchev added a comment -

          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.

          Show
          rstoya05-aop Rossen Stoyanchev added a comment - 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.

            People

            • Assignee:
              rstoya05-aop Rossen Stoyanchev
              Reporter:
              berzerker Spase Markovski
              Last updater:
              Juergen Hoeller
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                1 year, 15 weeks, 5 days ago