Bug 224276 - Update SFrame implementation to latest version
Summary: Update SFrame implementation to latest version
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:
Blocks:
 
Reported: 2021-04-07 03:07 PDT by youenn fablet
Modified: 2021-04-08 03:53 PDT (History)
10 users (show)

See Also:


Attachments
Patch (25.15 KB, patch)
2021-04-07 03:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (25.97 KB, patch)
2021-04-07 03:34 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (31.51 KB, patch)
2021-04-07 05:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (31.98 KB, patch)
2021-04-08 01:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (31.97 KB, patch)
2021-04-08 03:19 PDT, 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 2021-04-07 03:07:13 PDT
Update SFrame implementation to latest version
Comment 1 youenn fablet 2021-04-07 03:10:36 PDT
Created attachment 425374 [details]
Patch
Comment 2 youenn fablet 2021-04-07 03:34:13 PDT
Created attachment 425377 [details]
Patch
Comment 3 youenn fablet 2021-04-07 05:22:43 PDT
Created attachment 425386 [details]
Patch
Comment 4 Eric Carlson 2021-04-07 10:25:13 PDT
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
Comment 5 youenn fablet 2021-04-08 01:06:00 PDT
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.
Comment 6 Radar WebKit Bug Importer 2021-04-08 01:06:53 PDT
<rdar://problem/76388329>
Comment 7 youenn fablet 2021-04-08 01:26:04 PDT
Created attachment 425483 [details]
Patch for landing
Comment 8 EWS 2021-04-08 02:57:36 PDT
ChangeLog entry in Tools/ChangeLog contains OOPS!.
Comment 9 youenn fablet 2021-04-08 03:19:46 PDT
Created attachment 425491 [details]
Patch for landing
Comment 10 EWS 2021-04-08 03:53:49 PDT
Committed r275656: <https://commits.webkit.org/r275656>

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