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

Fix namespace switching bugs, release 3.1.2 #142

Merged
merged 4 commits into from
Jul 8, 2021
Merged

Fix namespace switching bugs, release 3.1.2 #142

merged 4 commits into from
Jul 8, 2021

Conversation

notriddle
Copy link
Member

@notriddle notriddle commented Jul 8, 2021

Reported as security vulnerability via private email.

The issue happens if developers added to the list of allowed tags any
tag which is parsed in RCDATA state, PLAINTEXT state or RCDATA state,
that is:

  • title
  • textarea
  • xmp
  • iframe
  • noembed
  • noframes
  • plaintext
  • noscript
  • style
  • script

An example in the wild is Plume, that allows iframe. So in next
examples I'll assume the following policy:

Builder::new()
   .add_tags(&["iframe"])

In HTML namespace <iframe> is parsed specially; that is, its content is
treated as text. For instance, the following html:

<iframe><a>test

Is parsed into the following DOM tree:

iframe
└─ #text: <a>test

So iframe cannot have any children other than a text node.

The same is not true, though, in "foreign content"; that is, within
<svg> or <math> tags. The following html:

<svg><iframe><a>test

is parsed differently:

svg
└─ iframe
   └─ a
      └─ #text: test

So in SVG namespace iframe can have children.

Ammonia disallows but it keeps its content after deleting it. And
the parser internally keeps track of the namespace of the element. So
assume we have the following snippet:

<svg><iframe><a title="</iframe><img src onerror=alert(1)>">test

It is parsed into:

svg
└─ iframe
   └─ a title="</iframe><img src onerror=alert(1)>"
      └─ #text: test

This DOM tree is harmless from ammonia point of view because the piece
of code that looks like XSS is in a title attribute. Hence, the
resulting "safe" HTML from ammonia would be:

<iframe><a title="</iframe><img src onerror=alert(1)>" rel="noopener noreferrer">test</a></iframe>

However, at this point, the information about namespace is lost, which
means that the browser will parse this snippet into:

├─ iframe
│  └─ #text: <a title="
├─ img src="" onerror="alert(1)"
└─ #text: " rel="noopener noreferrer">test

Leading to XSS.

To solve this issue, check for unexpected namespace switches after cleanup.
Elements which change namespace at an unexpected point are removed.
This function returns true if child should be kept, and false if it
should be removed.

@notriddle notriddle changed the title Fix namespace switching bugs Fix namespace switching bugs, release 3.1.2 Jul 8, 2021
Reported as security vulnerability via private email.
@notriddle notriddle requested a review from lnicola July 8, 2021 17:02
@lnicola
Copy link
Member

lnicola commented Jul 8, 2021

I can take a better look in an hour or two, but I didn't see anything obviously wrong.

@notriddle
Copy link
Member Author

Thanks.

I'm also filing a RUSTSEC advisory.

notriddle added a commit to notriddle/advisory-db that referenced this pull request Jul 8, 2021
Shnatsel pushed a commit to rustsec/advisory-db that referenced this pull request Jul 8, 2021
* Add rust-ammonia/ammonia#142

* Update RUSTSEC-0000-0000.md

* Update RUSTSEC-0000-0000.md
src/lib.rs Outdated Show resolved Hide resolved
@notriddle
Copy link
Member Author

Awesome. Let's go!

@lnicola
Copy link
Member

lnicola commented Jul 8, 2021

bora r+

@lnicola
Copy link
Member

lnicola commented Jul 8, 2021

@bors r+

?

@notriddle
Copy link
Member Author

bors ping

@bors
Copy link
Contributor

bors bot commented Jul 8, 2021

pong

@notriddle
Copy link
Member Author

bors r+

@notriddle
Copy link
Member Author

bora r+

Oops.

@bors
Copy link
Contributor

bors bot commented Jul 8, 2021

Build succeeded:

@bors bors bot merged commit bcdf2d8 into master Jul 8, 2021
@notriddle notriddle deleted the release-3.1.2 branch July 8, 2021 19:38
@notriddle
Copy link
Member Author

And both releases are now published!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants