Bug 155007 – CSP: Implement support for inline script and inline style hashes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

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.

[Daniel Bates]

<rdar://problem/24964098>

[Daniel Bates]

Created attachment 273027 [details]
Patch and Layout Test

[WebKit Commit Bot]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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! :-)

[Daniel Bates]

(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.

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 273335 [details]
Patch and Layout Test

[WebKit Commit Bot]

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.

[Daniel Bates]

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.

[Build Bot]

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

[Build Bot]

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

[Daniel Bates]

Created attachment 273515 [details]
Patch and Layout Test

[WebKit Commit Bot]

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.

[Brent Fulgham]

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

:-)

[Daniel Bates]

(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;
}

[Daniel Bates]

Committed r197940: <http://trac.webkit.org/changeset/197940>