AuthenticatorResponseData::decode should check bufferIsLargeEnoughToContain before allocating buffers

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Fujii Hironori]

AuthenticatorResponseData::decode should check bufferIsLargeEnoughToContain before allocating buffers

This is a sub-task of Bug 209131.
Bug 209131 – Don't allocate a buffer with the decoded size without ensuring bufferIsLargeEnoughToContain(size)

[Fujii Hironori]

Created attachment 393635 [details]
Patch

[Fujii Hironori]

Created attachment 393637 [details]
Patch

[Jiewen Tan]

Comment on attachment 393637 [details]
Patch

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

Please fix all build failures.

> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:114
> +    if (!decoder.template bufferIsLargeEnoughToContain<uint8_t>(rawIdLength))

This doesn't seem to be a valid C++ expression.

> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:134
> +        if (!decoder.template bufferIsLargeEnoughToContain<uint8_t>(attestationObjectLength))

This doesn't seem to be a valid C++ expression.

> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:150
> +    if (!decoder.template bufferIsLargeEnoughToContain<uint8_t>(authenticatorDataLength))

This doesn't seem to be a valid C++ expression.

> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:163
> +    if (!decoder.template bufferIsLargeEnoughToContain<uint8_t>(signatureLength))

This doesn't seem to be a valid C++ expression.

> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:189
> +    if (!decoder.template bufferIsLargeEnoughToContain<uint8_t>(userHandleLength))

This doesn't seem to be a valid C++ expression.

[Fujii Hironori]

Thanks. Will fix.

[Darin Adler]

Comment on attachment 393637 [details]
Patch

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

>> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:114
>> +    if (!decoder.template bufferIsLargeEnoughToContain<uint8_t>(rawIdLength))
> 
> This doesn't seem to be a valid C++ expression.

Needs to be rawIdLength.value(), but otherwise looks valid to me. Are you just saying you’re not familiar with the ".template" syntax?

[Fujii Hironori]

Oh, that part.  See
https://stackoverflow.com/q/3786360
https://stackoverflow.com/a/613132

Too long, I didn't read neither understand :-)

[Fujii Hironori]

Created attachment 393682 [details]
Patch

[Jiewen Tan]

(In reply to Darin Adler from comment #5)
> Comment on attachment 393637 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393637&action=review
> 
> >> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:114
> >> +    if (!decoder.template bufferIsLargeEnoughToContain<uint8_t>(rawIdLength))
> > 
> > This doesn't seem to be a valid C++ expression.
> 
> Needs to be rawIdLength.value(), but otherwise looks valid to me. Are you
> just saying you’re not familiar with the ".template" syntax?

My bad, it has been a while since last time I have to use .template.

[Darin Adler]

Comment on attachment 393682 [details]
Patch

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

> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:120
>      if (!rawIdLength)
>          return WTF::nullopt;
>  
> -    result.rawId = ArrayBuffer::create(rawIdLength.value(), sizeof(uint8_t));
> +    if (!decoder.template bufferIsLargeEnoughToContain<uint8_t>(rawIdLength.value()))
> +        return WTF::nullopt;
> +    result.rawId = ArrayBuffer::tryCreate(rawIdLength.value(), sizeof(uint8_t));
> +    if (!result.rawId)
> +        return WTF::nullopt;
>      if (!decoder.decodeFixedLengthData(reinterpret_cast<uint8_t*>(result.rawId->data()), rawIdLength.value(), 1))
>          return WTF::nullopt;

Feels to me like we should refactor so we can share more code. This pattern over and over again.

[Fujii Hironori]

Comment on attachment 393682 [details]
Patch

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

>> Source/WebCore/Modules/webauthn/AuthenticatorResponseData.h:120
>>          return WTF::nullopt;
> 
> Feels to me like we should refactor so we can share more code. This pattern over and over again.

Good idea. Will fix.

[Fujii Hironori]

Created attachment 393904 [details]
Patch

[Fujii Hironori]

Created attachment 393929 [details]
Patch

[Fujii Hironori]

Comment on attachment 393929 [details]
Patch

Clearing flags on attachment: 393929

Committed r258676: <https://trac.webkit.org/changeset/258676>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60615555>