Bug 146159

Summary: Check for SHA1 certificates ignores subresources
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKit2Assignee: Michael Catanzaro <mcatanzaro>
Severity: Normal CC: commit-queue, mcatanzaro, mitz, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=142461
Description Flags
Patch none

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.