From a0f238884255706e4fd71df7b25b0e3e9154a642 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 11:24:09 +0900 Subject: [PATCH 01/12] Add Session::getGenericToken() for general-purpose token handling --- common/framework/session.php | 21 +++++++++++++++++++++ tests/unit/framework/SessionTest.php | 7 +++++++ 2 files changed, 28 insertions(+) diff --git a/common/framework/session.php b/common/framework/session.php index 4379f8e56..8ef115376 100644 --- a/common/framework/session.php +++ b/common/framework/session.php @@ -373,6 +373,7 @@ class Session $_SESSION['RHYMIX']['timezone'] = DateTime::getTimezoneForCurrentUser(); $_SESSION['RHYMIX']['secret'] = Security::getRandom(32, 'alnum'); $_SESSION['RHYMIX']['tokens'] = array(); + $_SESSION['RHYMIX']['token'] = false; $_SESSION['is_webview'] = self::_isBuggyUserAgent(); $_SESSION['is_new_session'] = true; $_SESSION['is_logged'] = false; @@ -842,6 +843,26 @@ class Session } } + /** + * Get a generic token that is not restricted to any particular key. + * + * @return string|false + */ + public static function getGenericToken() + { + if (!self::isStarted()) + { + return false; + } + + if (!$_SESSION['RHYMIX']['token']) + { + $_SESSION['RHYMIX']['token'] = self::createToken(''); + } + + return $_SESSION['RHYMIX']['token']; + } + /** * Create a token that can only be verified in the same session. * diff --git a/tests/unit/framework/SessionTest.php b/tests/unit/framework/SessionTest.php index 2f6b34f1f..47ed06134 100644 --- a/tests/unit/framework/SessionTest.php +++ b/tests/unit/framework/SessionTest.php @@ -334,9 +334,16 @@ class SessionTest extends \Codeception\TestCase\Test $this->assertFalse(Rhymix\Framework\Session::verifyToken($token2, '/wrong/key')); $this->assertFalse(Rhymix\Framework\Session::verifyToken(strrev($token2))); + $token3 = Rhymix\Framework\Session::getGenericToken(); + $this->assertEquals(16, strlen($token3)); + $this->assertTrue(Rhymix\Framework\Session::verifyToken($token3)); + $this->assertTrue(Rhymix\Framework\Session::verifyToken($token3, '')); + $this->assertFalse(Rhymix\Framework\Session::verifyToken($token3, '/wrong/key')); + Rhymix\Framework\Session::destroy(); $this->assertFalse(Rhymix\Framework\Session::verifyToken($token1)); $this->assertFalse(Rhymix\Framework\Session::verifyToken($token, '/my/key')); + $this->assertFalse(Rhymix\Framework\Session::getGenericToken()); } public function testEncryption() From e2511a02692bff54553d6b628c563aaa4dbad67b Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 11:46:37 +0900 Subject: [PATCH 02/12] Insert CSRF token using meta tag in common_layout.html MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 태그의 속성이나 그 밖의 태그를 사용하지 않는 이유는 가 로딩되기 전에 먼저 AJAX 요청을 시도하는 서드파티 자료가 있기 때문이다. 상단에 CSRF 토큰을 넣어야 이런 자료에서도 토큰이 누락되지 않는다. 다른 CSM나 프레임워크들도 상단에 태그를 사용하여 CSRF 토큰을 삽입하는 사례가 많으며, csrf-token은 이런 용도로 WHATWG에 공식적으로 등록된 meta name이다. cf. https://wiki.whatwg.org/wiki/MetaExtensions --- common/tpl/common_layout.html | 1 + 1 file changed, 1 insertion(+) diff --git a/common/tpl/common_layout.html b/common/tpl/common_layout.html index e6350e4c2..4d12ec47a 100644 --- a/common/tpl/common_layout.html +++ b/common/tpl/common_layout.html @@ -10,6 +10,7 @@ + {Context::getBrowserTitle()} From b3fb993f73525e6854ef7461d4010d000178dfb2 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 11:51:38 +0900 Subject: [PATCH 03/12] Insert CSRF token in all AJAX requests via exec_xml(), exec_json(), exec_html() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 일단 공식적으로 지원하는 AJAX 함수 3종에 CSRF 토큰을 삽입해 본다. 추후 체크 방식을 변경하거나 보안을 더욱 강화할 경우 X-CSRF-Token 헤더와 비교할 수도 있다. 일반 폼 제출이나 임의의 AJAX 요청에도 CSRF 토큰을 삽입하는 것은 다음 커밋에... --- common/js/common.js | 7 +++++++ common/js/xml_handler.js | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/common/js/common.js b/common/js/common.js index 7198d0370..2a8e701dc 100644 --- a/common/js/common.js +++ b/common/js/common.js @@ -441,6 +441,13 @@ function move_url(url, open_window) { return false; } +/** + * @brief Get CSRF token for the document + */ +function getCSRFToken() { + return $("meta[name='csrf-token']").attr("content"); +} + /** * @brief 멀티미디어 출력용 (IE에서 플래쉬/동영상 주변에 점선 생김 방지용) **/ diff --git a/common/js/xml_handler.js b/common/js/xml_handler.js index 9f65f0990..12bd9d9b9 100644 --- a/common/js/xml_handler.js +++ b/common/js/xml_handler.js @@ -28,6 +28,7 @@ params.module = module; params.act = act; params._rx_ajax_compat = 'XMLRPC'; + params._rx_csrf_token = getCSRFToken(); // Fill in the XE vid. if (typeof(xeVid) != "undefined") params.vid = xeVid; @@ -148,6 +149,7 @@ $.ajax({ url : url, type : "POST", + headers: { "X-CSRF-Token": params._rx_csrf_token }, dataType : "json", data : params, success : successHandler, @@ -172,6 +174,7 @@ params.module = action[0]; params.act = action[1]; params._rx_ajax_compat = 'JSON'; + params._rx_csrf_token = getCSRFToken(); // Fill in the XE vid. if (typeof(xeVid) != "undefined") params.vid = xeVid; @@ -256,6 +259,7 @@ $.ajax({ type: "POST", dataType: "json", + headers: { "X-CSRF-Token": params._rx_csrf_token }, url: request_uri, data: params, success : successHandler, @@ -278,6 +282,7 @@ //if (action.length != 2) return; params.module = action[0]; params.act = action[1]; + params._rx_csrf_token = getCSRFToken(); // Fill in the XE vid. if (typeof(xeVid) != "undefined") params.vid = xeVid; @@ -319,6 +324,7 @@ $.ajax({ type: "POST", dataType: "html", + headers: { "X-CSRF-Token": params._rx_csrf_token }, url: request_uri, data: params, success: successHandler, From 14300cbcc3b1a42a877ed6b8060308905bd5f6a4 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 14:47:42 +0900 Subject: [PATCH 04/12] Insert CSRF token into every AJAX request --- common/js/common.js | 14 +++++++++++++- common/js/xml_handler.js | 3 --- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/common/js/common.js b/common/js/common.js index 2a8e701dc..52828761f 100644 --- a/common/js/common.js +++ b/common/js/common.js @@ -17,7 +17,19 @@ ($.os.Linux) ? 'Linux' : ($.os.Unix) ? 'Unix' : ($.os.Mac) ? 'Mac' : ''; - + + /* Intercept jQuery AJAX calls to add CSRF headers */ + $.ajaxPrefilter(function(options) { + var _u1 = $("").attr("href", location.href)[0]; + var _u2 = $("").attr("href", options.url)[0]; + if (_u2.hostname && (_u1.hostname !== _u2.hostname)) return; + var token = getCSRFToken(); + if (token) { + if (!options.headers) options.headers = {}; + options.headers["X-CSRF-Token"] = token; + } + }); + /* Intercept getScript error due to broken minified script URL */ $(document).ajaxError(function(event, jqxhr, settings, thrownError) { if(settings.dataType === "script" && (jqxhr.status >= 400 || (jqxhr.responseText && jqxhr.responseText.length < 40))) { diff --git a/common/js/xml_handler.js b/common/js/xml_handler.js index 12bd9d9b9..75e8642a0 100644 --- a/common/js/xml_handler.js +++ b/common/js/xml_handler.js @@ -149,7 +149,6 @@ $.ajax({ url : url, type : "POST", - headers: { "X-CSRF-Token": params._rx_csrf_token }, dataType : "json", data : params, success : successHandler, @@ -259,7 +258,6 @@ $.ajax({ type: "POST", dataType: "json", - headers: { "X-CSRF-Token": params._rx_csrf_token }, url: request_uri, data: params, success : successHandler, @@ -324,7 +322,6 @@ $.ajax({ type: "POST", dataType: "html", - headers: { "X-CSRF-Token": params._rx_csrf_token }, url: request_uri, data: params, success: successHandler, From e82e3fb18ce5e642911f84a0ab2e1e0c12e851a2 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 15:11:45 +0900 Subject: [PATCH 05/12] Implement isSameOrigin() to simplify origin determination --- common/js/common.js | 19 ++++++++++++++++--- common/js/xml_handler.js | 4 +--- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/common/js/common.js b/common/js/common.js index 52828761f..f898a409c 100644 --- a/common/js/common.js +++ b/common/js/common.js @@ -20,9 +20,7 @@ /* Intercept jQuery AJAX calls to add CSRF headers */ $.ajaxPrefilter(function(options) { - var _u1 = $("").attr("href", location.href)[0]; - var _u2 = $("").attr("href", options.url)[0]; - if (_u2.hostname && (_u1.hostname !== _u2.hostname)) return; + if (!isSameOrigin(location.href, options.url, true)) return; var token = getCSRFToken(); if (token) { if (!options.headers) options.headers = {}; @@ -453,6 +451,21 @@ function move_url(url, open_window) { return false; } +/** + * @brief Check if two URLs belong to the same origin + */ +function isSameOrigin(url1, url2, allow_relative_url2) { + var a1 = $("").attr("href", url1)[0]; + var a2 = $("").attr("href", url2)[0]; + if (!a2.hostname && allow_relative_url2) { + return true; + } + if (a1.protocol !== a2.protocol) return false; + if (a1.hostname !== a2.hostname) return false; + if (a1.port !== a2.port) return false; + return true; +} + /** * @brief Get CSRF token for the document */ diff --git a/common/js/xml_handler.js b/common/js/xml_handler.js index 75e8642a0..370aac865 100644 --- a/common/js/xml_handler.js +++ b/common/js/xml_handler.js @@ -47,9 +47,7 @@ } // Check whether this is a cross-domain request. If so, use an alternative method. - var _u1 = $("").attr("href", location.href)[0]; - var _u2 = $("").attr("href", url)[0]; - if (_u1.protocol != _u2.protocol || _u1.port != _u2.port) return send_by_form(url, params); + if (!isSameOrigin(location.href, url)) return send_by_form(url, params); // Delay the waiting message for 1 second to prevent rapid blinking. waiting_obj.css("opacity", 0.0); From 11afa4db42655125d756c6eedbed76467a42d006 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 15:37:18 +0900 Subject: [PATCH 06/12] Add CSRF token to all dynamic forms --- common/js/common.js | 88 ++++++++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 32 deletions(-) diff --git a/common/js/common.js b/common/js/common.js index f898a409c..02dbfec16 100644 --- a/common/js/common.js +++ b/common/js/common.js @@ -18,16 +18,6 @@ ($.os.Unix) ? 'Unix' : ($.os.Mac) ? 'Mac' : ''; - /* Intercept jQuery AJAX calls to add CSRF headers */ - $.ajaxPrefilter(function(options) { - if (!isSameOrigin(location.href, options.url, true)) return; - var token = getCSRFToken(); - if (token) { - if (!options.headers) options.headers = {}; - options.headers["X-CSRF-Token"] = token; - } - }); - /* Intercept getScript error due to broken minified script URL */ $(document).ajaxError(function(event, jqxhr, settings, thrownError) { if(settings.dataType === "script" && (jqxhr.status >= 400 || (jqxhr.responseText && jqxhr.responseText.length < 40))) { @@ -37,7 +27,56 @@ } } }); + + /** + * @brief Check if two URLs belong to the same origin + */ + window.isSameOrigin = function(url1, url2, allow_relative_url2) { + var a1 = $("").attr("href", url1)[0]; + var a2 = $("").attr("href", url2)[0]; + if (!a2.hostname && allow_relative_url2) { + return true; + } + if (a1.protocol !== a2.protocol) return false; + if (a1.hostname !== a2.hostname) return false; + if (a1.port !== a2.port) return false; + return true; + } + /** + * @brief Get CSRF token for the document + */ + window.getCSRFToken = function() { + return $("meta[name='csrf-token']").attr("content"); + } + + /* Intercept jQuery AJAX calls to add CSRF headers */ + $.ajaxPrefilter(function(options) { + if (!isSameOrigin(location.href, options.url, true)) return; + var token = getCSRFToken(); + if (token) { + if (!options.headers) options.headers = {}; + options.headers["X-CSRF-Token"] = token; + } + }); + + /* Add CSRF token to dynamically loaded forms */ + $.fn.addCSRFTokenToForm = function() { + var token = getCSRFToken(); + if (token) { + return $(this).each(function() { + if ($(this).data("csrf-token-checked") === "Y") return; + if (!isSameOrigin(location.href, $(this).attr("action"), true)) { + return $(this).data("csrf-token-checked", "Y"); + } + $("").attr({ type: "hidden", name: "_rx_csrf_token", value: token }).appendTo($(this)); + return $(this).data("csrf-token-checked", "Y"); + }); + } else { + return $(this); + } + }; + /* Array for pending debug data */ window.rhymix_debug_pending_data = []; @@ -164,6 +203,13 @@ /* jQuery(document).ready() */ jQuery(function($) { + /* CSRF token */ + $("form[method]").filter(function() { return this.method.toUpperCase() == "POST"; }).addCSRFTokenToForm(); + $(document).on("submit", "form[method='post']", $.fn.addCSRFTokenToForm); + $(document).on("focus", "input,select,textarea", function() { + $(this).parents("form[method]").filter(function() { return this.method.toUpperCase() == "POST"; }).addCSRFTokenToForm(); + }); + /* select - option의 disabled=disabled 속성을 IE에서도 체크하기 위한 함수 */ if(navigator.userAgent.match(/MSIE/)) { $('select').each(function(i, sels) { @@ -451,28 +497,6 @@ function move_url(url, open_window) { return false; } -/** - * @brief Check if two URLs belong to the same origin - */ -function isSameOrigin(url1, url2, allow_relative_url2) { - var a1 = $("").attr("href", url1)[0]; - var a2 = $("").attr("href", url2)[0]; - if (!a2.hostname && allow_relative_url2) { - return true; - } - if (a1.protocol !== a2.protocol) return false; - if (a1.hostname !== a2.hostname) return false; - if (a1.port !== a2.port) return false; - return true; -} - -/** - * @brief Get CSRF token for the document - */ -function getCSRFToken() { - return $("meta[name='csrf-token']").attr("content"); -} - /** * @brief 멀티미디어 출력용 (IE에서 플래쉬/동영상 주변에 점선 생김 방지용) **/ From b8569aa5aba47cc3a55046175ee4e10aae0f3488 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 15:43:40 +0900 Subject: [PATCH 07/12] Fix missing semicolon --- common/js/common.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/js/common.js b/common/js/common.js index 02dbfec16..d21b5df8a 100644 --- a/common/js/common.js +++ b/common/js/common.js @@ -41,14 +41,14 @@ if (a1.hostname !== a2.hostname) return false; if (a1.port !== a2.port) return false; return true; - } + }; /** * @brief Get CSRF token for the document */ window.getCSRFToken = function() { return $("meta[name='csrf-token']").attr("content"); - } + }; /* Intercept jQuery AJAX calls to add CSRF headers */ $.ajaxPrefilter(function(options) { From 89255d028133e1c43efed3d35f56c69a92900765 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 15:54:56 +0900 Subject: [PATCH 08/12] Initial implementation of CSRF token enforcement in Security class --- common/framework/security.php | 27 +++++++++++++++++++++------ tests/unit/framework/SecurityTest.php | 2 +- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/common/framework/security.php b/common/framework/security.php index f0eb05fb3..d93568220 100644 --- a/common/framework/security.php +++ b/common/framework/security.php @@ -307,16 +307,31 @@ class Security */ public static function checkCSRF($referer = null) { - if (!$referer) + if ($_SERVER['REQUEST_METHOD'] === 'GET') { - $referer = strval($_SERVER['HTTP_REFERER']); - if ($referer === '') + return true; + } + elseif ($token = $_SERVER['HTTP_X_CSRF_TOKEN']) + { + return Session::verifyToken($token); + } + elseif ($token = \Context::get('_rx_csrf_token')) + { + return Session::verifyToken($token); + } + else + { + trigger_error('CSRF token missing in POST request: ' . (\Context::get('act') ?: '(no act)'), \E_USER_WARNING); + $referer = strval($referer ?: $_SERVER['HTTP_REFERER']); + if ($referer !== '') { - return true; + return URL::isInternalURL($referer); + } + else + { + return false; } } - - return URL::isInternalURL($referer); } /** diff --git a/tests/unit/framework/SecurityTest.php b/tests/unit/framework/SecurityTest.php index adc5557b6..c6e0aea16 100644 --- a/tests/unit/framework/SecurityTest.php +++ b/tests/unit/framework/SecurityTest.php @@ -111,7 +111,7 @@ class SecurityTest extends \Codeception\TestCase\Test $this->assertTrue(Rhymix\Framework\Security::checkCSRF()); $_SERVER['REQUEST_METHOD'] = 'POST'; - $this->assertTrue(Rhymix\Framework\Security::checkCSRF()); + $this->assertFalse(Rhymix\Framework\Security::checkCSRF()); $_SERVER['HTTP_REFERER'] = 'http://www.foobar.com/'; $this->assertFalse(Rhymix\Framework\Security::checkCSRF()); From fea55d17b4eb4f0cf261ab6faa52fd7cb98db6f6 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 16:00:37 +0900 Subject: [PATCH 09/12] Add CSRF token to XpressEditor uploader --- modules/editor/tpl/js/uploader.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) mode change 100755 => 100644 modules/editor/tpl/js/uploader.js diff --git a/modules/editor/tpl/js/uploader.js b/modules/editor/tpl/js/uploader.js old mode 100755 new mode 100644 index beb4807a0..eadbfe052 --- a/modules/editor/tpl/js/uploader.js +++ b/modules/editor/tpl/js/uploader.js @@ -54,7 +54,8 @@ var uploadAutosaveChecker = false; mid : current_mid, act : 'procFileUpload', editor_sequence : seq, - uploadTargetSrl : editorRelKeys[seq].primary.value + uploadTargetSrl : editorRelKeys[seq].primary.value, + _rx_csrf_token : getCSRFToken() }, http_success : [302], file_size_limit : Math.floor( (parseInt(cfg.allowedFileSize,10)||1024) / 1024 ), From d62756dcd5f65374800cf7196116ee7de0322dcc Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 6 Mar 2017 16:02:50 +0900 Subject: [PATCH 10/12] Fix unit tests to ignore user warnings during CSRF test --- tests/unit/framework/SecurityTest.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/unit/framework/SecurityTest.php b/tests/unit/framework/SecurityTest.php index c6e0aea16..ca36d35a8 100644 --- a/tests/unit/framework/SecurityTest.php +++ b/tests/unit/framework/SecurityTest.php @@ -106,6 +106,8 @@ class SecurityTest extends \Codeception\TestCase\Test public function testCheckCSRF() { + $error_reporting = error_reporting(0); + $_SERVER['REQUEST_METHOD'] = 'GET'; $_SERVER['HTTP_REFERER'] = ''; $this->assertTrue(Rhymix\Framework\Security::checkCSRF()); @@ -117,6 +119,8 @@ class SecurityTest extends \Codeception\TestCase\Test $this->assertFalse(Rhymix\Framework\Security::checkCSRF()); $this->assertTrue(Rhymix\Framework\Security::checkCSRF('http://www.rhymix.org/')); + + error_reporting($error_reporting); } public function testCheckXEE() From 9a34341759111607edcbea4193ed53e53e5d9013 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 13 Mar 2017 16:41:08 +0900 Subject: [PATCH 11/12] Populate CSRF token in some non-member requests as well --- common/tpl/common_layout.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/tpl/common_layout.html b/common/tpl/common_layout.html index 3104e3c23..b59512931 100644 --- a/common/tpl/common_layout.html +++ b/common/tpl/common_layout.html @@ -10,7 +10,7 @@ - + {Context::getBrowserTitle()} From df59e541c9079132a6b90a9e223f454212afd495 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Mon, 13 Mar 2017 16:41:57 +0900 Subject: [PATCH 12/12] Skip diagnostic CSRF warning if the user is not logged in --- common/framework/security.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/common/framework/security.php b/common/framework/security.php index d93568220..f5591ed1b 100644 --- a/common/framework/security.php +++ b/common/framework/security.php @@ -321,7 +321,11 @@ class Security } else { - trigger_error('CSRF token missing in POST request: ' . (\Context::get('act') ?: '(no act)'), \E_USER_WARNING); + if (Session::getMemberSrl()) + { + trigger_error('CSRF token missing in POST request: ' . (\Context::get('act') ?: '(no act)'), \E_USER_WARNING); + } + $referer = strval($referer ?: $_SERVER['HTTP_REFERER']); if ($referer !== '') {