1271796 - Wrong SRI hash calculated for CSS asset

Status: RESOLVED FIXED

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

Description

[squarecode]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.94 Safari/537.36

Steps to reproduce:

1) Uploaded amalgamated CSS and JS assets to server (apache2, w/o CDN or cache).

2) Used https://report-uri.io/home/sri_hash to generate SRI attributes for both assets.

3) Site works as expected (in Chrome, Opera, Internet Explorer and Edge), but not in Firefox

3.1) Firefox accepts the JS file, but complains the CSS file doesn't match the hashes provided (which it absolutely does).




Actual results:

I verified that the above tool generated the actual correct hash values using the OpenSSL command (which btw. is also recommended by the Mozilla-recommended SRI tool at https://www.srihash.org/). For verification, I did the following:

3.1.a) Confirmed that the page works as intended in every browser except FF (including Nightly)

3.1.b) SSH into the webserver and ran [openssl dgst -sha384 -binary ***.min.css | openssl base64 -A ; echo], got: [aUEZSLnEpLLhLPtc6jQotKvOTBf5rE2GxK3CCGpPCowg1PYEqmLJxFIzMiRWTwll]

3.1.c) Fire up a local ubuntu machine and do [wget https://***.com/***.min.css], then repeat 3.1.b. Get the same (correct) result.

3.2) I retry with the Mozilla tool at https://www.srihash.org/ (remember, this is where the above OpenSSL command came from). This tool generates a completely wrong hash ([180/Df1WM1rZDbZcpnPbMDwdKvYwnqLTyPq7WL5Y0SRyN2Wr5OKVpqH4ak9gyp7q], instead of the correct [aUEZSLnEpLLhLPtc6jQotKvOTBf5rE2GxK3CCGpPCowg1PYEqmLJxFIzMiRWTwll])

Now comes the confusing part. If I actually put that wrong hash into the HTML, instead of the correct one, the page works in Firefox. But _only_ in Firefox, since every other browser recognizes the wrong digest and blocks the asset.


Expected results:

This should have happened:

- Firefox generates the wrong hash, it should generate the actual one. 
- The SHA-384 output of [https://www.srihash.org/] must be identical with that of [https://report-uri.io/home/sri_hash]
- [https://www.srihash.org/] should generate the actual hash.

# MORE INFO #

I can only disclose the specific files in private. A more detailed report of this issue can be found at [http://stackoverflow.com/q/37141644/3750388].

The hash Firefox calculates is provably wrong. The question is what's the cause. Does FF alter the CSS in transfer? Is there a bug in the FF internal digest routine for SRI, or SRI'd CSS assets in particular?

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Francois, can you take a look at this please and put it in the backlog if verified!

Comment 2

[Frederik Braun [:freddy]]

Thank you for reporting this, squarecode. Can you reduce the file to something minimal that still fails?
Alternatively, I would ask you to privately send the file to [e-mail] and [e-mail].
You can even use GPG if the file is highly confidential.

We will try to bring this down to a very minimal test case that is derived from your sample but will not contain any sensitive information, so that we can include it in our test suite.

Comment 3

[squarecode]

Hi there, I sent you both an email :-)

Comment 4

[Frederik Braun [:freddy]]

Attachment: test1.html

The problem seems encoding related. I'm attaching a reduced sample files that I am converting into a mochitest as we speak.

Comment 5

[Frederik Braun [:freddy]]

Attachment: mini-x97.css

Comment 6

[Frederik Braun [:freddy]]

Attachment: MozReview Request: Bug 1271796: Test case to check encoding problems in SRI r?francois

Review commit: https://reviewboard.mozilla.org/r/52157/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52157/

Comment 7

[François Marier [:francois]]

This bug could be a dupe of bug 1269241, but I'm not sure yet since I haven't dug deep into the test case that was sent to us via email.

Freddy, do you want to investigate whether or not it's the same bug? The work-around in https://bugzilla.mozilla.org/show_bug.cgi?id=1269241#c10 might help here.

Comment 8

[François Marier [:francois]]

Comment on attachment 8751676 [details]
MozReview Request: Bug 1271796: Test case to check encoding problems in SRI r?francois

I think your test case is already included in this (upcoming) one: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1269241&attachment=8751515

Comment 9

[squarecode]

(In reply to François Marier [:francois] from comment #7)
> This bug could be a dupe of bug 1269241, but I'm not sure yet since I
> haven't dug deep into the test case that was sent to us via email.
> 
> Freddy, do you want to investigate whether or not it's the same bug? The
> work-around in https://bugzilla.mozilla.org/show_bug.cgi?id=1269241#c10
> might help here.

I'll try the workaround in a few hours. Let's see if it works (or breaks something else).

Comment 10

[squarecode]

I stripped the BOM from the file (nothing obvious broke, but then again, why should it). My HTML already had this tag:

<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />

It still doesn't work. You can try it by just replacing "min.css" with "nobom.min.css" in the test URL I sent you. Firefox says "none of the sha512 hashes match".

Comment 11

[Frederik Braun [:freddy]]

Sorry I did not get around to do this. I am clearing the needinfo hoping someone else will pick this up before I come back.

Comment 12

[François Marier [:francois]]

Comment on attachment 8751676 [details]
MozReview Request: Bug 1271796: Test case to check encoding problems in SRI r?francois

https://reviewboard.mozilla.org/r/52157/#review54474

As per comment 8, this is already included in the patch for bug 1269241.

Comment 13

[François Marier [:francois]]

Jonathan, is this something you'd be able to take a look at? If we can find a work around for this, it will tell us whether or not it's the same issue as in bug 1269241.

Comment 14

[Jonathan Kingston [:jkt] he/him]

Looking into this now to see if there is a resolution or similarires to Bug 1269241.

Comment 15

[Jonathan Kingston [:jkt] he/him]

Would one of you be able to share the original file so I can create another minified test case? I want to make sure if the other bug captures the work which it sounds like it doesn't.

My email is: [e-mail]

Or Freddy if you see this but you are away.

Comment 16

[Jonathan Kingston [:jkt] he/him]

François managed to keep a backup of the file and sent it over to me. Thanks!

Comment 17

[Jonathan Kingston [:jkt] he/him]

This isn't a dupe of the other bug, freddies replication is correct and has no bomb in the file.
Windows escape sequences seem to cause this issue, Freddy has <97> but I also found <95> in the file which also causes the same issue. Is this worth a separate replication case?

Comment 18

[François Marier [:francois]]

(In reply to Jonathan Kingston [:jkt] from comment #17)
> Windows escape sequences seem to cause this issue, Freddy has <97> but I
> also found <95> in the file which also causes the same issue. Is this worth
> a separate replication case?

Yes, it would be great to add a test case for this in the existing unit test. Like I did in bug 1269241.

I will try and merge that test I've got so you can add another one after it to cover the problem you've found. We can land this with the test cases as "todo()" instead of "ok()" so that they won't make the tests fail for now.

Comment 19

[François Marier [:francois]]

Would you be able to re-test running your resource through srihash.org?

I merged a change that addresses bugs related to gzip encoding and changes the backend we use to fetch resources:

  https://github.com/mozilla/srihash.org/pull/142

I'm curious to see if the problem went away.

Comment 20

[squarecode]

(In reply to François Marier [:francois] from comment #19)
> Would you be able to re-test running your resource through srihash.org?
> 
> I merged a change that addresses bugs related to gzip encoding and changes
> the backend we use to fetch resources:
> 
>   https://github.com/mozilla/srihash.org/pull/142
> 
> I'm curious to see if the problem went away.

I will test it later this day. 

I know also now know what caused the stray control sequences to begin with. I used SciTe to concat the various CSS files. Now some of these files were UTF and some where not. While I blindly pasted them into one buffer, I didn't notice the UTF chars. When SciTe tried to convert the whole file to UTF8+BOM, it converted the chars to lower ASCII chars in some cases.

Comment 21

[squarecode]

Seems to work fine now.

Comment 22

[François Marier [:francois]]

(In reply to squarecode from comment #21)
> Seems to work fine now.

Thanks for double-checking!

Comment 23

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Bug 1271796 use raw bytes to calculate SRI hash

Review commit: https://reviewboard.mozilla.org/r/78556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78556/

Comment 24

[François Marier [:francois]]

Kate, does your patch make these two tests pass?

http://searchfox.org/mozilla-central/rev/bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/dom/security/test/sri/iframe_style_sameorigin.html#75
http://searchfox.org/mozilla-central/rev/bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/dom/security/test/sri/iframe_style_sameorigin.html#81

If so, we should flip them to "ok" in this patch too.

Comment 25

[François Marier [:francois]]

Comment on attachment 8790974 [details]
Bug 1271796 use raw bytes to calculate SRI hash

https://reviewboard.mozilla.org/r/78556/#review77150

We should get a necko peer to review the UnicharStreamLoader changes.

::: dom/security/test/sri/mochitest.ini:3
(Diff revision 1)
>  [DEFAULT]
>  support-files =
> +  file_bug_1271796.css

Are you using this file anywhere?

I only see `mini-x97.css` in the HTML...

Comment 26

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8790974 [details]
Bug 1271796 use raw bytes to calculate SRI hash

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78556/diff/1-2/

Comment 27

[François Marier [:francois]]

Comment on attachment 8790974 [details]
Bug 1271796 use raw bytes to calculate SRI hash

https://reviewboard.mozilla.org/r/78556/#review77452

Comment 28

[Honza Bambas (:mayhemer)]

Comment on attachment 8790974 [details]
Bug 1271796 use raw bytes to calculate SRI hash

https://reviewboard.mozilla.org/r/78556/#review77792

I wanted to give f+ or near r+, but RB doesn't allow it.  I would rather check the next version, hence r-.

Thanks for identifying the problem and providing this fix.  It looks good, I would just do one small change to save one unnecessary memory copying.

::: dom/security/SRICheck.cpp:189
(Diff revision 2)
>                            nsIConsoleReportCollector* aReporter)
>  {
>    NS_ENSURE_ARG_POINTER(aLoader);
>    NS_ENSURE_ARG_POINTER(aReporter);
>  
>    NS_ConvertUTF16toUTF8 utf8Hash(aString);

this is no longer used, remove it please (it's pretty expensive)

::: netwerk/base/nsIUnicharStreamLoader.idl:84
(Diff revision 2)
>     * called.
>     */
>    readonly attribute ACString charset;
> +
> +  /**
> +   * Get the raw bytes as seen on the wire prior to character converstion.

typo: conversion

::: netwerk/base/nsIUnicharStreamLoader.idl:87
(Diff revision 2)
> +
> +  /**
> +   * Get the raw bytes as seen on the wire prior to character converstion.
> +   * Used by Subresource Integrity checker to generate the correct hash.
> +   */
> +  void getRawBuffer(out ACString aRawBuffer);

ok, if you wanted to have a getter for ACString, then it would be better to have a readonly attribute.

however, this signature (and unfortunately limitation of out idl) makes the content be copied all the time.

I'd rather see this signature as following:
size_t getRawBufferPtr(out charPtr aRawBuffer);

make it return mRawBuffer.Length() and mRawBuffer.get().  the caller will take it and construct nsDependentCSubstring from those values, it can then be passed to the verifier.

that will save some memory allocs and cpu cycles.

::: netwerk/base/nsUnicharStreamLoader.cpp:233
(Diff revision 2)
>    uint32_t capacity = haveRead + dstLen;
>    if (!self->mBuffer.SetCapacity(capacity, fallible)) {
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  self->mRawBuffer.Append(aSegment, aCount);

nit: this should be fallible Append(), checked for failures, similarly to SetCapacity() call on mBuffer just above.

Comment 29

[Kate McKinley [:kmckinley, :Kate]]

(In reply to François Marier [:francois] from comment #24)
> Kate, does your patch make these two tests pass?
> 
> http://searchfox.org/mozilla-central/rev/
> bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/dom/security/test/sri/
> iframe_style_sameorigin.html#75
> http://searchfox.org/mozilla-central/rev/
> bcc9ea6947878ca6378e5b7d6f08c1c090ed9eb7/dom/security/test/sri/
> iframe_style_sameorigin.html#81
> 
> If so, we should flip them to "ok" in this patch too.

Yes, and those are flipped in the current patch.

Comment 30

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8790974 [details]
Bug 1271796 use raw bytes to calculate SRI hash

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78556/diff/2-3/

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 8790974 [details]
Bug 1271796 use raw bytes to calculate SRI hash

Thanks!

Comment 32

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8790974 [details]
Bug 1271796 use raw bytes to calculate SRI hash

https://reviewboard.mozilla.org/r/78556/#review79484

Carry over r+ from https://bugzilla.mozilla.org/show_bug.cgi?id=1271796#c31

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/735ae776c393
use raw bytes to calculate SRI hash r=francois

Comment 34

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/735ae776c393