812234 - SecReview Item: Test release kickoff system

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance, task)

Description

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

SecReview tracking bug
Actions regarding the review of the dependent bug should be tracked here.
----
Need to preform manual/automated testing of this new web based tool to ensure security practices

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Working on this today.

Comment 2

[[e-mail] (:bhearsum)]

Sorry - forgot to mention that the repository moved over to git.m.o: http://git.mozilla.org/?p=build/release-kickoff.git;a=summary

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I've looked at the code and I think this is all pretty good.

* Forms have good validation
* A high level framework is used to deal with forms (flask.ext.wtf)
* CSRF has been added as we suggested in the review meeting (through flask.ext.wtf)
* X-Frame-Options is set to SAMEORIGIN
* Database access code is using SQLAlchemy and programmatic queries/filters.
* No direct/constructed SQL
* Templates use Jinja2 with good escaping

Questions:

* What about the /csrf_token view that is exposed? I'm just wondering if that is really needed? It seems like a leftover from development or testing? If that is the case then I suggest to remove it.

* Does this app need CEF logging? jstevensen can answer that. (We probably want to log CSRF failures?)

* I see that the forms use {{form.foo()|safe}} for template values. The documentation for Flask-WTF says "However widgets in the latest version of WTForms return a HTML safe string so you shouldn’t need to use safe." - See http://packages.python.org/Flask-WTF/

Random notes:

* This is not security related but I would suggest to put the checkboxes for the product selection in front of the name or lay them out in a list to avoid confusion. I checked the wrong one for testing :-)

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Stefan Arentz [:st3fan] from comment #3)
> I've looked at the code and I think this is all pretty good.

Thanks!

> * What about the /csrf_token view that is exposed? I'm just wondering if
> that is really needed? It seems like a leftover from development or testing?
> If that is the case then I suggest to remove it.

We use this view in the command line client to get an initial token. We could get one by loading a full HTML page, I suppose, but this seems easier. Happy to change this if there's a better way.

> * I see that the forms use {{form.foo()|safe}} for template values. The
> documentation for Flask-WTF says "However widgets in the latest version of
> WTForms return a HTML safe string so you shouldn’t need to use safe." - See
> http://packages.python.org/Flask-WTF/

I'd barely given this part any thought, to be honest. Sounds like you're right - I'll make this change.

> Random notes:
> 
> * This is not security related but I would suggest to put the checkboxes for
> the product selection in front of the name or lay them out in a list to
> avoid confusion. I checked the wrong one for testing :-)

Yeah, I'm still working on prettying things up over in bug 810422 :).

Comment 5

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

> > * What about the /csrf_token view that is exposed? I'm just wondering if
> > that is really needed? It seems like a leftover from development or testing?
> > If that is the case then I suggest to remove it.
> 
> We use this view in the command line client to get an initial token. We
> could get one by loading a full HTML page, I suppose, but this seems easier.
> Happy to change this if there's a better way.

No i think that is fine if that view is behind the same auth checks as the forms. (Which it is, since the whole app is behind auth)

 S.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Ben, the only thing we would like to see is CEF logging as described on:

https://mana.mozilla.org/wiki/display/SECURITY/CEF+Guidelines+for+Application+Development+at+Mozilla

Specifically for things that are potential security issues like someone trying to submit a form with an invalid CSRF token or giving invalid input.

There are some python libraries listed at the end of that page. You can use syslog and it will be pretty simple to integrate.

Let me know if you have questions or if I can help with things.

Comment 7

[[e-mail] (:bhearsum)]

I've almost got the CEF logging done now. It's not clear to me where the messages should go though....should they just be in the application log with the rest of the logging information? Do I need a separate file? Sorry Stefan, I think I asked you about this last week already....

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #7)
> I've almost got the CEF logging done now. It's not clear to me where the
> messages should go though....should they just be in the application log with
> the rest of the logging information? Do I need a separate file? Sorry
> Stefan, I think I asked you about this last week already....

cc'ing Brandon, because he might know, too.

Comment 9

[[e-mail] (:bhearsum)]

https://github.com/bhearsum/release-kickoff/compare/master...cef-logging has an implementation that uses a separate file for CEF logs. Easy enough to switch to syslog or something else, though.

Comment 10

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #8)
> (In reply to Ben Hearsum [:bhearsum] from comment #7)
> > I've almost got the CEF logging done now. It's not clear to me where the
> > messages should go though....should they just be in the application log with
> > the rest of the logging information? Do I need a separate file? Sorry
> > Stefan, I think I asked you about this last week already....
> 
> cc'ing Brandon, because he might know, too.

I asked around on IRC and got an answer:
11:23 < parker> bhearsum: hi, ship them via syslog, you can use the local4 
11:24 < parker> bhearsum: which logs/app are these? thx
11:27 < bhearsum> parker: ship-it-dev.allizom.org
11:28 < bhearsum> will i need to request any permission changes to give the app 
                  write access?
11:30 < parker> bhearsum: nope if you write to the local4 syslog faclity it should 
                automaticaly get to security

Comment 11

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I think this is all good with the CEF logging implemented through 825238

Comment 12

[Brandon Burton [:solarce]]

Logging is making it to syslog1.private.phx1

[[e-mail]1 generic1.dev.webapp.phx1.mozilla.com]# grep wsgi 2013-01-10.log 
Jan 10 13:26:15 generic1 mod_wsgi: Jan 10 13:26:15 generic1.dev.webapp.phx1.mozilla.com CEF:0.9|Mozilla|Release Kickoff|N/A|User Input Failed|User Input Failed|4|cs1Label=requestClientApplication cs1=Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20130107 Firefox/20.0 requestMethod=POST request=/submit_release.html src=24.52.200.235 dhost=ship-it-dev.allizom.org suser=[e-mail] cs6Label=branch cs4Label=version cs2Label=buildNumber cs3Label=l10nChangesets cs5Label=mozillaRevision cs6=['Branch is required'] cs4=['Invalid version format.'] cs2=['Build number is required.'] cs3=['L10n Changesets are required.'] cs5=['Mozilla revision is required.']