959400 - [Notes] oAuth session forgery

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Notes, defect)

Description

[Dylan Oliver [:doliver]]

bug report received via email from Daniel DeFreez:

I am a PhD student in computer security at the University of California 
Davis. I am working on a project involving Firefox OS applications. One of 
the applications I tested was Notes Plus.

Notes Plus v2.1.1 is vulnerable an oAuth session forgery attack which can 
force saved notes to be sychronized with an attackers account. This is a 
result of two issues:

First, the app fails to validate the origin of message events during the 
authentication process. The message handler in evernote.js should validate 
the origin of the message containing the oAuth temporary token. The message 
handler could also be detached after it has been used.

Second, the app is vulnerable to HTML injection via the name of an Evernote 
note. The name of the note is used in two locations (at least), only one of 
which is filtered for html entities.

Daniel DeFreez

Comment 1

[Daniel Veditz [:dveditz]]

Paul: need a security rating on this ('high' for the app? 'moderate' for the phone as a whole?), and if you could track down who's responsible for fixing that would be great, too.

Who could send malicious messages to the app? IIRC 'app' permission is limited to certified apps so we might not be at risk in practice.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I believe Daniel is referring to an app in the marketplace https://marketplace.firefox.com/app/notesplus. At one time there was notes app in gaia I believe which may have been this, but not sure. (its not there now...)

I know Daniel so I'll reach out to get more information - I'm not sure of a rating without more information on the exact exploit scenario here.

Comment 3

[Dylan Oliver [:doliver]]

(In reply to Paul Theriault [:pauljt] from comment #2)
> I believe Daniel is referring to an app in the marketplace
> https://marketplace.firefox.com/app/notesplus. At one time there was notes
> app in gaia I believe which may have been this, but not sure. (its not there
> now...)

Correct. The source is here: https://github.com/mozilla-b2g/notes/tree/phase2

This app was originally written by the everything.me team but we have recently transitioned ownership of it to Mozilla (specifically to the Gaia Productivity team). 

It is a marketplace app but some partners have interest in pre-installing it on their builds.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I'm guessing this is what Daniel is referring to with first problem:

this.getAuthorization = function() {
        if (App.DEBUG) {
            Console.log('getAuthorization url: '+AUTHORIZATION_URL+'?oauth_token='+tmp_oauth_token);
        }
        authWindow = window.open(AUTHORIZATION_URL+'?oauth_token='+tmp_oauth_token);
        window.addEventListener('message', function onMessage(evt) {
            authWindow.close();
            tmp_oauth_token = evt.data.oauth_token;
            oauth_verifier = evt.data.oauth_verifier;

            self.getAccessToken();
        });
    };

However I'm wondering if the mozApp sandbox actually protects us here. This code should be verifying the origin of the message event here, but since you can't frame apps, unless you have the embed-apps permission, I am not sure there is actually a way for arbitrary content to actually send a message to the notes app in this case. 

Either way, there probably should be a check added to be safe, and the second bug sounds like an issue regardless.

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

github link for above code for better readability: https://github.com/mozilla-b2g/notes/blob/phase2/js/evernote.js#L178

Comment 6

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I was thinking about this the wrong way around. From Daniel:

Scenario is:
1) Attacker shares note with iframe in title (requires victim to accept sharing request)
2) Victim views list of shared notes, which opens iframe to malicious page
3) Malicious page posts message containing token back to app, accepted due to lack of origin check
5) Notes app synchronizes to wrong account

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So given that scenario, I would say this that your assessment above is accurate dveditz (ie high for the app, moderate for the phone as a whole). I'm not sure what, if any, builds still include this app by default though.

Comment 8

[Andrew McCreight [:mccr8]]

Dylan, is there somebody who can work on fixing this?  Thanks.

Comment 9

[Yan Or]

Attachment: Pull request

1. Added html sanitization to all rendering of notebook and note titles.
2. Added check in message callback to validate originating window.

Comment 10

[Frederik Braun [:freddy]]

Thanks for creating this fix, Yan!
I have a few comments though.

* js/common.js added line 2096-2097 (https://github.com/mozilla-b2g/notes/pull/22/files#diff-fde4e313d5f4400cd7405ed9baa7053cR2096):
This attempt to sanitize HTML is context dependent and harmful for future code.
Your current approach will not work if somebody ends up using it in other contexts
(JS, HTML attributes, ...).


* js/evernote.js added line 195 (https://github.com/mozilla-b2g/notes/pull/22/files#diff-73b5e651f8abbb0b1cd2c44dee9418feR195):

We want the window to be the same, but we also want to be sure that the window still
contains the correct web page. You can do that by checking the origin of the
postMessage (i.e. the window's location). I suggest doing something like this:
`if ((authWindow == evt.source) && (evt.origin == EVERNOTE_SERVER)) {`

Comment 11

[Frederik Braun [:freddy]]

I want to suggest scoping the authWindow variable correctly, by prefixing its declaration (evernote.js, line 193) with var. I.e., `var authWindow`.
AFAIU this should disallow other script parts accessing the window reference and reduce attack surface.

One could also remove the event listener after the postMessage has been received.

I just saw that quite a lot of the variables are in global scope, when they could be locally scoped and handed over to functions as required, which would prevent leaking the access tokens in case of XSS.

Comment 12

[Paul Theriault [:pauljt] (no longer reading bugmail)]

As freddyb said, the XSS fix could be more robust, but it does fix the current issue, so better than nothing. 

I don't think the message spoofing issue is fixed though until you validate the event.origin though as per comment 10.

Comment 13

[Yan Or]

Thanks for the feedback, Frederik & Paul.

Checking event.origin won't work here because the OAuth token is sent back from Evernote via a callback URL, so the postMessage is actually coming from the app's redirect.html.  Hence, I thought the next best thing is check the window.  I can scope the authWindow variable as Frederik suggested.

Any other suggestion?

Comment 14

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Yan Or from comment #13)
> Thanks for the feedback, Frederik & Paul.
> 
> Checking event.origin won't work here because the OAuth token is sent back
> from Evernote via a callback URL, so the postMessage is actually coming from
> the app's redirect.html.  Hence, I thought the next best thing is check the
> window.  I can scope the authWindow variable as Frederik suggested.
> 
> Any other suggestion?

So the origin of the message is the app's id, something like app://<guid> ? If so then what you should do, is set the origin of the app by adding something like the following to the manifest:

"origin":"app://notes.gaiamobile.org"

You can then check against this origin. 

See also: https://developer.mozilla.org/en-US/Apps/Build/Manifest#origin

Does that help?

Comment 15

[Yan Or]

Comment on attachment 8413324 [details] [review]
Pull request

Thanks, that helps.

Added the origin check and made authWindow variable local.

Comment 16

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8413324 [details] [review]
Pull request

Great, looks good to me, thanks!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Do Gaia patches have to go through the sec-approval process too? Because this appears to affect multiple branches and is sec-high.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 18

[Dylan Oliver [:doliver]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> Do Gaia patches have to go through the sec-approval process too? Because
> this appears to affect multiple branches and is sec-high.
> https://wiki.mozilla.org/Security/Bug_Approval_Process

Since the Notes app lives apart from Gaia as a Marketplace app, I think the process can be simpler. There are not any affected FxOS branches per se; it would be users across all FxOS versions who have installed the Notes+ app and enabled the Evernote integration.

We're planning to push a significant update to the Notes app this week, so it would be good to get this landed today. 

We should also uplift this to the 'phase2' branch though I'm not sure if there's any flag/whiteboard I can use to indicate that or if you'd prefer a separate pull request for that branch instead. Ryan, ni? to get your thoughts -- dealing with Marketplace apps is new territory for me.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/notes/commit/a1b1a4488c966757e56d6028bdb4d2850f537ea6

Yes to another PR for the phase2 branch.

Also, I'm not sure how much it matters currently, but you (Yan) might want to consider using a bit more innocuous of a commit message on the off-chance people are watching the commit history for interesting things.

Comment 20

[Dylan Oliver [:doliver]]

Yan, can you create a separate PR for the phase2 branch?

Comment 21

[Yan Or]

Attachment: Pull request for phase2

Comment 22

[Yan Or]

Added PR for phase2 here.  I assume we don't need to review it again?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

As a general rule of thumb, rebases don't require new review unless the changes are substantive (you're the patch author, so that's up to your judgment).

phase2: https://github.com/mozilla-b2g/notes/commit/3059d4dcd90e693b00b8ccf042c84294c9dc994c

Comment 24

[Dylan Oliver [:doliver]]

Comment on attachment 8416097 [details] [review]
Pull request for phase2

Verified changes are identical to the master patch. Carrying forward Paul's r+.

Comment 25

[Dylan Oliver [:doliver]]

This package failed Marketplace validation with the error: "Origin cannot use `gaiamobile.org` origin." 
https://developer.mozilla.org/en-US/Apps/Build/Manifest#origin

We might be able to get authorization to use that origin for this app if necessary, however:
during my simulated app upgrade process using App Manager that it installed a second copy of Notes rather than updating the 1.0.9 version I had already installed. That didn't happen yesterday. Is it possible that the change in origin prevented it from updating the existing app on the device?

Comment 26

[Dylan Oliver [:doliver]]

(In reply to Dylan Oliver [:doliver] from comment #25)
> during my simulated app upgrade process using App Manager that it installed
> a second copy of Notes rather than updating the 1.0.9 version I had already
> installed. That didn't happen yesterday. Is it possible that the change in
> origin prevented it from updating the existing app on the device?

I can't reproduce this part reliably -- would be interested to hear your results.

Comment 27

[Yan Or]

I also see the same thing.  Looks like changing the origin changes the identity of the app.

Paul, do you know of a workaround in this situation?

Comment 28

[Yan Or]

Here's the excerpt from the documentation:

"There is a special protocol that is internal to a packaged app, which is app://<UUID>, where <UUID> is a long string that is unique to each device that the app is installed on. This URL is not easily available to app developers at this time. The origin field enables you to replace this UUID with a single domain name that will be used by each installed app, such as app://my-app.com."

Comment 29

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Yan Or from comment #27)
> I also see the same thing.  Looks like changing the origin changes the
> identity of the app.
> 
> Paul, do you know of a workaround in this situation?

Oh, sorry I was thinking this was part of Gaia. Just don't use the gaiamobile.org origin since that is a reserved origin for gaia apps, sorry. I think that should resolve this issue.

Comment 30

[Paul Theriault [:pauljt] (no longer reading bugmail)]

ie "origin":"app://notes.local" or
ie "origin":"app://notes.mozilla.org"  

Not sure if this is a Mozilla app,  or what the appropriate convention is here.

Comment 31

[Dylan Oliver [:doliver]]

Thanks Paul. Yes, Mozilla has ownership of this app now. 

Yan, can you also bump the version number when you update the manifest?

Comment 32

[Dylan Oliver [:doliver]]

Unfortunately we'll need to find a solution here that doesn't involve changing the app origin. I confirmed with the Marketplace team today that a new origin basically means it's a new app and either the marketplace validation engine or the reviewers will reject an update submission that changes origin. And in any case the result would be installing a parallel app on the device and leaving the unpatched version in place. 

We should back out the commits from yesterday to get the manifest back into a good state.

Comment 33

[Yan Or]

Ryan,

Can you back out the merge or should I submit a new PR?

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Reverted.

Master: https://github.com/mozilla-b2g/notes/commit/0a176a552a8d1e7234d91aa149e5519b916784aa
phase2: https://github.com/mozilla-b2g/notes/commit/01eb29d5089543d733ff0718f6268136078e3ddb

Comment 35

[Yan Or]

If we can't change the origin, would it be sufficient to check that the origin is of the form "app://<UUID>"?  How would malicious code be able to fake such an origin?

Comment 36

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Yan Or from comment #35)
> If we can't change the origin, would it be sufficient to check that the
> origin is of the form "app://<UUID>"?  How would malicious code be able to
> fake such an origin?

Yes thats a good point. Since the message comes from the app's origin, you can probably just check that it matches something like "app://"+window.location.host I think.  Sorry that I didn't think of that sooner.

Comment 37

[Yan Or]

Attachment: Second fix attempt

Comment 38

[Yan Or]

Attachment: Second fix phase2

Comment 39

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8417814 [details] [review]
Second fix attempt

Looks good to me, sorry for delay on review.

Comment 40

[Dylan Oliver [:doliver]]

I noticed a diff between the patches and left a question on the phase2 PR for clarification.

Comment 41

[Yan Or]

You are right.  It was a mistake.  Thanks for the catch!

I've updated the phase2 patch.

Comment 42

[Dylan Oliver [:doliver]]

Looks good, we're ready to go.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/notes/commit/c1ec1bfe46bbb71636b73df47967c4fe87e61850
phase2: https://github.com/mozilla-b2g/notes/commit/d74007e51e844bf561ff74add1ceba4220b6e510

Comment 45

[Dylan Oliver [:doliver]]

This fix has been deployed in Marketplace updates for both the Notes and Notes+ apps.