Bug 142461 - [Cocoa] Add an option to treat certificate chains with SHA1-signed certificates as insecure
Summary: [Cocoa] Add an option to treat certificate chains with SHA1-signed certificat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-08 13:41 PDT by mitz
Modified: 2015-06-19 14:54 PDT (History)
5 users (show)

See Also:


Attachments
Add _treatsSHA1SignedCertificatesAsInsecure property to WKWebViewConfiguration (21.55 KB, patch)
2015-03-08 13:56 PDT, mitz
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2015-03-08 13:41:37 PDT
Add a WKWebViewConfiguration option to make the view treat certificate chains where the signature hash algorithm for a non-root certificate is SHA-1 as insecure for purposes of -[WKWebView containsInsecureContent].
Comment 1 Radar WebKit Bug Importer 2015-03-08 13:42:18 PDT
<rdar://problem/20086546>
Comment 2 mitz 2015-03-08 13:56:07 PDT
Created attachment 248202 [details]
Add _treatsSHA1SignedCertificatesAsInsecure property to WKWebViewConfiguration
Comment 3 WebKit Commit Bot 2015-03-08 13:58:38 PDT
Attachment 248202 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:35:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:36:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:37:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:38:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:39:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:40:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:41:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:42:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:43:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/SecuritySPI.h:49:  The parameter name "certificate" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 mitz 2015-03-10 00:35:28 PDT
Committed most of attachment 248202 [details] in <http://trac.webkit.org/r181317>, but left out the OS X part. I am going to turn that on soon.
Comment 5 mitz 2015-03-12 14:02:46 PDT
Finished landing this in <http://trac.webkit.org/r181455>.
Comment 6 Michael Catanzaro 2015-06-19 10:05:05 PDT
So it looks like these commits ensure that Apple port users get some warning if a SHA1 certificate is used in the chain for the main resource (without blocking anything). Kudos for that change. But it allows SHA1 certificates in subresources without any warning. Surely that is not the desired behavior?

Instead of adding the second hasInsecureContent parameter to PageLoadState::didCommitLoad (which should only be called for the main resource), it would probably be better to add a new member function to PageLoadState for this.

Aside #1: hasInsecureContent is not an accurate name for the new parameter to PageLoadState::didCommitLoad: the only reason it could be true is if the certificate uses a SHA1 signature, so hasInsecureContent will usually be *false* if the certificate is insecure. We should fix that. I don't have a great alternative name, but one possibility is hasDubiouslySecureContent, since "dubious" is often used to indicate that the security is acceptable (we're not going to block the connection) but not good. This isn't a great name either, but it's much better, since this code cannot ever block any content.

Aside #2: I'm working on a patch to replace treatsSHA1SignedCertificatesAsInsecure in WebPageConfiguration with a platform-specific object (my current working name for it is WebCore::CertificateInfo::DubiousnessConfiguration) since (a) the curl and soup ports will never use it, and (b) WebPageConfiguration is used to hold a few very important objects, not preferences (c) that is one highly-specific certificate check out of many, and WebPageProxy is not the right layer for it to be in. (Alternatively we could use conditional compilation for it, but that only solves (a) and not (b) or (c).)
Comment 7 mitz 2015-06-19 14:54:37 PDT
(In reply to comment #6)

Thanks for the comment and the new bugs you’ve filed, Michael!