Scrubbers should be able to reach document-level nodes outside the root element

[flavorjones]

Problem

When scrub! runs on a Nokogiri::XML::Document, Loofah traverses only document.root. The document-level nodes that are siblings of the root, meaning processing instructions, comments, and the DTD, are never visited. A scrubber therefore has no way to inspect or remove them. A custom scrubber written to sanitize an XML document cannot see a document-level <?xml-stylesheet?> processing instruction, for example, even though it can see everything inside the root.

This surfaced through a security report from @cyberlanc3r, but I concluded that it is not a vulnerability in the built-in scrubbers, because those scrubbers are intended to sanitize HTML for delivery as text/html, and the retained processing instruction is only active when the output is served with an XML content type, which those scrubbers do not support.

The underlying API gap is real regardless of that conclusion: Loofah gives a scrubber no way to act on document-level nodes, which prevents the implementation of a custom XML scrubber that wishes to remove or modify those nodes.

One possible solution

Add an overridable hook, Scrubber#traverse_document, whose base implementation preserves today's behavior, and provide a concern (an includable module) that overrides it to remove the document-level nodes. A scrubber opts in by including the module.

# in #scrub! call traverse_document(self) instead of traverse(root)
case self
when Nokogiri::XML::Document
  scrubber.traverse_document(self)
...

# lib/loofah/scrubber.rb — base hook: traverse the root, unchanged from today
def traverse_document(document)
  traverse(document.root) if document.root
end

# lib/loofah/scrubbers.rb — an includable mixin that removes the prolog nodes
module PrologScrubber
  def traverse_document(document)
    if document.xml?
      prolog_nodes = document.children.to_a - [document.root]
      prolog_nodes.each(&:unlink)
    end
    super
  end
end

Custom scrubbers could use it like this:

class MyScrubber < Loofah::Scrubber
  include Loofah::Scrubbers::PrologScrubber

  def scrub(node)
    # your existing logic for nodes inside the root
  end
end

The mixin removes the prolog nodes by unlinking them, so it is independent of the scrubber's own scrub method and is safe to include in any scrubber. It applies only to whole-document scrubs, because traverse_document is not invoked for fragments or individual nodes.

FAQ

1. Why not simply call the scrubber's #scrub method on the document-level nodes?

An existing scrub method may not work outside of the DOM tree. The scrub method may add an element or add a text node as part of its operation. XML, however, does not allow arbitrary document-level nodes, and the underlying libraries (libxml2 or xerces) enforce those constraints.

Open questions

1. Should any of the built-in scrubbers include this by default? Given the security discussion above, the built-in HTML scrubbers probably could, but it's not a security boundary, and the question is worth settling explicitly.
2. HTML documents are gated out here with document.xml?, so their DOCTYPE is preserved. Is that the behavior we want, or should HTML documents be handled too?
3. The name. "Prolog" is the XML term for the nodes before the root, but this mixin also removes nodes that trail the root. A more accurate name may be warranted.
4. Wholesale removal versus a per-node decision. This mixin removes every non-root document-level node. Should scrubbers instead be able to make the same keep-or-remove decision on document-level nodes that they make inside the root?

Related

Separately, the built-in scrubbers should document clearly that they are HTML-only, and we should consider having them warn or raise when they are applied to an XML document, so the mismatch is visible rather than silent.

[cyberlanc3r]

Thanks for opening this publicly. I understand your conclusion on the advisory classification: if the built-in safelist scrubbers are scoped to HTML intended for text/html, then the retained xml-stylesheet PI is outside that supported security boundary.

The remaining concern for me is the document-level traversal/API gap. A custom XML scrubber invoked through doc.scrub!(scrubber) can reasonably appear to cover the full XML document, but today it only receives the root subtree. That means it cannot inspect or remove browser-significant document-level nodes such as <?xml-stylesheet?>, or parser-significant nodes such as DTDs, through the normal scrubber API.

I think a traverse_document hook is a good direction. It gives XML scrubbers an explicit place to handle document-level nodes without forcing existing #scrub implementations to operate on nodes where add/replace operations may be invalid.

On the open questions:

• Built-in scrubbers: If the built-in safelist scrubbers are considered HTML-only, warning or raising when they are applied to XML documents seems like the least surprising behavior. If they continue to run on XML documents, then the current silent root-only behavior should at least be clearly documented. The main thing I would want to avoid is users believing they scrubbed a whole XML document when document-level nodes were skipped.

• HTML documents: I would not include HTML documents in an XML document-level-node removal mixin by default. HTML document behavior has different compatibility expectations, especially around DOCTYPE preservation. If HTML document-level nodes need policy handling, I would treat that separately from XML.

• Naming: PrologScrubber is understandable for XML users, but since the proposed mixin also removes nodes after the root, a name like DocumentLevelNodeScrubber, DocumentBoundaryScrubber, or NonRootDocumentNodeScrubber may be more precise.

• Wholesale removal vs per-node decision: For a safety-oriented opt-in mixin, wholesale removal of non-root XML document-level nodes seems like a reasonable default. Processing instructions, comments, and DTDs outside the root are rarely expected in sanitized XML output and can affect downstream browser or XML parser behavior. Longer term, I think traverse_document should allow custom scrubbers to inspect document-level nodes and make per-node decisions for applications that need a more nuanced XML policy.

Overall, I think the important behavior for users is that whole-document XML scrubbing should not silently skip nodes outside document.root, especially when those nodes can affect downstream browser or XML parser behavior.

Thanks again!