Implement Subresource Integrity #14865

Merged
merged 1 commit into from Jan 8, 2017

Projects

None yet

5 participants

@mrnayak
Contributor
mrnayak commented Jan 5, 2017 edited

Implemented response validation part of https://w3c.github.io/webappsec-subresource-integrity/.
Implemented step eighteen of the main fetch. If a request has integrity metadata, then following steps are performed

  1. Wait for response body
  2. If the response does not have a termination reason and response does not match request’s integrity metadata, set response and internalResponse to a network error.

Dependency updated: html5ever-atoms from 0.1.2 to 0.1.3. This will not completely fix #14523, It will implement changes related to response validation. Request validation algorithm implementation needs CSP.

I did not update any WPT-Test. In my local system, I found some assertion issue dependent on the order of execution of test-case. It would be helpful if someone could do "try" build on these changes to get wpt results.

r? @jdm


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes

This change is Reviewable

@mrnayak
Contributor
mrnayak commented Jan 5, 2017

r? @jdm

@jdm jdm was assigned by highfive Jan 5, 2017
@jdm
Member
jdm commented Jan 5, 2017
@bors-servo
Contributor

⌛️ Trying commit a64b964 with merge 2457bcf...

@bors-servo bors-servo added a commit that referenced this pull request Jan 5, 2017
@bors-servo bors-servo Auto merge of #14865 - mrnayak:sri-fetch, r=<try>
Implement Subresource Integrity

Implemented response validation part of https://w3c.github.io/webappsec-subresource-integrity/.
Implemented step eighteen of the main fetch. If a request has integrity metadata, then following steps are performed
1) Wait for response body
2) If the response does not have a termination reason and response does not match request’s integrity metadata, set response and internalResponse to a network error.

Dependency updated: html5ever-atoms from 0.1.2 to 0.1.3. This will not completely fix #14523, It will implement changes related to response validation. Request validation algorithm implementation needs CSP.

I did not update any WPT-Test. In my local system, I found some assertion issue dependent on the order of execution of test-case. It would be helpful if someone could do "try" build on these changes to get wpt results.

r? @jdm
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/14865)
<!-- Reviewable:end -->
2457bcf
@bors-servo
Contributor

💔 Test failed - mac-rel-wpt2

@jdm
Member
jdm commented Jan 5, 2017
  ▶ TIMEOUT [expected OK] /subresource-integrity/subresource-integrity.sub.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  └ 3.3 (Core Profile) Mesa 12.0.1

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ NOTRUN [expected PASS] Style: Same-origin with correct sha256 and sha512 hash, rel='alternate stylesheet' enabled

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  └ NOTRUN [expected PASS] Style: Same-origin with incorrect sha256 and sha512 hash, rel='alternate stylesheet' enabled

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  │ FAIL [expected PASS] Style: <crossorigin='anonymous'> with correct hash, ACAO: *
  │   → assert_unreached: Good load fired error handler. Reached unreachable code
  │ FAIL [expected PASS] Style: <crossorigin='use-credentials'> with correct hash, CORS-eligible
  │   → assert_unreached: Good load fired error handler. Reached unreachable code
  │ 
  │ SRIStyleTest.prototype.execute/</<@http://web-platform.test:8000/subresource-integrity/subresource-integrity.sub.html:132:39
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ SRIStyleTest.prototype.execute/<@http://web-platform.test:8000/subresource-integrity/subresource-integrity.sub.html:132:17

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  │ FAIL [expected PASS] Script: <crossorigin='use-credentials'> with correct hash, CORS-eligible
  │   → assert_unreached: Good load fired error handler. Reached unreachable code
  │ 
  │ SRIScriptTest.prototype.execute/</<@http://web-platform.test:8000/subresource-integrity/subresource-integrity.sub.html:59:39
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ SRIScriptTest.prototype.execute/<@http://web-platform.test:8000/subresource-integrity/subresource-integrity.sub.html:59:17

  ▶ Unexpected subtest result in /subresource-integrity/subresource-integrity.sub.html:
  │ FAIL [expected PASS] Style: Same-origin with incorrect hash.
  │   → assert_not_equals: got disallowed value "rgb(255, 255, 0)"
  │ FAIL [expected PASS] Style: Same-origin with sha256 match, sha512 mismatch
  │   → assert_not_equals: got disallowed value "rgb(255, 255, 0)"
  │ FAIL [expected PASS] Style: <crossorigin='anonymous'> with incorrect hash, ACAO: *
  │   → assert_not_equals: got disallowed value "rgb(255, 255, 0)"
  │ FAIL [expected PASS] Style: <crossorigin='use-credentials'> with incorrect hash CORS-eligible
  │   → assert_not_equals: got disallowed value "rgb(255, 255, 0)"
  │ FAIL [expected PASS] Style: <crossorigin='anonymous'> with CORS-ineligible resource
  │   → assert_not_equals: got disallowed value "rgb(255, 255, 0)"
  │ FAIL [expected PASS] Style: Cross-origin, not CORS request, with correct hash
  │   → assert_not_equals: got disallowed value "rgb(255, 255, 0)"
  │ FAIL [expected PASS] Style: Cross-origin, not CORS request, with hash mismatch
  │   → assert_not_equals: got disallowed value "rgb(255, 255, 0)"
  │ 
  │ SRIStyleTest.prototype.execute/</<@http://web-platform.test:8000/subresource-integrity/subresource-integrity.sub.html:141:21
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ SRIStyleTest.prototype.execute/<@http://web-platform.test:8000/subresource-integrity/subresource-integrity.sub.html:139:17
@jdm
Member
jdm commented Jan 5, 2017

Let me know if you want me to start reviewing the changes or whether I should wait until you look into the test failure.

@mrnayak
Contributor
mrnayak commented Jan 6, 2017

Thanks Josh, I will work on these test cases today. Will notify you once these gets fixed.

@mrnayak
Contributor
mrnayak commented Jan 6, 2017

None of the test case failures seem to be related to changes.

  1. assertion error in 7 test case is due to stylesheet already been loaded in other success test case. I moved all test case expecting assert_not_equals to execute before any success test case.
  2. crossorigin has not been implemented in htmllinkelement. So i changed assertion of two test case (Style: <crossorigin='anonymous'> with correct hash, ACAO: * and Style: <crossorigin='use-credentials'> with correct hash, CORS-eligible) to false.
  3. Two alternate stylesheet related tests got timed out since we ignore alternate stylesheet. I have filed an issue #14881. Currently, I have commented these tests.
  4. Script: <crossorigin='use-credentials'> with correct hash, CORS-eligible failed due to bug in step six of cors_check of http_loader . This has been fixed.

@jdm If my above analysis is correct, can you please review the changes.

@jdm
Member
jdm commented Jan 6, 2017

Rather than modifying the test file, we should be modifying tests/wpt/metadata/subresource-integrity/subresource-integrity.sub.html.ini to include the expected failing test results.

@jdm
Member
jdm commented Jan 6, 2017

Can you explain point 1 in more detail? Why does the order make a difference in the test result?

@mrnayak
Contributor
mrnayak commented Jan 6, 2017 edited

@jdm I am not sure if point 1 is a bug. Our assertion is to validate that div style background color is not yellow. Moment first success test case executes stylesheet gets loaded and subsequent all assertion fail though we don't load stylesheet again.

I will update tests/wpt/metadata/subresource-integrity/subresource-integrity.sub.html.ini, Thanks

@jdm
Member
jdm commented Jan 6, 2017

I know what the problem is - the test relies on removing the stylesheet from any previous test before starting the new test. However, Servo doesn't support removing stylesheets from the document yet (see #976).

@mrnayak
Contributor
mrnayak commented Jan 6, 2017

So I will modify subresource-integrity.sub.html.ini to include these tests as well?

@jdm
Member
jdm commented Jan 6, 2017

Yes please!

@mrnayak
Contributor
mrnayak commented Jan 6, 2017

@jdm I have done the changes

@jdm

This is really great work! I have a number of small changes, but in general I like the way the code is organized and found the changes easy to read.

components/net/fetch/methods.rs
@@ -1,7 +1,6 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
@jdm
jdm Jan 6, 2017 Member

nit: please add this newline back.

components/net/fetch/methods.rs
+ let ref integrity_metadata = *request.integrity_metadata.borrow();
+ if response.termination_reason.is_none() && !is_response_integrity_valid(integrity_metadata, &response) {
+ let mut response = Response::network_error(
+ NetworkError::Internal("Sub-resource integrity validation failed".into()));
@jdm
jdm Jan 6, 2017 Member

nit: Subresource should be one word, not hyphenated.

components/net/fetch/methods.rs
+ let mut response = Response::network_error(
+ NetworkError::Internal("Sub-resource integrity validation failed".into()));
+ response.internal_response = Some(Box::new(
+ Response::network_error(response.get_network_error().unwrap().clone())));
@jdm
jdm Jan 6, 2017 Member

I don't believe we need to duplicate the error for the internal value here. It should be enough just to have a default network error response.

components/net/fetch/methods.rs
+ Response::network_error(response.get_network_error().unwrap().clone())));
+ response
+ } else {
+ response
@jdm
jdm Jan 6, 2017 Member

nit: indentation

components/net/subresource_integrity.rs
+use std::sync::MutexGuard;
+lazy_static! {
+ //Key is supported algorithm and value is priority
+ static ref SUPPORTED_ALGORITHM: HashMap<&'static str, usize> = {
@jdm
jdm Jan 6, 2017 Member

We can do this instead:

const SUPPORTED_ALGORITHM: &'static [&'static str] = [
    "sha256",
    "sha384",
    "sha512",
];

and use the position in the array as the priority instead.

components/net/subresource_integrity.rs
+ let mut result = vec![];
+
+ // Step 3
+ let tokens: Vec<&str> = integrity_metadata.split(" ").collect();
@jdm
jdm Jan 6, 2017 Member

This doesn't quite match the spec's steps for splitting on spaces - we need to skip characters at the start and repeated characters in between tokens, and the space characters include other whitespace. I recommend using split_html_space_chars instead.

Also, we don't need to call collect here; we can iterate over the result of splitting instead.

components/net/subresource_integrity.rs
+ for token in tokens {
+ let parsed_data: Vec<&str> = token.split("-").collect();
+
+ if parsed_data.len() > 0 {
@jdm
jdm Jan 6, 2017 Member

This will always be true; splitting on a non-existent token returns an iterator with one element (the full string).

components/net/subresource_integrity.rs
+ if parsed_data.len() > 0 {
+ let alg = parsed_data[0];
+
+ if SUPPORTED_ALGORITHM.contains_key(alg) {
@jdm
jdm Jan 6, 2017 Member

Let's do:

if !SUPPORTED_ALGORITHM.contains_key(alg) {
    continue;
}
components/net/subresource_integrity.rs
+ let alg = parsed_data[0];
+
+ if SUPPORTED_ALGORITHM.contains_key(alg) {
+ let data: Vec<&str> = parsed_data[1].split("?").collect();
@jdm
jdm Jan 6, 2017 Member

We have not guaranteed that parsed_data[1] exists yet.

components/net/subresource_integrity.rs
+}
+
+/// https://w3c.github.io/webappsec-subresource-integrity/#getprioritizedhashfunction
+pub fn get_prioritized_hash_function(hash_func_left: &str, hash_func_right: &str) -> String {
@jdm
jdm Jan 6, 2017 Member

Let's return Option<&str> here instead, which will avoid the magic empty string behaviour.

components/net/subresource_integrity.rs
+/// https://w3c.github.io/webappsec-subresource-integrity/#getprioritizedhashfunction
+pub fn get_prioritized_hash_function(hash_func_left: &str, hash_func_right: &str) -> String {
+ let left_priority = SUPPORTED_ALGORITHM.get(hash_func_left).unwrap();
+ let right_priority = SUPPORTED_ALGORITHM.get(hash_func_right).unwrap();
@jdm
jdm Jan 6, 2017 Member

These can use SUPPORTED_ALGORITHM.position(|s| s == hash_func_left).unwrap() instead.

components/net/subresource_integrity.rs
+ let mut result: Vec<SriEntry> = vec![integrity_metadata_list[0].clone()];
+ let mut current_algorithm = result[0].alg.clone();
+
+ for i in 1..integrity_metadata_list.len() {
@jdm
jdm Jan 6, 2017 Member
for integrity_metadata in &integrity_metadata_list[1..] {
components/net/subresource_integrity.rs
+ let intgerity_metadata = integrity_metadata_list[i].clone();
+
+ let prioritized_hash = get_prioritized_hash_function(&intgerity_metadata.alg,
+ &*current_algorithm);
@jdm
jdm Jan 6, 2017 Member

nit: indentation.

components/net/subresource_integrity.rs
+ let response_digest = hash(message_digest, vec);
+ response_digest.to_base64(STANDARD)
+ } else {
+ "".to_owned()
@jdm
jdm Jan 6, 2017 Member

We should probably make this unreachable!("Tried to calculate digest of incomplete response body") instead.

components/net/subresource_integrity.rs
+ let parsed_metadata_list: Vec<SriEntry> = parsed_metadata(integrity_metadata);
+
+ // Step 2 & 4
+ if parsed_metadata_list.len() == 0 {
@jdm
jdm Jan 6, 2017 Member

if parse_metadata_list.is_empty() {

+ data.to_owned()
+ } else {
+ "".to_owned()
+ }
@jdm
jdm Jan 6, 2017 Member

This can be replaced by:

Some(ref meta_data) => meta_data.to_owned(),
None => "".to_owned(),
tests/unit/net/fetch.rs
+ response.send(MESSAGE).unwrap();
+ };
+ let (mut server, server_url) = make_server(handler);
+ let do_fetch = |url: ServoUrl| {
@jdm
jdm Jan 6, 2017 Member

No need for this closure; we can write it out inline because it's only used once.

tests/unit/net/fetch.rs
+ let mut request = Request::new(url, Some(origin), false, None);
+ *request.referrer.borrow_mut() = Referrer::NoReferrer;
+ *request.integrity_metadata.borrow_mut()
+ = "sha384-H8BRh8j48O9oYatfu5AZzq6A9RINhZO5H16dQZngK7T62em8MUt1FLm52t+eX6xO".to_owned();
@jdm
jdm Jan 6, 2017 Member

It would be useful to add a comment explaining how this was generated. Also, = should be on the previous line.

tests/unit/net/fetch.rs
+ };
+ let (mut server, server_url) = make_server(handler);
+
+ let do_fetch = |url: ServoUrl| {
@jdm
jdm Jan 6, 2017 Member

This code can be inline instead of using a closure.

tests/unit/net/fetch.rs
+ let mut request = Request::new(url, Some(origin), false, None);
+ *request.referrer.borrow_mut() = Referrer::NoReferrer;
+ *request.integrity_metadata.borrow_mut()
+ = "sha384-H8BRh8j48O9oYatfu5AZzq6A9RINhZO5H16dQZngK7T62em8MUt1FLm52t+eX6xO".to_owned();
@jdm
jdm Jan 6, 2017 Member

Let's add a comment about how this was generated.

tests/unit/net/fetch.rs
+
+ let _ = server.close();
+ assert!(response.is_network_error());
+ assert!(response.internal_response.unwrap().is_network_error());
@jdm
jdm Jan 6, 2017 Member

This assertion won't pass after previous changes I requested. We should be able to remove it.

tests/unit/net/subresource_integrity.rs
+}
+
+#[test]
+fn test_parsed_metadata_without_options() {
@jdm
jdm Jan 6, 2017 Member

Let's add a test for malformed integrity attributes, too. No -s, multiple ?s, etc.

@mrnayak
Contributor
mrnayak commented Jan 7, 2017

Thanks @jdm I have made changes. Please let me know if there are any other changes.

@jdm
Member
jdm commented Jan 7, 2017

@bors-servo: r+
Wonderful!

@bors-servo
Contributor

📌 Commit 49aff3e has been approved by jdm

@bors-servo
Contributor

⌛️ Testing commit 49aff3e with merge f3fe099...

@bors-servo bors-servo added a commit that referenced this pull request Jan 7, 2017
@bors-servo bors-servo Auto merge of #14865 - mrnayak:sri-fetch, r=jdm
Implement Subresource Integrity

Implemented response validation part of https://w3c.github.io/webappsec-subresource-integrity/.
Implemented step eighteen of the main fetch. If a request has integrity metadata, then following steps are performed
1) Wait for response body
2) If the response does not have a termination reason and response does not match request’s integrity metadata, set response and internalResponse to a network error.

Dependency updated: html5ever-atoms from 0.1.2 to 0.1.3. This will not completely fix #14523, It will implement changes related to response validation. Request validation algorithm implementation needs CSP.

I did not update any WPT-Test. In my local system, I found some assertion issue dependent on the order of execution of test-case. It would be helpful if someone could do "try" build on these changes to get wpt results.

r? @jdm
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/14865)
<!-- Reviewable:end -->
f3fe099
@bors-servo
Contributor

💔 Test failed - linux-rel-wpt

@mrnayak
Contributor
mrnayak commented Jan 7, 2017

I had missed updating expectation of two test case :(. I have corrected this.

@jdm
Member
jdm commented Jan 7, 2017

You will also need to adjust the expectations for /html/dom/interfaces.html, /html/dom/reflection-misc.html, and /html/dom/reflection-metadata.html. I recommend running
./mach test-wpt tests/wpt/web-platform-test/path/to/test.html --no-pause-after-test --log-raw /tmp/servo.log && ./mach update-wpt /tmp/servo.log for each of those tests.

@bors-servo
Contributor

☔️ The latest upstream changes (presumably #14867) made this pull request unmergeable. Please resolve the merge conflicts.

@mrnayak mrnayak Implement Subresource Integrity
Implemented response validation part of
https://w3c.github.io/webappsec-subresource-integrity/.
Implemented step eighteen of the main fetch. If a request has integrity
metadata, then following steps are performed
*Wait for response body
*If the response does not have a termination reason and response does not
match request’s integrity metadata, set response to a
network error.# Please enter the commit message for your changes. Lines starting
a302649
@mrnayak
Contributor
mrnayak commented Jan 8, 2017

@jdm Thanks, I have fixed expectations and merge conflicts

@jdm
Member
jdm commented Jan 8, 2017
@bors-servo
Contributor

📌 Commit a302649 has been approved by jdm

@bors-servo
Contributor

⌛️ Testing commit a302649 with merge f958daf...

@bors-servo bors-servo added a commit that referenced this pull request Jan 8, 2017
@bors-servo bors-servo Auto merge of #14865 - mrnayak:sri-fetch, r=jdm
Implement Subresource Integrity

Implemented response validation part of https://w3c.github.io/webappsec-subresource-integrity/.
Implemented step eighteen of the main fetch. If a request has integrity metadata, then following steps are performed
1) Wait for response body
2) If the response does not have a termination reason and response does not match request’s integrity metadata, set response and internalResponse to a network error.

Dependency updated: html5ever-atoms from 0.1.2 to 0.1.3. This will not completely fix #14523, It will implement changes related to response validation. Request validation algorithm implementation needs CSP.

I did not update any WPT-Test. In my local system, I found some assertion issue dependent on the order of execution of test-case. It would be helpful if someone could do "try" build on these changes to get wpt results.

r? @jdm
<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/14865)
<!-- Reviewable:end -->
f958daf
@bors-servo
Contributor

💔 Test failed - linux-dev

@mrnayak
Contributor
mrnayak commented Jan 8, 2017

Infra issue?

@KiChjang
Member
KiChjang commented Jan 8, 2017

@bors-servo retry

@bors-servo
Contributor

⚡️ Previous build results for android, arm64, linux-rel-wpt, mac-dev-unit, mac-rel-wpt1, windows-gnu-dev, windows-msvc-dev are reusable. Rebuilding only arm32, linux-dev, linux-rel-css, mac-rel-css, mac-rel-wpt2...

@bors-servo bors-servo merged commit a302649 into servo:master Jan 8, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@mrnayak mrnayak deleted the mrnayak:sri-fetch branch Jan 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment