Bug 189740 - [EME] Add WebM sanitization
Summary: [EME] Add WebM sanitization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 189696
  Show dependency tree / graph
 
Reported: 2018-09-19 03:04 PDT by Yacine Bandou
Modified: 2018-09-21 00:19 PDT (History)
4 users (show)

See Also:

Attachments
Patch (3.19 KB, patch)
2018-09-19 05:52 PDT, Yacine Bandou 		calvaris: review+
Details | Formatted Diff | Diff
Patch (3.17 KB, patch)
2018-09-19 06:17 PDT, Yacine Bandou 		no flags Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2018-09-20 03:38 PDT, Yacine Bandou 		no flags Details | Formatted Diff | Diff
Patch (1.80 KB, patch)
2018-09-20 08:58 PDT, Yacine Bandou 		no flags Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2018-09-20 09:08 PDT, Yacine Bandou 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yacine Bandou 2018-09-19 03:04:32 PDT 
Add Webm sanitization.
Comment 1 Yacine Bandou 2018-09-19 05:52:05 PDT 
Created attachment 350106 [details]
Patch
Comment 2 Yacine Bandou 2018-09-19 06:17:49 PDT 
Created attachment 350108 [details]
Patch
Comment 3 Xabier Rodríguez Calvar 2018-09-19 08:33:59 PDT 
Comment on attachment 350106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350106&action=review

> Source/WebCore/ChangeLog:10
> +        see https://www.w3.org/TR/encrypted-media/#dom-mediakeysession-generaterequest

Capital at the beginning, period at the end.

> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:160
> +    if (buffer.isEmpty() || buffer.size() > kWebmMaxContentEncKeyIDSize)
> +        return std::nullopt;
> +    keyIDs.append(buffer.copy());

Instead of this, I'd try to sanitize and if the result is null, bailout. Otherwise, move the WTFMove the result inside the append
Comment 4 Yacine Bandou 2018-09-20 03:38:41 PDT 
Created attachment 350184 [details]
Patch
Comment 5 Olivier Blin 2018-09-20 04:06:22 PDT 
Comment on attachment 350184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350184&action=review

> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:157
> +    RefPtr<SharedBuffer> sanitazedBuffer = sanitizeWebM(buffer);

You probably mean "sanitized"
Comment 6 WebKit Commit Bot 2018-09-20 04:22:07 PDT 
Comment on attachment 350184 [details]
Patch

Clearing flags on attachment: 350184

Committed r236256: <https://trac.webkit.org/changeset/236256>
Comment 7 WebKit Commit Bot 2018-09-20 04:22:09 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-09-20 04:23:26 PDT 
<rdar://problem/44636894>
Comment 9 Xabier Rodríguez Calvar 2018-09-20 06:15:52 PDT 
(In reply to Olivier Blin from comment #5)
> > Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:157
> > +    RefPtr<SharedBuffer> sanitazedBuffer = sanitizeWebM(buffer);
> 
> You probably mean "sanitized"

Very true! Good catch. Sad that the patch landed already. Can you file a follow up please?
Comment 10 Yacine Bandou 2018-09-20 08:58:08 PDT 
Reopening to attach new patch.
Comment 11 Yacine Bandou 2018-09-20 08:58:10 PDT 
Created attachment 350208 [details]
Patch
Comment 12 Yacine Bandou 2018-09-20 09:08:16 PDT 
Created attachment 350213 [details]
Patch
Comment 13 Yacine Bandou 2018-09-20 09:26:06 PDT 
I pushed the attached patch 350208 by mistake, instead of pushing it on the bug 189789, I pushed it on this one.
I pushed again the original patch of this bug for reference.