Opened 12 years ago
Last modified 12 years ago
#76 closed defect (FIXED)
prevent CSRF and similar things
|Reported by:||Jakob Kramer||Owned by:||nyergler|
At the moment you can perform cross-site request forgeries, cross-site scripting and similar things on MediaGoblin pages.
Change History (25)
comment:1 by , 12 years ago
comment:2 by , 12 years ago
:: X-Content-Type-Options: nosniff Some docs I could quickly find: - `https://bugzilla.mozilla.org/show\_bug.cgi?id=471020 <https://bugzilla.mozilla.org/show_bug.cgi?id=471020>`_ - noscript implements this for Firefox - IE has it IMHO this header should be on all responses by mediagoblin... just my 0.02 unit-of-currency.
comment:3 by , 12 years ago
"Content Security Policy" (CSP) might really be a good add on to have. Noone should rely solely on this, but it might make things a lot safer if other security guards fail. A simple ``allow 'self'`` might already get a lot of things better. - `https://developer.mozilla.org/en/Security/CSP/Introducing\_Content\_Security\_Policy <https://developer.mozilla.org/en/Security/CSP/Introducing_Content_Security_Policy>`_ - `https://developer.mozilla.org/en/Security/CSP/CSP\_policy\_directives#options <https://developer.mozilla.org/en/Security/CSP/CSP_policy_directives#options>`_ (from today's discussion)
comment:4 by , 12 years ago
Sorry, I just don't have enough time to do something for MediaGoblin at the moment.
comment:5 by , 12 years ago
Okay, thanks for letting us know, and we'll welcome contributions from you if in the future you do have time!
comment:6 by , 12 years ago
This bugs turns into some vague "We need better web security" bug. That said, if someone wants to work on some specific aspect of web security (say CSP), she/he should probably create a dedicated Bug and notify this bug.
comment:7 by , 12 years ago
I think this bug has kind of turned into that but there's a much more specific goal here, and that's to specifically handle CSRF issues in an easy way. Something like: `https://docs.djangoproject.com/en/dev/ref/contrib/csrf/ <https://docs.djangoproject.com/en/dev/ref/contrib/csrf/>`_
comment:8 by , 12 years ago
|Milestone:||0.0.3 → 0.0.4|
cwebber wanted to take care of CSRF for 0.0.4. I suggest, we turn this bug into the general "security tracking bug" and create a new "Create easy CSRF framework"-bug for the actual work.
comment:9 by , 12 years ago
Removing myself as the assignee since I haven't **really** started working on it yet, but I will shortly if nobody else jumps on this one and will reclaim.
comment:10 by , 12 years ago
|Milestone:||0.0.4 → 0.0.5|
We release 0.0.4, so I'm bumping this to 0.0.5.
comment:11 by , 12 years ago
- `http://w3af.sourceforge.net/videos/video-demos.php <http://w3af.sourceforge.net/videos/video-demos.php>`_ - `http://subgraph.com/products.html <http://subgraph.com/products.html>`_ Both look like good FOSS tools for scannign for vulnerabilities? Caleb has pointed to this article (thanks!): `http://sectooladdict.blogspot.com/2011/08/commercial-web-application-scanner.html <http://sectooladdict.blogspot.com/2011/08/commercial-web-application-scanner.html>`_ We should really only use FOSS tools; that article lists both proprietary and FOSS. It looks like there are quite a few good ones. W3AF appears to be the most feature-rich.
comment:12 by , 12 years ago
I did some scans with w3af today. It didn't see a CSRF or any other vulnerability after a full\_audit. There was one login it claimed to find: :: Found authentication credentials to: "http://127.0.0.1:6543/auth/login/". A correct user and password combination is: test1/53cr37. This vulnerability was found in the request with id 7679. however, these credentials didn't work in the webUI. One warning that gave me pause in interpreting the report was :: The remote network has an active filter. IMPORTANT: The result of all the other plugins will be unaccurate, web applications could be vulnerable but "protected" by the active filter. x(
comment:13 by , 12 years ago
Thanks for the scan, Caleb. Yeah, we do need to provide some sort of way to rate-limit logins and temporarily ban people who try logging in too much incorrectly. Weird error. I couldn't possibly guess what that means, especially when you're running on localghost. Are you firewalling against yourself? ;)
comment:14 by , 12 years ago
You bet. At this point we either use more scanning tools, benchmark them to make sure we aren't getting false negatives, and/or pony up for some real security code review. Specifically, if anyone can provide a recipe for exploiting CSRF in MediaGoblin, that would move the ball on this bug for sure.
comment:15 by , 12 years ago
`OWASP <https://www.owasp.org/index.php/Main_Page>`_ 's `CSRF mitigation page <https://www.owasp.org/index.php/CSRF_Guard>`_ could be helpful here
comment:16 by , 12 years ago
|Milestone:||0.0.5 → 0.1.0|
comment:16 by , 12 years ago
An initial implementation of CSRF protection, largely based on Django's, is in 569-application-middleware in my clone ([https://gitorious.org/\ :sub:`nyergler/mediagoblin/nyerglers-mediagoblin/commits/569-application-middleware](https://gitorious.org/`\ nyergler/mediagoblin/nyerglers-mediagoblin/commits/569-application-middleware))
comment:17 by , 12 years ago
Reading the code: I like it and am quite excited by this. A few thoughts: - Having a secret key requirement is one extra step for administrators, and probably one that in this case does no good? It looks like it's used for generation, but not checking. If that's true, maybe we can skip the whole secret key thing and just use a random number? - Could use tests :) - I wonder if instead of setting csrf\_token in render\_template if we should actually just tack it onto the request, like request.csrf\_token, in the middleware? Rationale: it keeps it contained to the middleware, and makes csrf token usable outside of render\_template? Not sure if this is true though. :) But now that I look at it again this is actually an embedded wtforms form, so maybe tacking it into the template rendering method code is right. Okay, that's some feedback but I leave this one up to you. :)
comment:18 by , 12 years ago
Updated the branch and merge request; I suggest we close this ticket if we're satisfied with this initial bit of CSRF protection, and then split off other parts of this as their own tickets (if needed).
comment:19 by , 12 years ago
|Status:||In Progress → Closed|
Merged, merged, merged! Woohoo!
comment:20 by , 12 years ago
Okay, some notes on the current CSRF implementation: - Great, that we have this now! - Why a permanent cookie living for a year? A session cookie should be the right thing. Yes, the server will need to create a new one, when/if the client starts a new session. But really, it's a session cookie thing. - I'd call the cookie ``mediagoblin_csrftoken``. Make things clearer IMHO. - ``path='/'`` is the default. One might use the actual subpath under which GMG runs - getrandbits seems nicer than randrange IMHO - Calling ``randrange()`` two times just doubles the random bits. This looks quite obscure. Aren't 63 bits okay? If double the bits are needed, replace the 63 by 128. But really, 64 bits seem enough. I have a local branch addressing all of this. If wanted, I can clean it up and push it somewhere. I leave re-opening / opening a new (Feature) bug to someone else.
comment:21 by , 12 years ago
I'd say merge your branch :).
comment:22 by , 12 years ago
Okay, my branch: idea/csrf\_improvement
comment:23 by , 12 years ago
Merged elrond's branch!
comment:24 by , 11 years ago
The original url for this bug was http://bugs.foocorp.net/issues/361 .
Note: See TracTickets for help on using tickets.