874767 - SecReview: ReviewBoard

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance: Review Request, task)

Description

[:glob ✱]

> Who is/are the point of contact(s) for this review?

byron jones ([e-mail])

> Please provide a short description of the feature / application

i am investigating replacing bugzilla.mozilla.org's "splinter" patch review system with something more functional, and reviewboard is the current favourite.

the plan is to deploy a customised version of reviewboard onto mozilla infrastructure and integrate it tightly with bmo.

webops haven't raised any concerns from an infra perspective, so i'd like to get very early feedback from security about this plan.

> Please provide links to additional information

product page: http://www.reviewboard.org/
source: https://github.com/reviewboard/reviewboard

as we are still in the investigation phase, customisation work for bmo integration hasn't started.  i am expecting this to take shape via a new auth backend for reviewboard, minor reviewboard ui changes, and a new bugzilla extension which uses revewboard's api to upload files.

> This review will be scheduled amongst other requested reviews.

i would like a preliminary "this seems sane" or "no way this is going to happen" as soon as possible. there's a lot of scoping work yet to happen, and i'm not keen on pouring a lot of time into that if this solution isn't one which will ever pass security review.

if viable, a review in early/mid Q3 would be desirable.

> To help prioritize this work request, does this project support a
> goal specifically listed on this quarter's goal list?

yes, https://wiki.mozilla.org/Auto-tools/Goals/2013Q2
"improve patch review system on bugzilla"

the original plan, which the timing of that goal currently reflects, wasn't to use reviewboard.  if reviewboard is viable, we'll probably aim for code completion before the end of Q2 with deployment in Q3.

> Does this feature or code change anything that Mozilla ships to end users?
> Are there any portions of the project that interact with 3rd party services?
> Will your application/service collect user data?

no to all three.

 > Other Feedback

there are two areas within reviewboard which will be the focus of cusomtisations.  first will be integration with bugzilla's security model, and the second will be changes to the process to match bugzilla's current system, and to ensure that bugzilla continues to be the central source of truth.

while i haven't spent any time on development, reviewboard does support multiple auth backends and it should be possible to write a backend which leverages bugzilla's auth webservices to protect security patches hosted on the reviewboard system.

Comment 1

[:glob ✱]

yvan, i'm keen to hear if this concept is something which security is ok with.  do you have any idea when you can get back to us?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey all - thought I'd chime in.

I'm a contributor to Review Board. I'm also a Mozillian. I can vouch for the sanity of its code, fwiw. :)

Comment 3

[cmtalbert]

Guys, we need to get this process started, and we don't want to start down this road only to find out that there are security issues with the approach later. Can you please give us an ETA when you'll have an answer w.r.t. the approach here?  One of the really nice things about having a separate ReviewBoard server is that we'll be able to allow other systems (i.e. alternate bugzilla front-ends) to use it as well.

Comment 4

[Yvan Boily [:ygjb][:yvan]]

I don't have any objections to this, the bigger issue is being able to clearly examine how the integration will work.

Comment 5

[cmtalbert]

thanks Yvan!  Once we have something to show we'll do the sec review and may hit you up sooner outside of bugzilla if we have integration questions.

Comment 6

[Mark Côté [:mcote]]

The fairly minor modifications are up at https://github.com/mozilla/reviewboard/tree/release-1.7.x

The only changes I did from stock ReviewBoard are at the top of the commit list.  They all use XMLRPC calls and never store any passwords, so this should be pretty simple.

In the future, we will be moving Bugzilla integration to its own extension, so it should be easier to review.

Comment 7

[Mark Côté [:mcote]]

One potential flaw that was pointed out is that we weren't clearing the Bugzilla login cookies if the user logged out of ReviewBoard.  I've just committed a fix for that; Bugzilla cookies are always cleared if the user is not authenticated with ReviewBoard.

I'm switching this needinfo to FreddyB, since I briefly chatted with him about this while in Brussels.

Comment 8

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

cleared the wrong flag and forgot the WB tag

Comment 9

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

clearing triage needed, this is going to have to wait for freddy as everyone else is just too busy right now

Comment 10

[Frederik Braun [:freddy]]

I have reviewed the last 5 commits (all by mcote) and they look good. It would be great if this bugzilla-patch for reviewboard would become a plugin or even backported into reviewboard, so that maintenance cannot turn into a security problem in the long run.

It would be great if I could play with a dev system if possible.


NB: All of this under the assumption that reviewboard is fine, it looks like they have an existing security process and their backlog of previous security issues is not too bad. If time permits I wouldn't mind performing a short test of reviewboard itself. Well, to be honest, if time permitted I wouldn't be opposed on giving reviewboard an exhaustive test...

Comment 11

[Mark Côté [:mcote]]

Yeah, the goal is to get this as a proper extension eventually.  At the time I started this, there weren't enough hooks in ReviewBoard to allow it to be an extension, but I've been working with the ReviewBoard devs to add the necessary functionality.  I figure when we get to the next steps of integration, after people have played with it a bit, then we'll move it over to being a extension.

There is a dev instance up at https://reviewboard-dev.allizom.org/.  It authenticates using bugzilla-dev.allizom.org.  You'll probably need your password reset on bugzilla-dev since it occasionally gets a refreshed, sanitized database; file a bug or ping glob or dkl in #bteam or #bmo.

Comment 12

[Frederik Braun [:freddy]]

> NB: All of this under the assumption that reviewboard is fine

Are we using the most recent version of reviewboard? There are some issues within this installation that we would have to resolve before this can go on :( I will file them individually..

Comment 13

[Mark Côté [:mcote]]

The deployed code is a couple months behind.  I can merge in recent changes and deploy them; I believe there have been some security fixes recently.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mark Côté ( :mcote ) from comment #13)
> The deployed code is a couple months behind.  I can merge in recent changes
> and deploy them; I believe there have been some security fixes recently.

Indeed there have been.

(In reply to Frederik Braun [:freddyb] from comment #12)
> > NB: All of this under the assumption that reviewboard is fine
> 
> Are we using the most recent version of reviewboard? There are some issues
> within this installation that we would have to resolve before this can go on
> :( I will file them individually..

Probably goes without saying, but security issues should be sent to [e-mail] and *not* filed in the issue tracker.

Comment 15

[Frederik Braun [:freddy]]

I know this comment is a bit late to the party, but the early comments that that the idea to integrate with ReviewBoard are still in an early phase. If there's a way to back out we might consider using something more lightweight, that doesn't duplicate a lot of the features that bugzilla already has (and sometimes in a less proven matter).

If that's not an option anymore, I would suggest reshifting focus for this review and giving the whole reviewboard software a security review, this would take two weeks or so, but is highly advised. Especially considering that we're going to tie it to Bugzilla authentication credentials.


I will make sure my findings will be reported to the reviewboard devs in a confidential manner, thank you noting that Mike.

Comment 16

[Mark Côté [:mcote]]

We've looked at a few candidates, and one of the only ones that stuck out and are easily adaptable to our workflow is ReviewBoard.  Others are tied too closely to a particular VCS, or don't offer much past what Splinter already offers, or are even heavier weight.  At this time it seems that our only other options would be to either write our own or heavily modify Splinter, both of which would undoubtedly have their own security concerns and would certainly be more work overall.  But if you have any ideas--it's entirely possible that there are tools out there that we haven't heard of--please let me know.

Anyway, we have done only minimal integration because we are looking for feedback like this.  It might turn out that many people think ReviewBoard is too heavy-weight a solution.  To expedite, we could take out what integration we have and just host it completely separate from Bugzilla, on an EC2 server or something, but I fear that will discourage experimentation, and then we won't get a good feel for what developers think of the tool.

Comment 17

[Frederik Braun [:freddy]]

Yeah, sorry for being the nasty troll before the bridge towards deployment here..

I think we could need more feedback indeed. I don't know of any other solution but will ask around. And as you say, something completely off bugzilla is a crappy experience :/

Comment 18

[Mark Côté [:mcote]]

Or actually, a middle road would be to host it, with the integration bits (and with recent commits to ReviewBoard itself), on an EC2 instance or somewhere similar, away from our network, and tell people not to use it with any confidential bugs for now--an acceptable request for a product in the evaluation phase.  

All the (minimal) integration with Bugzilla is through public APIs, so technically anyone could host something like this anywhere in the world.  At this point it sounds like you are more concerned with the security of ReviewBoard itself, which is understandable, so perhaps we can host it externally in the meantime and move it all over to a Mozilla network when the security check & any necessary fixes are complete.

Comment 19

[Frederik Braun [:freddy]]

The threat here is that something bad on *.mozilla.org is technically capable of messing with firefox download windows in other tabs. In addition, bugzilla contains a lot of sensitive stuff we don't want exposed as well. If there's a way to get on with the testing on a non-mozilla domain that does not handle the bugzilla cookies as careless as it is right now, I would be OK with moving on that way.

This might block less and give me the time to conduct a proper review?

Comment 20

[Frederik Braun [:freddy]]

Good news: I'm nearly done.
Is there any way to update the ReviewBoard installation to something newer, mcote? It also seems that 2.0 beta1 is out which sounds like a better target for our uses. Tthe markdown support, for example, will address some of my concerns with XSS.

Comment 21

[Mark Côté [:mcote]]

Hm, I would need to make sure my patches apply cleanly to version 2.0.  I can easily generate a new 1.7.x build, though--would that be sufficient for now?  And maybe upgrade to 2.0 when it is officially released?

Comment 22

[Frederik Braun [:freddy]]

Yes, most recent stable is still a win. Thanks.

Comment 23

[Mark Côté [:mcote]]

Okay, merged in recent changes to release-1.7.x and built a new version. Should be deployed in a day or so.

Comment 24

[Mark Côté [:mcote]]

Latest code has been deployed to dev and prod.

Comment 25

[Frederik Braun [:freddy]]

OK this is done, but the vulnerabilities identified and reported to ReviewBoard have not been backported to 1.7.x. If they don't backport this soon (I have asked just now), we will have to use 2.x.

Comment 26

[Frederik Braun [:freddy]]

Backported to the 1.7.x branch as commit 52804388a6b9632ee76d6284e9fdbb190d5f7564 on release-1.7.x.
Mark, can we somehow easily check if this is included in the code we're running?

Comment 27

[Mark Côté [:mcote]]

Yeah, just looking at the logs for https://github.com/mozilla/reviewboard/commits/release-1.7.x reveal that the last changes I merged it from upstream were from Nov 12, and the commit you mentioned was added on Nov 15, so, no, it isn't in our instance.  I will merge in recent changes, create a new release, and get it deployed.  Will comment here when it's up.

Comment 28

[Mark Côté [:mcote]]

Deployed.

Comment 29

[Frederik Braun [:freddy]]

Thanks! I'm done with all security testing.
Do you need something else to go forward? I still recommend hosting/testing it separate from Mozilla domains and infrastructure as indicated in comment 18 and 19

Comment 30

[Mark Côté [:mcote]]

Ah we actually already have a production environment set up on a Mozilla server; my suggestion was for an interim solution while the security review was under way.  Are there existing issues which make you uncomfortable having this on the Mozilla network?  I'd like this service to be entirely administered by IT (as it is now); moving it to, say, EC2 would make that more difficult (as far as I know).

Comment 31

[Frederik Braun [:freddy]]

I agree that it would be desirable to have it IT hosted. And even though I have tested it myself I think we're still taking a risk in connecting this big piece of software with something as critical as bugzilla.

But as long as we're just testing it's goint to stay on the allizom.org subdomain, right? Then I'm OK with it.

Comment 32

[Mark Côté [:mcote]]

Yeah I understand your concerns, although to reiterate it is doing very little integration with Bugzilla.  The prod system (the one hooked up to prod Bugzilla) is currently on a mozilla.org domain, but I can switch it to allizom.org for our trial period.

Thanks for all your time, and feel free to resolve.  I will open new bugs for reviews of the further integration bits, when we get to those.

Comment 33

[Frederik Braun [:freddy]]

OK, then we're done for now. Thank you for the patience on the additional review :)