Bug 218752 – Add a WebRTC SFrame transform

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Add a WebRTC SFrame transform

[youenn fablet]

Created attachment 413792 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/71335529>

[youenn fablet]

Created attachment 414016 [details]
Patch

[youenn fablet]

Created attachment 414019 [details]
Patch

[youenn fablet]

Created attachment 414023 [details]
Patch

[youenn fablet]

Created attachment 414024 [details]
Patch

[youenn fablet]

Created attachment 414026 [details]
Patch

[youenn fablet]

Created attachment 414029 [details]
Patch

[youenn fablet]

Created attachment 414038 [details]
Patch

[Eric Carlson]

Comment on attachment 414038 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.cpp:56
> +        promise.reject(Exception { TypeError, "Invalid key"_s });
> +        return;
> +    }
> +
> +    if (WTF::get<CryptoKeyAlgorithm>(algorithm).name != "HKDF") {
> +        promise.reject(Exception { TypeError, "Invalid key"_s });

It might be helpful to have different error strings to help a developer understand the failure.

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.cpp:75
> +    backend.setTransformableFrameCallback([transformer = m_transformer, backend = makeRef(backend)](auto&& frame) {
> +        auto chunk = frame.data();

Is there no chance the transform can be destroyed before the callback fires?

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:89
> +    if (firstByte & 0x80)

Nit: it might be nice to put this and the other bit tests below into a methods to make it clear what they do.

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

s/2018/2020/

> Tools/TestWebKitAPI/Tests/WebCore/RTCRtpSFrameTransformerTests.cpp:2
> + * Copyright (C) 2014-2016 Apple Inc. All rights reserved.

s/2014-2016/2020/

[youenn fablet]

Created attachment 414071 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 414071 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[youenn fablet]

Created attachment 414163 [details]
Rebasing

[EWS]

Committed r269830: <https://trac.webkit.org/changeset/269830>

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

[Sam Weinig]

Comment on attachment 414163 [details]
Rebasing

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

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:80
> +static inline Vector<uint8_t> computeIV(uint64_t counter, const Vector<uint8_t>& saltKey)
> +{
> +    Vector<uint8_t> iv(16);
> +    for (unsigned i = 0; i < 8; ++i) {
> +        auto value = (counter >> ((7 - i) * 8)) & 0xff;
> +        iv[i] = value ^ saltKey[i];
> +    }
> +    for (unsigned i = 8; i < 16; ++i)
> +        iv[i] = saltKey[i];
> +    return iv;
> +}

This seems unsafe. Nothing is bounds checking saltKey here.

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:233
> +        auto keyIdLength = lengthOfUInt64(m_keyId);
> +        writeUInt64(transformedData.data() + headerSize, m_keyId, keyIdLength);

What prevents keyIdLength from being bigger than transformedData.size() - headerSize (in which case I believe writeUInt64 will buffer overrun)?

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:237
> +    auto counterLength = lengthOfUInt64(m_counter);
> +    writeUInt64(transformedData.data() + headerSize, m_counter, counterLength);

Same question.

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformerCocoa.cpp:69
> +    Vector<uint8_t> result(CC_SHA256_DIGEST_LENGTH);
> +    CCHmac(kCCHmacAlgSHA256, key.data(), key.size(), data, size, result.data());
> +    return result;
> +}

This really shouldn't be calling CommonCrypto directly. The way we like to handle this in WebCore is to add an appropriate platform abstraction in the platform/ directory (or PAL) and use that.

[youenn fablet]

(In reply to Sam Weinig from comment #15)
> Comment on attachment 414163 [details]
> Rebasing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414163&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:80
> > +static inline Vector<uint8_t> computeIV(uint64_t counter, const Vector<uint8_t>& saltKey)
> > +{
> > +    Vector<uint8_t> iv(16);
> > +    for (unsigned i = 0; i < 8; ++i) {
> > +        auto value = (counter >> ((7 - i) * 8)) & 0xff;
> > +        iv[i] = value ^ saltKey[i];
> > +    }
> > +    for (unsigned i = 8; i < 16; ++i)
> > +        iv[i] = saltKey[i];
> > +    return iv;
> > +}
> 
> This seems unsafe. Nothing is bounds checking saltKey here.

I will add an ASSERT.

> > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:233
> > +        auto keyIdLength = lengthOfUInt64(m_keyId);
> > +        writeUInt64(transformedData.data() + headerSize, m_keyId, keyIdLength);
> 
> What prevents keyIdLength from being bigger than transformedData.size() -
> headerSize (in which case I believe writeUInt64 will buffer overrun)?
> > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:237
> > +    auto counterLength = lengthOfUInt64(m_counter);
> > +    writeUInt64(transformedData.data() + headerSize, m_counter, counterLength);
> 
> Same question.
> 

transformedData is allocating (frameSize + MaxHeaderSize + m_authenticationSize) data.
MaxHeaderSize is the max size of header (17 bytes), 1 byte + 8 bytes for key ID + 8 bytes for counter.

[Sam Weinig]

(In reply to youenn fablet from comment #16)
> (In reply to Sam Weinig from comment #15)
> > Comment on attachment 414163 [details]
> > Rebasing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=414163&action=review
> > 
> > > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:80
> > > +static inline Vector<uint8_t> computeIV(uint64_t counter, const Vector<uint8_t>& saltKey)
> > > +{
> > > +    Vector<uint8_t> iv(16);
> > > +    for (unsigned i = 0; i < 8; ++i) {
> > > +        auto value = (counter >> ((7 - i) * 8)) & 0xff;
> > > +        iv[i] = value ^ saltKey[i];
> > > +    }
> > > +    for (unsigned i = 8; i < 16; ++i)
> > > +        iv[i] = saltKey[i];
> > > +    return iv;
> > > +}
> > 
> > This seems unsafe. Nothing is bounds checking saltKey here.
> 
> I will add an ASSERT.

What is ensuring saltKey is long enough? It kind of seems like m_saltKey is maybe supposed to be a fixed size array of 16 bytes (from looking at computeSaltKey). One way to make this more clear and less error prone would be to use a better type for it, like a fixed size std::array rather than a Vector. 

> 
> > > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:233
> > > +        auto keyIdLength = lengthOfUInt64(m_keyId);
> > > +        writeUInt64(transformedData.data() + headerSize, m_keyId, keyIdLength);
> > 
> > What prevents keyIdLength from being bigger than transformedData.size() -
> > headerSize (in which case I believe writeUInt64 will buffer overrun)?
> > > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:237
> > > +    auto counterLength = lengthOfUInt64(m_counter);
> > > +    writeUInt64(transformedData.data() + headerSize, m_counter, counterLength);
> > 
> > Same question.
> > 
> 
> transformedData is allocating (frameSize + MaxHeaderSize +
> m_authenticationSize) data.
> MaxHeaderSize is the max size of header (17 bytes), 1 byte + 8 bytes for key
> ID + 8 bytes for counter.

Is there any way to make this clearer through the type system or judicious use of release asserts? This seems like something that could easily be broken if anyone changes any of the built in assumptions here.

[youenn fablet]

(In reply to Sam Weinig from comment #17)
> (In reply to youenn fablet from comment #16)
> > (In reply to Sam Weinig from comment #15)
> > > Comment on attachment 414163 [details]
> > > Rebasing
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=414163&action=review
> > > 
> > > > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:80
> > > > +static inline Vector<uint8_t> computeIV(uint64_t counter, const Vector<uint8_t>& saltKey)
> > > > +{
> > > > +    Vector<uint8_t> iv(16);
> > > > +    for (unsigned i = 0; i < 8; ++i) {
> > > > +        auto value = (counter >> ((7 - i) * 8)) & 0xff;
> > > > +        iv[i] = value ^ saltKey[i];
> > > > +    }
> > > > +    for (unsigned i = 8; i < 16; ++i)
> > > > +        iv[i] = saltKey[i];
> > > > +    return iv;
> > > > +}
> > > 
> > > This seems unsafe. Nothing is bounds checking saltKey here.
> > 
> > I will add an ASSERT.
> 
> What is ensuring saltKey is long enough? It kind of seems like m_saltKey is
> maybe supposed to be a fixed size array of 16 bytes (from looking at
> computeSaltKey). One way to make this more clear and less error prone would
> be to use a better type for it, like a fixed size std::array rather than a
> Vector.

It might depend on the encryption mechanism. We only support one at the moment.
I added an ASSERT when computing m_saltKey in https://bugs.webkit.org/show_bug.cgi?id=218981.
 
> > > > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:233
> > > > +        auto keyIdLength = lengthOfUInt64(m_keyId);
> > > > +        writeUInt64(transformedData.data() + headerSize, m_keyId, keyIdLength);
> > > 
> > > What prevents keyIdLength from being bigger than transformedData.size() -
> > > headerSize (in which case I believe writeUInt64 will buffer overrun)?
> > > > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:237
> > > > +    auto counterLength = lengthOfUInt64(m_counter);
> > > > +    writeUInt64(transformedData.data() + headerSize, m_counter, counterLength);
> > > 
> > > Same question.
> > > 
> > 
> > transformedData is allocating (frameSize + MaxHeaderSize +
> > m_authenticationSize) data.
> > MaxHeaderSize is the max size of header (17 bytes), 1 byte + 8 bytes for key
> > ID + 8 bytes for counter.
> 
> Is there any way to make this clearer through the type system or judicious
> use of release asserts? This seems like something that could easily be
> broken if anyone changes any of the built in assumptions here.

All of this computation is done in the same method so breakage can only happen in these 10 lines. I can add some comments.
Or I can rewrite to have a method generating the header as a Vector<uint8_t, 17> and memcpy the header content into transformedData.
I'll do that in https://bugs.webkit.org/show_bug.cgi?id=218981.