Bug 146159 - Check for SHA1 certificates ignores subresources
Summary: Check for SHA1 certificates ignores subresources
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Michael Catanzaro
Depends on:
Reported: 2015-06-19 13:29 PDT by Michael Catanzaro
Modified: 2015-06-20 09:07 PDT (History)
4 users (show)

See Also:

Patch (3.89 KB, patch)
2015-06-19 14:25 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.18 KB, patch)
2015-06-19 18:41 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.05 KB, patch)
2015-06-20 07:51 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-06-19 13:29:05 PDT
WebPageProxy::didCommitLoadForFrame decides whether to throw a security warning if the certificate chain includes a SHA1 certificate, but does so only for the main resource. Subresources are ignored. That's not good: the overall security of the page should be determined by the security of the least-secure subresource.

We should probably replace the second parameter to PageLoadState::didCommitLoad (which should only be called for the main resource) with a new PageLoadState member function for this.
Comment 1 Michael Catanzaro 2015-06-19 13:40:46 PDT
No need for a new function; there is already PageLoadState::didDisplayOrRunInsecureContent.
Comment 2 Michael Catanzaro 2015-06-19 14:25:33 PDT
Created attachment 255224 [details]
Comment 3 Michael Catanzaro 2015-06-19 14:28:06 PDT
The patch is really simple so I think it is safe, but note that this is Mac-specific functionality and I don't have a Mac.
Comment 4 mitz 2015-06-19 15:27:31 PDT
Comment on attachment 255224 [details]

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:-2947
> -        m_pageLoadState.didCommitLoad(transaction, hasInsecureCertificateChain);

When hasInsecureCertificateChain was false, this was the code that would reset the hasInsecureContent flag in the page load state when navigating from an “insecure content” page to a “secure” page.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2949
> +        m_pageLoadState.didDisplayOrRunInsecureContent(transaction);

This is no substitute for that, because it only changes the state from “secure” to “insecure”.
Comment 5 Michael Catanzaro 2015-06-19 17:32:32 PDT
Good catch, thanks! I was thinking that during the course of one load, we would never want to change the state from insecure to secure, but of course we need to do so for subsequent loads.

The easy fix to my patch is to reset the state to false in didCommitLoad, but that would be more fragile than the current code since the correctness would depend on the order the PageLoadState functions are called inside WebPageProxy::didCommitLoadForFrame. So we should keep the parameter and just rename it to something more accurate. It's hard to think of a good name though, so let's punt that to bug #146157.
Comment 6 Michael Catanzaro 2015-06-19 18:41:16 PDT
Created attachment 255265 [details]
Comment 7 mitz 2015-06-19 20:02:16 PDT
Comment on attachment 255265 [details]

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2944
> +    // The page is definitely insecure if true, but could still be insecure even if false.

I think I understand this comment, but I am not sure what motivates adding it here now. Doing so doesn’t seem to be directly related to fixing the bug.
Comment 8 Michael Catanzaro 2015-06-20 07:51:33 PDT
Created attachment 255289 [details]
Comment 9 Michael Catanzaro 2015-06-20 07:58:03 PDT
Comment on attachment 255289 [details]

Thanks for the review. I'll just remove the comment.
Comment 10 WebKit Commit Bot 2015-06-20 09:07:08 PDT
Comment on attachment 255289 [details]

Clearing flags on attachment: 255289

Committed r185795: <http://trac.webkit.org/changeset/185795>
Comment 11 WebKit Commit Bot 2015-06-20 09:07:12 PDT
All reviewed patches have been landed.  Closing bug.