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();
},