Bug 155007 - CSP: Implement support for inline script and inline style hashes
Summary: CSP: Implement support for inline script and inline style hashes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, WebExposed
Depends on: 154941 155008 155247
Blocks: 155184 155186 116508
  Show dependency treegraph
 
Reported: 2016-03-03 20:47 PST by Daniel Bates
Modified: 2016-03-10 09:46 PST (History)
9 users (show)

See Also:


Attachments
Patch and Layout Test (93.65 KB, patch)
2016-03-04 12:46 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and Layout Test (93.73 KB, patch)
2016-03-08 13:55 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (701.14 KB, application/zip)
2016-03-09 01:29 PST, Build Bot
no flags Details
Patch and Layout Test (94.33 KB, patch)
2016-03-09 17:23 PST, Daniel Bates
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2016-03-03 20:47:34 PST
We should implement support for inline script and inline stylesheet hashes per <https://www.w3.org/TR/2015/CR-CSP2-20150721/#script-src-hash-usage> and <https://www.w3.org/TR/2015/CR-CSP2-20150721/#style-src-hash-usage>, respectively.
Comment 1 Daniel Bates 2016-03-03 20:47:59 PST
<rdar://problem/24964098>
Comment 2 Daniel Bates 2016-03-04 12:46:10 PST
Created attachment 273027 [details]
Patch and Layout Test
Comment 3 WebKit Commit Bot 2016-03-04 12:47:39 PST
Attachment 273027 [details] did not pass style-queue:


ERROR: Source/WebCore/page/csp/ContentSecurityPolicyHash.h:51:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Brent Fulgham 2016-03-04 21:40:16 PST
It looks like this patch has some issues:

/Volumes/Data/EWS/WebKit/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:208:25: error: invalid range expression of type 'WTF::OptionSet<WebCore::ContentSecurityPolicyHashAlgorithm>'; no viable 'begin' function available
    for (auto algorithm : algorithms) {
                        ^ ~~~~~~~~~~
/Volumes/Data/EWS/WebKit/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:258:43: error: no member named 'isEmpty' in 'WTF::OptionSet<WebCore::ContentSecurityPolicyHashAlgorithm>'
    if (!m_hashAlgorithmsForInlineScripts.isEmpty() && !scriptContent.isEmpty()
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/Volumes/Data/EWS/WebKit/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:270:47: error: no member named 'isEmpty' in 'WTF::OptionSet<WebCore::ContentSecurityPolicyHashAlgorithm>'
    if (!m_hashAlgorithmsForInlineStylesheets.isEmpty() && !styleContent.isEmpty()
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
3 errors generated.
Comment 5 Brent Fulgham 2016-03-04 22:06:05 PST
Comment on attachment 273027 [details]
Patch and Layout Test

View in context: https://bugs.webkit.org/attachment.cgi?id=273027&action=review

This patch looks great! But it doesn't apply cleanly for some reason. Can you rebaseline it and see if we get a green EWS, and I'll approve it?

> Source/WebCore/ChangeLog:68
> +        by making use Vector's equality operator.

"by making use *OF* Vector's equality operator."

> Source/WebCore/dom/StyledElement.cpp:205
> +    } else if (reason == ModifiedByCloning || document().contentSecurityPolicy()->allowInlineStyle(document().url(), startLineNumber, String(), isInUserAgentShadowTree()))

Is it better to use "emptyString()" here? I can't remember under which circumstances it is more efficient.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:206
> +    // FIXME: Compute the digest with respect to the raw bytes received from the page.

Do we have a Radar or Bugzilla bug for this work?

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:243
> +// FIXME: We should compute the document encoding once and cache it instead of computing it on each invocation.

Can you please file a Bugzilla for this so we do not forget?

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:406
> +}

The presence of these "Found" / "Not found" comments makes it seem like this might be better served by returning an enum indicating found/not found.

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:440
> +    // FIXME: Normalize Base64URL to Base64 instead of decoding twice.

Bugzilla, please! :-)
Comment 6 Daniel Bates 2016-03-07 14:33:35 PST
(In reply to comment #4)
> It looks like this patch has some issues:
> 
> /Volumes/Data/EWS/WebKit/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:
> 208:25: error: invalid range expression of type
> 'WTF::OptionSet<WebCore::ContentSecurityPolicyHashAlgorithm>'; no viable
> 'begin' function available
>     for (auto algorithm : algorithms) {
>                         ^ ~~~~~~~~~~
> /Volumes/Data/EWS/WebKit/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:
> 258:43: error: no member named 'isEmpty' in
> 'WTF::OptionSet<WebCore::ContentSecurityPolicyHashAlgorithm>'
>     if (!m_hashAlgorithmsForInlineScripts.isEmpty() &&
> !scriptContent.isEmpty()
>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> /Volumes/Data/EWS/WebKit/Source/WebCore/page/csp/ContentSecurityPolicy.cpp:
> 270:47: error: no member named 'isEmpty' in
> 'WTF::OptionSet<WebCore::ContentSecurityPolicyHashAlgorithm>'
>     if (!m_hashAlgorithmsForInlineStylesheets.isEmpty() &&
> !styleContent.isEmpty()
>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> 3 errors generated.

These errors are because we need to fix for bug #154941. Bug #154941 implements support for iterating over an OptionSet and determining if the set is empty.
Comment 7 Daniel Bates 2016-03-08 13:43:56 PST
Comment on attachment 273027 [details]
Patch and Layout Test

View in context: https://bugs.webkit.org/attachment.cgi?id=273027&action=review

>> Source/WebCore/ChangeLog:68
>> +        by making use Vector's equality operator.
> 
> "by making use *OF* Vector's equality operator."

Will fix.

>> Source/WebCore/dom/StyledElement.cpp:205
>> +    } else if (reason == ModifiedByCloning || document().contentSecurityPolicy()->allowInlineStyle(document().url(), startLineNumber, String(), isInUserAgentShadowTree()))
> 
> Is it better to use "emptyString()" here? I can't remember under which circumstances it is more efficient.

I can use emptyString(). Using emptyString() is less efficient than creating a null string because it is an out-of-line call among other things.

From my understanding we use an empty string to convey a valid string that contains no characters and a null string to convey an invalid string. I chose to use the latter to convey the "invalidness" of the content of the style attribute - CSP should not consider the content of the HTML style attribute.

>> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:206
>> +    // FIXME: Compute the digest with respect to the raw bytes received from the page.
> 
> Do we have a Radar or Bugzilla bug for this work?

Filed bug #155184.

>> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:243
>> +// FIXME: We should compute the document encoding once and cache it instead of computing it on each invocation.
> 
> Can you please file a Bugzilla for this so we do not forget?

As per our in-person conversation today (03/08), this FIXME comment reflects an idealized implementation. I hope you do not mind that I defer filing a bug for this FIXME at this time. Should it turn out that the inefficiencies of this code show up in profiles then we can file a bug and address this FIXME.

For completeness, I wrote this comment to reflect my unhappiness that we compute the document encoding on each invocation as opposed to computing it once after the parser determines it. Addressing this FIXME may be a non-trivial refactoring and the performance inefficiencies of this code are unlikely to be significant enough to be worth the refactoring alone. Although I do not feel that a bug is needed to track this FIXME, I feel that there is value in having this FIXME comment to capture the inefficiency of this code and in the hopes that a reader (maybe myself) consider fixing it from a theoretical performance and architectural perspective.

>> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:406
>> +}
> 
> The presence of these "Found" / "Not found" comments makes it seem like this might be better served by returning an enum indicating found/not found.

Looking at this code again, I do not feel that these comments add much value. I will remove the comments unless you feel strongly about keeping the comments are using an enum.

>> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:440
>> +    // FIXME: Normalize Base64URL to Base64 instead of decoding twice.
> 
> Bugzilla, please! :-)

Filed bug #155186.
Comment 8 Daniel Bates 2016-03-08 13:55:17 PST
Created attachment 273335 [details]
Patch and Layout Test
Comment 9 WebKit Commit Bot 2016-03-08 13:57:23 PST
Attachment 273335 [details] did not pass style-queue:


ERROR: Source/WebCore/page/csp/ContentSecurityPolicyHash.h:51:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Daniel Bates 2016-03-08 14:03:51 PST
I am open to suggestion on this patch. I am unhappy about parseHashAlgorithmAdvancingPosition(). Maybe there is a better way to implement this function? One idea is to make use of StringView.startsWith() to make the implementation more straightforward to read. Another idea is to unroll the loop.
Comment 11 Build Bot 2016-03-09 01:29:32 PST
Comment on attachment 273335 [details]
Patch and Layout Test

Attachment 273335 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/946205

New failing tests:
http/tests/security/contentSecurityPolicy/1.1/scripthash-default-src.html
Comment 12 Build Bot 2016-03-09 01:29:35 PST
Created attachment 273412 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5
Comment 13 Daniel Bates 2016-03-09 17:23:36 PST
Created attachment 273515 [details]
Patch and Layout Test
Comment 14 WebKit Commit Bot 2016-03-09 17:26:10 PST
Attachment 273515 [details] did not pass style-queue:


ERROR: Source/WebCore/page/csp/ContentSecurityPolicyHash.h:51:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Brent Fulgham 2016-03-10 09:15:19 PST
Comment on attachment 273515 [details]
Patch and Layout Test

View in context: https://bugs.webkit.org/attachment.cgi?id=273515&action=review

r=me. Nice work!

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:410
> +        if (!stringView.startsWithIgnoringASCIICase(String(ASCIILiteral(entry.label))))

Could we compute the value of String(ASCIILiteral(entry.label)) when building the labelToHashAlgorithmTable, rather than doing so every time we enter this function?

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:412
> +        position += strlen(entry.label);

If we stored Strings instead of char*, this could just be entry.length().

> LayoutTests/TestExpectations:-830
> -webkit.org/b/154203 http/tests/security/contentSecurityPolicy/1.1/stylehash-default-src.html

:-)
Comment 16 Daniel Bates 2016-03-10 09:41:43 PST
(In reply to comment #15)
> Could we compute the value of String(ASCIILiteral(entry.label)) when
> building the labelToHashAlgorithmTable, rather than doing so every time we
> enter this function?
> 

Yes, we can compute these String object in advance. Will update parseHashAlgorithmAdvancingPosition() to read:

static bool parseHashAlgorithmAdvancingPosition(const UChar*& position, size_t length, ContentSecurityPolicyHashAlgorithm& algorithm)
{
    static struct {
        NeverDestroyed<String> label;
        ContentSecurityPolicyHashAlgorithm algorithm;
    } labelToHashAlgorithmTable[] {
        { ASCIILiteral("sha256"), ContentSecurityPolicyHashAlgorithm::SHA_256 },
        { ASCIILiteral("sha384"), ContentSecurityPolicyHashAlgorithm::SHA_384 },
        { ASCIILiteral("sha512"), ContentSecurityPolicyHashAlgorithm::SHA_512 },
    };

    StringView stringView(position, length);
    for (auto& entry : labelToHashAlgorithmTable) {
        String& label = entry.label.get();
        if (!stringView.startsWithIgnoringASCIICase(label))
            continue;
        position += label.length();
        algorithm = entry.algorithm;
        return true;
    }
    return false;
}
Comment 17 Daniel Bates 2016-03-10 09:46:00 PST
Committed r197940: <http://trac.webkit.org/changeset/197940>