|
Description
Brent Fulgham
2016-05-26 10:49:54 PDT
*** Bug 166958 has been marked as a duplicate of this bug. *** Created attachment 311301 [details]
Step 1: Add SecureContext extended attribute
Created attachment 312026 [details]
Part 1: Bindings
Created attachment 312030 [details]
Part 2: Implement 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(). Created attachment 312032 [details]
Part 3: Import Web Platform Tests
Can a context change from secure to not-secure in its lifetime? Or is it a static property of a document/worker? (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 312095 [details]
Part 2: Implement Secure Contexts
Created attachment 312101 [details]
Part 3: Import Web Platform Tests
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 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
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 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
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 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
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 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. 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. (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. Created attachment 312331 [details]
Part 1: Bindings
Updated patch based on feedback from Chris Dumez.
Comment on attachment 312331 [details]
Part 1: Bindings
r=me
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. Created attachment 312508 [details]
Part 2: Implement Secure Contexts
Updated patch based on feedback from Alex Christensen.
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.*.*.*. Committed r218027: <http://trac.webkit.org/changeset/218027> Re-opening bug for parts 2 and 3. Committed r218028: <http://trac.webkit.org/changeset/218028> Re-opening bug for part 3. 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 (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 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. 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 (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 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>. 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. (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.
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 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
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 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
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 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
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 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
Created attachment 312720 [details]
Part 4: Add runtime enabled feature flag for isSecureContext
Comment on attachment 312557 [details] Part 3: Import Web Platform Tests Clearing flags on attachment: 312557 Committed r218155: <http://trac.webkit.org/changeset/218155> Created attachment 312784 [details]
Part 4: Add runtime enabled feature flag for isSecureContext
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 312787 [details]
Part 4: Add runtime enabled feature flag for isSecureContext
Bump the version of IWebPreferencesPrivate.
Comment on attachment 312787 [details]
Part 4: Add runtime enabled feature flag for isSecureContext
Thank you for fixing the COM thing! r=me.
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 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
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 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. Committed r218196: <http://trac.webkit.org/changeset/218196> |