Adding secure configuration for XML parsers

[tophersmith]

This PR properly configures both the DocumentBuilderFactory and TransformerFactory instances in the XmlProcessor class to prevent XML eXternal Entity (XXE) attacks as recommended by OWASP.

A side note for anybody reading this in the future: The original implementation was not actually vulnerable to this attack using default built-in DBF or TF instances as by a set of slightly unique occurrences the code dodged all of the known attack types.

For example, XXE attacks are typically run by adding DOCTYPE and ENTITY declarations to the XML to trick the Java parsers into making protocol requests for more information to resolve malicious entities - file:// pulls in file and directory information into the XML doc, and http(s):// (s)ftp:// can trick the target server into making requests on behalf of an attacker (this is known as SSRF). Because the XmlProcessor wraps all incoming xml in a synthetic parent, This attack is nullified as DOCTYPEs and ENTITY defs can only be made before the first data tag.

The other attack vectors usually rely on XML implementations (XSLT and XSD) that require gathering more information from external sources, but the TransformerFactory's usage of output=XML and DOMSource negate that as well.

The good news about this is that this code fix should not have any behavior changes, as this will further enforce a case that can't happen out of the box.

All this being said, the PR should be included as this codefix protects against cases where an application may be using a non-default XML parser (most Java XML parsers are loaded via Service Loading or system properties at runtime with a fallback of one of the defaults). The PR sets all available best practice features or properties to catch these additional implementations.

[tophersmith]

This resolves #479

[tophersmith]

Looks like a test failure in regress-363258.js which is some kind of timer test? I will lay odds that a re-build will not have this come up.

[gbrail]

Thank you. I agree that this is something we should fix! I do think that we need to look in to three things, however:

First, Rhino is an old codebase that's used in lots of places, and in theory this will break backward compatibility. Therefore, I think that we need to add a flag to determine whether we configure the parser in "secure" mode. You can look at the Context and ContextFactory classes to see how feature flags are implemented, and then at how those flags are used to see how to check them. It's a common pattern in Rhino.

I would also suggest a flag with a name like "FEATURE_DISABLE_XML_EXTERNAL_ENTITIES" or something similar that will make it clear what it does.

However, since this is a potential security vulnerability, I think that the flag should be on by default. (That is, "disable XML external entities" should default to "true" in ContextFactory.hasFeature()). But by adding this flag, if someone does indeed depend on Rhino to process external entities, they still can.

Second, especially since we have a flag, I'd really like to see tests that check with the flag set both ways. (Adding a new file to testsrc/org/mozilla/rhino/javascript/tests is a fine way to do it.) I'm not sure how you'd test pulling down an external entity, however, but perhaps there is some other XML feature we can test for?

Third, I'm not 100% sure what we need to do if we cut and paste code from the OWASP GitHub repository, since it uses a different license from Rhino. I think I know a few people to ask.

[tophersmith]

@gbrail Thanks for the review. I've incorporated your suggestions. Feature switch is in place and I've added two tests for the PR. Unfortunately, the only way I could figure out a test case is not thread-safe, so we may want to look into how much we want this or if we can add some advice to the test runs. We might have a conflict if two tests that use XML happen simultaneously.

I don't have much to add on the direct copy (#3) point you raised, but OWASP Cheatsheets are under CC3, with allowance on copying and modifications, though I'm not sure if that's on the data sheet or the code as well.

[gbrail]

Unfortunately, the license on that GitHub code has the "ShareAlike" flag, set which puts some restrictions on the project if we "build upon" the code. A lawyer I informally consulted with suggested not incorporating that code, and that's the best advice I can get for now.

Sorry to be so literal about this, but these license things are difficult.

I see that Java documents two products that turn off external identification:

https://docs.oracle.com/en/java/javase/12/docs/api/java.xml/javax/xml/parsers/DocumentBuilderFactory.html#setAttribute(java.lang.String,java.lang.Object)

What if we just wrote our own code to set those two?

[tophersmith]

Hi @gbrail , I looked into this a bunch more. I'm committing a new version after reading through a bunch of confusing sun an javax code... The two attributes mentioned aren't quite enough to completely lock down the potential attacks (they don't handle ENTITY values or XIncludes)

So I've instead rewritten my code from my own research and it's significantly more concise (OWASP's recommendations are semi-tiered it seems, or just exhaustive and overlapping).

• All Doctype declarations are disabled (which is already kind of true from my first comment)
• XIncludes and external dtds are disabled in case the underlying parser is configured to allow them.

I've also extracted the configuration for the TransformerFactory to match the DBF setup.

[tophersmith]

Test failure is the timer resolution again

[gbrail]

This is very thoroughly done -- thanks! Just to keep things clean, I'm going to squash your commits and merge them. It also looks like we have another flaky test on Travis or it's time to find another CI setup...

[tirsoh]

Great to see this fix has been merged! Any idea when we can get a release with this change?

[tirsoh]

@gbrail Any update on when we can get a release of this?

[gbrail]

It's true that it doesn't take very long, but at the same time there are a few other things in flight. I'd like to do it around the end of the year. If that's a big problem then I can try to find more time.

…

On Wed, Nov 27, 2019 at 4:38 AM Troy Steffen ***@***.***> wrote: @gbrail <https://github.com/gbrail> Any update on when we can get a release of this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#600?email_source=notifications&email_token=AAD7I26NK5U6DUCERWRITQTQVZS3NA5CNFSM4IXYMFTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFJL2XY#issuecomment-559070559>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I24W5Y45GVW7YGGY5YLQVZS3NANCNFSM4IXYMFTA> .

[tirsoh]

@gbrail just checked to see if a release was available since it is the new year now but I don't see anything yet. Got any idea when you might be able to get a release cut?