Summary: | Add a WebRTC SFrame transform | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, benjamin, calvaris, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hta, jer.noble, jiewen_tan, kondapallykalyan, philipj, ryuan.choi, sam, sergio, tommyw, webkit-bug-importer, youennf | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 218751 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
youenn fablet
2020-11-10 07:16:06 PST
Created attachment 413792 [details]
WIP
Created attachment 414016 [details]
Patch
Created attachment 414019 [details]
Patch
Created attachment 414023 [details]
Patch
Created attachment 414024 [details]
Patch
Created attachment 414026 [details]
Patch
Created attachment 414029 [details]
Patch
Created attachment 414038 [details]
Patch
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/ Created attachment 414071 [details]
Patch for landing
Tools/Scripts/svn-apply failed to apply attachment 414071 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Created attachment 414163 [details]
Rebasing
Committed r269830: <https://trac.webkit.org/changeset/269830> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414163 [details]. 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. (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. (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. (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. |