Search bar binding shouldn't unsafely set IDs for menuitems

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Unfocused, Assigned: Unfocused)

Tracking

({sec-low})

Trunk
Firefox 35
sec-low
Points:
1
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox32 wontfix, firefox33 wontfix, firefox34 fixed, firefox35 fixed, firefox-esr31 affected)

Details

(Whiteboard: [adv-main34-])

Attachments

(1 attachment)

Seems the searchbar binding sets IDs for it's dropdown menuitems. Unfortunately, this is unsafe, as it uses the engine name as the ID value - which can come from content (although you have to explicitly choose to install the engine) and therefore can be anything. This can potentially cause ID conflicts in browser chrome, and potentially cause certain functionality to break... or worse.
Flags: qe-verify-
Flags: firefox-backlog+
Summary: Search bar binging shouldn't unsafely set IDs for menuitems → Search bar binding shouldn't unsafely set IDs for menuitems
Fixing this now.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
status-firefox32: --- → affected
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox-esr31: --- → affected

Updated

4 years ago
Iteration: --- → 35.3
Flags: needinfo?(mmucci)
Comment on attachment 8500366 [details] [diff] [review]
Patch v1

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

::: browser/components/search/content/search.xml
@@ -396,5 @@
>            for (var i = engines.length - 1; i >= 0; --i) {
>              var menuitem = document.createElementNS(kXULNS, "menuitem");
>              var name = engines[i].name;
>              menuitem.setAttribute("label", name);
> -            menuitem.setAttribute("id", name);

Another option would have been adding a prefix. Then you could still use it for bug 1068284.
Attachment #8500366 - Flags: review?(MattN+bmo) → review+
Keywords: sec-moderate
Comment on attachment 8500366 [details] [diff] [review]
Patch v1

Approval Request Comment
[Feature/regressing bug #]: Search bar
[User impact if declined]: Security fix
[Describe test coverage new/current, TBPL]: Try, see comment 2
[Risks and why]: Minimal - removes setting attributes that aren't used. Could possibly be used by an add-on, but unlikely (no way to check).
[String/UUID change made/needed]: None
Attachment #8500366 - Flags: approval-mozilla-beta?
Attachment #8500366 - Flags: approval-mozilla-aurora?
Sorry but this is too late for 33.
status-firefox32: affected → wontfix
status-firefox33: affected → wontfix
Attachment #8500366 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
This is a simple patch. Is there a reason that it hasn't landed on m-c yet?
Flags: needinfo?(bmcbride)
(In reply to Lawrence Mandel [:lmandel] from comment #7)
> This is a simple patch. Is there a reason that it hasn't landed on m-c yet?

It did in https://hg.mozilla.org/mozilla-central/rev/0554ceb29cdc but Tomcat didn't update the bug for whatever reason.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(bmcbride)
Resolution: --- → FIXED
Comment on attachment 8500366 [details] [diff] [review]
Patch v1

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #8)
> It did in https://hg.mozilla.org/mozilla-central/rev/0554ceb29cdc but Tomcat
> didn't update the bug for whatever reason.

That makes sense. Thanks for checking.

Given that this has already been on m-c for a couple of days, let's get it on Aurora for the last couple of days of this cycle. Aurora+
Attachment #8500366 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/20489053659f
status-firefox34: affected → fixed
status-firefox35: affected → fixed
Target Milestone: --- → Firefox 35
Attack is really limited to DoS, and requires installing the engine, so I think there's not too much value to keeping this hidden. Good to fix, though!
Group: core-security
Keywords: sec-moderate → sec-low
Whiteboard: [adv-main34-]
You need to log in before you can comment on or make changes to this bug.