Bug 178484

Summary: Adopt new secure coding APIs in WebCore
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, commit-queue, darin, dbates, eric.carlson, jlewis3, rniwa, ryanhaddad, thorton
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 180127    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch
none
Patch
none
Patch
none
Patch eric.carlson: review+

Description Brent Fulgham 2017-10-18 13:36:14 PDT
Switch away from older NSKeyedArchiver/NSKeyedUnarchive methods that supported non-NSSecureCoding serialization.
Comment 1 Brent Fulgham 2017-10-18 16:51:37 PDT
<rdar://problem/34837193>
Comment 2 Brent Fulgham 2017-10-18 17:01:37 PDT
Created attachment 324182 [details]
Patch
Comment 3 Brent Fulgham 2017-10-19 08:42:07 PDT
Created attachment 324238 [details]
Patch
Comment 4 Brent Fulgham 2017-10-19 10:11:31 PDT
Created attachment 324245 [details]
Patch
Comment 5 Brent Fulgham 2017-10-19 12:08:47 PDT
Created attachment 324258 [details]
Patch
Comment 6 Brent Fulgham 2017-10-19 13:23:56 PDT
Created attachment 324273 [details]
Patch
Comment 7 Brent Fulgham 2017-10-19 17:34:46 PDT
Created attachment 324322 [details]
Patch
Comment 8 Brent Fulgham 2017-10-19 20:03:53 PDT
Created attachment 324337 [details]
Patch
Comment 9 Brent Fulgham 2017-10-19 20:51:44 PDT
Created attachment 324344 [details]
Patch
Comment 10 Build Bot 2017-10-19 21:57:59 PDT
Comment on attachment 324344 [details]
Patch

Attachment 324344 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4928409

New failing tests:
http/tests/xmlhttprequest/reentrant-cancel-abort.html
http/tests/xmlhttprequest/reentrant-cancel.html
fast/loader/subresource-load-failed-crash.html
Comment 11 Build Bot 2017-10-19 21:58:00 PDT
Created attachment 324350 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Build Bot 2017-10-19 21:58:03 PDT
Comment on attachment 324344 [details]
Patch

Attachment 324344 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4928236

New failing tests:
http/tests/xmlhttprequest/reentrant-cancel-abort.html
http/tests/xmlhttprequest/reentrant-cancel.html
fast/loader/subresource-load-failed-crash.html
Comment 13 Build Bot 2017-10-19 21:58:04 PDT
Created attachment 324351 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 14 Build Bot 2017-10-19 22:30:55 PDT
Comment on attachment 324344 [details]
Patch

Attachment 324344 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4928452

New failing tests:
http/tests/xmlhttprequest/reentrant-cancel-abort.html
http/tests/xmlhttprequest/reentrant-cancel.html
fast/loader/subresource-load-failed-crash.html
Comment 15 Build Bot 2017-10-19 22:30:57 PDT
Created attachment 324357 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 16 Ryosuke Niwa 2017-10-19 22:48:11 PDT
Comment on attachment 324344 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:349
> +    ASSERT(renderer().document().frame());

I don't think we have to keep checking this condition over and over.
Comment 17 Brent Fulgham 2017-10-20 08:09:18 PDT
Created attachment 324400 [details]
Patch
Comment 18 Brent Fulgham 2017-10-20 09:34:57 PDT
(In reply to Ryosuke Niwa from comment #16)
> Comment on attachment 324344 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324344&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:349
> > +    ASSERT(renderer().document().frame());
> 
> I don't think we have to keep checking this condition over and over.

Yeah -- that wasn't supposed to be in the patch. It was unrelated debugging code.

Please look at the revised patch.
Comment 19 Build Bot 2017-10-20 15:48:51 PDT
Comment on attachment 324400 [details]
Patch

Attachment 324400 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4937386

New failing tests:
webrtc/video-replace-muted-track.html
Comment 20 Build Bot 2017-10-20 15:48:52 PDT
Created attachment 324454 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 21 Tim Horton 2017-10-20 17:23:35 PDT
Comment on attachment 324400 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:28
> +#if USE(APPLE_INTERNAL_SDK) && ((PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101302) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110200))

How sure are you about MIN_REQUIRED vs. MAX_AVAILABLE and whether the minor part of the version number is actually incremented?
Comment 22 Brent Fulgham 2017-10-20 19:25:15 PDT
Comment on attachment 324400 [details]
Patch

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

>> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:28
>> +#if USE(APPLE_INTERNAL_SDK) && ((PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101302) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110200))
> 
> How sure are you about MIN_REQUIRED vs. MAX_AVAILABLE and whether the minor part of the version number is actually incremented?

I think MIN_REQUIRED is right, because these calls are not available in earlier SDKs so we don’t want to try to use them when building for older OS’s. But maybe a build-type person can weigh in and confirm/deny.

I know that we do not update minor version numbers, so we will likely not use this code in production builds until we rev them again. Or, we could downgrade these checks once the updated API is available on our build bots.
Comment 23 Brent Fulgham 2017-10-21 15:57:52 PDT
(In reply to Build Bot from comment #19)
> New failing tests:
> webrtc/video-replace-muted-track.html

This test failure is not related to this patch.
Comment 24 Brent Fulgham 2017-10-21 15:58:53 PDT
I've sent a note to our build team to get confirmation about whether we should use MIN_REQUIRED or MAX_AVAILABLE (or some combo of the two).
Comment 25 Brent Fulgham 2017-10-23 11:15:34 PDT
Created attachment 324569 [details]
Patch
Comment 26 Brent Fulgham 2017-10-23 13:52:59 PDT
Created attachment 324583 [details]
Patch
Comment 27 Tim Horton 2017-10-23 16:18:12 PDT
Comment on attachment 324583 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:28
> +#define USE_NEW_ARCHIVER_API    ((PLATFORM(MAC) && __MAC_OS_X_VERSION_MAX_ALLOWED >= 101302 && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110200) || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 40200) || (PLATFORM(TVOS) && __TV_OS_VERSION_MIN_REQUIRED >= 110200))

Weird number of spaces before the first (

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:89
> +inline NSKeyedArchiver *_Nonnull secureArchiverFromMutableData(NSMutableData *_Nonnull mutableData)

This should return a RetainPtr or something so that clients don't have to adopt and so that it's harder to make a mistake. From the name it's totally unclear that this returns a +1 object.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:96
> +inline NSKeyedUnarchiver *_Nonnull secureUnarchiverFromData(NSData *_Nonnull data)

Ditto.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:99
> +    NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingFromData:data error:nil];

I think you may also want to set decodingFailurePolicy to NSDecodingFailurePolicyRaiseException (the old behavior), otherwise using the new constructor means you'll be in NSDecodingFailurePolicySetErrorAndReturn mode, and we don't check the error. But I'm not sure :P
Comment 28 Brent Fulgham 2017-10-23 17:03:46 PDT
Comment on attachment 324583 [details]
Patch

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

>> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:89
>> +inline NSKeyedArchiver *_Nonnull secureArchiverFromMutableData(NSMutableData *_Nonnull mutableData)
> 
> This should return a RetainPtr or something so that clients don't have to adopt and so that it's harder to make a mistake. From the name it's totally unclear that this returns a +1 object.

OK!

>> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:96
>> +inline NSKeyedUnarchiver *_Nonnull secureUnarchiverFromData(NSData *_Nonnull data)
> 
> Ditto.

OK!

>> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:99
>> +    NSKeyedUnarchiver *unarchiver = [[NSKeyedUnarchiver alloc] initForReadingFromData:data error:nil];
> 
> I think you may also want to set decodingFailurePolicy to NSDecodingFailurePolicyRaiseException (the old behavior), otherwise using the new constructor means you'll be in NSDecodingFailurePolicySetErrorAndReturn mode, and we don't check the error. But I'm not sure :P

OK. It makes the patch smaller, too, since we don't have to add new error handling logic.
Comment 29 Brent Fulgham 2017-10-23 17:08:50 PDT
Created attachment 324616 [details]
Patch
Comment 30 WebKit Commit Bot 2017-10-24 09:21:40 PDT
Comment on attachment 324616 [details]
Patch

Clearing flags on attachment: 324616

Committed r223889: <https://trac.webkit.org/changeset/223889>
Comment 31 WebKit Commit Bot 2017-10-24 09:21:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Matt Lewis 2017-10-24 11:14:08 PDT
Reverted r223889 for reason:

This caused multiple crashes on all platforms

Committed r223900: <https://trac.webkit.org/changeset/223900>
Comment 34 Brent Fulgham 2017-10-24 11:24:16 PDT
It looks like the Sierra bots are building High Sierra code. This might be a build system configuration issue.
Comment 35 Brent Fulgham 2017-10-24 11:39:57 PDT
There are related to the test dumping code. It looks like toggling "secure coding" on and off doesn't work properly.
Comment 36 Brent Fulgham 2017-10-24 11:43:37 PDT
Created attachment 324691 [details]
Patch for landing
Comment 37 Brent Fulgham 2017-10-24 11:45:17 PDT
Created attachment 324692 [details]
Patch for landing
Comment 38 WebKit Commit Bot 2017-10-24 12:07:22 PDT
Comment on attachment 324692 [details]
Patch for landing

Clearing flags on attachment: 324692

Committed r223908: <https://trac.webkit.org/changeset/223908>
Comment 39 WebKit Commit Bot 2017-10-24 12:07:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Ryan Haddad 2017-10-26 10:18:26 PDT
Reverted r223908 for reason:

Causes LayoutTest crashes with newer SDKs.

Committed r224023: <https://trac.webkit.org/changeset/224023>
Comment 41 Darin Adler 2017-10-28 15:46:59 PDT
Comment on attachment 324692 [details]
Patch for landing

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

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:28
> +#define USE_NEW_ARCHIVER_API ((PLATFORM(MAC) && __MAC_OS_X_VERSION_MAX_ALLOWED >= 101302 && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110200) || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 40200) || (PLATFORM(TVOS) && __TV_OS_VERSION_MIN_REQUIRED >= 110200))

Generally we try not to use the term "new" in WebKit code. It’s new now, but later it will be old or at least not new, and the code will still say "NEW". I think this could be fixed by changing this to be USE_LEGACY_ARCHIVER or something like that and reversing its definition and usage.
Comment 42 Brent Fulgham 2017-12-18 12:38:29 PST
Created attachment 329660 [details]
Patch
Comment 43 Brent Fulgham 2017-12-18 12:40:35 PST
This patch is now much smaller, as it only touches the WebCore code that uses the archiver in insecure ways.
Comment 44 Brent Fulgham 2017-12-20 13:46:07 PST
Created attachment 329950 [details]
Patch
Comment 45 Tim Horton 2017-12-20 13:51:56 PST
Comment on attachment 329950 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:91
> +    // This can be removed when <rdar://problem/31376830> is fixed.

Not sure this comment is quite right.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:93
> +        return [NSKeyedUnarchiver unarchiveObjectWithData:data];

Crazy that securelyUnarchiveObjectOfClassFromData sometimes does an insecure unarchive. Can this assert that it’s not NSAttributedString and put the branches at call sites? Or maybe not. I don’t know.
Comment 46 Brent Fulgham 2017-12-20 13:55:00 PST
(In reply to Tim Horton from comment #45)
> 
> Crazy that securelyUnarchiveObjectOfClassFromData sometimes does an insecure
> unarchive. Can this assert that it’s not NSAttributedString and put the
> branches at call sites? Or maybe not. I don’t know.

We'd have to add that check and branch in at least three call sites, so I kind of hate that idea.

I could change the name to something like, "securelyUnarchiveObjectOfClassFromDataIfPossible" or something?
Comment 47 Brent Fulgham 2017-12-20 13:56:10 PST
(In reply to Tim Horton from comment #45)
> Comment on attachment 329950 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329950&action=review
> 
> > Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:91
> > +    // This can be removed when <rdar://problem/31376830> is fixed.
> 
> Not sure this comment is quite right.

I'll rephrase that. Basically, I'm just trying to say "we can remove this ugly hack code when the fix from rdar 31376830 is deployed everywhere we target."
Comment 48 Brent Fulgham 2017-12-20 13:59:03 PST
Created attachment 329951 [details]
Patch
Comment 49 Tim Horton 2017-12-20 14:48:37 PST
How about dropping “securely” from the secure-most-of-the-time one, making it sound more default, and just make the always-insecure one have the explicitly-“insecure” name?
Comment 50 Brent Fulgham 2017-12-20 14:52:45 PST
(In reply to Tim Horton from comment #49)
> How about dropping “securely” from the secure-most-of-the-time one, making
> it sound more default, and just make the always-insecure one have the
> explicitly-“insecure” name?

Fine with me.
Comment 51 Brent Fulgham 2017-12-20 15:15:48 PST
Created attachment 329957 [details]
Patch
Comment 52 Eric Carlson 2017-12-21 08:32:47 PST
Comment on attachment 329957 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Switch to new NSKeyed[Un]Archiver methods that active and use
> +        NSSecureCoding by default.

Nit: I don't understand what "... methods that *active* and use ..." means.

> Source/WebCore/PAL/ChangeLog:15
> +        Rename 'insecurelyUnarchiveObjectOfClassFromData' to match NSKeyedArchiver naming
> +        (i.e., 'UnarchivedData' rather than 'UnarchiveData'), and move it earlier in the
> +        file so it can be reused by 'unarchiveObjectOfClassFromData'.

Nit: this wording made me think you had renamed securelyUnarchiveObjectOfClassFromData, not change from securelyUnarchiveObjectOfClassFromData to insecurelyUnarchiveObjectOfClassFromData.

Also, I don't understand the new name because the NSKeyedUnarchiver selector is "unarchiveObjectWithData", not "unarchivedObjectWithData" (no "d").
Comment 53 Brent Fulgham 2017-12-21 08:44:56 PST
Comment on attachment 329957 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        NSSecureCoding by default.
> 
> Nit: I don't understand what "... methods that *active* and use ..." means.

Wow! English fail. It should just say "Switch to new NSKeyed[Un]Archiver methods that use NSSecureCoding by default.

>> Source/WebCore/PAL/ChangeLog:15
>> +        file so it can be reused by 'unarchiveObjectOfClassFromData'.
> 
> Nit: this wording made me think you had renamed securelyUnarchiveObjectOfClassFromData, not change from securelyUnarchiveObjectOfClassFromData to insecurelyUnarchiveObjectOfClassFromData.
> 
> Also, I don't understand the new name because the NSKeyedUnarchiver selector is "unarchiveObjectWithData", not "unarchivedObjectWithData" (no "d").

Ugh. You are right. The new API says "unarchivedObjectOfClass:...", while the old API was "unarchiveObjectWIthData".

I'll change the name to match the old API, and since we don't give it a class name I'll remove that from the method name: "insecurelyUnarchiveObjectFromData"

I'll clarify in the ChangeLog.
Comment 54 Brent Fulgham 2017-12-21 09:18:33 PST
Committed r226224: <https://trac.webkit.org/changeset/226224>
Comment 55 Brent Fulgham 2017-12-21 15:58:57 PST
Follow-up fix landed in r226248.
Committed r226248: <https://trac.webkit.org/changeset/226248>