Closed Bug 853453 Opened 7 years ago Closed 6 years ago

[Security Review] New PanelUI and toolbar customization

Categories

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

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: freddyb, Assigned: dchanm+bugzilla)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Fx][Australis:M9][Australis:P1])

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
Whiteboard: [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd] → [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][Fx]
No longer blocks: 770135
Whiteboard: [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][Fx] → [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][Fx][Australis:M7]
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?
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..
Flags: needinfo?(fbraun)
(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.
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][Fx][Australis:M7] → [Australis:M?]
Whiteboard: [Australis:M?] → [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][Fx][Australis:M?]
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/.
Whiteboard: [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][Fx][Australis:M?] → [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][Fx][Australis:M?][Australis:P1]
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.)
Since this adds another content area chrome page, I'd like to reiterate my concern about bug 664556.
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.
Flags: needinfo?(jruderman)
Bug 664556 doesn't need testing at this point, just fixing ;)

I'm not familiar with drag-and-drop either.
Flags: needinfo?(jruderman)
Jesse/Frederik, what is the next step here?
Flags: needinfo?(jruderman)
Flags: needinfo?(fbraun)
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?
Flags: needinfo?(fbraun) → needinfo?(dchan+bugzilla)
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.
Flags: needinfo?(dchan+bugzilla)
Great! Thanks for looking in to this David.
Depends on: 910375
I think we can close this bug now. Curtis, any objections?
Flags: needinfo?(jruderman) → needinfo?(curtisk)
Assignee: fbraun → dchan+bugzilla
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(curtisk)
Resolution: --- → FIXED
Whiteboard: [pending secreview][start yyyy-mm-dd][target yyyy-mm-dd][Fx][Australis:M?][Australis:P1] → [Fx][Australis:M?][Australis:P1]
Whiteboard: [Fx][Australis:M?][Australis:P1] → [Fx][Australis:M9][Australis:P1]
Depends on: CVE-2014-1561
You need to log in before you can comment on or make changes to this bug.