Skip to content

Conversation

@ggolawski
Copy link
Contributor

This PR adds a query to detect XSLT injections. It flags the code where user-provided XSLT stylesheet is processed by Transformer.transform. The following use cases are supported:

  • XSLT stylesheet provided as StreamSource:
StreamSource source = new StreamSource(socket.getInputStream());
TransformerFactory.newInstance().newTransformer(source).transform();
  • XSLT stylesheet provided as SAXSource:
SAXSource source = new SAXSource(new InputSource(socket.getInputStream()));
TransformerFactory.newInstance().newTransformer(source).transform();
  • XSLT stylesheet provided as StAXSource:
StAXSource source = new StAXSource(XMLInputFactory.newInstance().createXMLEventReader(socket.getInputStream()));
TransformerFactory.newInstance().newTransformer(source).transform();
  • XSLT stylesheet provided as DOMSource:
DOMSource source = new DOMSource(DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(socket.getInputStream()));
TransformerFactory.newInstance().newTransformer(source).transform();
  • Transformer object created from Templates:
TransformerFactory.newInstance().newTemplates(source).newTransformer().transform();

Processing of unvalidated XSLT stylesheets can lead to XXE or remote code execution.

This query partially overlaps with XXE query from XXE.ql, but has the following noticeable differences:

  • XXE query doesn't raise the flag if ACCESS_EXTERNAL_STYLESHEET and ACCESS_EXTERNAL_SCHEMA are disabled. Disabling these options is enough to prevent XXE, but not enough to prevent RCE via XSLT injection. To prevent it, FEATURE_SECURE_PROCESSING must be enabled.
  • In some cases, XXE query highlights the source code line where XSLT stylesheet is parsed (e.g. to create a Document). XsltInjection query always highlights the source code line where the transformation happens (Transformer.transform method invocation) - this is the place where XSLT injection (which can lead to RCE) happens.
  • The case where Transformer is created from Templates (TransformerFactory.newTemplates(source).newTransformer().transform()) is not supported by XXE query.

The tests are also included.

@pwntester
Copy link
Contributor

@ggolawski could you please add support for SAXON (e.g: net.sf.saxon.s9api.XsltCompiler) and XALAN (e.g: org.apache.xalan.xslt.XSLTProcessor)?

@ggolawski
Copy link
Contributor Author

@pwntester I'll add support for SAXON.
Regarding Xalan, it looks like org.apache.xalan.xslt.XSLTProcessor was part of Xalan 1.x released in 2000 (?). It's very hard to find any distribution or documentation. It doesn't exist in Maven central either. And there're only ~200 references to org.apache.xalan.xslt.XSLTProcessor on GitHub (https://github.com/search?l=Java&q=%22org.apache.xalan.xslt.XSLTProcessor%22&type=Code).
Xalan 2.x implements javax.xml.transform interface and is handled by the current query implementation.

Does it make sense to add support for Xalan 1.x? What do you think?

@pwntester
Copy link
Contributor

Makes sense not to support 1.x than, thanks!

@ggolawski ggolawski requested a review from a team as a code owner May 14, 2020 22:12
@ggolawski
Copy link
Contributor Author

@pwntester I've addedd support for Saxon. The following cases are supported:

public void testSaxon(Socket socket) throws Exception {
    StreamSource source = new StreamSource(socket.getInputStream());
    XsltCompiler compiler = new Processor(true).newXsltCompiler();
    
    compiler.compile(source).load().transform();
    compiler.compile(source).load30().transform(null, null);
    compiler.compile(source).load30().applyTemplates((Source) null);
    compiler.compile(source).load30().applyTemplates((Source) null, null);
    compiler.compile(source).load30().applyTemplates((XdmValue) null);
    compiler.compile(source).load30().applyTemplates((XdmValue) null, null);
    compiler.compile(source).load30().callFunction(null, null);
    compiler.compile(source).load30().callFunction(null, null, null);
    compiler.compile(source).load30().callTemplate(null);
    compiler.compile(source).load30().callTemplate(null, null);
  }

  public void testSaxonXsltPackage(@RequestParam String param, Socket socket) throws Exception {
    URI uri = new URI(param);
    StreamSource source = new StreamSource(socket.getInputStream());
    XsltCompiler compiler = new Processor(true).newXsltCompiler();
    
    compiler.loadExecutablePackage(uri).load().transform();
    compiler.compilePackage(source).link().load().transform();
    compiler.loadLibraryPackage(uri).link().load().transform();
  }

Copy link
Contributor

@felicitymay felicitymay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have mentioned earlier, I was added as a reviewer automatically by the CODEOWNERS file. However this PR doesn't need a review from the docs team because it's changing the experimental directory. We've since updated the CODEOWNERS file to reflect this.

@adityasharad adityasharad changed the base branch from master to main August 14, 2020 18:34
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The test needs a minor tweak, but otherwise this looks ready to merge.

@ggolawski ggolawski requested a review from aschackmull August 30, 2020 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants