From ca07d013cf651dec7ff98de07a96f698866d897e Mon Sep 17 00:00:00 2001 From: Ben Willis Date: Mon, 16 Feb 2015 11:11:46 -0800 Subject: [PATCH 1/5] data-method whitelist to block adding CSRF If an attacker has the ability to create link tags and attributes they have the ability to intercept the CSRF token, even if CSP is enabled. By setting the href to another site and adding `data-method='post'` the CSRF token will automatically be added to the request and sent to an attackers site. It is important to note that this is only applicable to `handleMethod` (`data-method`) call as when done through `handleRemote` (`data-remote`) the CSP will block the request to the attackers domain. This fix adds a configurable whitelist of hrefs to verify the link against before adding the CSRF token. The default is to allow everything to avoid break existing clients. --- src/rails.js | 7 ++++++- test/public/test/data-method.js | 13 +++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/rails.js b/src/rails.js index a4fb0bed..431e66f9 100644 --- a/src/rails.js +++ b/src/rails.js @@ -178,7 +178,7 @@ form = $('
'), metadataInput = ''; - if (csrfParam !== undefined && csrfToken !== undefined) { + if (rails.isCsrfHrefWhitelisted(href) && csrfParam !== undefined && csrfToken !== undefined) { metadataInput += ''; } @@ -258,6 +258,11 @@ return answer && callback; }, + // Verify, when configured, that the href passes the whitelist for CSRF token usage. + isCsrfHrefWhitelisted: function(href){ + return rails.csrfWhitelistedHrefs ? rails.csrfWhitelistedHrefs.test(href) : true; + }, + // Helper function which checks for blank inputs in a form that match the specified CSS selector blankInputs: function(form, specifiedSelector, nonBlank) { var inputs = $(), input, valueToCheck, diff --git a/test/public/test/data-method.js b/test/public/test/data-method.js index c4426624..5456dbf4 100644 --- a/test/public/test/data-method.js +++ b/test/public/test/data-method.js @@ -39,6 +39,19 @@ asyncTest('link with "data-method" and CSRF', 1, function() { }); }); +asyncTest('non whitelisted links with "data-method" get no CSRF', 2, function() { + $.rails.csrfWhitelistedHrefs = /http:\/\/rubyonrails.org/ + + $('#qunit-fixture') + .append('') + .append(''); + + submit(function(data) { + strictEqual(data.params.authenticity_token, undefined); + strictEqual(data.HTTP_X_CSRF_TOKEN, undefined); + }); +}); + asyncTest('link "target" should be carried over to generated form', 1, function() { $('a[data-method]').attr('target', 'super-special-frame'); submit(function(data) { From e2cd047eb28dc4a8ea46938d8c53fa23ec99b087 Mon Sep 17 00:00:00 2001 From: Ben Willis Date: Mon, 16 Feb 2015 14:49:07 -0800 Subject: [PATCH 2/5] Fixing indentation --- test/public/test/data-method.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/public/test/data-method.js b/test/public/test/data-method.js index 5456dbf4..2b8ec7d5 100644 --- a/test/public/test/data-method.js +++ b/test/public/test/data-method.js @@ -40,16 +40,16 @@ asyncTest('link with "data-method" and CSRF', 1, function() { }); asyncTest('non whitelisted links with "data-method" get no CSRF', 2, function() { - $.rails.csrfWhitelistedHrefs = /http:\/\/rubyonrails.org/ + $.rails.csrfWhitelistedHrefs = /http:\/\/rubyonrails.org/ - $('#qunit-fixture') - .append('') - .append(''); + $('#qunit-fixture') + .append('') + .append(''); - submit(function(data) { - strictEqual(data.params.authenticity_token, undefined); - strictEqual(data.HTTP_X_CSRF_TOKEN, undefined); - }); + submit(function(data) { + strictEqual(data.params.authenticity_token, undefined); + strictEqual(data.HTTP_X_CSRF_TOKEN, undefined); + }); }); asyncTest('link "target" should be carried over to generated form', 1, function() { From d7ada5e5cd70e61f316994c2053a7a89309bdb11 Mon Sep 17 00:00:00 2001 From: Ben Willis Date: Mon, 16 Feb 2015 15:04:09 -0800 Subject: [PATCH 3/5] Changing configured href matching to domains It can be difficult to setup a whitelist of hrefs. This change adds in hostname matching so it should be friendlier to configure. --- src/rails.js | 12 ++++++++---- test/public/test/data-method.js | 14 +++++++++++++- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/rails.js b/src/rails.js index 431e66f9..a8e36f34 100644 --- a/src/rails.js +++ b/src/rails.js @@ -54,6 +54,9 @@ // Button onClick disable selector with possible reenable after remote submission buttonDisableSelector: 'button[data-remote][data-disable-with], button[data-remote][data-disable]', + // Whitelisted domains when using data-method='post' usages + csrfWhitelistedDomains: null, + // Make sure that every Ajax request sends the CSRF token CSRFProtection: function(xhr) { var token = $('meta[name="csrf-token"]').attr('content'); @@ -173,12 +176,13 @@ var href = rails.href(link), method = link.data('method'), target = link.attr('target'), + domain = link[0].hostname, csrfToken = $('meta[name=csrf-token]').attr('content'), csrfParam = $('meta[name=csrf-param]').attr('content'), form = $('
'), metadataInput = ''; - if (rails.isCsrfHrefWhitelisted(href) && csrfParam !== undefined && csrfToken !== undefined) { + if (rails.isCsrfDomainWhitelisted(domain) && csrfParam !== undefined && csrfToken !== undefined) { metadataInput += ''; } @@ -258,9 +262,9 @@ return answer && callback; }, - // Verify, when configured, that the href passes the whitelist for CSRF token usage. - isCsrfHrefWhitelisted: function(href){ - return rails.csrfWhitelistedHrefs ? rails.csrfWhitelistedHrefs.test(href) : true; + // Verify, when configured, that the href passes the whitelist and should send the CSRF token. + isCsrfDomainWhitelisted: function(domain){ + return rails.csrfWhitelistedDomains ? rails.csrfWhitelistedDomains.test(domain) : true; }, // Helper function which checks for blank inputs in a form that match the specified CSS selector diff --git a/test/public/test/data-method.js b/test/public/test/data-method.js index 2b8ec7d5..c554b2f7 100644 --- a/test/public/test/data-method.js +++ b/test/public/test/data-method.js @@ -39,8 +39,20 @@ asyncTest('link with "data-method" and CSRF', 1, function() { }); }); +asyncTest('whitelisted links with "data-method" get CSRF', 1, function() { + $.rails.csrfWhitelistedDomains = /localhost/ + + $('#qunit-fixture') + .append('') + .append(''); + + submit(function(data) { + strictEqual(data.params.authenticity_token, 'cf50faa3fe97702ca1ae'); + }); +}); + asyncTest('non whitelisted links with "data-method" get no CSRF', 2, function() { - $.rails.csrfWhitelistedHrefs = /http:\/\/rubyonrails.org/ + $.rails.csrfWhitelistedDomains = /http:\/\/rubyonrails.org/ $('#qunit-fixture') .append('') From 3cfe37b48ea89571f4bc0f91f92193296f0080fe Mon Sep 17 00:00:00 2001 From: Ben Willis Date: Tue, 17 Feb 2015 17:49:05 -0800 Subject: [PATCH 4/5] Updating the tests to use more secure whitelist --- test/public/test/data-method.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/public/test/data-method.js b/test/public/test/data-method.js index c554b2f7..57196816 100644 --- a/test/public/test/data-method.js +++ b/test/public/test/data-method.js @@ -40,7 +40,7 @@ asyncTest('link with "data-method" and CSRF', 1, function() { }); asyncTest('whitelisted links with "data-method" get CSRF', 1, function() { - $.rails.csrfWhitelistedDomains = /localhost/ + $.rails.csrfWhitelistedDomains = /^localhost$/ $('#qunit-fixture') .append('') @@ -52,7 +52,7 @@ asyncTest('whitelisted links with "data-method" get CSRF', 1, function() { }); asyncTest('non whitelisted links with "data-method" get no CSRF', 2, function() { - $.rails.csrfWhitelistedDomains = /http:\/\/rubyonrails.org/ + $.rails.csrfWhitelistedDomains = /^rubyonrails.org$/ $('#qunit-fixture') .append('') From 4866d8af4565897f1501f85517075abe79f55585 Mon Sep 17 00:00:00 2001 From: Ben Willis Date: Tue, 17 Feb 2015 17:53:47 -0800 Subject: [PATCH 5/5] Avoid using string construction when setting html attributes A potential attacker that can set the href or data-method attributes can also use attack strings to cause any html to be created which could allow bypassing of the whitelist. --- src/rails.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/rails.js b/src/rails.js index a8e36f34..78ff557c 100644 --- a/src/rails.js +++ b/src/rails.js @@ -179,16 +179,19 @@ domain = link[0].hostname, csrfToken = $('meta[name=csrf-token]').attr('content'), csrfParam = $('meta[name=csrf-param]').attr('content'), - form = $('
'), - metadataInput = ''; + form = $('
').attr('action', href), + methodInput = $('').attr('value', method); if (rails.isCsrfDomainWhitelisted(domain) && csrfParam !== undefined && csrfToken !== undefined) { - metadataInput += ''; + csrfInput = $('') + .attr('name', csrfParam) + .attr('value', csrfToken); + form.append(csrfInput); } if (target) { form.attr('target', target); } - form.hide().append(metadataInput).appendTo('body'); + form.hide().append(methodInput).appendTo('body'); form.submit(); },