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

Protect XML parsing against XXE attacks and load DTD from JAR #26

Merged
merged 2 commits into from
Oct 5, 2016

Conversation

solind
Copy link
Contributor

@solind solind commented Oct 5, 2016

  1. Take steps to guard against XXE attacks (note that <!DOCTYPE> cannot be disabled in XML plists). See: https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet#Java
  2. Resolve the actual PLIST DTD from inside the JAR file itself, and prevent resolution of other external XML resources.
  3. Make XMLPlistParser.getDocBuilder public.

…t <!DOCTYPE> cannot be disabled in XML plists).

2) Resolve the actual PLIST DTD from inside the JAR file itself, and prevent resolution of other external XML resources.
3) Make XMLPlistParser.getDocBuilder public
@3breadt
Copy link
Owner

3breadt commented Oct 5, 2016

First of all, I greatly appreciate your effort to make this library more secure.

Regarding 1)

The code, especially setting the features, needs to be tested under Android because it uses a different DocumentBuilder implementation than the Oracle JRE. And so far this library has remained Android compatible. Have you done this?

Regarding 2)

I would not redistribute the DTD which originates from http://www.apple.com/DTDs/PropertyList-1.0.dtd and thus may be copyrighted by Apple Inc.
That is why the current DocumentBuilder uses an EntityResolver which returns an empty input stream.

As DTD validation is anyway disabled I see no reason to change that behavior. Or does this in any way compromise the fix against XXE attacks?

@solind
Copy link
Contributor Author

solind commented Oct 5, 2016

Fair enough, I haven't tested whether these properties work on Android. I could wrap each in a try/catch block in case they're not supported. And I understand your copyright concern, I guess it's not strictly necessary to have there anyway unless one wants to validate the XML document. I'll update this PR.

@3breadt
Copy link
Owner

3breadt commented Oct 5, 2016

Sounds good, thanks!

@3breadt 3breadt merged commit 2120e77 into 3breadt:master Oct 5, 2016
@3breadt 3breadt self-assigned this Oct 5, 2016
@3breadt 3breadt changed the title Please consider a few minor changes Protect XML parsing against XXE attacks and load DTD from JAR Oct 5, 2016
@3breadt 3breadt mentioned this pull request May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants