Bug 178484 – Adopt new secure coding APIs in WebCore

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

Switch away from older NSKeyedArchiver/NSKeyedUnarchive methods that supported non-NSSecureCoding serialization.

[Brent Fulgham]

<rdar://problem/34837193>

[Brent Fulgham]

Created attachment 324182 [details]
Patch

[Brent Fulgham]

Created attachment 324238 [details]
Patch

[Brent Fulgham]

Created attachment 324245 [details]
Patch

[Brent Fulgham]

Created attachment 324258 [details]
Patch

[Brent Fulgham]

Created attachment 324273 [details]
Patch

[Brent Fulgham]

Created attachment 324322 [details]
Patch

[Brent Fulgham]

Created attachment 324337 [details]
Patch

[Brent Fulgham]

Created attachment 324344 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Ryosuke Niwa]

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.

[Brent Fulgham]

Created attachment 324400 [details]
Patch

[Brent Fulgham]

(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.

[Build Bot]

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

[Build Bot]

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

[Tim Horton]

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?

[Brent Fulgham]

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.

[Brent Fulgham]

(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.

[Brent Fulgham]

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).

[Brent Fulgham]

Created attachment 324569 [details]
Patch

[Brent Fulgham]

Created attachment 324583 [details]
Patch

[Tim Horton]

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

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 324616 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 324616 [details]
Patch

Clearing flags on attachment: 324616

Committed r223889: <https://trac.webkit.org/changeset/223889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

The patch caused multiple crashes on all platforms:

Example build and results:
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r223889%20(5218)/http/tests/webarchive/cross-origin-stylesheet-crash-crash-log.txt
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r223889%20(5218)/results.html

example history:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fwebarchive%2Fcross-origin-stylesheet-crash.html

[Matt Lewis]

Reverted r223889 for reason:

This caused multiple crashes on all platforms

Committed r223900: <https://trac.webkit.org/changeset/223900>

[Brent Fulgham]

It looks like the Sierra bots are building High Sierra code. This might be a build system configuration issue.

[Brent Fulgham]

There are related to the test dumping code. It looks like toggling "secure coding" on and off doesn't work properly.

[Brent Fulgham]

Created attachment 324691 [details]
Patch for landing

[Brent Fulgham]

Created attachment 324692 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 324692 [details]
Patch for landing

Clearing flags on attachment: 324692

Committed r223908: <https://trac.webkit.org/changeset/223908>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r223908 for reason:

Causes LayoutTest crashes with newer SDKs.

Committed r224023: <https://trac.webkit.org/changeset/224023>

[Darin Adler]

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.

[Brent Fulgham]

Created attachment 329660 [details]
Patch

[Brent Fulgham]

This patch is now much smaller, as it only touches the WebCore code that uses the archiver in insecure ways.

[Brent Fulgham]

Created attachment 329950 [details]
Patch

[Tim Horton]

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.

[Brent Fulgham]

(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?

[Brent Fulgham]

(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."

[Brent Fulgham]

Created attachment 329951 [details]
Patch

[Tim Horton]

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?

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 329957 [details]
Patch

[Eric Carlson]

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").

[Brent Fulgham]

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.

[Brent Fulgham]

Committed r226224: <https://trac.webkit.org/changeset/226224>

[Brent Fulgham]

Follow-up fix landed in r226248.
Committed r226248: <https://trac.webkit.org/changeset/226248>