Tip action should use POST #88
Comments
|
Ouch. Further, once this is indeed switched to POST, it'll need to use CSRF protection (otherwise a similar, only slightly harder attack is possible). I'm not familiar enough with Aspen to know if/how it handles CSRF protection, but I'm quite familiar with the vulnerability and the technique required to fix it, so please get in touch if you'd like a hand. I'd be happy to help if I can. |
|
The API is intended to support a Gittip widget for embedding on third-party sites (#41). How do we allow for that without the badness? |
|
So, I don't think you can make this work in a secure way with a widget. Probably the best a widget can do is set the user up and then redirect them to confirm it... |
|
Notifying tippers reticketed as #90. |
You really can't and also be safe against CSRF. If a widget allows one-click tipping, I can very easily spoof a request that (for example) causes anyone who visits my site to automatically tip me the max. The only secure thing would be a "tip " button that links to a confirmation page; the confirmation page would then execute a CSRF'd POST to do the actual tipping. |
|
Hmmm ... looking at CSRF ... my thought was to pull the session token out of the cookie and pass that in the ajax POST. But I have the session cookie set to HTTP only. What's best practice here? |
|
Re: Widget, I think an iframe is the answer. I just checked and that's what Flattr is doing. That way we should be able to ensure that only our javascript is calling our API, right? |
Well, the way Django does it is that the CSRF token is both in the form (as an |
This is so we can programmatically add the session token to ajax requests to prevent CSRF. (#88)
I was thinking that tip.json could be used to build a widget for embedding on other sites, but we need to use an iframe for that. So tip.json is not itself a public API anymore. (#88)
|
Also, not clear from your comment whether you're planning this or not, but please do note that the CSRF token needs to be different from the session token. There are other subtleties; getting this right is a bit difficult. You can check out Django's implementation if it helps, but one thing to notice is that we do CSRF slightly differently than most other frameworks: most frameworks tie CSRF tokens to the session. [There's absolutely nothing wrong with that approach, and in some ways it's easier; we chose not to so that we could keep CSRF protection independent of sessions.] So you may also want to look at other implementations, such as Pyramid's (docs; implementation). |
@jacobian So presumably BTW, I deployed 98258d2. |
Gah! Okay ... :-) /me keeps reading ... |
It's just a random string (Django, Pyramid). I'm not sure about the lifetime of the cookie.
I'm having trouble remembering the reasoning behind that — I'm not an expert here, just someone who listens to them :) — but I think it's partially because putting the session token in HTML is a potential session vulnerability. Hm, let's see if I can summon @PaulMcMillan and/or @spookylukey to this thread; they worked on Django's CSRF implementation and could probably answer questions better than I can. Let's see if that worked :) |
|
I borrowed Django's implementation for Gittip (will probably upstream to Aspen at some point). Deployed as 3.1.15. Interested in further insight from @PaulMcMillan and @spookylukey. I wasn't embedding the session token in HTML, just pulling programmatically from the cookie in the same way as with the CSRF token. The only difference jumping out at me is httponly for session and not csrf_token. Anything else before closing this ticket? Did we just fix the first Gittip security vulnerability? :-) |
|
LGTM, though again I'm not an expert. I think it's good though. |
|
Nice work! LGTM too. |
|
Thanks @d0ugal, thanks for letting me know. :-) |
|
No problem, thanks for the prompt fix. As @jacobian pointed out to me (on IRC), I probably shouldn't have reported it so publicly. Sorry about that. |
|
Bit late, but for the record: We don't link the CSRF token to the session (e.g. store it in the session record server side) for these reasons: 1) to keep them decoupled (in Django people might want CSRF protection without sessions) 2) to protect against "login CSRF" (CSRF that starts before you even have a session) 3) because people were getting CSRF errors when the session key was cycled. Some people do use the "double submit" method of using javascript to extract the session cookie and exclude it in the body of the POST request, both values being required for the request to succeed. This works because of javascript's stricter cross-domain policy. It would require setting the session cookie to not be 'http only', of course. An iframe is definitely the way to go, as well. |
|
Thanks @spookylukey, good info. @d0ugal No worries, I'd rather err on the side of openness. :-) |
The button to delete one's credit card did not get updated when we added the CSRF fix (#88).
We borrowed our CSRF implementation from Django (#88). This commit refactors away some of that legacy. In particular: - We now call _get_new_token from only one place, instead of three. - We now raise 403 from only one place, instead of two. - We now work with unicode instead of casting the token to str early. As a byproduct: - The set_cookie method now casts unicode key/values to bytestrings. - The csrf and utils modules are now __future__-proofed.
We borrowed our CSRF implementation from Django, way back in #88. This commit refactors away some of that legacy. In particular: - We now call _get_new_token from only one place, instead of three. - We now raise 403 from only one place, instead of two. - We now work with unicode instead of casting the token to str early. This all enables us to use more idiomatic Aspen algorithm functions. As a byproduct: - The set_cookie method now casts unicode key/values to bytestrings. - The csrf and utils modules are now __future__-proofed.
At the moment, you can just make a request to "https://www.gittip.com/USERNAME/tip.json?amount=1.28" and if the user is logged in they will start tipping you without knowing. Something like
<img src="https://www.gittip.com/USERNAME/tip.json?amount=1.28" />on your website would presumably do this. Therefore, it should at lest be done with POST and ideally include csrf protection too.Incidentally, it may be a good idea to email tippers when a tip is changed or added.
The text was updated successfully, but these errors were encountered: