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
Brent Fulgham
2017-10-18 13:36:14 PDT
Created attachment 324182 [details]
Patch
Created attachment 324238 [details]
Patch
Created attachment 324245 [details]
Patch
Created attachment 324258 [details]
Patch
Created attachment 324273 [details]
Patch
Created attachment 324322 [details]
Patch
Created attachment 324337 [details]
Patch
Created attachment 324344 [details]
Patch
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 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 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 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 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 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 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. Created attachment 324400 [details]
Patch
(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 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 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 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 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. (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. 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). Created attachment 324569 [details]
Patch
Created attachment 324583 [details]
Patch
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 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. Created attachment 324616 [details]
Patch
Comment on attachment 324616 [details] Patch Clearing flags on attachment: 324616 Committed r223889: <https://trac.webkit.org/changeset/223889> All reviewed patches have been landed. Closing bug. 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 Reverted r223889 for reason: This caused multiple crashes on all platforms Committed r223900: <https://trac.webkit.org/changeset/223900> It looks like the Sierra bots are building High Sierra code. This might be a build system configuration issue. There are related to the test dumping code. It looks like toggling "secure coding" on and off doesn't work properly. Created attachment 324691 [details]
Patch for landing
Created attachment 324692 [details]
Patch for landing
Comment on attachment 324692 [details] Patch for landing Clearing flags on attachment: 324692 Committed r223908: <https://trac.webkit.org/changeset/223908> All reviewed patches have been landed. Closing bug. Reverted r223908 for reason: Causes LayoutTest crashes with newer SDKs. Committed r224023: <https://trac.webkit.org/changeset/224023> 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. Created attachment 329660 [details]
Patch
This patch is now much smaller, as it only touches the WebCore code that uses the archiver in insecure ways. Created attachment 329950 [details]
Patch
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. (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? (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." Created attachment 329951 [details]
Patch
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? (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. Created attachment 329957 [details]
Patch
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 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. Committed r226224: <https://trac.webkit.org/changeset/226224> Follow-up fix landed in r226248. Committed r226248: <https://trac.webkit.org/changeset/226248> |