Bug 192074

Created attachment 355875 [details] Patch

This patch is a WIP and RFC based on the hsts branch in libsoup, that is being discussed here: https://gitlab.gnome.org/GNOME/libsoup/merge_requests/22 Important points of consideration with respect this patch (I might add this to the ChangeLog before landing when we do in some form or another): - Soup has two HSTS enforcers: a persistent one using sqlite and a non-persistent one. For the sake of simplicity this patch uses the non-persistent one, as I want to focus on the implementation issues below. A final version should use the persistent one. - NetworkDataTaskSoup::protocolUpgradedViaHSTS() is based on NetworkDataTaskSoup::continueHTTPRedirection(). The underlying issue here is that when libsoup rewrites the protocol in a URI prior to sending a message, there is no knowledge of this in clients. The idea of this method is to allow clients to be notified, for example, so that the URL bar in the browser gets updated as well. I am not 100% sure that this is the best way to do this so someone who knows better the network process in the soup ports should check this. - libsoup has no API (yet) to learn when a message has been upgraded to HTTPS via HSTS, so we're relying on the notify::uri signal in SoupMessage. This works, but might not be the most reliable way to do it, as the uri could change for other reasons (?). - The patch includes as well mitigation for HSTS protocol abuse, following what Apple implements for Safari. In particular, if a request for a HTTPS URI is not for the main document, disable the HSTS feature. This way we prevent subresources from adding potentially large amounts of HSTS policies that can later be used for user profiling. - The mitigation code relies on the fix for bug #184987, which is included here for testing purposes. Before landing this patch, we need to ensure that bug #184987 is fixed or find an alternative way to get the main document. In order to finish the HSTS branch in libsoup I would appreciate some comments in the WebKit implementation. A fresh pair of eyes will probably be helpful in ironing out important API details in the libsoup side.

Comment on attachment 355875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355875&action=review > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:154 > +#if SOUP_CHECK_VERSION(2, 65, 1) > + // Follow Apple's HSTS tracking mitigation. Disabling the HSTS enforcer for a HTTPS request that > + // is not a toplevel frame will prevent subresources from setting a STS value. > + if (m_currentRequest.url().protocolIs("https") && !m_currentRequest.isTopSite()) > + soup_message_disable_feature(soupMessage.get(), SOUP_TYPE_HSTS_ENFORCER); > +#endif From https://webkit.org/blog/8146/protecting-against-hsts-abuse/ I see that this is similar to Mitigation 2, with the difference that Apple's Mitigation 2 is more limited because it is tied in with the intelligent tracking prevention feature. i.e. the HSTS "redirect" should occur unless the subresource would be prevented from storing cookies. Any plans for this? And any plans for Mitigation 1? > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1131 > +void NetworkDataTaskSoup::protocolUpgradedViaHSTS(SoupMessage *soupMessage) SoupMessage* soupMessage > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1137 > + if (!redirectedURL.hasFragmentIdentifier() && request.url().hasFragmentIdentifier()) > + redirectedURL.setFragmentIdentifier(request.url().fragmentIdentifier()); Out of curiosity, do you know why do we have to manually restore the fragment? It seems weird that the fragment would be lost during a redirect? > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1154 > +void NetworkDataTaskSoup::notifyUri(SoupMessage* soupMessage, GParamSpec *spec, NetworkDataTaskSoup* task) GParamSpec* spec > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1156 > + if (soup_uri_get_scheme (soup_message_get_uri (soupMessage)) == SOUP_URI_SCHEME_HTTPS) soup_message_get_uri(soupMessage) > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1157 > + task->protocolUpgradedViaHSTS(soupMessage); Perhaps we can store the original URI on the SoupMessage using g_object_set_data()? Or just store it in NetworkDataTaskSoup? That way we would have it available for comparison here? > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.h:118 > + static void notifyUri(SoupMessage*, GParamSpec *, NetworkDataTaskSoup*); GParamSpec*

Comment on attachment 355875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355875&action=review >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:154 >> +#endif > > From https://webkit.org/blog/8146/protecting-against-hsts-abuse/ I see that this is similar to Mitigation 2, with the difference that Apple's Mitigation 2 is more limited because it is tied in with the intelligent tracking prevention feature. i.e. the HSTS "redirect" should occur unless the subresource would be prevented from storing cookies. Any plans for this? > > And any plans for Mitigation 1? No plans for anything yet. I had first thought to do the mitigations separately and afterwards but since I already had this, I put it in this patch. If you think it would make more sense to implement them separately from the basic HSTS feature, we can do that. As long as I have time at some point, yes, I would do them. If just doing the above in the meantime is not good, I can remove them from the patch. >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1137 >> + redirectedURL.setFragmentIdentifier(request.url().fragmentIdentifier()); > > Out of curiosity, do you know why do we have to manually restore the fragment? It seems weird that the fragment would be lost during a redirect? In this particular case, I think this doesn't make sense because we're not really following a redirect. We're just upgrading to HTTPS before we even load anything. So it's true that this is not needed. Which leads to the comment below. > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1152 > +} All of the code above is questionable because we're abusing NetworkLoadClient::willPerformHTTPRedirection(). This has problems: we're telling the network process that this should count as one redirect, and be treated as such, when this is not really what's happening here. In reality, all we need is to notify the clients that the protocol is being upgraded before any request is performed, so that clients are aware of the new URL with https scheme. I haven't found yet a clean way to do this. >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1157 >> + task->protocolUpgradedViaHSTS(soupMessage); > > Perhaps we can store the original URI on the SoupMessage using g_object_set_data()? Or just store it in NetworkDataTaskSoup? That way we would have it available for comparison here? I decided to add a hsts-enforced signal to the SoupHSTSEnforcer class so that we can do the above only on HSTS policy enforcing.

Created attachment 357314 [details] Patch

Patch rebased and cleaned up after the review. The new version of the patch uses the SoupHSTSEnforcer::hsts-enforced signal instead of relying on the notify signal. It's a bit cleaner. I also added cleaning up the signal handler, which was missing in the previous patch. After reviewing how the cocoa port deals with this, I realized that there's no better way than to act as in a HTTP redirection, so we probably want to do that. I did clean a bit ::protocolUpgradedViaHSTS() to the minimum that seems needed fo this to work. This is still missing persistency so I will probably tackle that in a next iteration. I would appreciate some comments on the protocolUpgradedViaHSTS() method, since that's the part that makes me unsure about this patch. Maybe Carlos?

Comment on attachment 357314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357314&action=review > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1136 > + auto response = ResourceResponse(m_response); I guess we should use ResourceResponse::syntheticRedirectResponse() no? > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1140 > + ResourceRequest request = m_currentRequest; > + URL redirectedURL = soupURIToURL(soup_message_get_uri(soupMessage)); > + request.setURL(redirectedURL); > + m_client->willPerformHTTPRedirection(WTFMove(response), WTFMove(request), [this, protectedThis = makeRef(*this)](const ResourceRequest& newRequest) { I think we are missing here all other things that are done before a normal redirection. I think we should make m_response the syntheiszed redirect response and simply call continueHTTPRedirection(). > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1157 > + if (soupMessage == task->m_soupMessage.get()) > + task->protocolUpgradedViaHSTS(soupMessage); You should also check the task state. if (task->state() == State::Canceling || task->state() == State::Completed || !task->m_client) { task->clearRequest(); return; }

(In reply to Michael Catanzaro from comment #3) > From https://webkit.org/blog/8146/protecting-against-hsts-abuse/ I see that > this is similar to Mitigation 2, with the difference that Apple's Mitigation > 2 is more limited because it is tied in with the intelligent tracking > prevention feature. i.e. the HSTS "redirect" should occur unless the > subresource would be prevented from storing cookies. Any plans for this? > > And any plans for Mitigation 1? For myself, I'd just like to see both mitigations implemented correctly.

(In reply to Carlos Garcia Campos from comment #7) > Comment on attachment 357314 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357314&action=review > > > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1136 > > + auto response = ResourceResponse(m_response); > > I guess we should use ResourceResponse::syntheticRedirectResponse() no? > > > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1140 > > + ResourceRequest request = m_currentRequest; > > + URL redirectedURL = soupURIToURL(soup_message_get_uri(soupMessage)); > > + request.setURL(redirectedURL); > > + m_client->willPerformHTTPRedirection(WTFMove(response), WTFMove(request), [this, protectedThis = makeRef(*this)](const ResourceRequest& newRequest) { > > I think we are missing here all other things that are done before a normal > redirection. I think we should make m_response the syntheiszed redirect > response and simply call continueHTTPRedirection(). > > > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1157 > > + if (soupMessage == task->m_soupMessage.get()) > > + task->protocolUpgradedViaHSTS(soupMessage); > > You should also check the task state. > > if (task->state() == State::Canceling || task->state() == State::Completed > || !task->m_client) { > task->clearRequest(); > return; > } Thanks for the review. What I infer from your review is that besides implementation details related to how the protocol upgrade is dealt with, in general lines the patch seems fine. This would mean that the libsoup API is sufficient to implement this in WebKit and it would probably be a good time to merge it upstream while I finish the WebKit implementation.

Up to you, but my recommendation would be to merge it upstream immediately after branching for the next release, since the WebKit side of the work is going to slip. That way, you can still make libsoup API changes until August if any unexpectedly prove to be needed.

The libsoup API looks good to me, but I don't know much about HSTS, and I haven't seen the libsoup implementation either.

Comment on attachment 357314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357314&action=review >>> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1136 >>> + auto response = ResourceResponse(m_response); >> >> I guess we should use ResourceResponse::syntheticRedirectResponse() no? > > Thanks for the review. What I infer from your review is that besides implementation details related to how the protocol upgrade is dealt with, in general lines the patch seems fine. This would mean that the libsoup API is sufficient to implement this in WebKit and it would probably be a good time to merge it upstream while I finish the WebKit implementation. Not sure about this, since we're not really synthesizing a redirection. We just want to let the clients know that we're changing the protocol to HTTPS. We haven't yet contacted the server, we're about to do it, but we want clients to know that the URL we're about to connect to is different. >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1140 >> + m_client->willPerformHTTPRedirection(WTFMove(response), WTFMove(request), [this, protectedThis = makeRef(*this)](const ResourceRequest& newRequest) { > > I think we are missing here all other things that are done before a normal redirection. I think we should make m_response the syntheiszed redirect response and simply call continueHTTPRedirection(). This simplifies the code a lot. I'm still unsure that it's right to treat this as a redirect, but I trust your judgement here. >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1157 >> + task->protocolUpgradedViaHSTS(soupMessage); > > You should also check the task state. > > if (task->state() == State::Canceling || task->state() == State::Completed || !task->m_client) { > task->clearRequest(); > return; > } I'm assuming that this should be done before performing the upgrade?

So I'm currently looking into making the HSTS enforcer persistent, as it's not very useful as it is in the patch without persistency. We've added to libsoup a sqlite HSTS enforcer, akin to the sqlite cookie jar. What I'm trying to decide now is what's the best way to decide where to store the database. If we want clients to be able to able to directly clean the HSTS cache, we would have to add API to set/expose the file location. There's also the data manager, that I think is what Cocoa is using these days for HSTS as well. Any input on this? There's also the question about making this feature runtime configurable, in case not all clients want to use HSTS always. Basically I need some input in deciding the WK api we will need for this ;)

(In reply to Claudio Saavedra from comment #13) > So I'm currently looking into making the HSTS enforcer persistent, as it's > not very useful as it is in the patch without persistency. We've added to > libsoup a sqlite HSTS enforcer, akin to the sqlite cookie jar. What I'm > trying to decide now is what's the best way to decide where to store the > database. If we want clients to be able to able to directly clean the HSTS > cache, we would have to add API to set/expose the file location. There's > also the data manager, that I think is what Cocoa is using these days for > HSTS as well. Any input on this? Hmm.... The cross-platform WebsiteDataStore already integrates with HSTS cache... somehow. But it's not clear to me what we should do. Option #1: Our WebKitWebsiteDataManager could expose a new hsts-cache-directory property and webkit_website_data_manager_get_hsts_cache_directory(). When the property is NULL, it should be stored under a subdirectory of the base-storage-directory. We should add a new WEBKIT_WEBSITE_DATA_HSTS_CACHE type to WebKitWebsiteDataTypes, then we can test that it can be cleared using webkit_website_data_manager_remove(). That would make the HSTS cache a normal website data type and fit well with our existing API. But it seems Apple doesn't take that approach. E.g. there's no API in WebsiteDataStoreConfiguration to set HSTS cache directory. There is a WebsiteDataType for HSTS cache, but it's not clear to me how it's used. Some more investigation required here. > There's also the question about making this feature runtime configurable, in > case not all clients want to use HSTS always. You can add a WebKitSetting, following the general pattern for new settings there.

Created attachment 375425 [details] Patch

The patch still misses tests for the new API so it's not ready for landing, but I would appreciate another round of review as I added most of what was missing: - Support for persistent HSTS storage. - API to clean HSTS entries via the website data manager. - Fixed the redirect handling as suggested by Carlos. It also depends in yet unreleased libsoup API but I will take care of releasing that before this lands.

(In reply to Michael Catanzaro from comment #3) > From https://webkit.org/blog/8146/protecting-against-hsts-abuse/ I see that > this is similar to Mitigation 2, with the difference that Apple's Mitigation > 2 is more limited because it is tied in with the intelligent tracking > prevention feature. i.e. the HSTS "redirect" should occur unless the > subresource would be prevented from storing cookies. Any plans for this? > > And any plans for Mitigation 1? I'd request that the changelog document your solution for both Mitigation 1 and Mitigation 2 to ensure this isn't going to allow HSTS supercookie abuse.

Created attachment 375615 [details] Patch

Updates with regards to the previous version: - Use GUniqueRefPtr where it makes sense, add definition for SoupHSTSPolicy free function. - Bumped the libsoup version in jhbuild modulesets to 2.67.91, which has all the required API. - Implement both mitigations 1 and 2 from Apple as separate methods and use them to decide to disable the HSTS enforcer for a request. Tests are still missing, but while I work on them, again, review of the implementation would be welcome.

Created attachment 375618 [details] Patch

Comment on attachment 375618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375618&action=review > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:64 > +#if USE(GLIB) // According to r245075 this will eventually move here. Yep

Looks very nice to me. Carlos Garcia will want to review it before it lands, of course.

Created attachment 375799 [details] Patch

OK, now with API tests. I fixed a few things I caught and I think this is ready for a final round of review.

Created attachment 376885 [details] Patch

Comment on attachment 376885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376885&action=review > Source/WebCore/platform/network/soup/GUniquePtrSoup.h:36 > +WTF_DEFINE_GPTR_DELETER(SoupHSTSPolicy, soup_hsts_policy_free); Remove the trailing ; > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:182 > +void SoupNetworkSession::setHSTSPersistentStorage(const CString& directory) > +{ > + hstsStorageDirectory() = directory; > +} The problem I see with this is that is a global setting, so it will be used for ephemeral sessions too. > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:184 > +void SoupNetworkSession::setupHSTSEnforcer() So, we could either receive here the session ID and check both hstsStorageDirectory().isNull() || session.isEphemeral() to use a session enforcer, or save the sessionID received in the constructor as a member to check it here. > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:215 > + for (GList* iter = domains.get(); iter != nullptr; iter = iter->next) { iter != nullptr -> iter > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:217 > + hostNames.add(domain.get()); String::fromUTF8(domain.get()) > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:230 > + if (!soup_session_has_feature(m_soupSession.get(), SOUP_TYPE_HSTS_ENFORCER)) > + return; > + > + SoupHSTSEnforcer* enforcer = SOUP_HSTS_ENFORCER(soup_session_get_feature(m_soupSession.get(), SOUP_TYPE_HSTS_ENFORCER)); Do we need to check first? can't we just get the feature and null check to return early? > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:231 > + for (auto& hostName : hostNames) { const auto& > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:232 > + GUniquePtr<SoupHSTSPolicy> policy(soup_hsts_policy_new(hostName.utf8().data(), SOUP_HSTS_POLICY_MAX_AGE_PAST, false)); FALSE > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:246 > + if (!soup_session_has_feature(m_soupSession.get(), SOUP_TYPE_HSTS_ENFORCER)) > + return; > + > + SoupHSTSEnforcer* enforcer = SOUP_HSTS_ENFORCER(soup_session_get_feature(m_soupSession.get(), SOUP_TYPE_HSTS_ENFORCER)); SAme here about has + get > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:247 > + GUniquePtr<GList> policies(soup_hsts_enforcer_get_policies(enforcer, false)); FALSE > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:252 > + GUniquePtr<SoupHSTSPolicy> newPolicy(soup_hsts_policy_new(policy.get()->domain, SOUP_HSTS_POLICY_MAX_AGE_PAST, false)); FALSE > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.h:116 > + bool shouldAllowHSTSPolicySetting(); const > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.h:117 > + bool shouldAllowHSTSProtocolUpgrade(); const > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:87 > + PROP_HSTS_DIRECTORY, I would call this PROP_HSTS_CACHE_DIRECTORY, and the same for the property name and variable member. > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:348 > + * The directory where the HTTP Strict-Transport-Security (HSTS) database will be stored. s/database/cache/ the fact that the cache is a db is an impl detail. > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:360 > + static_cast<GParamFlags>(WEBKIT_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_DEPRECATED))); DEPRECATED? :-) > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:645 > + * webkit_website_data_manager_get_hsts_directory: cache_directory > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebsiteData.cpp:510 > + // We wait 5 seconds before adding more entries so that we can test the clear API with a time other than zero. > + sleep(5); 5 seconds is too long for a test. maybe it's fine to only test the clear api with 0.

Created attachment 377436 [details] Patch

Committed r249192: <https://trac.webkit.org/changeset/249192>

<rdar://problem/54783982>