Bug 218752 - Add a WebRTC SFrame transform
Summary: Add a WebRTC SFrame transform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 218751
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-10 07:16 PST by youenn fablet
Modified: 2020-11-16 10:22 PST (History)
20 users (show)

See Also:


Attachments
WIP (59.41 KB, patch)
2020-11-11 00:31 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (76.71 KB, patch)
2020-11-13 02:07 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (77.33 KB, patch)
2020-11-13 02:28 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (77.43 KB, patch)
2020-11-13 02:42 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (77.48 KB, patch)
2020-11-13 02:43 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (78.11 KB, patch)
2020-11-13 02:50 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (78.11 KB, patch)
2020-11-13 03:55 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (78.65 KB, patch)
2020-11-13 06:23 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (79.03 KB, patch)
2020-11-13 11:44 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (78.39 KB, patch)
2020-11-15 05:38 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-11-10 07:16:06 PST
Add a WebRTC SFrame transform
Comment 1 youenn fablet 2020-11-11 00:31:43 PST
Created attachment 413792 [details]
WIP
Comment 2 Radar WebKit Bug Importer 2020-11-12 11:35:12 PST
<rdar://problem/71335529>
Comment 3 youenn fablet 2020-11-13 02:07:22 PST
Created attachment 414016 [details]
Patch
Comment 4 youenn fablet 2020-11-13 02:28:12 PST
Created attachment 414019 [details]
Patch
Comment 5 youenn fablet 2020-11-13 02:42:29 PST
Created attachment 414023 [details]
Patch
Comment 6 youenn fablet 2020-11-13 02:43:40 PST
Created attachment 414024 [details]
Patch
Comment 7 youenn fablet 2020-11-13 02:50:21 PST
Created attachment 414026 [details]
Patch
Comment 8 youenn fablet 2020-11-13 03:55:36 PST
Created attachment 414029 [details]
Patch
Comment 9 youenn fablet 2020-11-13 06:23:37 PST
Created attachment 414038 [details]
Patch
Comment 10 Eric Carlson 2020-11-13 08:17:09 PST
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/
Comment 11 youenn fablet 2020-11-13 11:44:02 PST
Created attachment 414071 [details]
Patch for landing
Comment 12 EWS 2020-11-15 05:34:54 PST
Tools/Scripts/svn-apply failed to apply attachment 414071 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 13 youenn fablet 2020-11-15 05:38:48 PST
Created attachment 414163 [details]
Rebasing
Comment 14 EWS 2020-11-15 10:00:18 PST
Committed r269830: <https://trac.webkit.org/changeset/269830>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414163 [details].
Comment 15 Sam Weinig 2020-11-15 11:31:45 PST
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.
Comment 16 youenn fablet 2020-11-16 00:05:43 PST
(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.
Comment 17 Sam Weinig 2020-11-16 09:47:41 PST
(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.
Comment 18 youenn fablet 2020-11-16 10:22:42 PST
(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.