Closed Bug 698808 Opened 13 years ago Closed 12 years ago

Security Review - BrowserID extension for b.m.o.

Categories

(mozilla.org :: Security Assurance: Applications, task)

Product:
mozilla.org
Component:
Security Assurance: Applications
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: gerv, Assigned: curtisk)

References

Details

(Whiteboard: [secr:curtisk]) 

This is a request for a security review for the Bugzilla extension implementing BrowserID. Therefore it is related to, but not the same as, bug 644776, which is about BrowserID itself.

> A quick intro to what this app does.

It enables users (both existing and new) to log in to Bugzilla using BrowserID. It is only permitted for users whose max privilege is editbugs or lower.

> Where is the source code located?

http://bzr.mozilla.org/bugzilla/extensions/browserid/trunk/files

> Is there a stage server running that we can also test against? If so, please indicate 
> what machine the web server is running on.

There is no staging server running yet. Installing it on the Bugzilla staging server is bug 698798. It's installed simply, like any other Bugzilla extension, and I'd be happy to help you install it on any personal test Bugzillas if you need help.

> Where would you like the bugs filed in bugzilla? Please specify the product, component > and if anyone specific should be copied on the bugs.

Please file bugs in bugzilla.mozilla.org/Extensions: Other, making sure I am CCed (gerv@mozilla.org).

> Will this application be collecting any personally identifiable information from users > (email address, physical address, phone number, etc)?

If they log in with BrowserID and have never used Bugzilla before, Bugzilla will then know their email address. This is equivalent to signing up in Bugzilla for an account. Unlike the standard signup process, there is no opportunity in the flow to supply the (optional) real name, but they can of course go to their preferences afterwards and supply it if they choose.

> Please describe if this app will be connecting to any internal or external services or > if it is able to interact with the OS.

It will connect to the browserid.org BrowserID service, via a) loading a JS file from that domain in the UI (at the moment, on every page, when not logged in - although it should be cached), and b) a single POST call on the back end to verify the provided assertion.

> Does this app support logins or multiple roles? If so, we'll need test accounts created > for each available role.

This app _is_ logins :-) You can create your own test accounts on b.m.o. The staging server has email disabled, so you can't retrieve a password. Ask any b.m.o. admin if you need some low-priv accounts - or, assuming it works, just log in with browserID using an email address you control!

> What is the worst case scenario that could happen with this system, data or connected 
> systems? (This is used to help understand the criticality of this server.)

Worst-case: validation is not done properly, the system fails open, and anyone can log in to Bugzilla as any account.

This scenario is avoided in three ways. Firstly, hopefully BrowserID works :-) Secondly, if browserid.org incorrectly returns success, we only let people in with editbugs or lower. Thirdly, if the request to browserid.org fails entirely, we catch the error in an eval {} (like a try/catch in Perl), fail closed and deny the login.

> Does this website contain an administration page? If so, have the admin page blockers 
> (listed here) all been addressed?

No admin; the list of permitted groups is hard-coded into the extension.

> This review will be scheduled amongst other requested reviews. What is the urgency or 
> needed completion date of this review?

Normal urgency.

Gerv
Blocks: 672841
So, I just took a quick look at bugzilla-stage-tip today, and I saw what Gerv mentions here:

> > Please describe if this app will be connecting to any internal or external services or > if it is able to interact with the OS.
> 
> It will connect to the browserid.org BrowserID service, via a) loading a JS
> file from that domain in the UI (at the moment, on every page, when not
> logged in - although it should be cached), and b) a single POST call on the
> back end to verify the provided assertion.

That boils down to this being on every page:
<script src="https://browserid.org/include.js" type="text/javascript"></script>

I don't think that's really acceptable to load a random .js file from some other non-bmo site. You're basically pushing browserid.org to the same trust level as bmo, hoping that browserid.org won't get owned. Considering BrowserID is a Labs project, I don't really think that's acceptable here.

Can that .js file just be copied to the local bugzilla extension (and just kept in sync somehow)? That would be an easy fix for this problem.
We could; I suspect they discourage that at the moment because the file may change. Perhaps we need to talk to them about what sort of notification mechanism we could have for that eventuality.

Gerv
Ben Adida says:

"Hosting it locally now will likely break things as we are not done updating that file. In the not too distant future (6 months?) it will probably be safe to host it locally.

Note that, of all the systems hosted by Mozilla, BrowserID is the one that implements the most security measures, by a good margin:

http://blog.mozilla.com/webappsec/2011/12/14/securing-browserid/"

I think we need to leave it to the security review team to decide whether this inclusion is a risk or not.

mcoates: any idea when this code might get to the top of the review pile?

Gerv
It *is* at the top of the pile; this, and a couple of items are on my plate to finish before the holidays!
removing sec-review-needed for infrasec, I think the wrong kw was used here
Keywords: sec-review-needed
:yvan: is this at the top of your pile yet? :-) 

Gerv
Since we have merged security teams and I am now program managing for all security reviews I am putting the flag back on this bug and marking myself as the resource for getting this handled. We will need to decide if this is a group or individual review.
Keywords: sec-review-needed
Whiteboard: [pending secreview] → [pending secreview][secr:curtisk]
curtisk: this bug isn't on https://wiki.mozilla.org/Security/Radar - is that a problem?

Gerv
gerv: Bugs do not/did not show up on radar as we were waiting for the bugzilla-mediawiki to land which it just did. This is on my personal radar [secr:curtisk] to follow up on. I think the plan at this point is to get with the dev team again and sit down to review this.
If you mean the extensions' dev team, then that's me :-) Happy to sit down and do a review at any point, perhaps via Vidyo or similar. It's not very much code.

Gerv
We hold the meetings on a regular schedule, M/W 1PM PST and Th/F at 10am PST and the calendar of available dates is here: https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html.

Any date that shows just SecReview is available. Just let me know which slot you want and I will setup the meeting. If these times don't work at all for you (I believe you are in the UK?) then let me know and I will see what I can do to find an alternate time that the necessary resources can make.
Can I have 10am PST, 6pm GMT on Friday 9th March, please? (Note: this is just _before_ the US DST switch, so I think those times are correct.)

Gerv
done, you should have an invite
Whiteboard: [pending secreview][secr:curtisk] → [pending secreview][secr:curtisk:sched]
I was just reminded that this is during cansec west and a good number of the security team is attending can we move this out a week?
(In reply to Reed Loden [:reed] (very busy) from comment #1)
> That boils down to this being on every page:
> <script src="https://browserid.org/include.js"
>         type="text/javascript"></script>

The <script> tag should being included only on pages where the user isn't logged in and/or when the user does not have privileges higher than editbugs.

By the way, what happens when somebody's privileges are elevated from (lower than) editbugs to higher than editbugs? Doesn't that mean they would not be able to log in at all?

When you log in with BrowserID, does that reset your regular password? It seems like maybe it should.
(In reply to Brian Smith (:bsmith) from comment #15)
> The <script> tag should being included only on pages where the user isn't
> logged in and/or when the user does not have privileges higher than editbugs.

> By the way, what happens when somebody's privileges are elevated from (lower
> than) editbugs to higher than editbugs? Doesn't that mean they would not be
> able to log in at all?

It is not really a higher or lower than comparison. The code is just making sure
that the user is one or more of the following groups:

'everyone', 'canconfirm', 'editbugs'

If the user falls out of all three for whatever reason, they are not allowed to 
login using BrowserID.
 
> When you log in with BrowserID, does that reset your regular password? It
> seems like maybe it should.

No, The normal BMO password stays what it was before and is just being masked by the login to BrowserID. If the user chooses to login using the normal Bugzilla login. they will need
to use their BMO password from before. There is no automatic syncing of any passwords.

dkl
(In reply to Brian Smith (:bsmith) from comment #15)
> The <script> tag should being included only on pages where the user isn't
> logged in and/or when the user does not have privileges higher than editbugs.

Forgot to comment on this in my comment 16. The external script is only loaded currently if the user is not logged in.

dkl
(In reply to David Lawrence [:dkl] from comment #16)
> It is not really a higher or lower than comparison. The code is just making
> sure that the user is one or more of the following groups:
> 
> 'everyone', 'canconfirm', 'editbugs'
> 
> If the user falls out of all three for whatever reason, they are not allowed
> to  login using BrowserID.

This seems to be the opposite of what was said above: "we only let people in with editbugs or lower." The idea is that highly-privileged accounts (e.g. security-group) should NOT be allowed to log in with browserid for right now.

And, that creates a problem when the user wasn't in security-group yesterday, but is in security-group today. Yesterday they could log in using browserid, but today they can't, and they might not have a password to log in without browserid.

> No, The normal BMO password stays what it was before and is just being
> masked by the login to BrowserID. If the user chooses to login using the
> normal Bugzilla login. they will need to use their BMO password from
> before. There is no automatic syncing of any passwords.

IMO, the user's password should be reset the first time they successfully log into bugzilla using BrowserID. Otherwise, they will forget about that alternate password, which might cause them problems later (e.g. it might be shared with another account that gets compromised, like their Twitter account or their wordpress account).
Curtis: sure, we can move it out a week. Fri 16th.

I'll respond to the other comments here tomorrow.

Gerv
(In reply to Brian Smith (:bsmith) from comment #19)
> This seems to be the opposite of what was said above: "we only let people in
> with editbugs or lower." The idea is that highly-privileged accounts (e.g.
> security-group) should NOT be allowed to log in with browserid for right now.
> 
> And, that creates a problem when the user wasn't in security-group
> yesterday, but is in security-group today. Yesterday they could log in using
> browserid, but today they can't, and they might not have a password to log
> in without browserid.

Ugh, I am having an off day. You are correct and I read the code today incorrectly since I have not looked at it in a while. If the user is in *any* other groups outside of "canconfirm", "everyone", and "editbugs", they *cannot* use BrowserID to login. So most Mozillians will not be able to use the feature. The original specification still stands.

> IMO, the user's password should be reset the first time they successfully
> log into bugzilla using BrowserID. Otherwise, they will forget about that
> alternate password, which might cause them problems later (e.g. it might be
> shared with another account that gets compromised, like their Twitter
> account or their wordpress account).

BMO would not have access to the user's password stored with the BrowserID verification service I would expect so we would not be able to change their password stored in the Bugzilla profiles table. We would have to throw up a form to have them change it manually. If they go for a while without using normal BMO login and they forget their password, they can always request an email to change their password using the Forgotten Password feature.

dkl
> BMO would not have access to the user's password stored with the BrowserID
> verification service I would expect so we would not be able to change their
> password stored in the Bugzilla profiles table. We would have to throw up a
> form to have them change it manually. If they go for a while without using
> normal BMO login and they forget their password, they can always request an
> email to change their password using the Forgotten Password feature.

we'd have to prompt for their password each time they sign in, as they may have changed their browserid password since the last time the logged in :(

perhaps we could set their bmo password to * when they sign in with browserid (with a large warning the first time).  this will disable their ability to login to bmo with password authentication.

i'm not sure this is the best idea, because you can't use browserid to authenticate via webservices, which a lot of systems use (eg. thunderbird/bmo addons).

perhaps we could give users the option of *switching* to browserid for auth?
It seems that most of the issues raised have been answered.

BrowserID is, at the moment at least, an alternative login method for existing accounts. That is to say, it's an alternative way, other than your Bugzilla password, of proving ownership of a given email address (and therefore Bugzilla account).

In the future, I suppose it could be that we have BrowserID-only accounts, either by creation, or by users opting for that. But we don't have code or capabilities for that yet. I certainly think that if we make it so that when someone logs in using BrowserID, their account becomes BrowserID-only, that will not be what people expect!

It is true that the group restrictions make BrowserID unusable for all Mozilla employees. So we may change this restriction after the security review and/or after we've tried it live. For example, we might start by loosening it to everyone who is not in a group with "security" in the name. And, if and when we have sufficient confidence in BrowserID (which we should have, given that we are now encouraging other sites to adopt it!) we can open it up to all accounts.

So what I'm saying is: let's take this one step at the time. The first step is providing BrowserID as an alternative login method for b.m.o., for low security accounts. Later can come BrowserID-only accounts, higher security account logins, and other such things.

Gerv
QA Contact: mcoates → jstevensen
The actions coming out of the security review are now all done:
https://wiki.mozilla.org/Security/Reviews/BZBrowserID

Can this bug be marked as FIXED?

Gerv
Do you have bug IDs or wiki pages or something we can point to in those completed items? I would like to have documentation for others to follow if they so choose. If not that is fine. That said, if the work is done the bug is fixed.
I linked some info from the list of tasks in the wiki page. Here you are:

* Update code to check for absence of "nobrowserid" group:
  DONE - http://bzr.mozilla.org/bugzilla/extensions/browserid/trunk/revision/8

* File bug on full verifier support (non blocker)
  DONE - bug 737480

* At appropriate moment, rename any UI elements to new branding
  Not needed - see http://identity.mozilla.com/post/18038609895/introducing-mozilla-persona

* Create nobrowserid group and put relevant groups in it - all security, HR, legal
  DONE - see Bugzilla config

Gerv
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee: security-assurance → curtisk
Status: RESOLVED → VERIFIED
Keywords: sec-review-needed → sec-review-complete
Whiteboard: [pending secreview][secr:curtisk:sched] → [secr:curtisk]
You need to log in before you can comment on or make changes to this bug.