Closed Bug 978976 Opened 12 years ago Closed 10 years ago

Security Review: Review Board 2.0

Categories

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

Product:
mozilla.org
Component:
Security Assurance: Review Request
Platform:
x86
macOS
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mcote, Unassigned)

Details

During our Review Board trial period we had a number of issues raised (many of which are documented in bug 947596), which have been (or are being) fixed in version 2.0 of Review Board. Once it is released, we'll want to upgrade our instance. We have, to my knowledge, no further objections to switching to Review Board as our central review tool, though further Bugzilla integration will be done over time. I am not sure how much of version 2.0 was reviewed in bug 874767, but regardless there have been a number of changes to 2.0 since that review. I just want to know if a further review is needed before we upgrade. I'd also eventually like to get approval to host this at a mozilla.org domain instead of allizom.org. Related, I split the Bugzilla-related stuff into its own extension (https://github.com/mozilla/rbbz). It's all the same code, just rejiggered a bit (and some was added directly to the core). The current Review Board 2.0 code is at https://github.com/reviewboard/reviewboard/tree/release-2.0.x. It is close to complete, but I couldn't tell you exactly how close. Most of bug 874767 comment 0 still applies, except for timing information, obviously. Although Review Board 2.0 isn't out yet, I want to put this on security's radar now so we can upgrade as soon as possible after the release.
Note that we're also developing some changes on top of RB, which should probably go through the security review once they're a bit stable.
Assignee: nobody → sarentz
Summary: Security Review for Review Board 2.0 → Security Review: Review Board 2.0
Do we host our own instance or do we use the www.reviewboard.org hosting?
Flags: needinfo?(mcote)
We host our own. We have a staging server set up at reviewboard.allizom.org, although it is old and won't be updated for another week or so. We also have a dev server at reviewboard-dev.allizom.org running the latest from the "customizations" branch of https://github.com/mozilla/reviewboard, which just has a few UI changes. Dev is also running the now-separate Bugzilla-integration plugin, https://github.com/mozilla/rbbz, which will be in active development for another week or so.
Flags: needinfo?(mcote)
(In reply to Mark Côté ( :mcote ) from comment #0) > I am not sure how much of version 2.0 was reviewed in bug 874767, but > regardless there have been a number of changes to 2.0 since that review. I > just want to know if a further review is needed before we upgrade. I'd also > eventually like to get approval to host this at a mozilla.org domain instead > of allizom.org. The previous review was focused on the 1.7 branch, but there has been little difference between 1.7 and 2.0 when I last looked. But wow, this is already a year ago... :)
Blocks: 889431
Assignee: sarentz → nobody
Stefan - why did this bug go back to nobody?
Flags: needinfo?(sarentz)
Because I don't think we will do a review for this app.
Flags: needinfo?(sarentz)
Why is that? We plan on rolling out a new set of features along with an upgraded Review Board core (2.0) in a few weeks. We only have an allizom.org domain right now, as security didn't want to give us a proper mozilla.org domain until Review Board had been through a more thorough security review. I was hoping a review of the latest 2.0 code could occur as we were finishing up a few bugs in our hg integration.
Hi Mark, I will be talking about some security review changes next week, but the reason why we are resisting certain security reviews is because they are extremely time consuming and challenging to perform, and there have been several concerns raised about the value and approach required for security reviews. Several processes block on feedback from the security team, but we have little input from what those teams expect from us, and we don't generally get the information we require to perform a proper security review. Bottom line, if a security review is required, or requested, we will conduct it, but we need to schedule a rapid risk assessment [1] scheduled to determine the nature and scope of the assessment required. We can get this scheduled early next week to ensure that we help with your release, but until we have scoped the expectations for a security review, and what the needs of the project are, we can't prioritize the work. [1] https://mana.mozilla.org/wiki/pages/viewpage.action?pageId=37684270
As promised during the assessment, here's what we're using. I'm going to write a document explaining our system in more detail at some point; I'll link it here when it's done. But here are the basics: These are the third-party pieces (the core): Review Board release-2.0.x branch from June 25 (we will probably update to 2.0.3 or whatever stable release is most current when we finish development on our customizations). https://github.com/reviewboard/reviewboard/tree/release-2.0.x rbtools master branch from around then. https://github.com/reviewboard/rbtools Djblets (base library used in Review Board) release-0.8.x branch from June 18 with one cherry-picked fix from June 28 (again we'll be upgrading to a stable release at some point). https://github.com/djblets/djblets/tree/release-0.8.x All our custom code is in hg.mozilla.org/hgcustom/version-control-tools: version-control-tools/pylib/rbbz contains the main Review Board extension that handles Bugzilla integration as well as translating some of the data from the hg review repo. version-control-tools/pylib/rbmozui contains a Review Board extension with UI enhancements suited towards our planned work flow. version-control-tools/hgext/reviewboard contains client and server extensions for hg that transform local commits into diffs for Review Board. This is the part that builds up the initial review requests in Review Board. It in turn relies on other libraries in version-control-tools/pylib (loaded via version-control-tools/hgext/bootstrap.py). The only bit of Bugzilla code is the bit that deletes review requests when a bug is made confidential, done in bug 993223.
Notes from the RRA done with Mark on 7/15: google doc (requires access) https://docs.google.com/a/mozilla.com/spreadsheets/d/1MXnvyGrOLOL0-jpowQY65uOLP6lL1USjNEq9m16tLoA/edit#gid=0 * The current version of ReviewBoard (2.0) targets public data types only, like public source code. Out of scope are Internal and confidential data (patches or other data types that could be reviewed through this system). We recommend to update the risk assessment and the review before using ReviewBoard for confidential data types. * HIGH risk on access control There is an attack vector through weak access controls that could lead to high PR/Image impacts. If an attacker gains access to a reviewer's account in reviewboard, the attacker can accept (r+) patches that are fraudulent. The sheriffs who perform the merges normally accept the patches of trusted reviewers. Therefore, weak access control in reviewboard could lead to fraudulent patches reaching the trees. This is partially mitigated by sending bugzilla notifications to reviewers. If a reviewer's account is compromised, and a patch is r+-ed, the reviewer will receive a notification of the review. However, this notification could easily slip through the large volume of bugmail developers receive every day. Recommendations: 1. Perform a code review of reviewboard's access control code, and mark the areas of the code that are sensitive for followup reviews when new versions are deployed. (needinfo :yvan for followup) 2. Ensure that reviewboard's access logs contains the identity of the reviewer when a patch is r+, as well as the IP address of the reviewer. Cross reference that data to track access from unusual locations. We recommend integrating with OpSec's MozDef log processing platform for this. (needinfo :jeff for followup) ReviewBoard relies on bugzilla's xmlrpc auth backend. More information from :mcote is copied below: ----- Review Board is a Django project to which we provide our own AuthBackend[1][2] which uses Bugzilla (no LDAP at all). When you log in (either via the Review Board API or the UI), your username and password are sent to Bugzilla via its XMLRPC API for verification by the Review Board server. If successful, Review Board gets back login tokens (cookies), which it stores in the server database, in session data. When Review Board needs to do something involving Bugzilla--querying for users, mirroring a comment, etc.--it gets the tokens from the database and sends those along with the request. All actions involving Bugzilla are confined to one class[3] in our Review Board extension (with another supporting class for the XMLRPC transport[4]). Easy auditing. :) The Review Board user table is populated with information about Bugzilla users (username, realname, Bugzilla ID), mainly just to use as foreign keys. Passwords are never stored, although as mentioned above, cookies are. FYI, a patch will be landing in upstream Bugzilla very shortly which adds support for revokable, application-specific API keys. Logging in via the REST API to get a token will be marked as deprecated and removed in a future version. At some point after this patch is applied to BMO, we will look into how we can convert the Review Board Bugzilla integration to use them, since it is a safer option. We'll also be investigating how to give fine-grained permissions to these keys, so that, for example, you could generate an API key that can only add comments and attachments. No timeline for these developments, however. ---
Flags: needinfo?(yboily)
Flags: needinfo?(jbryner)
What would be the ETA for this work? I'd love to be able to deploy in a few weeks.
Ping again. We are aiming for deployment in the last week of August; it's been almost 3 weeks since ulfr posted the notes from the RRA, so I'm really hoping someone can get to this soon.
Mark; do you know the best way to accomplish the hook necessary for the logging mentioned above: "2. Ensure that reviewboard's access logs contains the identity of the reviewer when a patch is r+, as well as the IP address of the reviewer. Cross reference that data to track access from unusual locations. We recommend integrating with OpSec's MozDef log processing platform for this. " specifically: when a patch is r+ ? would that also be caught via the xmlrpc to bugzilla?
Flags: needinfo?(jbryner)
Just to clarify, in Review Board terms, when a patch is reviewed and deemed worthy of committing, the reviewer clicks the "Ship it!" button. This, in turn, causes the associated patch in Bugzilla (which contains just the URL to the Review Board review request) to be r+ed (http://hg.mozilla.org/hgcustom/version-control-tools/file/6f1da8433d97/pylib/rbbz/rbbz/bugzilla.py#l209). Review Board also logs that the patch has been r+ed and by whom (although we would need to ensure that Review Board logs are being recorded properly when the system is deployed; I know sometimes this is problematic with an Apache + WSGI setup). We don't log the IP address, but this should be easy to add. I don't know about this log-processing platform, but I'm sure we could add functionality for that.
Great! Where does logging.info in this line: http://hg.mozilla.org/hgcustom/version-control-tools/file/6f1da8433d97/pylib/rbbz/rbbz/bugzilla.py#l212 end up? If it goes to an existing log destination (syslog, etc) we can pick it up from there. If not we can use something like https://github.com/gdestuynder/mozdef_lib to post a quick json message to mozdef
Steven, can you answer Jeff's question above, since Mark is out this week?
Flags: needinfo?(smacleod)
(In reply to Jeff Bryner [:jeff] (use NEEDINFO) from comment #15) > Great! > > Where does logging.info in this line: > http://hg.mozilla.org/hgcustom/version-control-tools/file/6f1da8433d97/pylib/ > rbbz/rbbz/bugzilla.py#l212 > > end up? > > If it goes to an existing log destination (syslog, etc) we can pick it up > from there. If not we can use something like > https://github.com/gdestuynder/mozdef_lib to post a quick json message to > mozdef Those will end up in the Review Board log. For reviewboard-dev.allizom.org that's at "/data/www/reviewboard-dev.allizom.org/reviewboard/logs/reviewboard.log" For reviewboard.mozilla.org it should be "/data/www/mozilla/reviewboard/logs/reviewboard.log" an example from the dev log is: > 2014-08-20 17:41:04,022 - INFO - - r+ from smacleod@mozilla.com on bug 1055929. > 2014-08-20 19:34:58,406 - INFO - - r+ from smacleod@mozilla.com on bug 1047465. It should be pretty easy to add more extensive logging if it turns out we need it.
Flags: needinfo?(smacleod) → needinfo?(jbryner)
(In reply to Steven MacLeod [:smacleod] from comment #17) > For reviewboard.mozilla.org it should be > "/data/www/mozilla/reviewboard/logs/reviewboard.log" Sorry, it would be "/data/www/reviewboard.mozilla.org/reviewboard/logs/reviewboard.log"
Is this on the dev server: reviewboard1.dev.webapp.scl3.mozilla.com A 10.22.81.181 For some reason I can't access it, but I can access prod. Prod doesn't seem to have any logs. We can go two ways depending on how you want to do this; the app can trigger a log to MozDef for just these records, or we can use heka to pick up the logs and send to MozDef. The app triggering the log would just be some python to send a json version of the above entry as a post to an http endpoint. The heka route would be an install of heka on the reviewboard servers to have it tail the log files, convert to json and post to an http end point.
You can't access dev because the mecurial repo is hosted locally: all ssh access is handled through pash, and all non-root access is handed off to mecurial, just like for hg.mozilla.org:22. So, try root@. Prod's hardly been up for more than a couple days (and web and mercurial are separate), but something did seem to be weird with the logging there. It's set the same as dev, so I'm not sure what's wrong. I have no experience with heka and I'm not a dev, so I don't have any particular input on how we get the logs to you. I assume we have a puppet module for the heka stuff? If so, that'll probably be faster to set up...
No longer blocks: 889431
Blocks: 1021929
Ahh..that makes sense, thanks. I'll see what I can do for a heka config to pick out the interesting lines (avoiding stack traces and the like)
Flags: needinfo?(jbryner)
(In reply to Steven MacLeod [:smacleod] from comment #17) > > For reviewboard-dev.allizom.org that's at > "/data/www/reviewboard-dev.allizom.org/reviewboard/logs/reviewboard.log" > > For reviewboard.mozilla.org it should be > "/data/www/mozilla/reviewboard/logs/reviewboard.log" Actually, prod is /data/www/reviewboard.mozilla.org/reviewboard/logs/reviewboard.log. And it's finally logging; I must have been brain dead when I last looked, because it was just a simple permissions change. /o\ :jeff, is there anything I need to do to make heka go and are there docs, or can you handle that?
Flags: needinfo?(jbryner)
Also, would like your (security's) opinion on bug 1067525. Short summary: while admitting that sending Bugzilla username & password over the wire isn't great, should we hack in an option to use a Bugzilla token instead before going live (which introduces a new auth endpoint), or are we okay for now, acknowledging that we're planning to switch to API tokens when they are available in Review Board and/or BMO?
(In reply to Mark Côté [:mcote] from comment #23) > Also, would like your (security's) opinion on bug 1067525. Short summary: > while admitting that sending Bugzilla username & password over the wire > isn't great, should we hack in an option to use a Bugzilla token instead > before going live (which introduces a new auth endpoint), or are we okay for > now, acknowledging that we're planning to switch to API tokens when they are > available in Review Board and/or BMO? FYI: The hacked-in option Mark is referring to is under review in Bug 1067525
We've almost got all blockers resolved. Any update here? I'd love to go live next week.
(In reply to Kendall Libby [:fubar] from comment #22) > (In reply to Steven MacLeod [:smacleod] from comment #17) > > > > For reviewboard-dev.allizom.org that's at > > "/data/www/reviewboard-dev.allizom.org/reviewboard/logs/reviewboard.log" > > > > For reviewboard.mozilla.org it should be > > "/data/www/mozilla/reviewboard/logs/reviewboard.log" > > Actually, prod is > /data/www/reviewboard.mozilla.org/reviewboard/logs/reviewboard.log. > And it's finally logging; I must have been brain dead when I last looked, > because it was just a simple permissions change. /o\ > > :jeff, is there anything I need to do to make heka go and are there docs, or > can you handle that? I'll take it.
Flags: needinfo?(jbryner)
(In reply to Steven MacLeod [:smacleod] from comment #17) > (In reply to Jeff Bryner [:jeff] (use NEEDINFO) from comment #15) > > Great! > > > > Where does logging.info in this line: > > http://hg.mozilla.org/hgcustom/version-control-tools/file/6f1da8433d97/pylib/ > > rbbz/rbbz/bugzilla.py#l212 > > > > end up? > > > > If it goes to an existing log destination (syslog, etc) we can pick it up > > from there. If not we can use something like > > https://github.com/gdestuynder/mozdef_lib to post a quick json message to > > mozdef > > Those will end up in the Review Board log. > > For reviewboard-dev.allizom.org that's at > "/data/www/reviewboard-dev.allizom.org/reviewboard/logs/reviewboard.log" > > For reviewboard.mozilla.org it should be > "/data/www/mozilla/reviewboard/logs/reviewboard.log" > > an example from the dev log is: > > 2014-08-20 17:41:04,022 - INFO - - r+ from smacleod@mozilla.com on bug 1055929. > > 2014-08-20 19:34:58,406 - INFO - - r+ from smacleod@mozilla.com on bug 1047465. > > It should be pretty easy to add more extensive logging if it turns out we > need it. Can you point me to the logging formatter for this? I'm writing the heka parser and want to make sure I get the format/fields/delimiters correct rather than just guessing based on the dev content. In particular I notice there are a couple fields that are always - and wondering if they are placeholders?
I believe it uses default Django logging (https://docs.djangoproject.com/en/1.6/topics/logging/). The "- -" part means that the logger module name is blank, which means the root logger, and this appears to be how Review Board does all logging. So "<date> <time> - <level> - - r+ from <user> on bug <bug id>" should be how all r+s are logged.
OK, heka config is up and working in dev and logs are going where they need to in MozDef.
Thanks jeff. ulfr, are we done here?
Flags: needinfo?(jvehent)
No longer blocks: 1021929
:jeff - is there any more work needed to complete reviewboard logging to mozdef in production? :mcote - we have logs, and we have an intelligent event analysis platform (mozdef). Two questions: 1. Do you think we could implement fraud detection rules that would be relevant to reviewboard? 2. Would you be interested in reporting based on the logs? It's python, we can do things.
Flags: needinfo?(jvehent)
Nope, it's deployed to dev and prod.
(In reply to Julien Vehent [:ulfr] (use needinfo) from comment #31) > :mcote - we have logs, and we have an intelligent event analysis platform > (mozdef). Two questions: > 1. Do you think we could implement fraud detection rules that would be > relevant to reviewboard? Off hand I'm not sure what we could do. Do you have any general ideas? > 2. Would you be interested in reporting based on the logs? It's python, we > can do things. Quite possibly! Needinfoing gps in case he has any ideas. I'm also going to clear yvan's needinfo since I dunno if he'll ever get around to commenting here.
Flags: needinfo?(yboily) → needinfo?(gps)
(In reply to Mark Côté [:mcote] from comment #33) > (In reply to Julien Vehent [:ulfr] (use needinfo) from comment #31) > > :mcote - we have logs, and we have an intelligent event analysis platform > > (mozdef). Two questions: > > 1. Do you think we could implement fraud detection rules that would be > > relevant to reviewboard? > > Off hand I'm not sure what we could do. Do you have any general ideas? Account theft is a concern, and I think we could experiment with geolocation to detect it, by tracking the general area of a reviewer based on the IP in the logs. If that area changes by a large margin, generate an alert (email to reviewer, IRC to sheriffs,...). If we are concerned about access control breach, we could alert on new reviewers, or reviewers that have very low activity (like one r+ every 90 days). We would need to tweak the detection thresholds, as I don't know what "normal" activity would look like on reviewboard. The general idea here is to identify reasonable usage patterns in our review workflow, and use log analysis to flag unusual behavior. That's partly what MozDef was built for. I think we could gradually implement fraud detection features for reviewboard over the next few quarters, while we ramp up its usage.
I would like to add a custom log stream to Review Board capturing events that are relevant to Mozilla. User X logged in, associated with Bugzilla account Y. Updated username of Z. Review request created. Review request published. Bug updated. etc. I want these for debugging and workflow analysis. But I imagine we could take the stream of account events for security/access control/fraud monitoring. I imagine such a log stream would write directly to Heka (I want to retain the structured format of events without involving log parsing). Security could be the first consumer (unless we encounter enough bugs after that that we need this stream to help diagnose bugs). If someone files a bug and gives me details on what events you'd like to see from Review Board, we can implement direct-to-heka logging in our RB customizations.
Flags: needinfo?(gps)
An audit log sounds wonderful. I think the current logging is a good start but a bit inconsistent: logins seem to be debug level, success logins don't seem to be logged, etc. The way you mention is a good way to start: think backwards from the workflow you mention and generate the logs at the important steps along the way. Ideally the log structure contains all the important points (ip, username, action, target) in a single log instead of forcing a correlation step. For the destination, heka can pick these up from the existing log (ideally key=value style) or we can do what marketplace does and post direct in CEF or JSON format to mozdef. Do you want a separate bug for this?
Please file a separate bug in the MozReview component for log collection. Propose your ideal data set and we'll see what we can provide.
(In reply to Jeff Bryner [:jeff] (use NEEDINFO) from comment #29) > OK, heka config is up and working in dev and logs are going where they need > to in MozDef. fyi, we're changing the log dir for RB to /var/log/reviewboard/. the logs are getting wiped out by the deploy script, which is bad. also, the old location is entirely non-obvious.
OK, let me know when it takes effect and I'll sync up heka
(In reply to Jeff Bryner [:jeff] (use NEEDINFO) from comment #39) > OK, let me know when it takes effect and I'll sync up heka Hi Jeff, this took effect a long time ago but it looks like we missed following up with you. Do you mind syncing up heka and confirming that you can still see our logs? Thanks!
Flags: needinfo?(jbryner)
OK, I think I've got it matched up to the new location. I'm getting logs now from reviewboard1.webapp.scl3.mozilla.com. Any other server I should be checking?
Flags: needinfo?(jbryner)
the other prod web VM is: reviewboard2.webapp.scl3.mozilla.com there's also reviewboard-hg1.dmz.scl3.mozilla.com, but it's an hg server, so the logging will be different (mostly openssh)
Hrm..can't seem to ssh into that hostname. Is there an alt name for it?
which, reviewboard-hg1? you'll need to use root@, as it uses pash to wrap hg serve.
I'm referring to reviewboard2.webapp.scl3.mozilla.com.
waaaat. That's the right hostname, and I see successful logins with the infrasec user but immediately ending: Aug 3 14:13:19 reviewboard2.webapp.scl3.mozilla.com sshd[31456]: Accepted publickey for infrasec from 10.22.75.203 port 46770 ssh2 Aug 3 14:13:19 reviewboard2.webapp.scl3.mozilla.com sshd[31456]: pam_unix(sshd:session): session opened for user infrasec by (uid=0) Aug 3 14:13:19 reviewboard2.webapp.scl3.mozilla.com sshd[31456]: User child is on pid 31458 Aug 3 14:13:19 reviewboard2.webapp.scl3.mozilla.com sshd[31458]: Connection closed by 10.22.75.203 Aug 3 14:13:19 reviewboard2.webapp.scl3.mozilla.com sshd[31458]: Transferred: sent 2168, received 1912 bytes Aug 3 14:13:19 reviewboard2.webapp.scl3.mozilla.com sshd[31458]: Closing connection to 10.22.75.203 port 46770 Aug 3 14:13:19 reviewboard2.webapp.scl3.mozilla.com sshd[31456]: pam_unix(sshd:session): session closed for user infrasec sadly, no errors anywhere that I can see.
Any luck with reviewboard2.webapp.scl3.mozilla.com?
Flags: needinfo?(jbryner)
Sorry for the late reply. Logs seem to be flowing correctly now.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(jbryner)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.