Bug 1297361 (CVE-2017-5390)

JSON Viewer: use listeners instead of exporting postChromeMessage

RESOLVED FIXED in Firefox -esr45

Status

defect
RESOLVED FIXED
3 years ago
23 days ago

People

(Reporter: Honza, Assigned: Honza)

Tracking

({csectype-priv-escalation, sec-high})

unspecified
Firefox 53
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr4551+ fixed, firefox50 wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])

Attachments

(2 attachments, 11 obsolete attachments)

5.58 KB, patch
Honza
: review+
Details | Diff | Splinter Review
5.29 KB, patch
Honza
: review+
Details | Diff | Splinter Review
Follow up for:
https://bugzilla.mozilla.org/show_bug.cgi?id=1295309#c26

JSON viewer uses `Cu.exportFunction` to export `postChromeMessage` method into content scope to create a communication channel (used for, copying & saving JSON data and HTTP headers from the content).

This should be replaced by window listeners.

Honza
Can we maybe go ahead a throw a sec-bounty? flag on here for the powers that be?
Flags: sec-bounty?
Jan, can you work on fixing this, or should I find somebody else? It is almost two months old. Thanks.
Flags: needinfo?(odvarko)
(In reply to Andrew McCreight [:mccr8] from comment #2)
> Jan, can you work on fixing this, or should I find somebody else? It is
> almost two months old. Thanks.

I've got a lot on my plate these days so, if you can find someone that would be great!

Honza
Flags: needinfo?(odvarko)
Joe, do you know who else might be able to work on this sec-high issue? I'm not sure who is familiar with this code. Thanks.
Flags: needinfo?(jwalker)
Dan - is Honza's description of what we should be doing about this in Comment 0 correct and sufficient?
Flags: needinfo?(jwalker) → needinfo?(dveditz)
That seems correct.
Flags: needinfo?(dveditz)
(In reply to Daniel Veditz [:dveditz] from comment #6)
> That seems correct.

Thanks Daniel, I'll look at this.

Honza
Assignee: nobody → odvarko
Any movement here?  I'm going to throw something together and see how lacking it is.
Ok I've identified the key points to replace, what would in fact be the proper way to pass objects through event listeners, or does someone else have an idea here?  I'll leave it this for now, with the rest of the work relating to this in place I'm somewhat hesitant to change anything yet.
I guess I'll try to implement JSON.stringify for passing objects.  I'll contact you on IRC Honza, just to see if you have anything already going in this direction.

I did test the other defense in depth patches regarding this and they seem solid as of right now.  Let's hope no regressions break things =)
Thanks Jerri!

Honza
I haven't gotten to this yet, having some problems with hg and mc cloning for some reason.  I'll grab a bundle and get to it soon though.

Comment 13

2 years ago
FWIW, it should be possible to use a custom event (new CustomEvent("myEvent", {detail: obj}) ), I think.
Posted patch Untested patch (obsolete) — Splinter Review
Here is what I had so far, currently waiting to get another system configured to do the test build there.

Can you guys look this over / pick this up from here?

Thanks

Comment 15

2 years ago
Do we think we need the JSON-ification for safety? Otherwise, I would have thought we could just pass the object directly on the event's detail property - is there some reason that doesn't work?
I guess it would, I discussed this with Honza and now that I'm thinking about it that is probably the better solution.  I was stuck on how to have JSON parse the received object back without security issues anyway.

I'm still trying to get a source building setup right, any chance you can give that a try Gijs?
Comment on attachment 8812986 [details] [diff] [review]
Untested patch

Review of attachment 8812986 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Jerri for working on this! I believe that this is going the right direction.

I know this is just a testing patch, but I put made some comments inline...

And, yeah, agree with Gijs, passing direct object would be better if safe.

Honza

::: devtools/client/jsonview/converter-child.js
@@ +140,4 @@
>      JsonViewUtils.exportIntoContentScope(win, Locale, "Locale");
>  
>      Events.once(win, "DOMContentLoaded", event => {
> +      win.addEventListener("contentMessage", getContentMessage, false, true);

nit: What about renaming getContentMessage to onContentMessage?

getContentMessage => this.getContentMessage.bind(this) or any other way to properly deal with |this| inside getContentMessage method.

@@ +144,2 @@
>        });
>      });

Remove extra pair of brackets.

@@ +262,5 @@
>  
>    // Chrome <-> Content communication
>  
> +  getContentMessage:  function(e) {
> +    if (typeof e.detail.val != 'string') {

Nit: what about renaming 'val' field to (a bit more clear) 'value'?

::: devtools/client/jsonview/json-viewer.js
@@ +43,5 @@
>    input.actions = {
>      onCopyJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      value = JSON.stringify(value);
> +      let contentMessageEvent = new CustomEvent('contentMessage', [detail:{type:"copy",val:value}]);

The second argument is supposed to be an object with field 'detail', right?
Not an array...

@@ +56,4 @@
>      },
>  
>      onCopyHeaders: function () {
> +      let value = JSON.stringify(value);

You should use `input.headers` to get the headers.

@@ +56,5 @@
>      },
>  
>      onCopyHeaders: function () {
> +      let value = JSON.stringify(value);
> +      let contentMessageEvent = new CustomEvent('contentMessage', [detail:{type:"copy-headers,val:value}]);

missing quotation mark.


Also, the source line is too long.
Posted patch jsonview-WIP-2 (obsolete) — Splinter Review
Something like this Honza?
Attachment #8812986 - Attachment is obsolete: true
Alright locally this looks broken but I'll wait on someone else.  I have the source building now, but I believe not properly setup.

*stares down local javascript files*
Comment on attachment 8813274 [details] [diff] [review]
jsonview-WIP-2

Review of attachment 8813274 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jerri Rice (rehash pending) from comment #19)
> Alright locally this looks broken but I'll wait on someone else.  I have the
> source building now, but I believe not properly setup.
Check out my inline comments to see why it's broken.

This patch seems to be almost ready.
As soon as you resolve my comments we can ask for sec review.

Thanks for working on this!

Honza

::: devtools/client/jsonview/converter-child.js
@@ +140,4 @@
>      JsonViewUtils.exportIntoContentScope(win, Locale, "Locale");
>  
>      Events.once(win, "DOMContentLoaded", event => {
> +      win.addEventListener("contentMessage", this.onContentMessage.bind(this), false, true);

The line is too long. Maximum length of a line is 90 characters.

@@ +261,5 @@
>  
>    // Chrome <-> Content communication
>  
> +  onContentMessage:  function(e) {
> +   

Remove an empty line

::: devtools/client/jsonview/json-viewer.js
@@ +43,5 @@
>    input.actions = {
>      onCopyJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage', {detail:{type:"copy",value:value}});
> +      window.dispatchEvent(contentMessageEvent);

The line is too long. Maximum length of a line is 90 characters.

Also use proper spacing:

new CustomEvent('contentMessage', {detail: {type: "copy", value: value}});

Or you can do:

let data = {
  detail: {
    type: "copy",
    value: value
  }
}

new CustomEvent('contentMessage', data);

@@ +48,5 @@
>      },
>  
>      onSaveJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage', {detail:{type:"save",value:value}});

The line is too long. Maximum length of a line is 90 characters.

@@ +54,5 @@
>      },
>  
>      onCopyHeaders: function () {
> +      let value = input.headers;
> +      let contentMessageEvent = new CustomEvent('contentMessage', \

Remove the '\' characters at the end of the line.

This is breaking the JSON view entirely.
Posted patch json-viewer-3.patch (obsolete) — Splinter Review
Here try this.
Attachment #8813274 - Attachment is obsolete: true
Comment on attachment 8813786 [details] [diff] [review]
json-viewer-3.patch

Review of attachment 8813786 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my inline comments

Note that some review comments from my previous review were not resolved.

Also, please set properly the review flag for me so, I can see it in my review queue
(and it isn't forgotten)

Thanks!

Honza

::: devtools/client/jsonview/converter-child.js
@@ +140,4 @@
>      JsonViewUtils.exportIntoContentScope(win, Locale, "Locale");
>  
>      Events.once(win, "DOMContentLoaded", event => {
> +      win.addEventListener("contentMessage", this.onContentMessage.bind(this), false, true);

This line is too long (over 90 chars) please split.

@@ +261,5 @@
>  
>    // Chrome <-> Content communication
>  
> +  onContentMessage:  function(e) {
> +   

Please remove this empty line

::: devtools/client/jsonview/json-viewer.js
@@ +43,4 @@
>    input.actions = {
>      onCopyJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage', {detail:{type:"copy",value:value}});

This line is too long, please split.

@@ +48,5 @@
>      },
>  
>      onSaveJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage', {detail:{type:"save",value:value}});

This line is too long, please split.

Also, see my previous review comment about spacing.

@@ +54,5 @@
>      },
>  
>      onCopyHeaders: function () {
> +      let value = input.headers;
> +      let contentMessageEvent = new CustomEvent('contentMessage', 

Remove empty space at the end of the line
Posted patch json-view-4.patch (obsolete) — Splinter Review
Here, I think that's it.  I tested the functionality a few minutes ago.  With those small changes of spacing I think it's ready.

Thanks Honza
Attachment #8813786 - Attachment is obsolete: true
Attachment #8814197 - Flags: review?(odvarko)
After this patch, running debug mode when the json-viewer content renders on screen the console window lights up with this error.

[Child 3176] ###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file c:
/Users/[redacted]/mozilla-central/dom/events/EventDispatcher.cpp, line 673

I'm going to file a follow up bug and leave the number here so we can discuss if this is actually a security issue or if the assertion is misleading considering the security measures here.

Maybe the event listener needs to check the event source, but I feel that too can be done in follow up bug.  Will post the follow up bug here in a second and leave the bug number here and we can go from there.
That bug to follow up bug is bug 1320228.  I know it's Thanksgiving, but I haven't had much of a chance to just get work done recently so I actually took tonight to touch on things some.  You guys enjoy the holiday time though :)
Comment on attachment 8814197 [details] [diff] [review]
json-view-4.patch

Review of attachment 8814197 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good to me, there are just some
minor coding-style issues that should be fixed
before r+.

Thanks,
Honza

::: devtools/client/jsonview/converter-child.js
@@ +140,5 @@
>      JsonViewUtils.exportIntoContentScope(win, Locale, "Locale");
>  
>      Events.once(win, "DOMContentLoaded", event => {
> +      win.addEventListener("contentMessage",
> +      this.onContentMessage.bind(this), false, true);

Please use indentation as follows:

Events.once(win, "DOMContentLoaded", event => {
  win.addEventListener("contentMessage",
    this.onContentMessage.bind(this), false, true);
});


You might also see our code-style guidelines:
https://wiki.mozilla.org/DevTools/CodingStandards

::: devtools/client/jsonview/json-viewer.js
@@ +44,5 @@
>      onCopyJson: function () {
>        let value = input.prettified ? input.jsonPretty : input.jsonText;
> +      let contentMessageEvent = new CustomEvent('contentMessage',
> +        {detail:{type:"copy",value:value}});
> +      window.dispatchEvent(contentMessageEvent);

The spacing is still not resolved, can you please do it?
(see my previous review comments)

Honza
Attachment #8814197 - Flags: review?(odvarko)

Updated

2 years ago
Depends on: 1320228
Jerri, can you please CC me on the bug 1320228? I don't have an access to it.
Thanks
Honza
Done very strange since I was sure I added you at first.  Also I used to have the spacing guidelines down, but I usually don't contribute code here, just break it :)
Jan, could you possibly fix the spacing problems?  I'm not going to have the bandwidth or the system it would take to do it properly since I'm kind of with family for right now.  I also don't think this should be held back any longer due to some standards like ISO coding standards holding it back.

Thanks I hope, if not I can do it soon.  The functionality is there though :-)
I'm on very limited data right now, but if I have to I'll get the proper text editors and such setup.  I've been doing this with the rawest of tools so far =/
Posted patch jsonview-proper.patch (obsolete) — Splinter Review
I didn't go back over the guidelines, but IIRC something like this is more what you're wanting right.

Sorry, I tend to be a little rough around the edges, but I think this should serve you well for review.
Attachment #8814197 - Attachment is obsolete: true
Attachment #8814702 - Flags: review?(odvarko)
Posted patch jsonview-proper-2.patch (obsolete) — Splinter Review
Here, I'm working from two bases for this patch and that one still had an unneeded blank line.

I also have an alternate version of this patch with spacing the only other way I think it could make sense if this one isn't right.  Just let me know, killing my data setting these tools up though =/
Attachment #8814702 - Attachment is obsolete: true
Attachment #8814702 - Flags: review?(odvarko)
Attachment #8814705 - Flags: review?(odvarko)
No longer depends on: 1320228
Posted patch jsonview-5.patch (obsolete) — Splinter Review
Here I believe the spacing is pretty spot on.  I'm a little low on data now though and kind of in the middle of nowhere lol.

Thank for looking this over Honza.  I think it takes care of all of our concerns about exporting postChromeMessage.

Happy Holidays and cheers all.
Attachment #8814705 - Attachment is obsolete: true
Attachment #8814705 - Flags: review?(odvarko)
Attachment #8815006 - Flags: review?(odvarko)
Posted patch jsonview-6.patch (obsolete) — Splinter Review
Patch updated, spacing and Eslint warnings fixed.

Gijs, can you please review this, thanks!

Honza
Attachment #8815006 - Attachment is obsolete: true
Attachment #8815006 - Flags: review?(odvarko)
Attachment #8815683 - Flags: review?(gijskruitbosch+bugs)

Comment 36

2 years ago
Comment on attachment 8815683 [details] [diff] [review]
jsonview-6.patch

Review of attachment 8815683 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/jsonview/converter-child.js
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const {Cc, Ci, components} = require("chrome");

Personally, I'd leave the "Cu" in, but if eslint complains I guess we can do this...

@@ +261,5 @@
>    },
>  
>    // Chrome <-> Content communication
>  
> +  onContentMessage: function (e) {

Should we verify that we're still on a JSON viewer document somehow, to be very safe? Note also e.g. bug 1044586 comment 13 and further. We should be very sure that if you e.g.:

myWin = window.open("some json", "_blank")
myWin.location.href = "my evil code that fires these custom events so that the converter child will then do stuff with chrome"

wouldn't work.

::: devtools/client/jsonview/json-viewer.js
@@ +70,5 @@
> +          type: "copy-headers",
> +          value: value
> +        }
> +      });
> +      window.dispatchEvent(contentMessageEvent);

We're duplicating this 3 times - we could create a helper to make this cleaner?

Either in the helper or all 3 times, please leave a trailing comma after the value prop, and just use the ES6 shorthand instead of duplicating "value: value"
Attachment #8815683 - Flags: review?(gijskruitbosch+bugs) → feedback+
Posted patch bug1297361.patch (obsolete) — Splinter Review
(In reply to :Gijs Kruitbosch from comment #36)
> Comment on attachment 8815683 [details] [diff] [review]
> jsonview-6.patch
> 
> Review of attachment 8815683 [details] [diff] [review]:
> Personally, I'd leave the "Cu" in, but if eslint complains I guess we can do
> this...
Yeah, eslint was complaining.

> 
> @@ +261,5 @@
> >    },
> >  
> >    // Chrome <-> Content communication
> >  
> > +  onContentMessage: function (e) {
> 
> Should we verify that we're still on a JSON viewer document somehow, to be
> very safe? Note also e.g. bug 1044586 comment 13 and further. We should be
> very sure that if you e.g.:
Done

> We're duplicating this 3 times - we could create a helper to make this
> cleaner?
Done

> Either in the helper or all 3 times, please leave a trailing comma after the
> value prop, and just use the ES6 shorthand instead of duplicating "value:
> value"
Fixed


Thanks!

Honza
Attachment #8815683 - Attachment is obsolete: true
Attachment #8815695 - Flags: feedback?(gijskruitbosch+bugs)

Comment 38

2 years ago
Comment on attachment 8815695 [details] [diff] [review]
bug1297361.patch

Review of attachment 8815695 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/jsonview/json-viewer.js
@@ +54,5 @@
> +      dispatchEvent({
> +        detail: {
> +          type: "save",
> +          value: input.prettified ? input.jsonPretty : input.jsonText,
> +        }

I'd push this into the helper, passing the helper values for "type" and "value", and then just calling it with 2 args to make 'type' and 'value' happen.
Attachment #8815695 - Flags: feedback?(gijskruitbosch+bugs) → review+
Posted patch bug1297361.patch (obsolete) — Splinter Review
(In reply to :Gijs Kruitbosch from comment #38)
> Comment on attachment 8815695 [details] [diff] [review]
> bug1297361.patch
> 
> Review of attachment 8815695 [details] [diff] [review]:
> -----------------------------------------------------------------
> I'd push this into the helper, passing the helper values for "type" and
> "value", and then just calling it with 2 args to make 'type' and 'value'
> happen.
Done

Thanks for the review!
Reusing R+ 

Honza
Attachment #8815695 - Attachment is obsolete: true
Attachment #8815779 - Flags: review+
Comment on attachment 8815779 [details] [diff] [review]
bug1297361.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This is hard to estimate, but this comment can help:
https://bugzilla.mozilla.org/show_bug.cgi?id=1295309#c24

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
Target Milestone for JSON viewer (introducing the original implementation) was Firefox 44. I am not sure about affected branches.

If not all supported branches, which bug introduced the flaw?
The JSON Viewer has been introduced in Bug 1132203

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
They are no backports. They could be probably the same and representing low risk.

How likely is this patch to cause regressions; how much testing does it need?
Low risk. All affected JSON Viewer features have been manually tested and they are also covered by automated tests.

Honza
Attachment #8815779 - Flags: sec-approval?
Comment on attachment 8815779 [details] [diff] [review]
bug1297361.patch

sec-approval+ for trunk.

Once it is on trunk, please nominate branch patches for Aurora, Beta, and ESR45 as well so we can fix it in affected branches.
Attachment #8815779 - Flags: sec-approval? → sec-approval+
backed out for es lint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=40306741&repo=mozilla-inbound
Flags: needinfo?(odvarko)
I'll try to fix these failures, getting a nice text editor with an es lint plugin setup in windows right now.  Don't let me step on your toes here though Honza.

Will be in touch, although I'm not sure I have try server access ;-)
Posted patch bug1297361-2.patch (obsolete) — Splinter Review
Here is a patch to apply after the above patch which should take care of es lint warnings.  At least I hope, tried to redo the whole thing only to realize the previous patch has landed =/

Hope this helps guys, sorry if this makes it more of a mess.
It looks as though the es lint problems were solved, but for right now I do see one mochi devtools chrome test failing.

Can someone else take a look and see what they think here?  hmm

Comment 48

2 years ago
(In reply to Jerri Rice (rehash pending) from comment #47)
> It looks as though the es lint problems were solved, but for right now I do
> see one mochi devtools chrome test failing.
> 
> Can someone else take a look and see what they think here?  hmm

The retriggers were green and the test seems unrelated, so I don't expect it's an issue to do with this patch.
That sounds good to me, Honza can you verify we're good here and mark resolved?  Thanks.

Comment 50

2 years ago
(In reply to Jerri Rice (rehash pending) from comment #49)
> That sounds good to me, Honza can you verify we're good here and mark
> resolved?  Thanks.

Bugs are only marked resolved when they get merged to m-c.
The original patch updated (including changes from Jerri - fixing eslint warnings).

Let's try to land again.

Thanks Jerri & Gijs!

Honza
Attachment #8815779 - Attachment is obsolete: true
Attachment #8816988 - Attachment is obsolete: true
Attachment #8817070 - Flags: review+

Comment 52

2 years ago
This landed already (comment 46).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d7431e24546
https://hg.mozilla.org/mozilla-central/rev/26025c7f0d29

this should have needed sec-approval right ?
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(odvarko)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I can't speak for Honza and I know that there is a protocol, but nothing in the patch was modified in any real way.

A comment was removed, and a space before a parentheses was removed.

I'll let him follow up.
ah right seems just the flag was lost :)

Comment 56

2 years ago
Leaving the ni for Honza to file the uplift requests.
Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

Re-requesting sec-approval (already approved in comment #41).

@abillings: the patch has been updated to fix eslint warnings. A comment was removed, and a space before a parentheses was removed.

Honza

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This is hard to estimate, but this comment can help:
https://bugzilla.mozilla.org/show_bug.cgi?id=1295309#c24

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
Target Milestone for JSON viewer (introducing the original implementation) was Firefox 44. I am not sure about affected branches.

If not all supported branches, which bug introduced the flaw?
The JSON Viewer has been introduced in Bug 1132203

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
They are no backports. They could be probably the same and representing low risk.

How likely is this patch to cause regressions; how much testing does it need?
Low risk. All affected JSON Viewer features have been manually tested and they are also covered by automated tests.

Honza
Flags: needinfo?(odvarko) → needinfo?(abillings)
Attachment #8817070 - Flags: sec-approval?

Comment 58

2 years ago
Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

This doesn't need new sec approval - it already landed and made it to nightly. It needs uplift requests for aurora and beta. Honza, can you fill those out?
Flags: needinfo?(odvarko)
Flags: needinfo?(abillings)
Attachment #8817070 - Flags: sec-approval?
Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1132203
[User impact if declined]: No UI impact
[Is this code covered by automated tests? Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: Very low risk
[Why is the change risky/not risky?]: Relate UI features covered by tests 
[String changes made/needed]: n/a
Flags: needinfo?(odvarko)
Attachment #8817070 - Flags: approval-mozilla-aurora?
Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1132203
[User impact if declined]: No UI impact
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: Very low risk
[Why is the change risky/not risky?]: Related UI features covered by tests 
[String changes made/needed]: n/a
Attachment #8817070 - Flags: approval-mozilla-beta?
Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: No UI changes
Fix Landed on Version: Nightly
Risk to taking this patch (and alternatives if risky): Very low risk
String or UUID changes made by this patch: n/a
Attachment #8817070 - Flags: approval-mozilla-esr45?
Can this wait for 51 and 45.7.0esr in January?
I think this should wait. I'd like to start the ESR build tonight.

Comment 64

2 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #62)
> Can this wait for 51 and 45.7.0esr in January?

Yes.
Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 7.
Attachment #8817070 - Flags: approval-mozilla-beta?
Attachment #8817070 - Flags: approval-mozilla-beta+
Attachment #8817070 - Flags: approval-mozilla-aurora?
Attachment #8817070 - Flags: approval-mozilla-aurora+
I'm glad to see this one about wrapped up.  Bugs setting for multiple months is kind of new, I'll try to start bringing the patch with the work ;-)
Group: firefox-core-security → core-security-release
Blocks: 1132203
Flags: sec-bounty? → sec-bounty+
Comment on attachment 8817070 [details] [diff] [review]
bug1297361.patch

let's take that in esr too.
Attachment #8817070 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Needs rebasing for ESR45.
Flags: needinfo?(odvarko)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #71)
> Needs rebasing for ESR45.

Patch for ESR45 attached.

Honza
Flags: needinfo?(odvarko)
Attachment #8823106 - Flags: review+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Alias: CVE-2017-5390
I believe there may be a bit of a blunder here.  While trying to test for a stacktrace for bug 1330173 I thought wouldn't it be interesting to try to load the json viewer in a tab.

This still works for top level windows, and a point I believe was missed originally was that if you load it into a top level window, that window remains fully accessible from the content window that opened it.

I'll probably make another bug to handle that, but in everything that happened here I did forget to continue to stress that one point.

So if the patches here don't check for trusted events(will look it all over again in a second) the fact that the window is accessible could still be a hole.

Man this is kind of a mess, because I'm on limited time and I keep finding more things to dig into.  I don't want to leave any of them hanging.  Will follow up after dedicated a few hours to bug 1330173.
Ok yeah, the patch here is lacking.  If someone with the proper bugzilla rights can strike my previous comment and possibly even this one, this can go ahead and go public and we can handle the window being accessible in another bug.

Sorry guys I slipped a little on this one.  I remember mentioning it in the bug that this bug was spawned from but I forgot to continue to stress that key point.

Now I know how Gijs has felt about some of those regressions.  Sorry man, but when we all work together we can find things like this much easier :-)
See bug 1333210 for the follow up from here.

Updated

2 years ago
Depends on: 1333210
Group: core-security-release

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.