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/
(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().
(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.
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
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
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
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
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.
(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.
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.
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;
}
(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.
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.*.*.*.
(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.
(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?
(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.
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.
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
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
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
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
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.
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
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
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.
2017-05-25 15:18 PDT, Daniel Bates
2017-06-05 13:27 PDT, Daniel Bates
2017-06-05 14:29 PDT, Daniel Bates
2017-06-05 14:40 PDT, Daniel Bates
2017-06-06 11:32 PDT, Daniel Bates
2017-06-06 12:06 PDT, Daniel Bates
2017-06-06 12:40 PDT, Build Bot
2017-06-06 12:47 PDT, Build Bot
2017-06-06 13:29 PDT, Build Bot
2017-06-06 13:34 PDT, Build Bot
2017-06-08 12:04 PDT, Daniel Bates
2017-06-09 16:25 PDT, Daniel Bates
2017-06-09 21:05 PDT, Daniel Bates
2017-06-11 10:05 PDT, Daniel Bates
2017-06-11 11:14 PDT, Build Bot
2017-06-11 11:24 PDT, Build Bot
2017-06-11 11:39 PDT, Build Bot
2017-06-11 11:45 PDT, Build Bot
2017-06-12 16:27 PDT, Daniel Bates
2017-06-13 10:20 PDT, Daniel Bates
2017-06-13 10:52 PDT, Daniel Bates
buildbot: commit-queue-
2017-06-13 12:07 PDT, Build Bot
2017-06-13 12:31 PDT, Build Bot