From 102e0828fd162539afe7765bd0e8be906b84d365 Mon Sep 17 00:00:00 2001
From: Juan Leyva <juanleyvadelgado@gmail.com>
Date: Thu, 21 Mar 2019 15:39:21 +0100
Subject: [PATCH] MDL-65075 tool_mobile: Allow auto-login keys only for
requests from Moodle apps
This change enhances security and avoid any kind of XSS attack.
---
admin/tool/mobile/classes/api.php | 5 +++
admin/tool/mobile/classes/external.php | 2 ++
admin/tool/mobile/db/services.php | 3 +-
admin/tool/mobile/lang/en/tool_mobile.php | 1 +
admin/tool/mobile/tests/externallib_test.php | 34 ++++++++++++++++++++
admin/tool/mobile/upgrade.txt | 5 +++
6 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/admin/tool/mobile/classes/api.php b/admin/tool/mobile/classes/api.php
index bd49699e398..f2fc6806093 100644
--- a/admin/tool/mobile/classes/api.php
+++ b/admin/tool/mobile/classes/api.php
@@ -297,6 +297,11 @@ class api {
throw new moodle_exception('enablewsdescription', 'webservice');
}
+ // Only requests from the Moodle mobile or desktop app. This enhances security to avoid any type of XSS attack.
+ if (!\core_useragent::is_moodle_app()) {
+ throw new moodle_exception('apprequired', 'tool_mobile');
+ }
+
if (!is_https()) {
throw new moodle_exception('httpsrequired', 'tool_mobile');
}
diff --git a/admin/tool/mobile/classes/external.php b/admin/tool/mobile/classes/external.php
index 17956350a89..830c0602390 100644
--- a/admin/tool/mobile/classes/external.php
+++ b/admin/tool/mobile/classes/external.php
@@ -262,6 +262,8 @@ class external extends external_api {
/**
* Creates an auto-login key for the current user. Is created only in https sites and is restricted by time and ip address.
*
+ * Please note that it only works if the request comes from the Moodle mobile or desktop app.
+ *
* @param string $privatetoken the user private token for validating the request
* @return array with the settings and warnings
* @since Moodle 3.2
diff --git a/admin/tool/mobile/db/services.php b/admin/tool/mobile/db/services.php
index 0b50d6853cf..d53f7b41a7f 100644
--- a/admin/tool/mobile/db/services.php
+++ b/admin/tool/mobile/db/services.php
@@ -57,7 +57,8 @@ $functions = array(
'classname' => 'tool_mobile\external',
'methodname' => 'get_autologin_key',
'description' => 'Creates an auto-login key for the current user.
- Is created only in https sites and is restricted by time and ip address.',
+ Is created only in https sites and is restricted by time, ip address and only works if the request
+ comes from the Moodle mobile or desktop app.',
'type' => 'write',
'services' => array(MOODLE_OFFICIAL_MOBILE_SERVICE),
),
diff --git a/admin/tool/mobile/lang/en/tool_mobile.php b/admin/tool/mobile/lang/en/tool_mobile.php
index a3738785296..0b01cd02a57 100644
--- a/admin/tool/mobile/lang/en/tool_mobile.php
+++ b/admin/tool/mobile/lang/en/tool_mobile.php
@@ -27,6 +27,7 @@ $string['androidappid'] = 'Android app\'s unique identifier';
$string['androidappid_desc'] = 'This setting may be left as default unless you have a custom Android app.';
$string['apppolicy'] = 'App policy URL';
$string['apppolicy_help'] = 'The URL of a policy for app users which is listed on the About page in the app. If the field is left empty, the site policy URL will be used instead.';
+$string['apprequired'] = 'This functionality is only available when accessed via the Moodle mobile or desktop app.';
$string['autologinkeygenerationlockout'] = 'Auto-login key generation is blocked. You need to wait 6 minutes between requests.';
$string['autologinnotallowedtoadmins'] = 'Auto-login is not allowed for site admins.';
$string['cachedef_plugininfo'] = 'This stores the list of plugins with mobile addons';
diff --git a/admin/tool/mobile/tests/externallib_test.php b/admin/tool/mobile/tests/externallib_test.php
index 4045ab14d26..1fee731019c 100644
--- a/admin/tool/mobile/tests/externallib_test.php
+++ b/admin/tool/mobile/tests/externallib_test.php
@@ -213,6 +213,10 @@ class tool_mobile_external_testcase extends externallib_advanced_testcase {
// Enable requeriments.
$_GET['wstoken'] = $token->token; // Mock parameters.
+ // Fake the app.
+ core_useragent::instance(true, 'Mozilla/5.0 (Linux; Android 7.1.1; Moto G Play Build/NPIS26.48-43-2; wv) ' .
+ 'AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/71.0.3578.99 Mobile Safari/537.36 MoodleMobile');
+
// Even if we force the password change for the current user we should be able to retrieve the key.
set_user_preference('auth_forcepasswordchange', 1, $user->id);
@@ -240,6 +244,10 @@ class tool_mobile_external_testcase extends externallib_advanced_testcase {
global $CFG;
$this->resetAfterTest(true);
+ // Fake the app.
+ core_useragent::instance(true, 'Mozilla/5.0 (Linux; Android 7.1.1; Moto G Play Build/NPIS26.48-43-2; wv) ' .
+ 'AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/71.0.3578.99 Mobile Safari/537.36 MoodleMobile');
+
// Need to disable webservices to verify that's checked.
$CFG->enablewebservices = 0;
$CFG->enablemobilewebservice = 0;
@@ -256,6 +264,10 @@ class tool_mobile_external_testcase extends externallib_advanced_testcase {
public function test_get_autologin_key_missing_https() {
global $CFG;
+ // Fake the app.
+ core_useragent::instance(true, 'Mozilla/5.0 (Linux; Android 7.1.1; Moto G Play Build/NPIS26.48-43-2; wv) ' .
+ 'AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/71.0.3578.99 Mobile Safari/537.36 MoodleMobile');
+
// Need to simulate a non HTTPS site here.
$CFG->wwwroot = str_replace('https:', 'http:', $CFG->wwwroot);
@@ -276,6 +288,10 @@ class tool_mobile_external_testcase extends externallib_advanced_testcase {
$this->resetAfterTest(true);
$this->setAdminUser();
+ // Fake the app.
+ core_useragent::instance(true, 'Mozilla/5.0 (Linux; Android 7.1.1; Moto G Play Build/NPIS26.48-43-2; wv) ' .
+ 'AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/71.0.3578.99 Mobile Safari/537.36 MoodleMobile');
+
$this->expectException('moodle_exception');
$this->expectExceptionMessage(get_string('autologinnotallowedtoadmins', 'tool_mobile'));
$result = external::get_autologin_key('');
@@ -296,6 +312,10 @@ class tool_mobile_external_testcase extends externallib_advanced_testcase {
$token = external_generate_token_for_current_user($service);
$_GET['wstoken'] = $token->token; // Mock parameters.
+ // Fake the app.
+ core_useragent::instance(true, 'Mozilla/5.0 (Linux; Android 7.1.1; Moto G Play Build/NPIS26.48-43-2; wv) ' .
+ 'AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/71.0.3578.99 Mobile Safari/537.36 MoodleMobile');
+
$result = external::get_autologin_key($token->privatetoken);
$result = external_api::clean_returnvalue(external::get_autologin_key_returns(), $result);
@@ -311,6 +331,20 @@ class tool_mobile_external_testcase extends externallib_advanced_testcase {
$result = external::get_autologin_key($token->privatetoken);
}
+ /**
+ * Test get_autologin_key missing app_request.
+ */
+ public function test_get_autologin_key_missing_app_request() {
+ global $CFG;
+
+ $this->resetAfterTest(true);
+ $this->setAdminUser();
+
+ $this->expectException('moodle_exception');
+ $this->expectExceptionMessage(get_string('apprequired', 'tool_mobile'));
+ $result = external::get_autologin_key('');
+ }
+
/**
* Test get_content.
*/
diff --git a/admin/tool/mobile/upgrade.txt b/admin/tool/mobile/upgrade.txt
index ec720c3491a..b368e939976 100644
--- a/admin/tool/mobile/upgrade.txt
+++ b/admin/tool/mobile/upgrade.txt
@@ -4,6 +4,11 @@ Information provided here is intended especially for developers.
=== 3.7 ===
* New external function tool_mobile::tool_mobile_call_external_function allows calling multiple external functions and returns all responses.
+ * External function tool_mobile::get_autologin_key now only works if the request comes from the Moodle mobile or desktop app.
+ This increases confidence that requests did originate from the mobile app, decreasing the likelihood of an XSS attack.
+ If you want to use this functionality, please override the Web Service via the override_webservice_execution callback although
+ this is not recommended or encouraged.
+>>>>>>> a7ccde5003f... MDL-65075 tool_mobile: Allow auto-login keys only for requests from Moodle apps
=== 3.5 ===