1269241 - SRI checks fail on UTF-8 stylesheets that include a BOM

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Corey Kozlowski]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/601.5.17 (KHTML, like Gecko) Version/9.1 Safari/601.5.17

Steps to reproduce:

Generated valid assets with SHA256 integrity tags in Jekyll using Jekyll-Assets. Deployed site in both production and local environments and then accessed them with Firefox.


Actual results:

Firefox did not load the assets - the console gave an error:
None of the "sha256" hashes in the integrity attribute match the content of the subresource.


Expected results:

The SHA hashes are indeed correct, and load properly in other integrity-enforcing browsers. Firefox should have loaded the assets as valid files.

More details about this bug can be found here:
https://github.com/jekyll/jekyll-assets/issues/270#issuecomment-216091919

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Francois, can you take a look what the problem might be here? If verified, please add it to the backlog and blocking the SRI meta bug. Thanks!

Comment 2

[François Marier [:francois]]

Corey, do you have a test URL I can take a look at?

I tried http://wiki.shadowlinkit.com/ (referenced in the Github issue) but it doesn't appear to have any integrity attributes.

Comment 3

[Corey Kozlowski]

Hey - I stripped the integrity tags off of our production server almost immediately as it broke the entire site. I will re-create the issue on a VPS tonight and provide a link.

Comment 4

[Corey Kozlowski]

Ok - I've re-created the issue and hosted it for verification:

http://sha-test.shadowlinkit.com/

The site loads correctly in Safari (9.1 [11601.5.17.1]) and Google Chrome (50.0.2661.94 [64-bit]).

Comment 5

[Loic]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=11dc79e232110ba6de5179e46dfbda77b52a88c3&tochange=4313752f69956ae248bd4e7ff3913c8dd4252698

Francois Marier — Bug 1205448 - Ship subresource integrity enabled by default. r=ckerschb

Comment 6

[François Marier [:francois]]

(In reply to Corey Kozlowski from comment #4)
> Ok - I've re-created the issue and hosted it for verification:
> 
> http://sha-test.shadowlinkit.com/

Thanks for the test page Corey, I can reproduce the bug and I've added some debugging info which show that we're computing a different hash:

  [Main Thread]: D/SRI SRICheckDataVerifier::VerifyHash, hash[0]=VmsSGlINSLNyy9xK3ypHxEcRQFkD+MBYjEmjKYOdVSM=
  [Main Thread]: D/SRI SRICheckDataVerifier::VerifyHash, mComputedHash=ZOex7Aa0kxyysvNJgNx6Q4kBOlnxniWECFwOejGhCHU=
  [Main Thread]: D/SRI SRICheckDataVerifier::VerifyHash, hash[0] did not match

I will dig into this further to try and find out why the hash is different.

Comment 7

[François Marier [:francois]]

When I put the same resource inside a script element, it works fine:

  [Main Thread]: D/SRI nsScriptLoader::PreloadURI, integrity=sha256-VmsSGlINSLNyy9xK3ypHxEcRQFkD+MBYjEmjKYOdVSM=
  [Main Thread]: D/SRI SRICheckDataVerifier::Verify, url=http://localhost/francois/sri/1269241.css (length=70306)
  [Main Thread]: D/SRI SRICheck::IsEligible, documentURI=http://localhost/francois/sri/1269241.html; requestURI=http://localhost/francois/sri/1269241.css; finalURI=http://localhost/francois/sri/1269241.css
  [Main Thread]: D/SRI SRICheck::IsEligible, same-origin
  [Main Thread]: D/SRI SRICheckDataVerifier::VerifyHash, hash[0]=VmsSGlINSLNyy9xK3ypHxEcRQFkD+MBYjEmjKYOdVSM=
  [Main Thread]: D/SRI SRICheckDataVerifier::VerifyHash, mComputedHash=VmsSGlINSLNyy9xK3ypHxEcRQFkD+MBYjEmjKYOdVSM=
  [Main Thread]: D/SRI SRICheckDataVerifier::VerifyHash, hash[0] verified successfully

I noticed that the file has a BOM:

  $ file main-566b121a520d48b372cbdc4adf2a47c44711405903f8c0588c49a329839d5523.css 
  main-566b121a520d48b372cbdc4adf2a47c44711405903f8c0588c49a329839d5523.css: UTF-8 Unicode (with BOM) text, with very long lines

The problem most likely lies in the conversion from UTF16 to UTF8:

  https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/dom/security/SRICheck.cpp#187

which is done because the CSS loader uses nsIUnicharStreamLoader to process the data in UTF16:

  https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/netwerk/base/nsIUnicharStreamLoader.idl#51-59

Comment 8

[François Marier [:francois]]

Attachment: bug1269241-test-and-logging.patch

Test case that reproduces the problem.

Comment 9

[François Marier [:francois]]

The fix will most likely involve wrapping the nsIUnicharStreamLoader in the CSS loader with something that hashes the raw bytes before passing them along to nsIUnicharStreamLoader.

Comment 10

[François Marier [:francois]]

I was able to find a work-around to get Corey's file to work:

1. strip out the BOM at the beginning of the file (tail --bytes=+4 file.css > file-nobom.css)
2. recompute the hash (sha256-ZOex7Aa0kxyysvNJgNx6Q4kBOlnxniWECFwOejGhCHU=)
3. set the charset of the file to UTF8 on the webserver ("AddDefaultCharset utf-8" in Apache)

Comment 11

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #10)
> 3. set the charset of the file to UTF8 on the webserver ("AddDefaultCharset
> utf-8" in Apache)

An alternative to that is to put this in the page head:

<meta charset="utf8">

since that seems to influence the parsing of the stylesheets loaded afterwards.

Comment 12

[François Marier [:francois]]

Attachment: bug1269241-test-and-logging.patch

Updated patch with an extra test case. We should probably add the same tests for the script element too to make sure they continue to pass.

Comment 13

[François Marier [:francois]]

Attachment: bug1269241-test-and-logging.patch

Comment 14

[Connor Shea]

Any updates on this? We just ran into this at GitLab, would be great to have a fix otherwise we can't use SRI.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Connor Shea from comment #14)
> Any updates on this? We just ran into this at GitLab, would be great to have
> a fix otherwise we can't use SRI.

Francois, are you still on this one?

Comment 16

[François Marier [:francois]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> (In reply to Connor Shea from comment #14)
> > Any updates on this? We just ran into this at GitLab, would be great to have
> > a fix otherwise we can't use SRI.
> 
> Francois, are you still on this one?

Yes, I'm planning on fixing this in July.

Comment 17

[François Marier [:francois]]

Attachment: bug1269241-test-and-logging.patch

I'm going to land the new tests first so that Jonathan can add his own ones to the file too.

Comment 18

[François Marier [:francois]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eea2d6caaae

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8766553 [details] [diff] [review]
bug1269241-test-and-logging.patch

Review of attachment 8766553 [details] [diff] [review]:
-----------------------------------------------------------------

Jonathan, do you mind reviewing those tests? I can sign off in the end.

Comment 20

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8766553 [details] [diff] [review]
bug1269241-test-and-logging.patch

Looks good to me, will add my tests to this. Sorry for the delay.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8766553 [details] [diff] [review]
bug1269241-test-and-logging.patch

Review of attachment 8766553 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, r=me

::: dom/security/test/sri/style4.css
@@ +1,1 @@
> +/* François was here. */

ha ha ha

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a64cf8e2fb5
Add SRI tests for UTF-8 stylesheets. r=ckerschb,r=jkt

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a64cf8e2fb5

Comment 24

[Kate McKinley [:kmckinley, :Kate]]

The fix for 1271796 also resolves this issue, marking duplicate.

Comment 25

[Corey Kozlowski]

This bug does not appear to be fixed.

Running the latest build of Firefox (50.0) on Mac OS X 10.11.6, I still cannot properly load:
http://sha-test.shadowlinkit.com/

Can anyone else verify?

Comment 26

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Corey Kozlowski from comment #25)
> This bug does not appear to be fixed.
> 
> Running the latest build of Firefox (50.0) on Mac OS X 10.11.6, I still
> cannot properly load:
> http://sha-test.shadowlinkit.com/
> 
> Can anyone else verify?

The fix is in Firefox 52, which is currently Aurora, so you will see it in Firefox Dev Edition or Nightly.

Comment 27

[Kate McKinley [:kmckinley, :Kate]]

(In reply to Kate McKinley [:kmckinley] from comment #26)
> (In reply to Corey Kozlowski from comment #25)
> > This bug does not appear to be fixed.
> > 
> > Running the latest build of Firefox (50.0) on Mac OS X 10.11.6, I still
> > cannot properly load:
> > http://sha-test.shadowlinkit.com/
> > 
> > Can anyone else verify?
> 
> The fix is in Firefox 52, which is currently Aurora, so you will see it in
> Firefox Dev Edition or Nightly.

I realize that some of the tracking info says Firefox 50, but this was checked in on 52, not 50, and it wasn't uplifted. Sorry for the confusion.

Comment 28

[Corey Kozlowski]

Whoops, my bad - I tested it with Nightly (53.0a1) and it is indeed working.

Comment 29

[Loic]

(In reply to Corey Kozlowski from comment #25)
> This bug does not appear to be fixed.
> 
> Running the latest build of Firefox (50.0) on Mac OS X 10.11.6, I still
> cannot properly load:
> http://sha-test.shadowlinkit.com/
> 
> Can anyone else verify?

Your bug is bug 1271796 which is fixed in 52+. This current bug (which doesn't fix your issue) is fixed in 50+.