Bug 158121 – Implement W3C Secure Contexts Draft Specification

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Brent Fulgham]

This specification defines "secure contexts", thereby allowing user agent implementers and specification authors to enable certain features only when certain minimum standards of authentication and confidentiality are met.”
https://w3c.github.io/webappsec-secure-contexts/

[Brent Fulgham]

<rdar://problem/23751632>

[Brent Fulgham]

<rdar://problem/26012994>

[Jiewen Tan]

*** Bug 166958 has been marked as a duplicate of this bug. ***

[Daniel Bates]

Created attachment 311301 [details]
Step 1: Add SecureContext extended attribute

[Daniel Bates]

Created attachment 312026 [details]
Part 1: Bindings

[Daniel Bates]

Created attachment 312030 [details]
Part 2: Implement Secure Contexts

[Daniel Bates]

(In reply to Daniel Bates from comment #6)
> Created attachment 312030 [details]
> Part 2: Implement Secure Contexts

This patch depends on "Part 1: Bindings". In this patch I moved ScriptExecutionContext::isSecureContext() to SecurityContext to group it with other security functions. Let me know if its preferred to keep this function on ScriptExecutionContext and/or if we should expose a securityContextForDOMObject() function analogous to scriptExecutionContextForDOMObject().

[Daniel Bates]

Created attachment 312032 [details]
Part 3: Import Web Platform Tests

[Sam Weinig]

Can a context change from secure to not-secure in its lifetime? Or is it a static property of a document/worker?

[Daniel Bates]

(In reply to Sam Weinig from comment #9)
> Can a context change from secure to not-secure in its lifetime? Or is it a
> static property of a document/worker?

No, a context cannot change from secure to non-secure in its lifetime (*). Will update the patch to compute potential trustworthiness on construction of the SecurityOrigin.

(*) Disregarding the open discussion about whether to consider the opener when determining if a context is secure (https://github.com/w3c/webappsec-secure-contexts/issues/42) the spec as currently written leaves open the question of how to treat a HTTPS window whose HTTP opener is disowned (say, by closing the opener). In this case, the orphaned HTTPS window could be considered a secure context. As Web APIs can be enabled/disabled based on whether the page is a secure context such a switch would likely be confusing to both a user and a web developer. So, the security value of such is switch is questionable with regards to the amount of confusion it may cause.

[Daniel Bates]

Created attachment 312095 [details]
Part 2: Implement Secure Contexts

[Daniel Bates]

Created attachment 312101 [details]
Part 3: Import Web Platform Tests

[Build Bot]

Comment on attachment 312101 [details]
Part 3: Import Web Platform Tests

Attachment 312101 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3883171

New failing tests:
imported/w3c/web-platform-tests/secure-contexts/basic-popup-and-iframe-tests.html

[Build Bot]

Created attachment 312107 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312101 [details]
Part 3: Import Web Platform Tests

Attachment 312101 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3883185

New failing tests:
imported/w3c/web-platform-tests/secure-contexts/basic-popup-and-iframe-tests.html

[Build Bot]

Created attachment 312108 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312101 [details]
Part 3: Import Web Platform Tests

Attachment 312101 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3883304

New failing tests:
imported/w3c/web-platform-tests/secure-contexts/basic-popup-and-iframe-tests.html

[Build Bot]

Created attachment 312115 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312101 [details]
Part 3: Import Web Platform Tests

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

New failing tests:
imported/w3c/web-platform-tests/secure-contexts/basic-popup-and-iframe-tests.html

[Build Bot]

Created attachment 312116 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Chris Dumez]

Comment on attachment 312026 [details]
Part 1: Bindings

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

> Source/WebCore/ChangeLog:13
> +        (ScriptExecutionContext::isSecureContext()) always returns true. We will flush out the

flush out or flesh out?

> Source/WebCore/ChangeLog:16
> +        * bindings/scripts/CodeGeneratorJS.pm:

Do not forget to update Source/WebCore/bindings/scripts/IDLAttributes.json at some point. SecureContext is marked as unsupported in there. Technically it is still not supported after this patch so I guess we want to update IDLAttributes.json once it actually works.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3327
> +    push @conjuncts, "scriptExecutionContextForDOMObject(this)->isSecureContext()" if $context->extendedAttributes->{SecureContext};

I do not believe scriptExecutionContextForDOMObject() is needed. I think we usually do "wrapped().scriptExecutionContext()" to get the DOMObject's scriptExecutionContext.

[Chris Dumez]

Comment on attachment 312095 [details]
Part 2: Implement Secure Contexts

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

> Source/WebCore/ChangeLog:9
> +        Part 2

I think this patch should probably update IDLAttributes.json to mark SecureContext as supported.

[Daniel Bates]

(In reply to Chris Dumez from comment #21)
> Comment on attachment 312026 [details]
> Part 1: Bindings
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312026&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        (ScriptExecutionContext::isSecureContext()) always returns true. We will flush out the
> 
> flush out or flesh out?
> 

Will fix.

> > Source/WebCore/ChangeLog:16
> > +        * bindings/scripts/CodeGeneratorJS.pm:
> 
> Do not forget to update Source/WebCore/bindings/scripts/IDLAttributes.json
> at some point. SecureContext is marked as unsupported in there. Technically
> it is still not supported after this patch so I guess we want to update
> IDLAttributes.json once it actually works.
> 

Will do.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3327
> > +    push @conjuncts, "scriptExecutionContextForDOMObject(this)->isSecureContext()" if $context->extendedAttributes->{SecureContext};
> 
> I do not believe scriptExecutionContextForDOMObject() is needed. I think we
> usually do "wrapped().scriptExecutionContext()" to get the DOMObject's

This will not work for arbitrary DOM objects (e.g. SubtleCrypto).

> scriptExecutionContext.

[Chris Dumez]

Comment on attachment 312026 [details]
Part 1: Bindings

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

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3327
>>> +    push @conjuncts, "scriptExecutionContextForDOMObject(this)->isSecureContext()" if $context->extendedAttributes->{SecureContext};
>> 
>> I do not believe scriptExecutionContextForDOMObject() is needed. I think we usually do "wrapped().scriptExecutionContext()" to get the DOMObject's scriptExecutionContext.
> 
> This will not work for arbitrary DOM objects (e.g. SubtleCrypto).

Sorry, I believe it should be scriptExecutionContext(). this is a JSDOMObject and JSDOMObject has a scriptExecutionContext() getter.

[Daniel Bates]

Created attachment 312331 [details]
Part 1: Bindings

Updated patch based on feedback from Chris Dumez.

[Chris Dumez]

Comment on attachment 312331 [details]
Part 1: Bindings

r=me

[Alex Christensen]

Comment on attachment 312095 [details]
Part 2: Implement Secure Contexts

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

> Source/WebCore/page/SecurityOrigin.cpp:113
> +    if (url.hostType() == URL::HostType::IPv6Address && url.host() == "::1")

I think this is supposed to be "[::1]" which leads me to believe that this is not tested.

> Source/WebCore/platform/URL.h:228
> +    HostType m_hostType { HostType::Domain };

Let's not add more member variables to the URL.  We are trying to work towards reducing the number of member variables in the URL object.  Instead we should make a function that checks URL.host() for a valid 127.*.*.* IPv4 address or an IPv6 address that is [::1].

static bool isPotentiallyTrustworthy(const URL& url)
{
    if (!url.isValid())
        return false;
    auto host = url.host();
    if (host == "[::1]")
        return true;
    
    // Check to see if it's a valid IPv4 address in 127.*.*.*
    if (!host.startsWith("127."))
        return false;
    size_t dotsFound = 0;
    for (size_t i = 0; i < host.length(); ++i) {
        if (host[i] == '.') {
            dotsFound++;
            continue;
        }
        if (!isASCIIDigit(host[i]))
            return false;
    }
    return dotsFound == 3;
}

[Daniel Bates]

(In reply to Alex Christensen from comment #27)
> Comment on attachment 312095 [details]
> Part 2: Implement Secure Contexts
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312095&action=review
> 
> > Source/WebCore/page/SecurityOrigin.cpp:113
> > +    if (url.hostType() == URL::HostType::IPv6Address && url.host() == "::1")
> 
> I think this is supposed to be "[::1]" which leads me to believe that this
> is not tested.
> 

Oops! As you can see the Web Platform Tests (attachment #312101 [details]) do not test using an IPv6 address. Will upstream a test to the Web Platform Tests project if wptserve supports IPv6. Otherwise, will add a WebKit-specific test case.

> > Source/WebCore/platform/URL.h:228
> > +    HostType m_hostType { HostType::Domain };
> 
> Let's not add more member variables to the URL.  We are trying to work
> towards reducing the number of member variables in the URL object. 

I was not aware of this effort. If the intention is that URL.h should only hold instance varibles for scheme, host, port, path, username, and password then we should convey this in a comment at the top of the class and/or in a webkit-dev email as such a restriction is not obvious.

When I originally wrote this patch I was concerned about increasing the size of the URL object by adding m_hostType and this led me to place m_hostType after the bit fields so as to not change the size of the URL object (72 bytes).

> Instead we should make a function that checks URL.host() for a valid 127.*.*.* IPv4
> address or an IPv6 address that is [::1].
> 
> static bool isPotentiallyTrustworthy(const URL& url)
> {
>     if (!url.isValid())
>         return false;
>     auto host = url.host();
>     if (host == "[::1]")
>         return true;
>     
>     // Check to see if it's a valid IPv4 address in 127.*.*.*
>     if (!host.startsWith("127."))
>         return false;
>     size_t dotsFound = 0;
>     for (size_t i = 0; i < host.length(); ++i) {
>         if (host[i] == '.') {
>             dotsFound++;
>             continue;
>         }
>         if (!isASCIIDigit(host[i]))
>             return false;
>     }
>     return dotsFound == 3;
> }

OK. Will add a helper function that uses a similar approach.

When I originally wrote this patch my thought was to reuse the URLParser parsing logic for IPv4 and IPv6 addresses and avoid a second iteration through the characters of the host. I mean, the URLParser already iterated through the URL and validated whether the parsed host is a valid IPv4 address, IPv6 address, or domain.

[Daniel Bates]

Created attachment 312508 [details]
Part 2: Implement Secure Contexts

Updated patch based on feedback from Alex Christensen.

[Daniel Bates]

Comment on attachment 312508 [details]
Part 2: Implement Secure Contexts

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

> Source/WebCore/page/SecurityOrigin.cpp:109
> +    // Check to see if it's a valid IPv4 address in 127.*.*.*

Will change this comment to read:

Check to see if it's a valid IPv4 address that has the form 127.*.*.*.

[Daniel Bates]

Committed r218027: <http://trac.webkit.org/changeset/218027>

[Daniel Bates]

Re-opening bug for parts 2 and 3.

[Daniel Bates]

Committed r218028: <http://trac.webkit.org/changeset/218028>

[Daniel Bates]

Re-opening bug for part 3.

[Oliver Hunt]

Comment on attachment 312508 [details]
Part 2: Implement Secure Contexts

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

> Source/WebCore/page/WindowOrWorkerGlobalScope.idl:44
> +    readonly attribute boolean isSecureContext;

Given this is exposed you should be able to have a test for this

[Sam Weinig]

(In reply to Oliver Hunt from comment #35)
> Comment on attachment 312508 [details]
> Part 2: Implement Secure Contexts
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312508&action=review
> 
> > Source/WebCore/page/WindowOrWorkerGlobalScope.idl:44
> > +    readonly attribute boolean isSecureContext;
> 
> Given this is exposed you should be able to have a test for this

It should also be conditionally enabled behind a Setting, as is the way for new features.

[Daniel Bates]

(In reply to Oliver Hunt from comment #35)
> Comment on attachment 312508 [details]
> Part 2: Implement Secure Contexts
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312508&action=review
> 
> > Source/WebCore/page/WindowOrWorkerGlobalScope.idl:44
> > +    readonly attribute boolean isSecureContext;
> 
> Given this is exposed you should be able to have a test for this

Tests are in part 3 (attachment #312101 [details]). Feel free to review them.

[Ryosuke Niwa]

I think this patch caused an assertion to be hit in fast/dom/Window/property-access-on-cached-window-after-frame-removed.html:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fdom%2FWindow%2Fproperty-access-on-cached-window-after-frame-removed.html

[Daniel Bates]

(In reply to Sam Weinig from comment #36)
> (In reply to Oliver Hunt from comment #35)
> > Comment on attachment 312508 [details]
> > Part 2: Implement Secure Contexts
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=312508&action=review
> > 
> > > Source/WebCore/page/WindowOrWorkerGlobalScope.idl:44
> > > +    readonly attribute boolean isSecureContext;
> > 
> > Given this is exposed you should be able to have a test for this
> 
> It should also be conditionally enabled behind a Setting, as is the way for
> new features.

Will add a setting to toggle exposing this attribute. Or are you asking for this and the equivalent of webSecurityEnabled - a setting to treat every context as a secure context?

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #38)
> I think this patch caused an assertion to be hit in
> fast/dom/Window/property-access-on-cached-window-after-frame-removed.html:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Fdom%2FWindow%2Fproperty-access-on-cached-
> window-after-frame-removed.html

Committed fix in <https://trac.webkit.org/changeset/218037>.

[Daniel Bates]

Created attachment 312557 [details]
Part 3: Import Web Platform Tests

Rebase patch now that part 1 (attachment #312331 [details]) and part 2 (attachment #312508 [details]) landed so that EWS can process it.

[Sam Weinig]

(In reply to Daniel Bates from comment #39)
> (In reply to Sam Weinig from comment #36)
> > (In reply to Oliver Hunt from comment #35)
> > > Comment on attachment 312508 [details]
> > > Part 2: Implement Secure Contexts
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=312508&action=review
> > > 
> > > > Source/WebCore/page/WindowOrWorkerGlobalScope.idl:44
> > > > +    readonly attribute boolean isSecureContext;
> > > 
> > > Given this is exposed you should be able to have a test for this
> > 
> > It should also be conditionally enabled behind a Setting, as is the way for
> > new features.
> 
> Will add a setting to toggle exposing this attribute. Or are you asking for
> this and the equivalent of webSecurityEnabled - a setting to treat every
> context as a secure context?

The former. We are trying to enforce the policy that 'new features' have settings to toggle them on and off. Though I will admit, the definition of 'new feature' is a bit vague right now.

[Daniel Bates]

Created attachment 312606 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

This patch adds a runtime enabled feature flag to toggle exposing the global property isSecureContext (defaults: true - expose the property). It also adds a test to verify that isSecureContext is not exposed (is undefined) when the runtime enabled feature flag is disabled.

I added a runtime enabled feature flag as opposed to a Setting because the global property isSecureContext is exposed to Web Workers and Web Workers do not have access to their associated page's Settings object.

[Build Bot]

Comment on attachment 312606 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

Attachment 312606 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3913005

New failing tests:
security/isSecureContext-disabled.html

[Build Bot]

Created attachment 312608 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312606 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

Attachment 312606 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3913027

New failing tests:
security/isSecureContext-disabled.html

[Build Bot]

Created attachment 312610 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312606 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

Attachment 312606 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3913034

New failing tests:
security/isSecureContext-disabled.html

[Build Bot]

Created attachment 312612 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312606 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

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

New failing tests:
security/isSecureContext-disabled.html

[Build Bot]

Created attachment 312614 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Daniel Bates]

Created attachment 312720 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

[Daniel Bates]

Comment on attachment 312557 [details]
Part 3: Import Web Platform Tests

Clearing flags on attachment: 312557

Committed r218155: <http://trac.webkit.org/changeset/218155>

[Daniel Bates]

Created attachment 312784 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

[Brent Fulgham]

Comment on attachment 312784 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

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

This looks great, except the fragility of COM Interfaces means that you have to make a new IWebPreferencesPrivate5 to hold the new method. Otherwise shipping this code will break existing COM clients that don't expect those new methods in the interface.

> Source/WebKit/win/Interfaces/IWebPreferencesPrivate.idl:205
> +    HRESULT setIsSecureContextAttributeEnabled([in] BOOL enabled);

These two new values must be moved to a new IWebPreferencesPrivate5 that inherits from IWebPreferencesPrivate4, or else you will break shipping WebKit clients.

> Source/WebKit/win/WebView.cpp:5288
> +    hr = prefsPrivate->isSecureContextAttributeEnabled(&enabled);

You must construct an IWebPreferencesPrivate5 now to access this new method. Or use one of the QueryInterface cast functions to get the proper handle.

[Daniel Bates]

Created attachment 312787 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

Bump the version of IWebPreferencesPrivate.

[Brent Fulgham]

Comment on attachment 312787 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

Thank you for fixing the COM thing! r=me.

[Build Bot]

Comment on attachment 312787 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

Attachment 312787 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3924549

New failing tests:
imported/w3c/web-platform-tests/secure-contexts/basic-popup-and-iframe-tests.html

[Build Bot]

Created attachment 312791 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312787 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

Attachment 312787 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3924522

New failing tests:
imported/w3c/web-platform-tests/secure-contexts/basic-popup-and-iframe-tests.html

[Build Bot]

Created attachment 312793 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Daniel Bates]

Comment on attachment 312787 [details]
Part 4: Add runtime enabled feature flag for isSecureContext

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

> Tools/DumpRenderTree/TestOptions.h:38
> +    bool enableIsSecureContextAttribute { false };

Will change default value for enableIsSecureContextAttribute from false to true before landing to fix test failure of imported/w3c/web-platform-tests/secure-contexts/basic-popup-and-iframe-tests.html as reported by the WebKit1 EWS bots.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:855
> +    [preferences setIsSecureContextAttributeEnabled:YES];

Will revert this change before landing as it is unnecessary. We will enable the isSecureContext attribute depending on the result of parsing the test option enableIsSecureContextAttribute.

[Daniel Bates]

Committed r218196: <http://trac.webkit.org/changeset/218196>