Closed Bug 1078213 Opened 5 years ago Closed 5 years ago

Search bar binding shouldn't unsafely set IDs for menuitems

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 35
Iteration:
35.3
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
firefox-esr31 --- affected

People

(Reporter: Unfocused, Assigned: Unfocused)

Details

(Keywords: sec-low, Whiteboard: [adv-main34-])

Attachments

(1 file)

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)
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+
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.
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
Closed: 5 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+
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-moderatesec-low
Whiteboard: [adv-main34-]
You need to log in before you can comment on or make changes to this bug.