853453 - [Security Review] New PanelUI and toolbar customization

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance: Review Request, task, P3)

Description

[Frederik Braun [:freddy]]

Risk/Priority Ranking Exercise https://wiki.mozilla.org/Security/RiskRatings

Priority: 3 (P3) - Overall Mozilla Quarterly Goal

Operational: 0 - N/A
User: 4 - Critical
Privacy: 0 - N/A
Engineering: 3 - Major
Reputational: 4 - Critical

Priority Score: 44

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Australis is planning on landing in m-c near the start of the Firefox 25 cycle (which merges on June 24th).

Does this block that landing? If so, is there anything we need to do to push this along?

Comment 2

[Frederik Braun [:freddy]]

I had the impression that the developement was not ready for testing yet. I'm also a bit worried that the original sec review for the toolbar customization is now blocking the whole australis bug. This surely does not mean that the review's scope has changed, does it?

I did some preliminary testing in March/April and will continue from there..

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Frederik Braun [:freddyb] from comment #2)
> I had the impression that the developement was not ready for testing yet.

There are still a few things that are not implemented, and still bugs to fix, but it's starting to stabilize. It is certainly usable for everyday browsing.

> I'm also a bit worried that the original sec review for the toolbar
> customization is now blocking the whole australis bug. This surely does not
> mean that the review's scope has changed, does it?
> 

It's only blocking the whole Australis bug because the toolbar customization is (a major) part of the project. The other part of the project is the restyling of the tabstrip. That's not in scope for this bug.

> I did some preliminary testing in March/April and will continue from there..

Ok, sounds good. UX Nightly builds are what I'd suggest to use.

Thanks - and let me know if there are further questions.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 5

[Justin Dolske [:Dolske]]

Calling this P1, but I don't think there's really much meat here for a secreview since it's all chrome UI.

Nightly UX builds are available at http://people.mozilla.org/~jwein/ux-nightly/.

Comment 6

[Jesse Ruderman]

Could a malicious page craft a drag item that does something evil when dropped into the toolbar, menu panel, or customization palette?

Conversely, can a page gather any information by seeing what happens when a toolbar item is dragged into it?

(Both of these scenarios involve having about:customizing open in one tab and a malicious page in another.)

Comment 7

[Jesse Ruderman]

Since this adds another content area chrome page, I'd like to reiterate my concern about bug 664556.

Comment 8

[Frederik Braun [:freddy]]

I had the impression that there is indeed little to test.
I looked into the way drag & drop is being handled (using the Browser Debugger feature from DevTools) and tried dropping other things than existing XUL elements into the customize window, but with little success.

As a sidenote, I have little to no experience with drag&drop handlers and possibly evil XUL elements.

Jesse, I took a look at bug 664556, but didn't really understand the issue you were trying to point out. Can you help finishing the rest of this review or explain a testing mechanism in a few clear steps?

I don't mind going along with what you will propose. Otherwise, you can just assign it to yourself and do additional chrome-to-content navigation and drag&drop testing. Whichever you prefer.

Comment 9

[Jesse Ruderman]

Bug 664556 doesn't need testing at this point, just fixing ;)

I'm not familiar with drag-and-drop either.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Jesse/Frederik, what is the next step here?

Comment 11

[Frederik Braun [:freddy]]

Sorry it got stuck in the queue.
I felt a bit uncomfortable about marking this done without understanding the drag&drop parts.

I asked dchan if it would make sense for him to take a quick look to. Did you already, david?

Comment 12

[David Chan [:dchan]]

There doesn't appear to be any sensitive information leaked from chrome to content. If a user drags one of the customizable UI pieces into a content window, they will get some metadata of the form

const kDragDataTypePrefix = "text/toolbarwrapper-id/";
...
dt.mozSetDataAt(kDragDataTypePrefix + documentId, draggedItem.id, 0);

where documentId is the window id and draggedItem.id is the button id. All the IDs are in the source already.

I did discover that it is possible to "spoof" events to the customizable UI from content to chrome. This still requires a drag and drop. Also the source of the dragevent has little influence over the customization aside from specifying an element to move. I will file another bug for that.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Great! Thanks for looking in to this David.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

I think we can close this bug now. Curtis, any objections?