3.23 KB, patch
|Details | Diff | Splinter Review|
3.24 KB, patch
|Details | Diff | Splinter Review|
Nothing guarantees UnRegisterActivityObserver() ever succeeds to remove the registration since the current document of the window may have already changed. I'll try to write a testcase.
Regression from bug 969372. I don't have testcase yet, but it would be something like: var win = window.open("foobar.html"); new win.MediaRecorder(stream); // trigger GC/CC synchronously
Created attachment 8811050 [details] [diff] [review] v1 Haven't managed to test this yet, since mach refuses to run media tests on my machine :/
Comment on attachment 8811050 [details] [diff] [review] v1 Finally got media tests running locally. Seems to pass them.
Comment on attachment 8811050 [details] [diff] [review] v1 Review of attachment 8811050 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks great. So the testcase scenario is that CC happened but GC never come?
the testcase scenario is that we swap the initial about:blank document from inner window after creating MediaRecorder. (remember, window<-> document relationship is _not_ 1-1 relationship. One inner window may have 1 or 2 documents during its lifetime, and one document may have any number of inner windows).
Comment on attachment 8811050 [details] [diff] [review] v1 [Security approval request comment] How easily could an exploit be constructed based on the patch? I'd say not too easily, but the patch does pinpoint where the issue is. Bug 1317409 is related. The issue was found when reading code, but I think the issue is rather obvious, even if hard to actually crash the browser unsafely. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Commit message could be " Bug 1317805, ensure to update MediaRecorder when document's activity state changes, r=bechen" Which older supported branches are affected by this flaw? all Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? esr45 needs a different patch because of some renaming in the code. Patch coming real soon. How likely is this patch to cause regressions; how much testing does it need? Should be reasonable safe. If there is some weird memory management somewhere, adding a new strong reference could be an issue. String or UUID changes made by this patch: NA
As far as I see this is sec-high or critical
This was marked as an audit because it was found by a code audit, I believe, with no specific examples of exploitability. I'll give sec-approval but I'm not sure we're going to take it in branches without more explanation as to why we'd need to bump it up the trains.
As far as I see, when the bug occurs, results are similar to Bug 1317409. Would probably take couple of hours to write a crasher testcase.
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #8) > As far as I see this is sec-high or critical Thanks. As a member of the security team please feel free to correct these ratings when you think they're wrong. If you'd prefer us to make the final decision but want to "appeal" a rating a bug comment is likely going to be missed. Make the comment (we need something to look at) but then * ping us in #security ("please see https://bugzilla.mozilla.org/show_bug.cgi?id=1317805#c8") * mail firstname.lastname@example.org * needinfo? Al or myself (or whoever replaces us in the future) * clear the sec- keyword to bump it back into triage (but then please leave a comment so we don't scratch our heads and then assume it was a mistake). those are roughly in my order of preference but they're all about the same.
I guess we should backport this then!
Comment on attachment 8811050 [details] [diff] [review] v1 Sec-high, meets the triage bar for inclusion in 50.1.0, ESR45.6