Opened 10 years ago

Last modified 10 years ago

#76 closed defect (FIXED)

prevent CSRF and similar things

Reported by: Jakob Kramer Owned by: nyergler
Priority: major Milestone: 0.1.0
Component: programming Keywords:
Cc: Parent Tickets:


At the moment you can perform cross-site request forgeries,
cross-site scripting and similar things on MediaGoblin pages.


Change History (25)

comment:1 Changed 10 years ago by Elrond

I'm not an expert on website security, but two things I remember,
that should be considered:

1. I think, it's called "upsniffing". Browsers sometimes try to
   detect the mime-type from the actual content and use that instead
   of the supplied mime-type. This is to help broken websites or so.
   mediagoblin should not be broken. And: If someone uploads html as
   .jpg the browser is forced to jpeg and wont try to do html on the
   file (despite it's html). So disable upsniffing, or whatever it was
2. One can mark pages so they're not loaded in a frame/iframe. The
   problem seems to be, that bad pages embed the victim's page, put
   some image over it, and let the user click there.

Just some random thoughts. Hopefully the list of security things to
consider will be expanded by someone.

comment:2 Changed 10 years ago by Elrond


    X-Content-Type-Options: nosniff

Some docs I could quickly find:

-  `\_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 Changed 10 years ago by Elrond

"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

-  `\_Content\_Security\_Policy <>`_
-  `\_policy\_directives#options <>`_

(from today's discussion)

comment:4 Changed 10 years ago by Jakob Kramer

Owner: set to Jakob Kramer
Sorry, I just don't have enough time to do something for
MediaGoblin at the moment.

comment:5 Changed 10 years ago by Christopher Allan Webber

Okay, thanks for letting us know, and we'll welcome contributions
from you if in the future you do have time!

comment:6 Changed 10 years ago by Elrond

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 Changed 10 years ago by Christopher Allan Webber

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:
` <>`_

comment:8 Changed 10 years ago by Elrond

Owner: set to Christopher Webber
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 Changed 10 years ago by Christopher Allan Webber

Component: Programming
Owner: set to Christopher Webber
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 Changed 10 years ago by Will Kahn-Greene

We release 0.0.4, so I'm bumping this to 0.0.5.

comment:11 Changed 10 years ago by Christopher Allan Webber

-  ` <>`_
-  ` <>`_

Both look like good FOSS tools for scannign for vulnerabilities?

Caleb has pointed to this article (thanks!):
` <>`_

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 Changed 10 years ago by Caleb Davis

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: "". 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.


comment:13 Changed 10 years ago by Christopher Allan Webber

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

Weird error. I couldn't possibly guess what that means, especially
when you're running on localghost. Are you firewalling against
yourself? ;)

comment:14 Changed 10 years ago by Caleb Davis

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 Changed 10 years ago by Caleb Davis

`OWASP <>`_ 's
`CSRF mitigation page <>`_
could be helpful here

comment:16 Changed 10 years ago by Christopher Allan Webber

Owner: set to Nathan Yergler

comment:16 Changed 10 years ago by nyergler

An initial implementation of CSRF protection, largely based on
Django's, is in 569-application-middleware in my clone
([\ :sub:`nyergler/mediagoblin/nyerglers-mediagoblin/commits/569-application-middleware](`\ nyergler/mediagoblin/nyerglers-mediagoblin/commits/569-application-middleware))

comment:17 Changed 10 years ago by Christopher Allan Webber

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 Changed 10 years ago by nyergler

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

comment:19 Changed 10 years ago by Christopher Allan Webber

Status: In ProgressClosed
Merged, merged, merged! Woohoo!

comment:20 Changed 10 years ago by Elrond

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

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 Changed 10 years ago by nyergler

I'd say merge your branch :).

comment:22 Changed 10 years ago by Elrond

Okay, my branch: idea/csrf\_improvement

comment:23 Changed 10 years ago by Christopher Allan Webber

Merged elrond's branch!

comment:24 Changed 10 years ago by Will Kahn-Greene

The original url for this bug was .
#51: related

Note: See TracTickets for help on using tickets.