1078213 - Search bar binding shouldn't unsafely set IDs for menuitems

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Fixing this now.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=346f4d3e8b5c

Comment 3

[Matthew N. [:MattN]]

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.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/0554ceb29cdc

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Sorry but this is too late for 33.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

This is a simple patch. Is there a reason that it hasn't landed on m-c yet?

Comment 8

[Matthew N. [:MattN]]

(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.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/20489053659f

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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!