Bug 233011 - [WebAuthn] Stop serializing BufferSource and Vector<uint8_t> duplicates of identifiers
Summary: [WebAuthn] Stop serializing BufferSource and Vector<uint8_t> duplicates of id...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-11 13:42 PST by Brent Fulgham
Modified: 2021-11-11 17:42 PST (History)
6 users (show)

See Also:


Attachments
Patch (63.89 KB, patch)
2021-11-11 14:37 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (64.71 KB, patch)
2021-11-11 16:43 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2021-11-11 13:42:10 PST
The original WebAuthn logic converted WebCore::BufferSource objects to Vector<uint8_t> for serialization, which created a weird design where some code dealt with BufferSource objects, and other with Vectors, and lots of converting to and from these types. It also caused WebAuthn data structures to have two places where this information might live, with the UIProcess using one representation, and the WebContent process using another.

This patch revises the code as follows:

1. The identifiers are always stored as BufferSource, and the same member is used in UIProcess and WebContent process when accessing this information.
2. We now serialize BufferSource directly.
Comment 1 Radar WebKit Bug Importer 2021-11-11 13:43:43 PST
<rdar://problem/85313807>
Comment 2 Brent Fulgham 2021-11-11 14:37:16 PST
Created attachment 444005 [details]
Patch
Comment 3 Chris Dumez 2021-11-11 15:01:54 PST
Comment on attachment 444005 [details]
Patch

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

r=me, very nice!

> Source/WebCore/Modules/webauthn/fido/U2fCommandConstructor.cpp:88
> +    return constructU2fSignCommand(applicationParameter, challengeParameter, keyVector, checkOnly);

Doesn't need to be done in this patch but constructU2fSignCommand() should probably take in a WTF::Span<const uint8_t> instead of a Vector<uint8_t>.

> Source/WebCore/bindings/js/BufferSource.h:99
> +    auto dataSize = CheckedSize { *size } * sizeof(uint8_t);

nit: The `* sizeof(uint8_t)` doesn't look terribly useful :)

> Source/WebCore/bindings/js/BufferSource.h:106
> +    return BufferSource(JSC::ArrayBuffer::tryCreate(reinterpret_cast<const void*>(data), dataSize.value()));

nit: Would a static_cast<const void*> work?

> Source/WebCore/bindings/js/BufferSource.h:117
> +    return BufferSource(JSC::ArrayBuffer::tryCreate(reinterpret_cast<const uint8_t*>(data.bytes), data.length));

static_cast<> should suffice since bytes returns a const void*

> Source/WebCore/bindings/js/BufferSource.h:128
> +using WebCore::toBufferSource;

I am not opposed to it but I didn't realize we were doing this outside of WTF.
Comment 8 Brent Fulgham 2021-11-11 16:43:19 PST
Created attachment 444024 [details]
Patch for landing
Comment 9 EWS 2021-11-11 17:42:49 PST
Committed r285698 (244165@main): <https://commits.webkit.org/244165@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444024 [details].