Bug 200945 – Add default to disable legacy TLS versions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Add default to disable legacy TLS versions

[Alex Christensen]

Created attachment 376811 [details]
Patch

[Geoffrey Garen]

Comment on attachment 376811 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:284
> +    parameters.defaultDataStoreParameters.networkSessionParameters.minimumTLSVersion = [defaults boolForKey:@"WebKitDisableLegacyTLS"] ?

Can we use an experimental feature flag for this instead? That way, it's easier for folks to toggle it when testing.

[Alex Christensen]

Created attachment 376817 [details]
Patch

[Alex Christensen]

This goes with rdar://54530661
Experimental features don't go in the NetworkProcess :(
Also, there's no easy way to set experimental features for all apps, but there is a way for NSUserDefaults

[Brady Eidson]

Comment on attachment 376817 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:72
> +    auto minimumTLSVersion = [defaults boolForKey:@"WebKitDisableLegacyTLS"] ?

Lets not add another [defaults <fooForKey>] in here. The others need to be reworked to be API/SPI calls instead of relying on NSUserDefaults. Please do the same for yours.

Otherwise this all seems fine,

[Brady Eidson]

(In reply to Brady Eidson from comment #5)
> Comment on attachment 376817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376817&action=review
> 
> > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:72
> > +    auto minimumTLSVersion = [defaults boolForKey:@"WebKitDisableLegacyTLS"] ?
> 
> Lets not add another [defaults <fooForKey>] in here. The others need to be
> reworked to be API/SPI calls instead of relying on NSUserDefaults. Please do
> the same for yours.
> 
> Otherwise this all seems fine.

So, I wrote this comment WITHOUT having look at the previous conversation. 

I stand by it. No NSUserDefaults please. (Doesn't have to be experimental)

[Tim Horton]

Comment on attachment 376817 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:72
>>> +    auto minimumTLSVersion = [defaults boolForKey:@"WebKitDisableLegacyTLS"] ?
>> 
>> Lets not add another [defaults <fooForKey>] in here. The others need to be reworked to be API/SPI calls instead of relying on NSUserDefaults. Please do the same for yours.
>> 
>> Otherwise this all seems fine,
> 
> So, I wrote this comment WITHOUT having look at the previous conversation. 
> 
> I stand by it. No NSUserDefaults please. (Doesn't have to be experimental)

+1 to no NSUserDefaults. Doesn't seem like something we want apps to use, and NSUserDefaults is a free hole for people to fiddle with bits. We do have the few debug-style globally-settable preferences (like layer borders) but this seems like it doesn't fit into that category either.

[Alex Christensen]

Created attachment 376944 [details]
Patch

[Alex Christensen]

Comment on attachment 376944 [details]
Patch

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

> Source/WTF/ChangeLog:3
> +        Add default to disable legacy TLS versions

I'll change the title to "Disable legacy TLS versions and add a temporary default to re-enable it"

[Alex Christensen]

Created attachment 376952 [details]
Patch

[Alex Christensen]

We need an NSUserDefault for the corresponding internal change, and we can't use a debug WebPreference because it is needed when the NetworkSessionParameters are made, and we don't know which WKWebView to use if it were a preference on a WKWebView.  The default now turns it off instead of on.

[Brady Eidson]

Comment on attachment 376952 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:284
> +    parameters.defaultDataStoreParameters.networkSessionParameters.enableLegacyTLS = [defaults boolForKey:@"WebKitEnableLegacyTLS"];

Oof

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:72
> +    bool enableLegacyTLS = [defaults boolForKey:@"WebKitDisableLegacyTLS"];

Oof.

[Brady Eidson]

Comment on attachment 376952 [details]
Patch

We're doing enable, right? Make it enable everywhere.

[Alex Christensen]

Created attachment 377021 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 377021 [details]
patch for landing

Clearing flags on attachment: 377021

Committed r249019: <https://trac.webkit.org/changeset/249019>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54606353>

[Darin Adler]

Comment on attachment 377021 [details]
patch for landing

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

> Source/WTF/wtf/Platform.h:1614
> +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000 || PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500)
> +#define HAVE_TLS_PROTOCOL_VERSION_T 1
> +#endif

What about non-IOS IOS_FAMILY platforms?

[Tim Horton]

(In reply to Darin Adler from comment #18)
> Comment on attachment 377021 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377021&action=review
> 
> > Source/WTF/wtf/Platform.h:1614
> > +#if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000 || PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500)
> > +#define HAVE_TLS_PROTOCOL_VERSION_T 1
> > +#endif
> 
> What about non-IOS IOS_FAMILY platforms?

I second this (and in fact the leftover deprecation warnings are breaking the build).

[Alex Christensen]

http://trac.webkit.org/r249340