1300807 - Calling chrome.pageAction.show() crashes because PageActions WebExtensions API passes a non-UUID add-on id to Fennec's PageActions.jsm

Status: RESOLVED FIXED

(WebExtensions :: Android, defect, P2)

Description

[Wladimir Palant]

I've been trying the PageAction API on Android and entered the following line in the remote debugger:

> chrome.pageAction.show(0);

Passing in 0 as tab ID here because chrome.tabs API is unsupported so there is nothing to get a proper tab ID from - the number passed in doesn't seem to matter anyway. The browser crashes immediately, you can see my crash report under https://crash-stats.mozilla.com/report/index/c7cc38cf-667f-4d45-8955-92aa72160906:

java.lang.IllegalArgumentException: Invalid UUID: asypasswords@palant.
	at java.util.UUID.fromString(UUID.java:198)
	at org.mozilla.gecko.toolbar.PageActionLayout$PageAction.<init>(PageActionLayout.java:331)
	at org.mozilla.gecko.toolbar.PageActionLayout.access$000(PageActionLayout.java:35)
	at org.mozilla.gecko.toolbar.PageActionLayout$1.run(PageActionLayout.java:98)
	at android.os.Handler.handleCallback(Handler.java:733)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:136)
	at android.app.ActivityThread.main(ActivityThread.java:5021)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:515)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:827)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:643)
	at dalvik.system.NativeStart.main(Native Method)

Note that extension ID is [e-mail] in this case.

Comment 1

[Matthew Wein [:mattw]]

Attachment: Bug 1300807 - Switch to using a uuid for the PageAction ID.

Review commit: https://reviewboard.mozilla.org/r/78258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78258/

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8790455 [details]
Bug 1300807 - Switch to using a uuid for the PageAction ID.

https://reviewboard.mozilla.org/r/78258/#review76782

This needs to be reviewed by a Fennec peer.

Comment 3

[Richard Newman [:rnewman]]

Why don't you simply pass a UUID here instead of your addon's foo@bar ID?

All of this happens because you're passing {id: "[e-mail]", ...} to PageActions.jsm, here:

  add: function(aOptions) {
    let id = aOptions.id || uuidgen.generateUUID().toString()

    Messaging.sendRequest({
      type: "PageActions:Add",
      id: id,

Either pass a valid UUID, or just leave out the ID altogether and let one be generated for you.

This is presumably a UUID rather than a counter so that when an add-on registers a page action more than once, we can spot duplicates…

Comment 4

[Matthew Wein [:mattw]]

Thanks for hte feedback.

(In reply to Richard Newman [:rnewman] from comment #3)
> Why don't you simply pass a UUID here instead of your addon's foo@bar ID?
> 
> All of this happens because you're passing {id: "[e-mail]",
> ...} to PageActions.jsm, here:
> 
>   add: function(aOptions) {
>     let id = aOptions.id || uuidgen.generateUUID().toString()
> 
>     Messaging.sendRequest({
>       type: "PageActions:Add",
>       id: id,
> 
> Either pass a valid UUID, or just leave out the ID altogether and let one be
> generated for you.

That's a good point, I didn't see that it generates one if one isn't passed in.  I think not passing in the extension ID is the simplest solution.

> 
> This is presumably a UUID rather than a counter so that when an add-on
> registers a page action more than once, we can spot duplicates…

That makes sense.  This shouldn't be an issue for WebExtensions since the API only allows one PageAction per extension.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Matthew Wein [:K-9] from comment #4)
> Thanks for hte feedback.
> 
> (In reply to Richard Newman [:rnewman] from comment #3)
> > Why don't you simply pass a UUID here instead of your addon's foo@bar ID?
> > 
> > All of this happens because you're passing {id: "[e-mail]",
> > ...} to PageActions.jsm, here:
> > 
> >   add: function(aOptions) {
> >     let id = aOptions.id || uuidgen.generateUUID().toString()
> > 
> >     Messaging.sendRequest({
> >       type: "PageActions:Add",
> >       id: id,
> > 
> > Either pass a valid UUID, or just leave out the ID altogether and let one be
> > generated for you.
> 
> That's a good point, I didn't see that it generates one if one isn't passed
> in.  I think not passing in the extension ID is the simplest solution.

I still think we should generate a UUID from the extension ID.

Comment 6

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #5)
> I still think we should generate a UUID from the extension ID.

This leaves the (relatively small) problem that given an extension that uses the non-uuid form, a malicious developer could trivially compute the hash themselves and register an extension with the hash as the id which I think would let them either clobber or intercept the original extension's events?  Its not a huge problem since the attacker has to convince a user with the victim extension to also install the doppelganger extension, but it seems like something we should avoid.

I only point this out since there's a closely related issue over on bug 1271663.  If we decide we want to be able to use the uuid form everywhere, we could punt this problem over to AMO and hash non-uuid ids before doing the id collision check.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #6)
> (In reply to Kris Maglione [:kmag] from comment #5)
> > I still think we should generate a UUID from the extension ID.
> 
> This leaves the (relatively small) problem that given an extension that uses
> the non-uuid form, a malicious developer could trivially compute the hash
> themselves and register an extension with the hash as the id which I think
> would let them either clobber or intercept the original extension's events? 

If we use a broken hash algorithm, sure.

Comment 8

[Wladimir Palant]

(In reply to Richard Newman [:rnewman] from comment #3)
> Why don't you simply pass a UUID here instead of your addon's foo@bar ID?
> 
> All of this happens because you're passing {id: "[e-mail]",
> ...} to PageActions.jsm, here:

The parameters here are determined by framework code so you probably didn't mean to ask me - this is a WebExtensions bug and my extension cannot do anything about it.

> This leaves the (relatively small) problem that given an extension that uses
> the non-uuid form, a malicious developer could trivially compute the hash
> themselves and register an extension with the hash as the id which I think
> would let them either clobber or intercept the original extension's events?

That's assuming that the UUID derivation algorithm will only be applied to extension IDs that aren't in UUID form already. If the algorithm assumes the extension ID to be an unstructured blob and applies to it regardless then the only issue would be a hash collision - and that possibility can probably be safely ignored.

Then again, generating a random UUID might give you the same result with less effort. From what I can tell there is no reason why an extension's pageAction UUID would need to be deterministic.

Comment 9

[Matthew Wein [:mattw]]

I don't see an advantage in adding the extra code to generate a UUID from the extension ID over passing in a random one. If we let a new UUID get generated each time PageActions.add is called as mentioned in comment 7, then it would allow for duplicate PageActions to exist for the same extension.  However, if we pass in the same UUID each time, random or otherwise, the check for duplicates would still work.

Comment 10

[Matthew Wein [:mattw]]

(as mentioned in comment 3, not comment 7)

Comment 11

[Matthew Wein [:mattw]]

Comment on attachment 8790455 [details]
Bug 1300807 - Switch to using a uuid for the PageAction ID.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78258/diff/1-2/

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8790455 [details]
Bug 1300807 - Switch to using a uuid for the PageAction ID.

https://reviewboard.mozilla.org/r/78258/#review77092

::: mobile/android/components/extensions/ext-pageAction.js:39
(Diff revision 2)
>  
>    this.popupUrl = options.default_popup;
>  
>    this.options = {
>      title: options.default_title || extension.name,
> -    id: extension.id,
> +    id: uuidgen.generateUUID().toString(),

Consider storing these in a sibling of `pageActionMap` -- a `WeakMap` from `Extension` to UUID string. That at least gives us a consistent UUID for each extension.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8790455 [details]
Bug 1300807 - Switch to using a uuid for the PageAction ID.

https://reviewboard.mozilla.org/r/78258/#review77098

This should be uplifted to 49, where bug 1267124 landed.

::: mobile/android/components/extensions/ext-pageAction.js:39
(Diff revision 2)
>  
>    this.popupUrl = options.default_popup;
>  
>    this.options = {
>      title: options.default_title || extension.name,
> -    id: extension.id,
> +    id: uuidgen.generateUUID().toString(),

Let's just use `{${extension.uuid}}`

Comment 14

[Matthew Wein [:mattw]]

Comment on attachment 8790455 [details]
Bug 1300807 - Switch to using a uuid for the PageAction ID.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78258/diff/2-3/

Comment 15

[Matthew Wein [:mattw]]

Comment on attachment 8790455 [details]
Bug 1300807 - Switch to using a uuid for the PageAction ID.

Approval Request Comment
[Feature/regressing bug #]: 1267124
[User impact if declined]: The browser will crash if a user tries to upload an extension on Android that has ID which is not a valid UUID and calls browser.pageAction.show().
[Describe test coverage new/current, TreeHerder]: The main unit test for the PageAction API, test_ext_pageAction.html, has been modified to test this edge case.
[Risks and why]: Low risk because this is a small change and all unit tests pass.
[String/UUID change made/needed]:

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6b3befb87a2
Switch to using a uuid for the PageAction ID. r=kmag

Comment 17

[Gregory Szorc [:gps]]

I had to back this out for eslint bustage like https://queue.taskcluster.net/v1/task/DDcd_4r1RC-m-G5KAMDkPA/runs/0/artifacts/public%2Flogs%2Flive_backing.log.

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/100b44ed0e01
Backed out changeset e6b3befb87a2 for eslint bustage

Comment 19

[Phil Ringnalda (:philor)]

It also broke the test, https://treeherder.mozilla.org/logviewer.html#?job_id=3768555&repo=autoland

Comment 20

[Matthew Wein [:mattw]]

Comment on attachment 8790455 [details]
Bug 1300807 - Switch to using a uuid for the PageAction ID.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78258/diff/3-4/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/724fb38fce19
Switch to using a uuid for the PageAction ID. r=kmag

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/724fb38fce19

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Wladimir, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8790455 [details]
Bug 1300807 - Switch to using a uuid for the PageAction ID.

Recent regression, we need this on both Beta50 and Aurora51. Matt, fyi, in case the patch needs re-basing for aurora.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7a23223dee5

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ac2c88544d05