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

Prevent injection vulnerability in image page #12679

Merged
merged 1 commit into from Aug 1, 2016

Conversation

Projects
None yet
6 participants
@johannhof
Contributor

johannhof commented Aug 1, 2016

This is taking up nox' suggestion from #12542 and creates an img element using Rust code instead of escaping the URL. I will look at the neterror.html URL strings separately, we might do those in a similar way.

To reproduce, visit e.g. the following URL with your vulnerable Servo:

https://servo.org/screenshot.png?'onload='document.body.innerHTML=`hacked`'

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #12542
  • These changes do not require tests because this is just fixing up existing behavior and I'm not sure how to test it

r?@jdm


This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Aug 1, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/servohtmlparser.rs
@highfive

This comment has been minimized.

highfive commented Aug 1, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Manishearth

This comment has been minimized.

Member

Manishearth commented Aug 1, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 1, 2016

📌 Commit c4e4ccb has been approved by Manishearth

@highfive highfive assigned Manishearth and unassigned jdm Aug 1, 2016

@@ -112,9 +117,16 @@ impl AsyncResponseListener for ParserContext {
match content_type {
Some(ContentType(Mime(TopLevel::Image, _, _))) => {
self.is_synthesized_document = true;
let page = format!("<html><body><img src='{}' /></body></html>", self.url);
let page = format!("<html><body></body></html>");

This comment has been minimized.

@notriddle

notriddle Aug 1, 2016

Contributor

Does this really need format still?

This comment has been minimized.

@johannhof

johannhof Aug 1, 2016

Contributor

Nope, I just glanced at the block below and that one also has an unneeded format!, so I thought I'd stay consistent. I could change both, what would you prefer?

This comment has been minimized.

@Manishearth

Manishearth Aug 1, 2016

Member

Nope 😄

@Manishearth

This comment has been minimized.

Member

Manishearth commented Aug 1, 2016

@bors-servo r-

Please remove the format!. While you're at it, try to replace the expect message with something more descriptive.

r=me post nits

@johannhof johannhof force-pushed the johannhof:image-inject branch from c4e4ccb to ff6283a Aug 1, 2016

@johannhof

This comment has been minimized.

Contributor

johannhof commented Aug 1, 2016

@Manishearth updated

@Manishearth

This comment has been minimized.

Member

Manishearth commented Aug 1, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 1, 2016

📌 Commit ff6283a has been approved by Manishearth

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 1, 2016

⌛️ Testing commit ff6283a with merge 144b980...

bors-servo added a commit that referenced this pull request Aug 1, 2016

Auto merge of #12679 - johannhof:image-inject, r=Manishearth
Prevent injection vulnerability in image page

This is taking up nox' suggestion from #12542 and creates an img element using Rust code instead of escaping the URL. I will look at the neterror.html URL strings separately, we might do those in a similar way.

To reproduce, visit e.g. the following URL with your vulnerable Servo:
```
https://servo.org/screenshot.png?'onload='document.body.innerHTML=`hacked`'
```

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12542

- [x] These changes do not require tests because this is just fixing up existing behavior and I'm not sure how to test it

r?@jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12679)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Aug 1, 2016

@bors-servo bors-servo merged commit ff6283a into servo:master Aug 1, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@johannhof johannhof deleted the johannhof:image-inject branch Aug 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment