CSRF Support #606
CSRF Support #606
Conversation
| func init() { | ||
| revel.TemplateFuncs["csrftoken"] = func(renderArgs map[string]interface{}) template.HTML { | ||
| tokenFunc, ok := renderArgs["_csrftoken"] | ||
| if !ok { |
There was a problem hiding this comment.
Why 2 lines instead of 1?
There was a problem hiding this comment.
Sorry, definitely some rough edges left from the refactoring that needs to be cleaned up still. When we've got some consolidated feedback I'll go through and incorporate everything at once.
There was a problem hiding this comment.
Looking at this again. Is your comment about line 134? The tokenFunc variable gets used outside of the if statement, which is why I didn't declare them in the if's initialization statement.
There was a problem hiding this comment.
You could do something like
if tokenFunc, ok := renderArgs["_csrftoken"];!ok {
panic("REVEL CSRF: _csrftoken missing from RenderArgs.")
} else {
return template.HTML(tokenFunc.(func() string)())
}
|
This is a great first stab :) @revel/core any input? Thanks for the tests/examples, too. That makes things a lot easier! As soon as we push out v0.10 we cna look into this more closely. |
|
Mad props! Great implementation and pull request overall. I'm just curious though, why go with a UUID instead of using a SHA hash or something among those lines? |
|
Probably based in on the behaviour that was corrected in #552. As @landr0id mentioned, definitely switch this one over as well. |
|
So instead of using From what I've seen, this is more common than using UUID generators. I used the UUID simply because thats what was in use at the time for sessions in master. |
|
@iamjem yep, exactly. |
|
@landr0id @pushrax thanks for bring that up and the issue link, too. I knew UUID was a recent conversation, but couldn't recall where we discussed it. I think we all agree that this change is needed. |
|
Does this handle per-page CSRF tokens? for example, what happens if I have multiple tabs open of the same form? or two different pages from the same site? looks good, thanks? |
|
Hadn't seen this project before, implementation-wise it looks pretty similar. What is the use case for per-page tokens? I guess I don't see what the security concern would be there? If a user logs in at some point, you'd want to refresh the token (which there's a convenience function for). On an unrelated note, I'm wondering if there's a more elegant way to handle exemptions than manually registering absolute paths as is done in this package. |
|
Talked with another Revel enthusiast who had some more insight into some tweaks we should consider to improve security:
|
|
@iamjem for the leaking. I was referring to any sort of CORS functionality that may be in use in conjunction with this feature. If we have a route that supports CORS, we wouldn't want to return the token in a form/header for that request. Because then, in theory, an attacker would be able to grab a valid token from the response cross-site and possibly submit requests for non CORS routes. Additionally, we should make sure that any sort of method override functionality transforms the the logical method prior to passing to this. Here is a good post where this has occurred in another framework http://blog.nodesecurity.io/post/60555138201/bypass-connect-csrf-protection-by-abusing. |
|
Hmm thats interesting. As far as I know there's no way of doing anything similar to what the methodOverride middleware in Connect does within Revel. And really, it sounds like a dangerous feature to have period! For the CORS it looks like we just need to ensure the scheme and host match for the request and referrer. Right now its only checking the scheme. |
|
For CORS it should be ok to check for the existence of the "Origin" header and if present, not populate the token. |
ghost
mentioned this pull request
|
I want to get this merged ASAP. Mad rush to start confirming v0.11's PR's. Are we good to merge this into |
|
I'll look through this once more and make sure everyone's comments were On Thu, Sep 11, 2014 at 2:20 PM, Brenden Soares notifications@github.com
|
TODO
@iamjem when can you get this done? Thanks! |
| "encoding/base64" | ||
| "encoding/hex" | ||
| "fmt" | ||
| "github.com/revel/revel" |
There was a problem hiding this comment.
Can you move the revel import down to its own section. We're trying to separate the imports into "built-in" and "other" sections.
|
@iamjem can we get this done in a day or so? If not, I'll merge this and then finish up the TODOs as noted above. Thanks for your help so far! |
|
@brendensoares sorry for my absence, made some updates, i guess i had some laying around for a while back that hadn't been pushed either. take a look. the one thing i'm still missing is CSRF exempt. really think it should be convenient for a user.. the existing CSRF library for revel makes you manually register route paths, which I think blows. probably a way to mark an entire controller as CSRF exempt, but that has implications code-wise for the user. any ideas? in python and node this is normally wrapping a method/function in a special CSRF exempt decorator, and you're done. |
ghost
mentioned this pull request
|
@AnonX Uniform Resource Name (had to Google that). Either way, using the controller action may be a cleaner/easier solution, but what if an action is targeted by multiple routes? Probably not common enough to care, but curious enough to ask. |
|
G'day, @brendensoares. Why would somebody want to exempt |
|
@brendensoares @AnonX at the time the CSRF filter is running, does it know what the target controller method is? I agree, registering a URL explicitly is duplicative since its already in the routes. And for the controller action duplication, I think thats a valid point. Someone might have the same controller action registered for different HTTP methods, or even different nearly identical routes, and have conditional logic within it based on which method? |
|
@AnonX @iamjem I suppose we could just suggest the best practice of not using the same action for different routes. It does seem to be a rare edge case. @iamjem can you add the |
|
Just a status update, should have a first stab at things done tonight for review tomorrow. Planning on accepting string paths, compiled regex, and method expressions to the |
|
Few stumbling points I've ran into. First off, I'm not finding a way to reflect a method expression to get its method name? You can reflect the function's args of course, the first of which would be the controller struct. But I would need to do some validation of the method expression to ensure its a valid struct (that contains an embedded revel.Controller), and then piece together the controller name and action to compare against controller.Action? type MyController struct {
*revel.Controller
}
func (m MyController) Index() revel.Result {}
csrf.MarkExempt(MyController.Index)
// or this should work too
csrf.MarkExempt((*MyController).Index)Or is this even worth it? Second topic is concurrency. I would like to avoid having to use synchronization on the maps/slices that hold these exempt values since these filters would be getting called on every request, and that could have performance implications. I believe this would only be safe if values were only being read from once the server is accepting requests. I would imagine that a developer would typically mark exempt methods in the various init hooks throughout their app before and not while the server is running. Is that a safe assumption? |
|
What's the advantage of options as a map instead of a struct? |
| requestToken = c.Request.Header.Get("X-CSRFToken") | ||
| } | ||
|
|
||
| if requestToken == "" || !compareToken(requestToken, csrfSecret) { |
There was a problem hiding this comment.
Would make more sense if this if statement (line 85) was inside the previous if statement block (line 81). Otherwise it appears like your checking the same thing twice.
Leave as is, code is correct as stands
There was a problem hiding this comment.
This is a valid point that would improve code readability.
To my view of thinking, it's a good idea to keep everything idiot-proof and make sure that Nothing bad will happen if I pass
I think, yes. It should be called from What do you think, @brendensoares? |
|
Keep in mind too one of my questions: is it possible to reflect the name of the method in a method expression? My earlier attempts weren't working. I could figure out the controller no problem, but whatever magic happens when you use a method expression seemed to hide the original methods name. Not a type I've personally had to introspect before. |
|
@iamjem, there is a |
|
@AnonX my question is, can we use reflection to determine the name of the method in a method expression? I haven't had any luck doing so. Code wise, this is what I'm expecting func MarkExempt(item interface{}) {
// First check and handle when item is a string (URI), regex (code omitted)
// Now check if item is a function (in the case of a method expression)
if reflect.TypeOf(item).Kind() == reflect.Func {
// Now we can inspect item's arguments, the first of which
// should be a controller.. but how do we know what the name of
// the method on the controller is that we care about? We need it for
// validation later, when its compared against *controller.Action...
// This (along with other things I've tried) doesn't return the method name...
// We basically want to find "Index" if someone did MarkExempt(SomeController.Index)
fmt.Println(reflect.TypeOf(item).Name())
}
}
// Sample controller
type HomeController {
*revel.Controller
}
func (h HomeController) Index() revel.Result {}
// Example of marking it exempt in the app init:
csrf.MarkExempt(HomeController.Index)
// or
csrf.MarkExempt((*HomeController).Index)
// or we say forget it, and they just use a string
csrf.MarkExempt("HomeController.Index") |
|
Believe I have the couple code cleanup items, exemptions, and tests committed now. After talking with @brendensoares on IRC, we decided to go with exemptions for paths and controller actions (as strings). So, to mark exempt, you'd do something like this: // as a path
csrf.MarkExempt("/ControllerName/ActionName")
// as a controller action, validated against controller.Action
csrf.MarkExempt("ControllerName.ActionName")Some possible future updates could include accepting method expressions, and potentially regular expressions. |
|
👍 Can't wait tp get this released. Much anticopated :) |
|
@iamjem Can we merge this in the next day or so? |
|
@brendensoares what do you need from me? |
|
@iamjem last time we chatted you said you had some buttoning up to do for this PR. Is it ready to be merged? |
|
My last commit should have covered everything in that regard. I think its On Wed, Oct 22, 2014 at 9:56 AM, Brenden Soares notifications@github.com
|
|
@iamjem Great! I'll do a final review and test tonight. |
|
I'll merge this and then add your usage instructions as comments to the filter. Thanks @iamjem! |
| // If the Request method isn't in the white listed methods | ||
| if !allowedMethods[c.Request.Method] && !IsExempt(c) { | ||
| // Token wasn't present at all | ||
| if !foundToken { |
There was a problem hiding this comment.
Sorry, can foundToken be ever false on line 53? Haven't we already checked it on line 43 and called RefreshToken(c)?
There was a problem hiding this comment.
@AnonX yes, if the token wasn't sent in the form (right?).
This is a first attempt at adding CSRF support. I've modeled the implementation closely after the Express.js CSRF middleware and Django's CSRF middleware.
I preferred some of the subtleties found in Express' version, namely that tokens weren't stored "as-is" in the session cookie, but rather a secret that was used to generate them lazily. Django had some additional strictness with the HTTP referer which I thought was important as well.
Usage is pretty simple. Add
csrf.CsrfFilterto the app's filters (it must come after therevel.SessionFilter), and you're good to go. To add CSRF fields to a form, use the template tag{{ csrftoken . }}. The filter adds a function closure to theRenderArgsthat can pull out the secret and make the token as-needed, caching the value in the request. Ajax support provided through theX-CSRFTokenheader.The
csrf.RefreshSecretshould be called after a user logs in.Like I said, first stab at things. Happy to make/discuss improvements.