214952 2020-07-29 17:39:20 -0700 Strip pointers instead of authing for byteOffset to not allow for a possible way to guess data pac 2020-08-01 14:47:01 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified REOPENED InRadar P2 Normal --- 215065 1 saam saam commit-queue darin ews-watchlist fpizlo keith_miller mark.lam msaboff tzagallo webkit-bug-importer ysuzuki oldest_to_newest 1676256 0 saam 2020-07-29 17:39:20 -0700 The byteOffset operation doesn't load, and just returns an int. It's fine to not auth in this scenario. 1676261 1 405534 saam 2020-07-29 17:49:56 -0700 Created attachment 405534 patch 1676267 2 405534 darin 2020-07-29 18:00:23 -0700 Comment on attachment 405534 patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review > Source/JavaScriptCore/ChangeLog:15 > + Since byteOffset does no loads/stores, it suffices to just strip the PAC > + bits before doing the subtraction. This eliminates any such attacks like > + the above because the PAC bits are ignored. Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? 1676277 3 saam 2020-07-29 18:26:42 -0700 (In reply to Darin Adler from comment #2) > Comment on attachment 405534 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405534&action=review > > > Source/JavaScriptCore/ChangeLog:15 > > + Since byteOffset does no loads/stores, it suffices to just strip the PAC > > + bits before doing the subtraction. This eliminates any such attacks like > > + the above because the PAC bits are ignored. > > Just curious: Why is stripping needed? Won’t both pointers have the same PAC > bits? One of the pointers (the base) has no PAC bits. 1676278 4 405534 saam 2020-07-29 18:27:25 -0700 Comment on attachment 405534 patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review >>> Source/JavaScriptCore/ChangeLog:15 >>> + the above because the PAC bits are ignored. >> >> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? > > One of the pointers (the base) has no PAC bits. This isn't true. Ignore me. 1676281 5 405534 saam 2020-07-29 18:28:34 -0700 Comment on attachment 405534 patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review >>>> Source/JavaScriptCore/ChangeLog:15 >>>> + the above because the PAC bits are ignored. >>> >>> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? >> >> One of the pointers (the base) has no PAC bits. > > This isn't true. Ignore me. The real reason is this: The base may be a vector of length M. Signed using M. The view into that vector has length N, signed using N. N has to be <= M. Therefore, they might have the same bits, when M == N, but there is no guarantee they do. 1676284 6 405534 darin 2020-07-29 18:34:38 -0700 Comment on attachment 405534 patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review I can almost review, but not quite. The code looks right to me, but I don’t know enough about conventions in this code to be a helpful reviewer. >>>>> Source/JavaScriptCore/ChangeLog:15 >>>>> + the above because the PAC bits are ignored. >>>> >>>> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? >>> >>> One of the pointers (the base) has no PAC bits. >> >> This isn't true. Ignore me. > > The real reason is this: > > The base may be a vector of length M. Signed using M. The view into that vector has length N, signed using N. N has to be <= M. Therefore, they might have the same bits, when M == N, but there is no guarantee they do. In some cases like this I have seen people do arithmetic and then strip the result, to do half a much stripping. But I have no idea if this is relevant, acceptable, and valuable enough to be worthwhile here. 1676286 7 saam 2020-07-29 18:38:44 -0700 (In reply to Darin Adler from comment #6) > Comment on attachment 405534 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405534&action=review > > I can almost review, but not quite. The code looks right to me, but I don’t > know enough about conventions in this code to be a helpful reviewer. > > >>>>> Source/JavaScriptCore/ChangeLog:15 > >>>>> + the above because the PAC bits are ignored. > >>>> > >>>> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? > >>> > >>> One of the pointers (the base) has no PAC bits. > >> > >> This isn't true. Ignore me. > > > > The real reason is this: > > > > The base may be a vector of length M. Signed using M. The view into that vector has length N, signed using N. N has to be <= M. Therefore, they might have the same bits, when M == N, but there is no guarantee they do. > > In some cases like this I have seen people do arithmetic and then strip the > result, to do half a much stripping. But I have no idea if this is relevant, > acceptable, and valuable enough to be worthwhile here. Let me look it up in some CPU docs. My assumption in writing this code is that stripping is 1-cycle (since it's essentially a masking operation). Even if it's not, to do the arithmetic, I think we'd need to do an extra load to get to the number that we need to do arithmetic on. 1676289 8 405534 keith_miller 2020-07-29 19:22:52 -0700 Comment on attachment 405534 patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review r=me. >>>>>>> Source/JavaScriptCore/ChangeLog:15 >>>>>>> + the above because the PAC bits are ignored. >>>>>> >>>>>> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? >>>>> >>>>> One of the pointers (the base) has no PAC bits. >>>> >>>> This isn't true. Ignore me. >>> >>> The real reason is this: >>> >>> The base may be a vector of length M. Signed using M. The view into that vector has length N, signed using N. N has to be <= M. Therefore, they might have the same bits, when M == N, but there is no guarantee they do. >> >> In some cases like this I have seen people do arithmetic and then strip the result, to do half a much stripping. But I have no idea if this is relevant, acceptable, and valuable enough to be worthwhile here. > > Let me look it up in some CPU docs. My assumption in writing this code is that stripping is 1-cycle (since it's essentially a masking operation). Even if it's not, to do the arithmetic, I think we'd need to do an extra load to get to the number that we need to do arithmetic on. From conversations with CPU folks, my understanding is that stripping is "free". And FWIW, at least on Apple Silicon ™️, it's also "free" to do an auth as long as the result of that auth doesn't flow (either directly or via a load/store) into a branch. 1676569 9 ews-feeder 2020-07-30 14:44:50 -0700 Committed r265097: <https://trac.webkit.org/changeset/265097> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405534. 1676570 10 webkit-bug-importer 2020-07-30 14:45:19 -0700 <rdar://problem/66348653> 1677154 11 commit-queue 2020-08-01 14:47:01 -0700 Re-opened since this is blocked by bug 215065 405534 2020-07-29 17:49:56 -0700 2020-07-30 14:44:51 -0700 patch b-backup.diff text/plain 5378 saam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjY1MDcxKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBA CisyMDIwLTA3LTI5ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IFN0cmlwIHBvaW50ZXJzIGluc3RlYWQgb2YgYXV0aGluZyBmb3IgYnl0ZU9mZnNldCB0byBub3Qg YWxsb3cgZm9yIGEgcG9zc2libGUgd2F5IHRvIGd1ZXNzIGRhdGEgcGFjCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMTQ5NTIKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbiB0aGUgb2xkIHdheSBvZiBkb2lu ZyB0aGluZ3MsIHdlIHdvdWxkIGF1dGggdGhlIHZlY3RvciBwb2ludGVyIGJlZm9yZSBzdWJ0cmFj dGluZworICAgICAgICB0aGUgYmFzZSBmcm9tIGl0LiBTaW5jZSB3ZSBuZXZlciB2YWxpZGF0ZWQg dGhlIGF1dGgsIHRoaXMgYWxsb3dlZCBmb3IgYQorICAgICAgICBwb3RlbnRpYWwgZGF0YS1QQUMg YnlwYXNzIGJ5IGp1c3QgcmVwZWF0ZWRseSBjYWxsaW5nIGJ5dGVPZmZzZXQgaW4gYSBsb29wCisg ICAgICAgIGFuZCBvYnNlcnZpbmcgdGhlIGludGVnZXIgcmVzdWx0IG9mIHRoZSBvcGVyYXRpb24u CisgICAgICAgIAorICAgICAgICBTaW5jZSBieXRlT2Zmc2V0IGRvZXMgbm8gbG9hZHMvc3RvcmVz LCBpdCBzdWZmaWNlcyB0byBqdXN0IHN0cmlwIHRoZSBQQUMKKyAgICAgICAgYml0cyBiZWZvcmUg ZG9pbmcgdGhlIHN1YnRyYWN0aW9uLiBUaGlzIGVsaW1pbmF0ZXMgYW55IHN1Y2ggYXR0YWNrcyBs aWtlCisgICAgICAgIHRoZSBhYm92ZSBiZWNhdXNlIHRoZSBQQUMgYml0cyBhcmUgaWdub3JlZC4K KworICAgICAgICAqIGRmZy9ERkdTcGVjdWxhdGl2ZUpJVC5jcHA6CisgICAgICAgIChKU0M6OkRG Rzo6U3BlY3VsYXRpdmVKSVQ6OmNvbXBpbGVHZXRUeXBlZEFycmF5Qnl0ZU9mZnNldCk6CisgICAg ICAgICogZnRsL0ZUTExvd2VyREZHVG9CMy5jcHA6CisgICAgICAgIChKU0M6OkZUTDo6REZHOjpM b3dlckRGR1RvQjM6OmNvbXBpbGVHZXRUeXBlZEFycmF5Qnl0ZU9mZnNldCk6CisKIDIwMjAtMDct MjkgIFRhZGV1IFphZ2FsbG8gIDx0emFnYWxsb0BhcHBsZS5jb20+CiAKICAgICAgICAgV2ViQXNz ZW1ibHkgdmFsaWRhdGlvbiBmb3IgY2FsbF9pbmRpcmVjdCBpcyBpbmNvcnJlY3QKSW5kZXg6IFNv dXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHU3BlY3VsYXRpdmVKSVQuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHU3BlY3VsYXRpdmVKSVQuY3BwCShyZXZp c2lvbiAyNjUwNDkpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1NwZWN1bGF0aXZl SklULmNwcAkod29ya2luZyBjb3B5KQpAQCAtNzMyNiwxNyArNzMyNiwxMyBAQCB2b2lkIFNwZWN1 bGF0aXZlSklUOjpjb21waWxlR2V0VHlwZWRBcnJhCiAgICAgICAgIFRydXN0ZWRJbW0zMihXYXN0 ZWZ1bFR5cGVkQXJyYXkpKTsKIAogICAgIG1faml0LmxvYWRQdHIoTWFjcm9Bc3NlbWJsZXI6OkFk ZHJlc3MoYmFzZUdQUiwgSlNBcnJheUJ1ZmZlclZpZXc6Om9mZnNldE9mVmVjdG9yKCkpLCB2ZWN0 b3JHUFIpOwotCi0gICAgSklUQ29tcGlsZXI6Okp1bXAgbnVsbFZlY3RvciA9IG1faml0LmJyYW5j aFB0cihKSVRDb21waWxlcjo6RXF1YWwsIHZlY3RvckdQUiwgVHJ1c3RlZEltbVB0cihKU0FycmF5 QnVmZmVyVmlldzo6bnVsbFZlY3RvclB0cigpKSk7CisjaWYgQ1BVKEFSTTY0RSkKKyAgICBtX2pp dC5yZW1vdmVBcnJheVB0clRhZyh2ZWN0b3JHUFIpOworI2VuZGlmCiAKICAgICBtX2ppdC5sb2Fk UHRyKE1hY3JvQXNzZW1ibGVyOjpBZGRyZXNzKGJhc2VHUFIsIEpTT2JqZWN0OjpidXR0ZXJmbHlP ZmZzZXQoKSksIGRhdGFHUFIpOwogICAgIG1faml0LmNhZ2VXaXRob3V0VW50YWdnaW5nKEdpZ2Fj YWdlOjpKU1ZhbHVlLCBkYXRhR1BSKTsKLQotICAgIGNhZ2VUeXBlZEFycmF5U3RvcmFnZShiYXNl R1BSLCB2ZWN0b3JHUFIpOwotCiAgICAgbV9qaXQubG9hZFB0cihNYWNyb0Fzc2VtYmxlcjo6QWRk cmVzcyhkYXRhR1BSLCBCdXR0ZXJmbHk6Om9mZnNldE9mQXJyYXlCdWZmZXIoKSksIGFycmF5QnVm ZmVyR1BSKTsKLSAgICAvLyBGSVhNRTogVGhpcyBuZWVkcyBjYWdpbmcuCi0gICAgLy8gaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3NTUxNQogICAgIG1faml0LmxvYWRQ dHIoTWFjcm9Bc3NlbWJsZXI6OkFkZHJlc3MoYXJyYXlCdWZmZXJHUFIsIEFycmF5QnVmZmVyOjpv ZmZzZXRPZkRhdGEoKSksIGRhdGFHUFIpOwogI2lmIENQVShBUk02NEUpCiAgICAgbV9qaXQucmVt b3ZlQXJyYXlQdHJUYWcoZGF0YUdQUik7CkBAIC03MzQ2LDE3ICs3MzQyLDEwIEBAIHZvaWQgU3Bl Y3VsYXRpdmVKSVQ6OmNvbXBpbGVHZXRUeXBlZEFycmEKICAgICAKICAgICBKSVRDb21waWxlcjo6 SnVtcCBkb25lID0gbV9qaXQuanVtcCgpOwogICAgIAotI2lmIENQVShBUk02NEUpCi0gICAgbnVs bFZlY3Rvci5saW5rKCZtX2ppdCk7Ci0jZW5kaWYKICAgICBlbXB0eUJ5dGVPZmZzZXQubGluaygm bV9qaXQpOwogICAgIG1faml0Lm1vdmUoVHJ1c3RlZEltbVB0cihudWxscHRyKSwgdmVjdG9yR1BS KTsKICAgICAKICAgICBkb25lLmxpbmsoJm1faml0KTsKLSNpZiAhQ1BVKEFSTTY0RSkKLSAgICBB U1NFUlQoIUpTQXJyYXlCdWZmZXJWaWV3OjpudWxsVmVjdG9yUHRyKCkpOwotICAgIG51bGxWZWN0 b3IubGluaygmbV9qaXQpOwotI2VuZGlmCiAKICAgICBzdHJpY3RJbnQzMlJlc3VsdCh2ZWN0b3JH UFIsIG5vZGUpOwogfQpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2Z0bC9GVExMb3dlckRG R1RvQjMuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9mdGwvRlRMTG93 ZXJERkdUb0IzLmNwcAkocmV2aXNpb24gMjY1MDQ5KQorKysgU291cmNlL0phdmFTY3JpcHRDb3Jl L2Z0bC9GVExMb3dlckRGR1RvQjMuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00MzYxLDMxICs0MzYx LDIzIEBAIHByaXZhdGU6CiAgICAgICAgIExWYWx1ZSBiYXNlUHRyID0gbG93Q2VsbChtX25vZGUt PmNoaWxkMSgpKTsgICAgCiAKICAgICAgICAgTEJhc2ljQmxvY2sgd2FzdGVmdWxDYXNlID0gbV9v dXQubmV3QmxvY2soKTsKLSAgICAgICAgTEJhc2ljQmxvY2sgbm90TnVsbCA9IG1fb3V0Lm5ld0Js b2NrKCk7CiAgICAgICAgIExCYXNpY0Jsb2NrIGNvbnRpbnVhdGlvbiA9IG1fb3V0Lm5ld0Jsb2Nr KCk7CiAgICAgICAgIAotICAgICAgICBWYWx1ZUZyb21CbG9jayBudWxsVmVjdG9yT3V0ID0gbV9v dXQuYW5jaG9yKG1fb3V0LmNvbnN0SW50UHRyKDApKTsKKyAgICAgICAgVmFsdWVGcm9tQmxvY2sg bm9uV2FzdGVmdWxSZXN1bHQgPSBtX291dC5hbmNob3IobV9vdXQuY29uc3RJbnRQdHIoMCkpOwog CiAgICAgICAgIExWYWx1ZSBtb2RlID0gbV9vdXQubG9hZDMyKGJhc2VQdHIsIG1faGVhcHMuSlNB cnJheUJ1ZmZlclZpZXdfbW9kZSk7CiAgICAgICAgIG1fb3V0LmJyYW5jaCgKICAgICAgICAgICAg IG1fb3V0Lm5vdEVxdWFsKG1vZGUsIG1fb3V0LmNvbnN0SW50MzIoV2FzdGVmdWxUeXBlZEFycmF5 KSksCiAgICAgICAgICAgICB1bnN1cmUoY29udGludWF0aW9uKSwgdW5zdXJlKHdhc3RlZnVsQ2Fz ZSkpOwogCi0gICAgICAgIExCYXNpY0Jsb2NrIGxhc3ROZXh0ID0gbV9vdXQuYXBwZW5kVG8od2Fz dGVmdWxDYXNlLCBub3ROdWxsKTsKKyAgICAgICAgTEJhc2ljQmxvY2sgbGFzdE5leHQgPSBtX291 dC5hcHBlbmRUbyh3YXN0ZWZ1bENhc2UsIGNvbnRpbnVhdGlvbik7CiAKLSAgICAgICAgTFZhbHVl IHZlY3RvciA9IG1fb3V0LmxvYWRQdHIoYmFzZVB0ciwgbV9oZWFwcy5KU0FycmF5QnVmZmVyVmll d192ZWN0b3IpOwotICAgICAgICBtX291dC5icmFuY2gobV9vdXQuZXF1YWwodmVjdG9yLCBtX291 dC5jb25zdEludFB0cihKU0FycmF5QnVmZmVyVmlldzo6bnVsbFZlY3RvclB0cigpKSksIAotICAg ICAgICAgICAgdW5zdXJlKGNvbnRpbnVhdGlvbiksIHVuc3VyZShub3ROdWxsKSk7Ci0KLSAgICAg ICAgbV9vdXQuYXBwZW5kVG8obm90TnVsbCwgY29udGludWF0aW9uKTsKKyAgICAgICAgTFZhbHVl IHZlY3RvclB0ciA9IG1fb3V0LmxvYWRQdHIoYmFzZVB0ciwgbV9oZWFwcy5KU0FycmF5QnVmZmVy Vmlld192ZWN0b3IpOworICAgICAgICB2ZWN0b3JQdHIgPSByZW1vdmVBcnJheVB0clRhZyh2ZWN0 b3JQdHIpOwogCiAgICAgICAgIExWYWx1ZSBidXR0ZXJmbHlQdHIgPSBjYWdlZChHaWdhY2FnZTo6 SlNWYWx1ZSwgbV9vdXQubG9hZFB0cihiYXNlUHRyLCBtX2hlYXBzLkpTT2JqZWN0X2J1dHRlcmZs eSksIGJhc2VQdHIpOwogICAgICAgICBMVmFsdWUgYXJyYXlCdWZmZXJQdHIgPSBtX291dC5sb2Fk UHRyKGJ1dHRlcmZseVB0ciwgbV9oZWFwcy5CdXR0ZXJmbHlfYXJyYXlCdWZmZXIpOwogCi0gICAg ICAgIExWYWx1ZSB2ZWN0b3JQdHIgPSBjYWdlZChHaWdhY2FnZTo6UHJpbWl0aXZlLCB2ZWN0b3Is IGJhc2VQdHIpOwotCi0gICAgICAgIC8vIEZJWE1FOiBUaGlzIG5lZWRzIGNhZ2luZy4KLSAgICAg ICAgLy8gaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3NTUxNQogICAg ICAgICBMVmFsdWUgZGF0YVB0ciA9IG1fb3V0LmxvYWRQdHIoYXJyYXlCdWZmZXJQdHIsIG1faGVh cHMuQXJyYXlCdWZmZXJfZGF0YSk7CiAgICAgICAgIGRhdGFQdHIgPSByZW1vdmVBcnJheVB0clRh ZyhkYXRhUHRyKTsKIApAQCAtNDM5NCw3ICs0Mzg2LDcgQEAgcHJpdmF0ZToKICAgICAgICAgbV9v dXQuanVtcChjb250aW51YXRpb24pOwogICAgICAgICBtX291dC5hcHBlbmRUbyhjb250aW51YXRp b24sIGxhc3ROZXh0KTsKIAotICAgICAgICBzZXRJbnQzMihtX291dC5jYXN0VG9JbnQzMihtX291 dC5waGkocG9pbnRlclR5cGUoKSwgbnVsbFZlY3Rvck91dCwgd2FzdGVmdWxPdXQpKSk7CisgICAg ICAgIHNldEludDMyKG1fb3V0LmNhc3RUb0ludDMyKG1fb3V0LnBoaShwb2ludGVyVHlwZSgpLCBu b25XYXN0ZWZ1bFJlc3VsdCwgd2FzdGVmdWxPdXQpKSk7CiAgICAgfQogCiAgICAgdm9pZCBjb21w aWxlR2V0UHJvdG90eXBlT2YoKQo=