Add CSRF token prevention. #28

Merged
merged 16 commits into from Jul 13, 2012

Projects

None yet

3 participants

@cmeiklejohn
Contributor

Add CSRF token prevention to Riak Control.

  • Break out resources which perform potentially destructive actions.
  • Switch these from GET requests to POST requests.
  • No longer serve index.html off of the file system, serve from erlydtl template so we can insert a CSRF token into the header as a meta tag. (Potentially problematic because users might have bookmarked /admin/ui/index.html).

The changes to the API are less than awesome, but are good enough to get us to a place where things feel much safter (POST vs non-idempotent GET).

@cmeiklejohn
Contributor

Ah, cool. @dizzyd pointed me at rel/reltool.config in basho/riak. Once I get this working, I'll open a pull request there.

@seancribbs seancribbs and 1 other commented on an outdated diff Jul 12, 2012
src/admin_cluster_down.erl
+ forbidden/2
+ ]).
+
+%% riak_control and webmachine dependencies
+-include_lib("riak_control/include/riak_control.hrl").
+-include_lib("webmachine/include/webmachine.hrl").
+
+%% mappings to the various content types supported for this resource
+-define(CONTENT_TYPES,[{"application/json",to_json}]).
+
+%% defines the webmachine routes this module handles
+routes () ->
+ [{admin_routes:cluster_route(["down",node]),?MODULE,[]}].
+
+%% entry-point for the resource from webmachine
+init (Action) ->
@seancribbs
seancribbs Jul 12, 2012

What is Action here? Should be just an empty list, right? Seems unnecessary to pass it along in the resource state.

@cmeiklejohn
cmeiklejohn Jul 12, 2012 Contributor

Corrected in [0ec17aa].

@seancribbs seancribbs and 1 other commented on an outdated diff Jul 12, 2012
src/admin_cluster_join.erl
+ process_post/2,
+ is_authorized/2,
+ service_available/2,
+ forbidden/2
+ ]).
+
+%% riak_control and webmachine dependencies
+-include_lib("riak_control/include/riak_control.hrl").
+-include_lib("webmachine/include/webmachine.hrl").
+
+%% mappings to the various content types supported for this resource
+-define(CONTENT_TYPES,[{"application/json",to_json}]).
+
+%% defines the webmachine routes this module handles
+routes () ->
+ [{admin_routes:cluster_route(["join",node]),?MODULE,[]}].
@seancribbs
seancribbs Jul 12, 2012

Out of curiosity, in this route and the one for the 'down' command, why is the node name in the URL and not part of the POST?

@cmeiklejohn
cmeiklejohn Jul 12, 2012 Contributor

Ah, all of these destructive functions in the API (see cluster join, leave, node down, stop) put the node name in the URI. I can work on changing that; what are your thoughts on addressing that in a different pull request?

@seancribbs
seancribbs Jul 12, 2012

Seems appropriate not to grow the scope of this fix, defer.

@seancribbs seancribbs and 1 other commented on an outdated diff Jul 12, 2012
src/admin_gui.erl
@@ -52,20 +49,6 @@ routes () ->
init (Resource) ->
{ok,Resource}.
-%% redirect to the index page if no file given
-moved_permanently (Req,index) ->
- {{true,"/admin/ui/index.html"},Req,undefined};
@seancribbs
seancribbs Jul 12, 2012

Rather than removing it completely, you might want to redirect the other way; that is, if they request index.html, point to the template-rendered resource instead.

@cmeiklejohn
cmeiklejohn Jul 12, 2012 Contributor

Corrected in [21e1572].

@cmeiklejohn cmeiklejohn referenced this pull request in basho/riak Jul 12, 2012
Merged

Ensure erlydtl is packaged. #182

@cmeiklejohn
Contributor

See: basho/riak_ee#73, basho/riak#182.

@coderoshi coderoshi and 1 other commented on an outdated diff Jul 12, 2012
priv/admin/js/cluster.js
$.ajax({
url: action,
- dataType: 'json',
+ type: 'POST',
+ data: "csrf_token=" + encodeURIComponent(csrf_token),
+ dataType: 'application/json',
@coderoshi
coderoshi Jul 12, 2012

I'm pretty sure it was correct the first time as "dataType: 'json'"

@cmeiklejohn
cmeiklejohn Jul 12, 2012 Contributor

You are absolutely right; nice catch! Fixed in [08e3f2a].

@coderoshi coderoshi and 2 others commented on an outdated diff Jul 12, 2012
src/riak_control_security.erl
+get_csrf_token(RD, _Ctx) ->
+ wrq:get_cookie_value("csrf_token", RD).
+
+%% @doc ensure this request contains a valid csrf protection token.
+is_valid_csrf_token(RD, Ctx) ->
+ Body = mochiweb_util:parse_qs(wrq:req_body(RD)),
+ BodyToken = proplists:get_value("csrf_token", Body),
+ CookieToken = get_csrf_token(RD, Ctx),
+ case BodyToken of
+ undefined ->
+ false;
+ CookieToken ->
+ true;
+ _ ->
+ false
+ end.
@coderoshi
coderoshi Jul 12, 2012

I'm not much of an erlang programmer, but doesn't _ -> include undefined? Wouldn't this suffice?

CookieToken ->
true;
_ ->
false

@cmeiklejohn
cmeiklejohn Jul 12, 2012 Contributor

The problem is when CookieToken and BodyToken are both undefined, the first clause matches.

@coderoshi
coderoshi Jul 12, 2012

gotcha - I'm not used to the "capitalization equals variable" thing

@seancribbs
seancribbs Jul 13, 2012

@cmeiklejohn Sure, but he's still right. You're not doing anything special with the undefined clause. Why not simplify this case clause to:

BodyToken /= undefined andalso BodyToken == CookieToken.
@cmeiklejohn
cmeiklejohn Jul 13, 2012 Contributor

Very nice, thanks. I think this is covered in [21d8b19].

@seancribbs

Other than the case-clause wonkiness, I'm +1 to this. Make it happen.

@cmeiklejohn cmeiklejohn merged commit eecb4ff into master Jul 13, 2012
@seancribbs seancribbs deleted the csm_riak_control_csrf_tokens_rebased branch Apr 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment