Bug 224276 – Update SFrame implementation to latest version

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

[youenn fablet]

Update SFrame implementation to latest version

[youenn fablet]

Created attachment 425374 [details]
Patch

[youenn fablet]

Created attachment 425377 [details]
Patch

[youenn fablet]

Created attachment 425386 [details]
Patch

[Eric Carlson]

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

[youenn fablet]

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.

[Radar WebKit Bug Importer]

<rdar://problem/76388329>

[youenn fablet]

Created attachment 425483 [details]
Patch for landing

[EWS]

ChangeLog entry in Tools/ChangeLog contains OOPS!.

[youenn fablet]

Created attachment 425491 [details]
Patch for landing

[EWS]

Committed r275656: <https://commits.webkit.org/r275656>

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