1357856 - Security review of logins API

Status: RESOLVED INVALID

(Firefox Graveyard :: Security: Review Requests, enhancement, P3)

Description

[Bob Silverberg [:bsilverberg]]

We are building a new WebExtensions API called logins, which interacts with stored logins via nsILoginManager. It is based on an experiment which is documented at [1], but the implementation in Firefox differs from that quite a bit.

1. What does the API do?

The API provides the following methods:

search - Returns a list of stored logins. For each login the following information is available:
  - Id, which is the guid from the login metadata
  - username
  - password
  - origin, which is the hostname of the login 
  - httpRealm
  - formActionOrigin, which is the formSubmitURL of the login

create - Creates a new login and stores it. Any of the fields above (with the exception of id) can be provided.

update - Updates an existing login. The caller provides an id from an existing login and an object which can contain any of the fields mentioned above (with the exception of id).

remove - Removes an existing login. The caller provides an id from an existing login, which is used to look up and remove the login.

Note that host permissions are respected for all of these methods. An extension must request access to hosts and only logins involving those hosts will be available for reading and modification. Note also that requesting access to "<all_urls>" will grant access to all logins.

2. Does the API expose any PII or confidential information?

The API exposes usernames and passwords chosen by the user, as well as sites they have visited via origin and formActionOrigin.

3. Is there any difference in behaviour or special concerns around private browsing?

The API will behave the same in regular and private browsing windows.

4. Does this API have it's own permission?

Yes, in order to use the API an extension will need to request the "logins" permission in its manifest.

5. Does it respect host permissions (if relevant)?

Yes, please see above (in the answer to question #1).

6. Is the user shown the permission?

Yes, the user will be shown the permission on AMO, the install prompt and optional usage prompt. We are currently working on the exact wording for this prompt.


[1] https://github.com/web-ext-experiments/logins/blob/master/api.md

Comment 1

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

Thanks for filing Bob. This API at first glance is significantly more powerful than existing APIs (I suppose arguably session cookies ~= logins, but people re-use passwords, and password dont expire etc). Neither here nor there, but wanted to set the expectation that we will likely need more security mitigations here than for other APIs. 

My team is pretty loaded with reviews at the moment, but I can probably take a first pass at this some time next week. Is there any deadline or date I should be aware of, or is the review just ASAP.

My main question is how Web Extensions deal with permission prompts, and what we show in the UI. Ive not had any experience with Web Extension API permission prompts yet, and as far as I know they are not exposed to the user in the browser UI? This seems like the sort of permission that would benefit from an explicit permission grant (upon first use, or when the extension updates etc maybe?) Thats probably a question broader than the scope of this review, but its something we'll need to sort out.

Comment 2

[Bob Silverberg [:bsilverberg]]

(In reply to Paul Theriault [:pauljt] from comment #1)
> Thanks for filing Bob. This API at first glance is significantly more
> powerful than existing APIs (I suppose arguably session cookies ~= logins,
> but people re-use passwords, and password dont expire etc). Neither here nor
> there, but wanted to set the expectation that we will likely need more
> security mitigations here than for other APIs. 
> 
> My team is pretty loaded with reviews at the moment, but I can probably take
> a first pass at this some time next week. Is there any deadline or date I
> should be aware of, or is the review just ASAP.

Other than the fact that landing the logins API is a Q2 goal, there is no other deadline, so there is no huge rush on this, but ASAP would be appreciated.

> 
> My main question is how Web Extensions deal with permission prompts, and
> what we show in the UI. Ive not had any experience with Web Extension API
> permission prompts yet, and as far as I know they are not exposed to the
> user in the browser UI? 

As of Firefox 55 (I believe), they are exposed. I can put together some screenshots that would show what a user would see when installing an extension that uses this API and I'll add them to this bug.

> This seems like the sort of permission that would
> benefit from an explicit permission grant (upon first use, or when the
> extension updates etc maybe?) Thats probably a question broader than the
> scope of this review, but its something we'll need to sort out.

Comment 3

[Andy McKay]

On Nightly hit AMO and install a WebExtension, you'll see a prompt, eg:

https://addons.mozilla.org/en-US/firefox/addon/ghostery/

And you'll see the permission goodness.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Andy asked me to make some comments on use cases resulting from a discussion we had. As the logins API uses nsILoginManager in the background, it will also provide developers a way to securely save private information. Developers may use the api for other data that needs encryption since there is no encrypted storage.

One possible use case is storing oauth tokens from self-initiated oauth logins. This has been common practice in Thunderbird (which is admittedly not the target, but the use case is the same).

For larger amounts of data, the developer may use a crypto library to encrypt the contents of browser.storage.* and then use the logins api to store the encryption key.

Comment 5

[Bob Silverberg [:bsilverberg]]

Attachment: Permissions dialog when requesting only the logins permission

This is the dialog that appears if a developer only specifies the "logins" permission in the WebExtension manifest. Just requesting "logins" without any host permissions will result in the logins API being usable only with logins owned by the extension.

Comment 6

[Bob Silverberg [:bsilverberg]]

Attachment: Permissions dialog when requesting <all_urls>

This is the dialog that appears if a developer specifies the "logins" permission and the "<all_urls>" permission in the WebExtension manifest. This will grant access to all logins from all external sites.

Comment 7

[Bob Silverberg [:bsilverberg]]

Attachment: Permissions dialog when requesting a specific host

This is the dialog that appears if a developer specifies the "logins" permission and a specific host permission in the WebExtension manifest. This will grant access to all logins for that host.

Comment 8

[Rémy Hubscher (:natim)]

I plan on using this API and my use case is to store an encryption key locally in a safe place and retrieve it. I don't want to key to be retrieved by another add-on nor to be able to retrieve other add-ons or website secrets.

Comment 9

[Greg Cox [:gcox]]

comment 2 lists landing the logins API (bug 1324919) as a Q2 goal, and we're into Q3.
Anything we can do on triaging this up?

Personally, this blocks pursuing converting an old password-managing addon to webext.

Comment 10

[blk]

With the release of Firefox 57 today, add-ons that relied on nsiLoginManager or nsiLoginManagerStorage are now non-functional. Has there been any progress on this security review? Looking forward to when APIs replacing nsiLoginManager / nsiLoginManagerStorage can come into being.

Comment 11

[Mike Conca [:mconca]]

Does security require anything more from the engineering team? As comment 10 points out, the release of 57 has left a certain number of extensions without a path forward. Many requests for a WebExensions login API have come in, so it would be nice to get some traction on this if possible.

Comment 12

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

Is there anything to review yet? I wasn't aware that the engineering team were working on this API (I was told it wasn't a priority and it doesn't sounds like the case from the last comment in 1324919.) I'm certainly am not looking at this currently but I could prioritize it if engineering is planning on working on this.

I'm a little unclear on where I would start though. As far as I know we have a proposal from a year ago [1] and an experimental patch in bug 1324919). Is that still the plan of record? Has anyone from the engineering team actually thought about the security of this API, and how it fits into broader security model of Web Extensions? How does this interact with Host Permissions?  How does it interact with Sync  and the master password? Is any of this documented anywhere?

IE is this bug for Security Review, or for Security Design? It would seem like we need the latter first, which has been the common problem I'm seeing with APIs that come for "review". Still happy to try to help if I can on the design side, but this isn't blocked on "review" - I'd direct questions like this to original bug (1324919).


[1] https://github.com/web-ext-experiments/logins/blob/master/api.md

Comment 13

[Andrew Swan [:aswan]]

(In reply to Paul Theriault [:pauljt] from comment #12)
> Is there anything to review yet?

There is a patch in bug 1324919 based on the work from the experimental api on github.

> Has anyone from the engineering team actually
> thought about the security of this API, and how it fits into broader
> security model of Web Extensions? How does this interact with Host
> Permissions?

The document you linked to addresses these questions

> How does it interact with Sync  and the master password?

This has not been considered, but getting some input from sync is a good idea.  Do you view this as part of the security review or a separate thing?  Or in other words, should that discussion happen on this bug or on bug 1324919?

Comment 14

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

I'm doing clean-up of the review queue, so I'm closing this as incomplete at the moment, since as far as I know, it isn't being work on. Please feel free to reopen once the work has been done to figure out that security model for this API.

Comment 15

[Anselmo Canfora]

Hello sirs, here a simple FF user since v0.6 (I am not an add-on developer). I would like to know if a password/credentials/logins API will be available for WebExtensions in time for release of FF 60 ESR. At this moment, me and every other professional FF user I know are stuck either to FF 52 ESR or FF 56 due to the absolute need of following add-ons:

https://addons.mozilla.org/it/firefox/addon/saved-password-editor/
https://addons.mozilla.org/it/firefox/addon/password-exporter/

the first one in particular is very useful for people dealing with SSO scenarios (LDAP/SAML/etc), I understand that statistically this could be a tiny percentage of your user base, but nonetheless would like to let you know that perhaps you are underestimating the importance of credentials APIs: Chrome and IE have anything comparable to the functionality that the above cited add-ons bring to Firefox. Perhaps would be wise to keep the few advantages that FF has over strong competitors like Chrome.
Thank you in advance for the attention you may give to this message.

Comment 16

[Andrew Swan [:aswan]]

(In reply to Paul Theriault [:pauljt] from comment #14)
> I'm doing clean-up of the review queue, so I'm closing this as incomplete at
> the moment, since as far as I know, it isn't being work on. Please feel free
> to reopen once the work has been done to figure out that security model for
> this API.

Wait, nobody has worked on the implementation recently since we were told to wait until the security review was done.
What exactly does the work to "figure out that security model for this API" entail?'

Comment 17

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

I'll take this to email, there's clearly a gap in communication here.

Comment 18

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

I've had a look at this today, as I suspected above, this API seems like it needs more work. The proposed permission model of having a "logins" permission, and having the API respect host permissions makes sense to me in a general sense, but I we need to be careful of edge cases with host permission, and also you seem to be making some incorrect assumptions about the data in nsIloginManager (or I'm out of the loop - Mattn can probably speak better to this than I can). 

Some initial questions:

1. The documentation above doesn’t match the code. (the code has different methods etc). Which one is the source of truth, the code I assume, but :frown:. 

2. The code in 1324919 seems to assume that hostname (origin), formSubmitUrl (formActionOrigin) and httpRealm can all be converted to a url and that this url is same origin. That doesn't seem to be the case in practice (at least for my own data).  formSubmitUrl is often on a different origin to hostname  (https://secure.foo.com & http://foo.com is a common example) and httprealm is just string, right? I.e. it can be anything? Or do you intend to add constraints?

3. Bug 1415644  blocks this API, because it completely invalidates the proposed permission model

4. How will this interact with the activetab permission. Does activetab + login mean access to the login for the current tab? (could this be abused?)

5. What passwords can an extension with only the Login permission access? Just ones where the hostname matches its own moz-extension URI? Is that useful (why not use indexeddb in that case? Just in case the user has a master password set?)


That will do to start (see more notes at [2]).  But we aren't going to "security review" our way to a safe API here - which comes back to the security model question. The document at [1] is a start, though it doesn't match the code as implemented. The bigger question for me is that if we want to start exposing APIs which are more powerful, does our existing model provide enough safety for users  - I don't think it does in this case, but I'm not really the owner/decider here. 

Given that <all_urls> grants access to sniff any password/cookies, password disclosure isn't my first concern here (though I am a bit worried about things like the sync key being only protected by the whims of host permissions). But what about protecting users for losing all their passwords because an addon they installed accidentally wiped all their passwords? That sort of risk of permanent data loss at least needs some consideration, and I don't think just saying "well their fault, they should have read the prompt" really is sufficient. Mitigations we might consider are:
 - maybe the Web APIs "remove" function a soft-delete (disable) (but then what about update()
 - adding a password backup feature (is that sync? But sync will delete your sync'd data if you remove it locally wont it?)
 - maybe require a more 'opt-in' style of switch for powerful features (like the switch in chrome for "enable in incognito mode" or "allow access to file system")

That's the sort of thing I was hoping to see documented. But it sounds like perhaps we both had that expectation. 

Given the number of powerful new APIs in the pipelines, maybe we need a framework for deciding what level of security controls are appropriate for a given API? Which partly goes back to getting an overall "Web Extension Security Model" documented - I'll reach out via email to talk next steps for that effort.

Hopefully that helps. For reference, I'm keeping security review notes at [2]. 


[1] https://github.com/web-ext-experiments/logins/blob/master/api.md
[2] https://docs.google.com/document/d/1Ml_ueogAL9iwsfLXLVlAp9DLLvaxUnFM6jdH5435cRk/edit#

Comment 19

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

David, I did an urgent review based on comment 16 a couple months ago. IIRC at this time you were planning to have a work week to sort out priorities. I haven't seen any response here, so I'm moving this to our backlog. Please needinfo me if this changes.

Comment 20

[David Durst [:ddurst]]

This is in our backlog as well, so we'll iron out the Security review and interaction first and re-visit this then.

Comment 21

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

Closing until there is more info here to clean up our queue. Please either re-open this, or ask for a new review once there is a design here.