Closed Bug 731749 Opened 12 years ago Closed 12 years ago

Security review request for MozTrap (take 2)

Categories

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

Product:
mozilla.org
Component:
Security Assurance: Review Request
Type:
task
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: camd, Assigned: amuntner)

References

Details

1. A quick intro to what this app does.

Case Conductor is a Test Case Management system.  We are writing this to replace Litmus for Mozilla QA.  This is also an open-source project that anyone can access from Github.

2. Where is the source code located?

https://github.com/mozilla/caseconductor

Documentation on setup and using is located here:

http://readthedocs.org/docs/case-conductor/en/latest/index.html

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

IT is in progress on deploying this to caseconductor.allizom.org, but as of today, it is still not setup yet.  It can be run locally like any Django project following the setup instructions in the docs.

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

Product: Mozilla QA
Component: Case Conductor

Please always CC cdawson@mozilla.com

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

It will collect an email address.

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

Case Conductor has Open Web App support.  And it will also support Browser ID soon.  Eventually, we may integrate this with Mozillians, but not in 1.0.  It has its own database.

7. Does this app support logins or multiple roles? If so, we'll need test accounts created for each available role.
Yes.  It has authentication and roles.

8. 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.)
If a user got admin access, they could systematically delete all the data that was established for tests and products.  Worst case scenario would be email theft, and data destruction of test cases, test suites, test runs, test cycles, products and environment settings and users.  So they could wipe out the active objects.  

9. Does this website contain an administration page? If so, have the admin page blockers (listed here) all been addressed?
Yes, it has an admin page at <host>/admin/
And yes we have addressed all the items on the page.

9. This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?
Urgency is fairly high.  We want to release 1.0 on 3/30.
Whiteboard: [pending secreview] → [pending secreview][secr:adamm]
Please also cc me (cmeyer@mozilla.com) on any bugs filed as part of this review.

I also have a couple specific issues to mention from my review of the secure coding guidelines. I believe that I've addressed everything in that document apart from these issues:

1. Logging calls for failed login, ratelimit hit, admin pass reset, admin change pass, new admin user. I'd like to discuss the specific requirements here (CEF?) before working on the implementation.

2. Limiting file upload size. This is possible to do correctly in Django, but not trivial (where by "correctly" I mean taking the upload in chunks and rejecting as soon as it goes over the limit, not accepting the entire big file and then checking its size), and I think it's quite a bit easier to do as a configuration option in most front-end webservers (e.g. Apache LimitRequestBody), so I'd like to know whether just doing it in the front-end server is an option.

3. I have the banned-passwords feature implemented, but the secure coding guidelines say to "contact InfraSec for the full list" - I'll add that full list as soon as I get it.

4. The secure coding guidelines recommend that email/account activation keys should expire after 8 hours. The registration app I'm using only allows it to be set in day increments, and it would be non-trivial to override that. I've set it to one day as the default; is that acceptable?
Dev server:

http://caseconductor-dev.allizom.org

secadmin: Admin
sectestmgr: Test Manager
sectestcreator: Test Creator
sectester: Tester

Password for all: mozilla156
Hi, I'm wondering how this is progressing.  Do you need anything more from me on this?  We initiated a privacy review as well.  You can see that document here: https://wiki.mozilla.org/Privacy/Reviews/CaseConductor
I just sent a response to your last mail regarding some changes we still need.
Whiteboard: [pending secreview][secr:adamm] → [secr:adamm]
QA Contact: mcoates → jstevensen
I have one more question for the security reviewer (in addition to the four in my comment above): is the 15-minute user session timeout recommended in the secure coding guidelines a hard requirement? We are seeing real usability problems with that in user testing, as some forms on the site can easily take longer than 15 mins to fill out, possibly resulting in user session timeout and lost user input. We'd prefer to have the session timeout be at least a full day.
Curtis, I know the Privacy review is in the public review process.  Is there anything on the security side that we can do in the mean time?  Or do those two things go hand-in-hand?  

QA is eager to start using this, but usage on staging is risky due to the 24 hour time window in which data can be lost if there is a db issue. (already hit us once last Friday)

Thanks.
This is assigned to Adam to do security tasks, it was felt a large format team review was unnecessary for this. Adam what is the status here?
So, we changed the name of the product just prior to launch.  The new name is "MozTrap"

latest server details have changed as well.  There are domain redirects, but here's the current domains:

staging: moztrap.allizom.org

We now use BrowserID / Persona for authentication.  So your best bet is to create accounts with emails such as these, and I will apply the appropriate roles to them when you have created them.

Admin:         cdawson+secadmin@mozilla.com
Test Manager:  cdawson+sectestmgr@mozilla.com
Test Creator:  cdawson+sectestcreator@mozilla.com
Tester:        cdawson+sectester@mozilla.com
Assignee: security-assurance → amuntner
I realize you guys are working on it, and thank you. But since we're a month in and are trying to manage expectations around when the production system will be ready, do you have an estimation on when the sec review will be complete?

Thanks!
Component: Security Assurance: Applications → Security Assurance: Review Needed
Is there anything I can do to help move this along?  QA is eagerly awaiting a production version of MozTrap to begin safely using it without the threat of lost data on Staging.  Thanks.
This bug is assigned to Adam for security and the privacy side is also actively being worked. This task is not slated for a meeting type of security review. 

Privacy wise this closed on public comment on Monday (10-Apr). At this point I see one item you all will have to deal with left from the privacy review

https://wiki.mozilla.org/Privacy/Reviews/CaseConductor#Alignment_with_Privacy_Operating_Principles

 Principle: Transparency / No Surprises

Users of the system must create an account and use the system specifically for testing. The system's operation must be clear to the users, both by opening the source (done) and by providing information to users at appropriate times about data collection, etc. For users who upload test cases (things they created themselves), it's obvious and unsurprising what happens.

The Risk: is that test-performing users may be submitting data to the Web App without knowing they're submitting it. For example, I may run a test and submit my results but it may not be clear whether the results contain my browsing history, search queries, etc. It should be clear when I submit data what I'm submitting.

Recommendations: Ensure that users submitting test data to the system are aware of the types of data being submitted (and have access to the data itself). This can be done through a prompt ("Hey, thanks for finishing the test, we're going to collect x, y, z from you now") or a dashboard where users can see the type of data they submit. 

The other items (logging) are blocked by bug 741810 as we don't have a policy for that at this point.
Depends on: 741810
Curtis, thanks for the info.  So, for the data they submit, the URL for "View Results" shows them everything they submitted (/results/runs/).

And the result they put for pass / fail / invalid submits the result as soon as they click the button.  Are you thinking that perhaps there's one of the status messages at the top (like if you edit a test case) that says "Result submitted" or something along those lines?  For each case they mark with a result?

The following data is submitted when they run a test:

Pass:
1. their username as "Tester"
2. pass status

Fail:
1. their username as "Tester"
2. comment
3. optional bug url

Invalid:
1. their username as "Tester"
2. comment

So does the "thanks" indication above, along with the existing results pages address everything required?
Oops.  On Fail and Invalid above, I neglected to mention that, obviously, fail or invalid status is also submitted respectively.
Cam, 
Just to be clear I did not write that bit. I copied it from the wiki for the privacy review for this, I suspect that Sid wrote it. That said, what he is getting at is that when anyone submits the data there should be some kind of notification to them that the data was submitted, however you all want to accomplish that. The second part is telling the user exactly what was submitted (urls, history, whatever). As for weather it passes privacy muster that will have to be decided by Sid so I suggest starting a conversation with him on this topic.
Hi Curtis/Adam:  I just got off a call with Sid.  So he's cleared us on the part where you notify the user what info you're submitting, and says the only last bit is the part where everyone else is with regard to standardized logging practices policy that's not complete yet on your guy's end.

So we are unblocked for Privacy now.  And I wanted to see what else we may need to do to get the security review done, and get approval to push MozTrap to prod.
Well we have at least 3 reviews blocked by the logging thing. I will have to get with mcoates and see what he wants to do. He is traveling for MozCamp this week so I don't think we will have resolution on the blocker till next week sometime. He might be OK with going with out logging laid out and he might not.
This is the REAL security review request.  The other one was old.  Sorry I didn't close it before.

I updated the title to reflect the product's current name: MozTrap.
Summary: Security review request for Case Conductor (take 2) → Security review request for MozTrap (take 2)
Curtis/Adam/Gary: other than the logging part, is there any other action required by me to get through the security review?  Is there an ETA for this being complete?  Bob Moss is asking for some status on this as QA is waiting for it.

Thanks,
-Cam
Severity: normal → major
(In reply to Curtis Koenig [:curtisk] from comment #16)
> Well we have at least 3 reviews blocked by the logging thing. I will have to
> get with mcoates and see what he wants to do. He is traveling for MozCamp
> this week so I don't think we will have resolution on the blocker till next
> week sometime. He might be OK with going with out logging laid out and he
> might not.

Curtis, I'm sorry the logging thing is blocking you, but why can't we get this security review over with and update the MozTrap site after you guys have completed your logging review (if we in fact need to make changes, pending on that review's outcome).  I saw in the logging bug that it is a Q2 goal for your team; however, having MozTrap live was a *Q1* goal for us.

We are happy to make changes to MozTrap on an ongoing basis as needed (especially any bugs you want addressed), but we simply cannot wait until Q3 to get a clear go-ahead or a clear set of security issues that need to be fixed before we push to production.

We have several QA teams blocked by this issue, and we need some kind of resolution here. Can we get either an "all clear" or a list of things you need us to fix before pushing to production by the end of this week?
So I think some wires are crossed, the logging thing blocks the privacy review. Per Sid:

Hey Curtis,

I ran through the review results with Cam, and the only thing unresolved
is the logging (which has an action item for secass, er I mean
Assurance.  So I told him that I have no qualms about them releasing as
long as y'all are green lighting them.

Once the logging/retention policy is done and public, we can resolve
this case conductor review as complete.

-Sid

So that item remains on the table and we can fix it later if needed.

As for the security review, that is assigned out to Adam per comment 11 (owner of the bug). So that said; Adam what is the timeline looking like for you to complete your work on this bug?
CC'ing Adam, he may not have gotten bugmail since he was only an assignee and not on the cc list.

Adam, there are several questions in comment 7, comment 11, comment 19, and comment 21, and please note this secreview blocks the several QA teams from moving forward (as per comment 20).
I didn't see any of the messages - thanks Gary - I will update the status later today.
Cameron: I created four accounts:

    amuntner+secadmin@mozilla.com
    amuntner+sectestcreator@mozilla.com
    amuntner+sectester@mozilla.com
    amuntner+sectestmgr@mozilla.com

Please let me know when they're provisioned. In the meanwhile, I'll start testing what I can see, so far. My feel right now is that it should be completed on Tues.
Adam: this is the correct new repo for the code:
https://github.com/mozilla/moztrap.  Though the other one will point you to this one as well.

I applied the corresponding roles to those new users.  Thanks, and let me know if you have any questions.  We are on irc at #moztrap.
Thanks. I'll let you know if the staging server moztrap.allizom.org acts up while I'm testing.
Hi Adam,

Did you need to re-create those users on the -dev site?  If so, please create them and let me know, then I'll apply the appropriate roles to them.
Keywords: privacy-review-needed, sec-review-needed
Whiteboard: [secr:adamm] → [pending secreview]
Hi Adam--  Just wanted to check on the latest status of your security review.  Thanks.
Hi Cameron, I expect to finish it this evening
Adam, were you able to finish the sec review testing last night?  What's the status?.  thanks.
Apology - I wrote an update into this bug, but never clicked save "changes."

I  only found one minor issue, that the /admin dir password formfield isn't set to autocomplete=off.

I'll file a bug for that one, wanted to shift gears to work on something else, and just give it another once-over this evening to make sure I didn't miss anything, if that's OK.
Two more - that's it. Good job on this app, guys! It frustrated the heck out of me. 

Logout CSRF https://moztrap-dev.allizom.org/users/logout/?next=/manage/profiles/

django /admin is publicly accessible

I will open tickets in the AM
Logout CSRF - 751932

Password field autocomplete enabled - 751936

I'm not going to file one for /admin being publicly accessible - looking at our review guide, it appears that the policy is /admin should be SSL protected, which it is, and doesn't mention that it should not be easily Internet accessible.
Adam: thanks!  this is great.  Those are both pretty easy to fix, so this should go quickly.  Though I'm not able to view bug 751932.  Would you CC cmeyer@mozilla.com and I on that one?

So, once these are fixed, what's our next step?  You verify them and we are then cleared for going to production?  Sid says we're good to go on the Privacy side.  

Thanks!
Hey Adam: what's the status on this guy?  Could we get the go ahead by the end of the week to go to prod?

Thanks.
-Cam
Whiteboard: [pending secreview] → [secreview complete]
Keywords: sec-review-complete
Whiteboard: [secreview complete]
Adam, if you can't resolve this bug due to the policy on logging being incomplete, would you please add a comment that we are cleared to go to production?
Sure, you're all clear to go into production.
Flags: sec-review+
changing to resolved-fixed based on comments, when defendant bugs are closed we can go to verified-fixed
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.