Closed Bug 1269241 Opened 5 years ago Closed 5 years ago

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

Categories

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

46 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: ckreon, Assigned: francois)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 3 obsolete files)

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
Component: Untriaged → DOM: Security
Product: Firefox → Core
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!
Flags: needinfo?(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.
Assignee: nobody → francois
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(francois) → needinfo?(ckreon)
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.
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]).
Whiteboard: [domsecurity-active]
(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.
Flags: needinfo?(ckreon)
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
Summary: SHA Integrity Check Fails on Valid Hashes → SRI checks fail on UTF-8 stylesheets that include a BOM
Test case that reproduces the problem.
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.
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)
(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.
Blocks: 1271796
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.
Attachment #8751515 - Attachment is obsolete: true
Attachment #8751995 - Attachment is obsolete: true
Priority: -- → P2
Any updates on this? We just ran into this at GitLab, would be great to have a fix otherwise we can't use SRI.
(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?
Flags: needinfo?(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.
Flags: needinfo?(francois)
I'm going to land the new tests first so that Jonathan can add his own ones to the file too.
Attachment #8752429 - Attachment is obsolete: true
Attachment #8766553 - Flags: review?(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.
Attachment #8766553 - Flags: review?(jkt)
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.
Attachment #8766553 - Flags: review?(jkt) → review+
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
Attachment #8766553 - Flags: review?(ckerschb) → review+
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a64cf8e2fb5
Add SRI tests for UTF-8 stylesheets. r=ckerschb,r=jkt
https://hg.mozilla.org/mozilla-central/rev/0a64cf8e2fb5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The fix for 1271796 also resolves this issue, marking duplicate.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1271796
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?
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(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.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
(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.
Whoops, my bad - I tested it with Nightly (53.0a1) and it is indeed working.
(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+.
You need to log in before you can comment on or make changes to this bug.