1771026 - content process value used to index to an object in addTags()

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Andrew McCreight [:mccr8]]

While auditing code for variants of part of the Pwn2Own exploit, I came across some code in Pocket that indexes into an object with a value provided by a content process (probably a privileged content process?). Fortunately, I don't think there's a security impact, but it would be good to switch this code to use a Map or something to avoid future issues. (I'm hiding this bug because it kind of explains how bug 1770137 works.)

var usedTags = usedTagsJSON ? JSON.parse(usedTagsJSON) : {};
...
  var tagToSave = tags[i].trim();
  var newUsedTagObject = {
    tag: tagToSave,
    timestamp: new Date().getTime(),
  };
  usedTags[tagToSave] = newUsedTagObject;

tags is a value passed in from the content process.

If tagToSave is the string "__proto__", then the prototype of usedTags will get set to the Object newUsedTagObject instead of adding a new property. The only thing that is done with usedTags is that it is passed to JSON.stringify(), and in my local experimentation this seems to have no effect on it.

I believe that the only real impact is that you can't tag a Pocket article with "__proto__", but I haven't actually tried it.

Comment 1

[:Gijs (he/him)]

I'm not sure what to do here. The JSON serialization to/from settings already kind of assumes objects rather than Map, and to get the thing back into object form for serialization you'd run into the same problem... Using map.entries() would get you an array of arrays, which avoids it, but would require a data migration step for users with "old" usedTags data in their profile. Would we want to go with the second option or is there a more obvious fix we can apply here?

Comment 2

[Andrew McCreight [:mccr8]]

This sounds a bit like the situation in bug 1770137. Could we null out the proto, and copy over the values from the JSON result into a new object like they do in filterNonAppNotifications?

Comment 3

[Andrew McCreight [:mccr8]]

I guess that doesn't actually fix the issue of setting a tag of __proto__.

Comment 4

[Scott [:thecount] Downe]

So two things that come to mind.

First, for what it's worth looking at the code for where we fetch usedTags from settings, and then following it along through PKT_getTags, to the 2 places it is used (here and here), I think it may be dead code, and can maybe just be removed instead of fixed.

What I think usedTags used to be doing, and why it's now dead, is because there are two places where we store tags. "extensions.pocket.settings.tags" and "extensions.pocket.settings.usedTags".
The tags setting looks to be just an array of tags, returned from the server after the user saves something, of all the tags a user has used.
The usedTags setting looks a bit more complex where it also stores the time in was tagged, but only tags used locally while in firefox, and not from the server.
I suspect we used to order the tags so the most recently used tag was displayed first, but we now use a datalist, and instead rely on a text based search implementation that probably is more useful than time based sorting, hence why it's not used anymore.
I am unsure if the potentially newer tags, datalist implementation, and setting also shares concern with bug 1770137.

Second, looking at getSetting it looks like it ideally should not be stored in a pref, and ultimately should be migrated to some sort of local storage. Not familiar with how bug 1770137 worked, so not sure if we moved this to a proper local storage instead of a pref if it would also fix the issue, but if so might be worth exploring fixing it that way.

Comment 5

[Kris Maglione [:kmag]]

I'm currently thinking that we should maybe add a chrome-only JSON parser that creates objects with null prototypes by default, since that's come up twice already since investigating this bug...

Comment 6

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #5)

I'm currently thinking that we should maybe add a chrome-only JSON parser that creates objects with null prototypes by default, since that's come up twice already since investigating this bug...

It wouldn't be enough here as the fallback for null JSON content is an object literal that would continue to have the same problem. In the original case, the __proto__ string was also separate from the JSON / object content. So I don't think the effort of doing what you suggest would be worth it - if we made it opt-in, engineers would not realize they needed to use it, but even if it was "magically" applied to privileged JSON.parse calls, the fallback cases (as here) would probably be written as {} rather than Object.create(null), which would bypass the fix as well. Plus we're looking at just getting rid of the ability to modify Object.__proto__ for chrome scripts, which I think renders this avenue of attack ineffective as well?

Based on comment #4 I think we should just remove this code. Scott, is that something you or someone on your team has time to work on?

Comment 7

[Kris Maglione [:kmag]]

(In reply to :Gijs (he/him) from comment #6)

(In reply to Kris Maglione [:kmag] from comment #5)

I'm currently thinking that we should maybe add a chrome-only JSON parser that creates objects with null prototypes by default, since that's come up twice already since investigating this bug...

It wouldn't be enough here as the fallback for null JSON content is an object literal that would continue to have the same problem.

It wouldn't, but it would still prevent potential future issues if we continued to use these objects as dictionaries after parsed from JSON.

In the original case, the __proto__ string was also separate from the JSON / object content.

Well, in one bug, we thought we'd fixed it, but then had a case where it wasn't fixed because we serialized to JSON and then parsed.

So I don't think the effort of doing what you suggest would be worth it - if we made it opt-in, engineers would not realize they needed to use it, but even if it was "magically" applied to privileged JSON.parse calls

I was thinking it would be a separate API, and we'd add linting to warn against JSON.parse

the fallback cases (as here) would probably be written as {} rather than Object.create(null), which would bypass the fix as well.

Yes, we'd have to lint for that too.

Plus we're looking at just getting rid of the ability to modify Object.__proto__ for chrome scripts, which I think renders this avenue of attack ineffective as well?

Yes, but there are still other potential prototype issues when using objects as dictionaries. Passing an arbitrary key could still lead us to access a property on a prototype we weren't intending to, which could have arbitrary consequences.

I'm not thinking of this as a full fix, just as a defense in depth measure, since it's come up more than once while investigating this bug.

Comment 8

[Scott [:thecount] Downe]

(In reply to :Gijs (he/him) from comment #6)

Based on comment #4 I think we should just remove this code. Scott, is that something you or someone on your team has time to work on?

I can find time today.

Comment 9

[Scott [:thecount] Downe]

I have a fix locally. It is pretty small.

But I think I need guidance, as I am not sure what is the best process in this case for pushing a security related fix to phabricator.

Do I go the obscure route and file a new bug with this title "Remove dead tagging code in Pocket extension" and push to phabricator and never mention this bug or that it is security related. This would mean the new bug and phabricator is public to not draw attention to it. This would be my guess.

Or do I go the private route, and once the code lands users can see the changes and see the fact that it was security related, but not know any details.

Is there another option?

Comment 10

[Andrew McCreight [:mccr8]]

Phabricator patches are hidden by default. I'd just attach a patch to this bug with a description like "Remove dead tagging code in Pocket extension". This bug is sec-other, so it doesn't need to go through the security approval process. In any event, the patch won't reveal any more information than the patches for bug 1770137 did, and those have been public for about a week.

Comment 11

[Scott [:thecount] Downe]

(In reply to Andrew McCreight [:mccr8] from comment #10)

Phabricator patches are hidden by default. I'd just attach a patch to this bug with a description like "Remove dead tagging code in Pocket extension". This bug is sec-other, so it doesn't need to go through the security approval process. In any event, the patch won't reveal any more information than the patches for bug 1770137 did, and those have been public for about a week.

Thanks, that makes sense, I'll do that.

Comment 12

[Scott [:thecount] Downe]

Attachment: Bug 1771026 - Remove some dead tagging code in Pocket panels

Comment 13

[Scott [:thecount] Downe]

No rush on my end, but just an fyi this is ready to land, but doesn't look like I have permission to land a sec-other patch.

Comment 14

[Andrew McCreight [:mccr8]]

I've queued it for landing. At least for me, I just needed to check the box saying that I was following the sec process. I don't know if that requires extra permissions or not.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Remove some dead tagging code in Pocket panels r=Gijs
https://hg.mozilla.org/integration/autoland/rev/79125ed9e0ff09ab444749009a0b49b91322a1de
https://hg.mozilla.org/mozilla-central/rev/79125ed9e0ff

Comment 16

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #14)

I've queued it for landing. At least for me, I just needed to check the box saying that I was following the sec process. I don't know if that requires extra permissions or not.

I think the other thing you need is an API key from phabricator. The process is easy enough once you know how to do it, but it's not immediately obvious if you don't. I'll ask about docs for this on slack; I'm not sure we have any.

Comment 17

[:Gijs (he/him)]

FWIW, Scott, assuming I guessed correctly, https://moz-conduit.readthedocs.io/en/latest/lando-user.html#viewing-a-confidential-revision may be of interest. :-)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta & ESR91 approval when you get a chance.

Comment 19

[Scott [:thecount] Downe]

Comment on attachment 9278336 [details]
Bug 1771026 - Remove some dead tagging code in Pocket panels

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version: 103
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Removing dead code

Beta/Release Uplift Approval Request

• User impact if declined:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Ensure tagging auto complete works with Pocket saved panel.

1. Log into Pocket.
2. Save something to Pocket using the Pocket button.
3. Add a tag to that item.
   Expected: If you've saved a tag before, you should see that tag appear in a dropdown autocomplete if you type a similar tag. If you don't have a similar tag, proceed to steps 4 and 5.
4. Repeat step 2.
5. Try to add the same tag as in step 3, that tag should now auto complete.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Removing dead code
• String changes made/needed: None
• Is Android affected?: No

Comment 20

[Pascal Chevrel:pascalc]

AFAIK, this landed during the 102 cycle, so this is already in beta after the merge?

Comment 21

[Scott [:thecount] Downe]

@pascal, Yeah I think you're right.

According to the tracking flags in this bug it landed in 103, but according to searchfox it is in Beta, I think probably the tracking flags were just a bit off is all, and that we probably got it into 102 just in time.

Would that impact the esr91, and is simply closing the beta request enough?

Comment 22

[Pascal Chevrel:pascalc]

I updated the flag and cancelled the beta request.

Comment 23

[Simona Rosu [:srosu], Ecosystem QA]

I‘ve verified this issue using the latest Firefox Nightly 103.0a1 (Build ID: 20220603093350) and Firefox Beta 102.0b3 (Build ID: 20220602190016) on Windows 10 x64, macOS 12.3.1, and Ubuntu 20.04 x64.

• An existing tag appears in a dropdown autocomplete when start typing its first characters.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9278336 [details]
Bug 1771026 - Remove some dead tagging code in Pocket panels

Approved for 91.11esr.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/12db647f1127

Comment 26

[Simona Rosu [:srosu], Ecosystem QA]

I‘ve verified this issue using Firefox ESR 91.11.0 (Build ID: 20220607191431) build downloaded from treeherder, on Windows 10 x64, macOS 12.3.1, and Linux Mint 20.2 x64.

• An existing tag appears in a dropdown autocomplete when start typing its first characters.