Bug 238966

<rdar://problem/91449906>

Created attachment 457020 [details] Patch

Created attachment 457257 [details] Patch

Created attachment 457271 [details] Patch

Comment on attachment 457271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457271&action=review > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:65 > + return m_transports; Should probably be inline in the header. > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.h:43 > + Vector<AuthenticatorTransport> getTransports() const; should return a const Vector<AuthenticatorTransport>& > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.idl:33 > + sequence<AuthenticatorTransport> getTransports(); We don't usually do alignment like this in modern WebKit code. > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.idl:34 > + ArrayBuffer getAuthenticatorData(); ditto. > Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:151 > + result.transports = *transports; WTFMove(*transports) > Source/WebCore/Modules/webauthn/WebAuthenticationConstants.h:96 > +constexpr const char authenticatorTransportUsb[] = "usb"; These should likely be defined as ASCIILiteral, not const char*. > Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.cpp:89 > + for (auto& transport : transports) { Seems like we want a function that returns a String from a AuthenticatorTransport. Then we wouldn't need this whole function to convert the vector, you would just use transports.map(convertAuthenticatorTransportToString). Vector::map() would also be more efficient than your implementation. > Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.cpp:92 > + stringTransports.append(String::fromLatin1(WebCore::authenticatorTransportUsb)); Then here you wouldn't need the String::fromLatin1(). String construction would also be more efficient as we wouldn't need to copy the chars. > Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h:64 > + const std::optional<Vector<WebCore::AuthenticatorTransport>> transports() const { return m_transports; } should return a `const std::optional<Vector<WebCore::AuthenticatorTransport>>&`, not a `const std::optional<Vector<WebCore::AuthenticatorTransport>>` > Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:345 > + Vector<WebCore::AuthenticatorTransport> transports; Should call reserveInitialCapacity() > Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:347 > + transports.append(static_cast<WebCore::AuthenticatorTransport>(ascTransport.intValue)); Should call uncheckedAppend() once you reserve capacity. > Source/WebKit/UIProcess/WebAuthentication/fido/CtapDriver.h:56 > + CtapDriver(WebCore::AuthenticatorTransport transport) { m_transport = transport; } Please use the initialization list, not an assignment in the constructor body.

Created attachment 457282 [details] Patch

Comment on attachment 457282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457282&action=review I think this looks good, but please correct the ArrayBuffer handling. At present, I think you will end up returning deallocated memory as the ArrayBuffer will be +0 reference count as currently written. > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:63 > +ArrayBuffer* AuthenticatorAttestationResponse::getAuthenticatorData() const This should be RefPtr<ArrayBuffer> ... > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:68 > + return nullptr; You could do something fancy for JavaScript: if (!decodedResponse || !decodedResponse->isMap()) return Exception { SyntaxError, "CBOR parsing failed"_s }; > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:74 > + return nullptr; Ditto: if (it == attObjMap.end() || !it->second.isByteString()) return Exception { SyntaxError, "Attestation missing authData <<OR SOMETHING>>"_s {; > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:75 > + } Is there any benefit to caching the converted bytes so we don't have to do this work repeatedly? Or does the m_attestationObject change too much for that to be worth doing? > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:77 > + return ArrayBuffer::create(authData.data(), authData.size()).ptr(); Why don't we return the Ref<> instead of this bare pointer? Seems likely to be the source of memory issues. Instead, I think you should do: return ArrayBuffer::tryCreate(authData.data(), authData.size()); Better yet, change the signature to ExceptionOr<RefPtr<JSC::ArrayBuffer>>, too. > Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.h:44 > + ArrayBuffer* getAuthenticatorData() const; I think this should be something like: ExceptionOr<RefPtr<JSC::ArrayBuffer>> getAuthenticatorData(); Or just: RefPtr<JSC::ArrayBuffer> getAuthenticationData();

Comment on attachment 457282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457282&action=review >> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:77 >> + return ArrayBuffer::create(authData.data(), authData.size()).ptr(); > > Why don't we return the Ref<> instead of this bare pointer? Seems likely to be the source of memory issues. > > Instead, I think you should do: > > return ArrayBuffer::tryCreate(authData.data(), authData.size()); > > Better yet, change the signature to ExceptionOr<RefPtr<JSC::ArrayBuffer>>, too. I agree about returning a RefPtr<<JSC::ArrayBuffer>>, the current code is definitely unsafe. However, I don't understand why we would want to return a `ExceptionOr<RefPtr<JSC::ArrayBuffer>>` as Brent suggested, given that this function never returns any exception. This will result in an extra branch in JS bindings code for no reason.

Comment on attachment 457282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457282&action=review >>> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:77 >>> + return ArrayBuffer::create(authData.data(), authData.size()).ptr(); >> >> Why don't we return the Ref<> instead of this bare pointer? Seems likely to be the source of memory issues. >> >> Instead, I think you should do: >> >> return ArrayBuffer::tryCreate(authData.data(), authData.size()); >> >> Better yet, change the signature to ExceptionOr<RefPtr<JSC::ArrayBuffer>>, too. > > I agree about returning a RefPtr<<JSC::ArrayBuffer>>, the current code is definitely unsafe. > However, I don't understand why we would want to return a `ExceptionOr<RefPtr<JSC::ArrayBuffer>>` as Brent suggested, given that this function never returns any exception. This will result in an extra branch in JS bindings code for no reason. Doh! I missed that Brent suggested above to return an Exception in some cases. Please disregard my comment then :)

(In reply to Chris Dumez from comment #8) > Comment on attachment 457282 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457282&action=review > > >> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:77 > >> + return ArrayBuffer::create(authData.data(), authData.size()).ptr(); > > > > Why don't we return the Ref<> instead of this bare pointer? Seems likely to be the source of memory issues. > > > > Instead, I think you should do: > > > > return ArrayBuffer::tryCreate(authData.data(), authData.size()); > > > > Better yet, change the signature to ExceptionOr<RefPtr<JSC::ArrayBuffer>>, too. > > I agree about returning a RefPtr<<JSC::ArrayBuffer>>, the current code is > definitely unsafe. > However, I don't understand why we would want to return a > `ExceptionOr<RefPtr<JSC::ArrayBuffer>>` as Brent suggested, given that this > function never returns any exception. This will result in an extra branch in > JS bindings code for no reason. Pascoe mentioned this was primarily a convenience function for websites, so I thought that returning an Exception in the two failure cases (above) would be helpful to the developer. For example, missing "authData" might be something they could fix or report to the user in some useful way.

(In reply to Brent Fulgham from comment #10) > (In reply to Chris Dumez from comment #8) > > Comment on attachment 457282 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=457282&action=review > > > > >> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:77 > > >> + return ArrayBuffer::create(authData.data(), authData.size()).ptr(); > > > > > > Why don't we return the Ref<> instead of this bare pointer? Seems likely to be the source of memory issues. > > > > > > Instead, I think you should do: > > > > > > return ArrayBuffer::tryCreate(authData.data(), authData.size()); > > > > > > Better yet, change the signature to ExceptionOr<RefPtr<JSC::ArrayBuffer>>, too. > > > > I agree about returning a RefPtr<<JSC::ArrayBuffer>>, the current code is > > definitely unsafe. > > However, I don't understand why we would want to return a > > `ExceptionOr<RefPtr<JSC::ArrayBuffer>>` as Brent suggested, given that this > > function never returns any exception. This will result in an extra branch in > > JS bindings code for no reason. > > Pascoe mentioned this was primarily a convenience function for websites, so > I thought that returning an Exception in the two failure cases (above) would > be helpful to the developer. > > For example, missing "authData" might be something they could fix or report > to the user in some useful way. Yes, I had missed that you suggested raising an exception in some cases. Note that it really depends if these cases are supposed to be possible or not. Given that there was an ASSERT_NOT_REACHED(), I assumed we don't expect this to happen in practice so presumably, whatever exception we generate would not be observable (unless the assertion is wrong). I don't really mind adding exception throwing code though here if we're unsure this can happen in practice or not.

Comment on attachment 457282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457282&action=review >>>>>> Source/WebCore/Modules/webauthn/AuthenticatorAttestationResponse.cpp:77 >>>>>> + return ArrayBuffer::create(authData.data(), authData.size()).ptr(); >>>>> >>>>> Why don't we return the Ref<> instead of this bare pointer? Seems likely to be the source of memory issues. >>>>> >>>>> Instead, I think you should do: >>>>> >>>>> return ArrayBuffer::tryCreate(authData.data(), authData.size()); >>>>> >>>>> Better yet, change the signature to ExceptionOr<RefPtr<JSC::ArrayBuffer>>, too. >>>> >>>> I agree about returning a RefPtr<<JSC::ArrayBuffer>>, the current code is definitely unsafe. >>>> However, I don't understand why we would want to return a `ExceptionOr<RefPtr<JSC::ArrayBuffer>>` as Brent suggested, given that this function never returns any exception. This will result in an extra branch in JS bindings code for no reason. >>> >>> Doh! I missed that Brent suggested above to return an Exception in some cases. Please disregard my comment then :) >> >> Pascoe mentioned this was primarily a convenience function for websites, so I thought that returning an Exception in the two failure cases (above) would be helpful to the developer. >> >> For example, missing "authData" might be something they could fix or report to the user in some useful way. > > Yes, I had missed that you suggested raising an exception in some cases. Note that it really depends if these cases are supposed to be possible or not. > Given that there was an ASSERT_NOT_REACHED(), I assumed we don't expect this to happen in practice so presumably, whatever exception we generate would not be observable (unless the assertion is wrong). I don't really mind adding exception throwing code though here if we're unsure this can happen in practice or not. That's a good point. Probably better to avoid the complexity and runtime cost since these are ASSERT_NOT_REACHED cases. :-)

Comment on attachment 457282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457282&action=review > Source/WebCore/Modules/webauthn/WebAuthenticationConstants.h:96 > +constexpr ASCIILiteral authenticatorTransportUsb = { "usb"_s }; nit: Could use use auto instead of ASCIILiteral here and for other below. > Source/WebCore/Modules/webauthn/WebAuthenticationUtils.cpp:49 > + return convertBytesToVector(reinterpret_cast<uint8_t*>(buffer->data()), buffer->byteLength()); This should be a static_cast<>, not a reinterpret_cast<>. > Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h:64 > + const std::optional<Vector<WebCore::AuthenticatorTransport>> transports() const { return m_transports; } As mentioned in my earlier review, I believe this should return a `const std::optional<Vector<WebCore::AuthenticatorTransport>>&`, not a `const std::optional<Vector<WebCore::AuthenticatorTransport>>` > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:350 > + if (!transportString.isString()) Isn't it a little odd that having a non-string in the array will cause us to return nullopt but having an invalid string in that array will just be ignored and will return a vector? The inconsistency doesn't seem great unless there is a good reason for it. > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:353 > + if (transport) could reduce scope by doing: if (auto transport = convertStringToAuthenticatorTransport(transportString.getString())) > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:-52 > - I think we should keep this blank line. Seems unusual otherwise. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:67 > +#import "AuthenticationServicesCoreSoftLink.h" Why isn't this with the other includes? > Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:348 > + transports.uncheckedAppend(static_cast<WebCore::AuthenticatorTransport>(ascTransport.intValue)); Don't we need to do some validation here? What if the array contains an int for the given enum? > Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp:390 > + if (infoTransports) if (auto infoTransports = m_info.transports()) to reduce scope. Also, if m_info.transports() returns a reference (hard for me to tell here), this would be even better: if (auto& infoTransports = m_info.transports()) > Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.h:73 > + Vector<WebCore::AuthenticatorTransport> transports(); can this be const?

Really appreciate all the comments / review. Will fix these issues and put up another revision shortly. (In reply to Chris Dumez from comment #13) > > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:350 > > + if (!transportString.isString()) > > Isn't it a little odd that having a non-string in the array will cause us to > return nullopt but having an invalid string in that array will just be > ignored and will return a vector? > The inconsistency doesn't seem great unless there is a good reason for it. > > > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:353 > > + if (transport) > This is a list of transports the authenticator supports, specified here: https://fidoalliance.org/specs/fido-v2.1-rd-20201208/fido-client-to-authenticator-protocol-v2.1-rd-20201208.html#getinfo-transports The spec says "Platforms MUST tolerate unknown values," so invalid strings are fine. We don't want to reject future authenticators that support transport="carrier-pigeon"

(In reply to j_pascoe@apple.com from comment #14) > Really appreciate all the comments / review. Will fix these issues and put > up another revision shortly. > > (In reply to Chris Dumez from comment #13) > > > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:350 > > > + if (!transportString.isString()) > > > > Isn't it a little odd that having a non-string in the array will cause us to > > return nullopt but having an invalid string in that array will just be > > ignored and will return a vector? > > The inconsistency doesn't seem great unless there is a good reason for it. > > > > > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:353 > > > + if (transport) > > > > This is a list of transports the authenticator supports, specified here: > https://fidoalliance.org/specs/fido-v2.1-rd-20201208/fido-client-to- > authenticator-protocol-v2.1-rd-20201208.html#getinfo-transports > > The spec says "Platforms MUST tolerate unknown values," so invalid strings > are fine. We don't want to reject future authenticators that support > transport="carrier-pigeon" So maybe we can ignore values that aren't strings (use continue instead of return), similarly to what we do for invalid strings? Also how about JS values that can be converted into a String? For e.g., what should happen if the array contains this: { toString: () => { return "usb"; } }; Because you reject non-strings right now, you would ignore it. If you weren't rejecting non-string and calling JSValue::toString() instead of JSValue::getString(), you would treat it as "usb". Not sure which is right, just thought I would mention it.

(In reply to Chris Dumez from comment #15) > (In reply to j_pascoe@apple.com from comment #14) > > Really appreciate all the comments / review. Will fix these issues and put > > up another revision shortly. > > > > (In reply to Chris Dumez from comment #13) > > > > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:350 > > > > + if (!transportString.isString()) > > > > > > Isn't it a little odd that having a non-string in the array will cause us to > > > return nullopt but having an invalid string in that array will just be > > > ignored and will return a vector? > > > The inconsistency doesn't seem great unless there is a good reason for it. > > > > > > > Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp:353 > > > > + if (transport) > > > > > > > This is a list of transports the authenticator supports, specified here: > > https://fidoalliance.org/specs/fido-v2.1-rd-20201208/fido-client-to- > > authenticator-protocol-v2.1-rd-20201208.html#getinfo-transports > > > > The spec says "Platforms MUST tolerate unknown values," so invalid strings > > are fine. We don't want to reject future authenticators that support > > transport="carrier-pigeon" > > So maybe we can ignore values that aren't strings (use continue instead of > return), similarly to what we do for invalid strings? > > Also how about JS values that can be converted into a String? For e.g., what > should happen if the array contains this: > { toString: () => { return "usb"; } }; > > Because you reject non-strings right now, you would ignore it. If you > weren't rejecting non-string and calling JSValue::toString() instead of > JSValue::getString(), you would treat it as "usb". > Not sure which is right, just thought I would mention it. We could, the spec says transports MUST either not be present or be a non-empty array of strings. We don't need to worry about JS values that can be converted to string here as we are parsing CBOR.

*** Bug 204807 has been marked as a duplicate of this bug. ***

Created attachment 457643 [details] Patch

Comment on attachment 457643 [details] Patch Thanks for making those changes! r=me

Committed r292913 (249683@main): <https://commits.webkit.org/249683@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 457643 [details].