Summary: | Update SFrame implementation to latest version | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Local Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
youenn fablet
2021-04-07 03:07:13 PDT
Created attachment 425374 [details]
Patch
Created attachment 425377 [details]
Patch
Created attachment 425386 [details]
Patch
Comment on attachment 425386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425386&action=review > Source/WebCore/ChangeLog:8 > + Update implementation according latest draft at https://github.com/eomara/sframe/blob/master/draft-omara-sframe.md. s/according latest draft/according to latest draft/ > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:74 > static inline Vector<uint8_t> computeIV(uint64_t counter, const Vector<uint8_t>& saltKey) > { > Vector<uint8_t> iv(16); A comment about what this does would be helpful. Maybe you should quote the current spec text "The key for the encryption is the sframe_key and the nonce is formed by XORing ..."? > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:80 > + for (unsigned i = 11; i >= 4; --i) { > + auto value = counter & 0xff; > + counter = counter >> 8; > iv[i] = value ^ saltKey[i]; `value` isn't used. Can you convert the salt key to big-endian before XORing to make this clearer? > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:167 > + ASSERT(saltKeyResult.returnValue().size() >= 12); It would be nice to have this magic value in a named constant to help someone not completely familiar with the spec who reads this Thanks for the review. (In reply to Eric Carlson from comment #4) > Comment on attachment 425386 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425386&action=review > > > Source/WebCore/ChangeLog:8 > > + Update implementation according latest draft at https://github.com/eomara/sframe/blob/master/draft-omara-sframe.md. > > s/according latest draft/according to latest draft/ OK > > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:74 > > static inline Vector<uint8_t> computeIV(uint64_t counter, const Vector<uint8_t>& saltKey) > > { > > Vector<uint8_t> iv(16); > > A comment about what this does would be helpful. Maybe you should quote the > current spec text "The key for the encryption is the sframe_key and the > nonce is formed by XORing ..."? I added a comment. I should probably refactor a bit more the code to more closely follow the new spec and new terminology (s/iv/nonce/ for instance). > > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:80 > > + for (unsigned i = 11; i >= 4; --i) { > > + auto value = counter & 0xff; > > + counter = counter >> 8; > > iv[i] = value ^ saltKey[i]; > > `value` isn't used. > Can you convert the salt key to big-endian before XORing to make this > clearer? The salt is already big-endian. We do as if we convert the counter to a 12 bytes big endian and XOR both of them. I added a comment to explain all this. > > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransformer.cpp:167 > > + ASSERT(saltKeyResult.returnValue().size() >= 12); > > It would be nice to have this magic value in a named constant to help > someone not completely familiar with the spec who reads this OK. Created attachment 425483 [details]
Patch for landing
ChangeLog entry in Tools/ChangeLog contains OOPS!. Created attachment 425491 [details]
Patch for landing
Committed r275656: <https://commits.webkit.org/r275656> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425491 [details]. |