From 7679452caff6faa33f00d3f0589c5190bc01a933 Mon Sep 17 00:00:00 2001 From: David Woloszyn Date: Wed, 6 Sep 2023 16:12:19 +1000 Subject: [PATCH] MDL-77846 core: Make endpoint revision number checks stricter In some places we prevented cache poisoning, in others we did not. We also did not place any restriction on the minimum value for a revision. This change introduces a new set of functions for configonly endpoints which validates the revision numbers passed in. If the revision is either too old, or too new, it is rejected and the file content is not cached. The content is still served, but caching headers are not sent, and any local storage caching is prevented. The current time is used as the maximum version, with 60 seconds added to allow for any clock skew between cluster nodes. Previously some locations used one hour, but there should never be such a large clock skew on a correctly configured system. Co-authored-by: Andrew Nicols --- lib/configonlylib.php | 54 +++++++++++++++++++++++ lib/editor/tiny/lang.php | 3 +- lib/editor/tiny/loader.php | 3 +- lib/javascript.php | 8 +++- lib/tests/configonlylib_test.php | 73 +++++++++++++++++++++++++++++++- theme/font.php | 5 +++ theme/image.php | 5 +++ theme/javascript.php | 5 +++ theme/styles.php | 7 +++ theme/yui_combo.php | 10 +++-- 10 files changed, 164 insertions(+), 9 deletions(-) diff --git a/lib/configonlylib.php b/lib/configonlylib.php index df542688628..bcdfa9bee90 100644 --- a/lib/configonlylib.php +++ b/lib/configonlylib.php @@ -205,3 +205,57 @@ function min_get_slash_argument($clean = true) { } return $relativepath; } + +/** + * Get the lowest possible currently valid revision number. + * + * This is based on the current Moodle version. + * + * @return int Unix timestamp + */ +function min_get_minimum_revision(): int { + static $timestamp = null; + + if ($timestamp === null) { + global $CFG; + require("{$CFG->dirroot}/version.php"); + // Get YYYYMMDD from version. + $datestring = floor($version / 100); + // Parse the date components. + $year = intval(substr($datestring, 0, 4)); + $month = intval(substr($datestring, 4, 2)); + $day = intval(substr($datestring, 6, 2)); + // Return converted GMT Unix timestamp. + $timestamp = gmmktime(0, 0, 0, $month, $day, $year); + } + + return $timestamp; +} + +/** + * Get the highest possible currently valid revision number. + * + * This is based on the current time, allowing for a small amount of clock skew between servers. + * + * Future values beyond the clock skew are not allowed to avoid the possibility of cache poisoning. + * + * @return int + */ +function min_get_maximum_revision(): int { + return time() + 60; +} + +/** + * Helper function to determine if the given revision number is valid. + * + * @param int $revision A numeric revision to check for validity + * @return bool Whether the revision is valid + */ +function min_is_revision_valid_and_current(int $revision): bool { + // Invalid revision. + if ($revision <= 0) { + return false; + } + // Check revision is within range. + return $revision >= min_get_minimum_revision() && $revision <= min_get_maximum_revision(); +} diff --git a/lib/editor/tiny/lang.php b/lib/editor/tiny/lang.php index 058c41fff1d..b4240c1f903 100644 --- a/lib/editor/tiny/lang.php +++ b/lib/editor/tiny/lang.php @@ -95,7 +95,8 @@ class lang { */ protected function serve_file(): void { // Attempt to send the cached langpack. - if ($this->rev > 0) { + // We only cache the file if the rev is valid. + if (min_is_revision_valid_and_current($this->rev)) { if ($this->is_candidate_file_available()) { // The send_cached_file_if_available function will exit if successful. // In theory the file could become unavailable after checking that the file exists. diff --git a/lib/editor/tiny/loader.php b/lib/editor/tiny/loader.php index 1539a6ccfb7..68baaac0ce9 100644 --- a/lib/editor/tiny/loader.php +++ b/lib/editor/tiny/loader.php @@ -114,7 +114,8 @@ class loader { */ public function serve_file(): void { // Attempt to send the cached filepathpack. - if ($this->rev > 0) { + // We only cache the file if the rev is valid. + if (min_is_revision_valid_and_current($this->rev)) { if ($this->is_candidate_file_available()) { // The send_cached_file_if_available function will exit if successful. // In theory the file could become unavailable after checking that the file exists. diff --git a/lib/javascript.php b/lib/javascript.php index 7e7869fbac1..a6b0e0940c8 100644 --- a/lib/javascript.php +++ b/lib/javascript.php @@ -47,6 +47,11 @@ if ($slashargument = min_get_slash_argument()) { $file = min_optional_param('jsfile', '', 'RAW'); // 'file' would collide with URL rewriting! } +if (!min_is_revision_valid_and_current($rev)) { + // If the rev is invalid, normalise it to -1 to disable all caching. + $rev = -1; +} + // some security first - pick only files with .js extension in dirroot $jsfiles = array(); $files = explode(',', $file); @@ -79,8 +84,7 @@ if (!$jsfiles) { $etag = sha1($rev.implode(',', $jsfiles)); -// Use the caching only for meaningful revision numbers which prevents future cache poisoning. -if ($rev > 0 and $rev < (time() + 60*60)) { +if ($rev > 0) { $candidate = $CFG->localcachedir.'/js/'.$etag; if (file_exists($candidate)) { diff --git a/lib/tests/configonlylib_test.php b/lib/tests/configonlylib_test.php index 1032e440241..01d8215aca1 100644 --- a/lib/tests/configonlylib_test.php +++ b/lib/tests/configonlylib_test.php @@ -21,7 +21,6 @@ defined('MOODLE_INTERNAL') || die(); // Global $CFG not used here intentionally to make sure it is not required inside the lib. require_once(__DIR__ . '/../configonlylib.php'); - /** * Unit tests for config only library functions. * @@ -144,4 +143,76 @@ class configonlylib_test extends \advanced_testcase { // Windows dir separators are removed, multiple ... gets collapsed to one . $this->assertSame('/standardcore/./5/u/f1', min_get_slash_argument()); } + + /** + * Test the min_get_minimum_version function. + * + * @covers ::min_get_minimum_version + */ + public function test_min_get_minimum_version(): void { + // This is fairly hard to write a test for, but we can at least check that it returns a number + // greater than the version when the feature was first introduced. + $firstintroduced = 1693612800; // Equivalent to 20230902 00:00:00 GMT. + $this->assertGreaterThanOrEqual($firstintroduced, min_get_minimum_revision()); + } + + /** + * Test the min_get_maximum_version function. + * + * @covers ::min_get_maximum_version + */ + public function test_min_get_maximum_version(): void { + // The maximum version should be set to a time in the near future. + // This is currently defined as "in the next minute". + // Note: We use a 65 second window to allow for slow test runners. + $this->assertGreaterThan(time(), min_get_maximum_revision()); + $this->assertLessThanOrEqual(time() + 65, min_get_maximum_revision()); + } + + /** + * Test the min_is_revision_valid_and_current function. + * + * @covers ::min_is_revision_valid_and_current + * @dataProvider min_is_revision_valid_and_current_provider + */ + public function test_min_is_revision_valid_and_current(int $revision, bool $expected): void { + $this->assertEquals($expected, min_is_revision_valid_and_current($revision)); + } + + /** + * Data provider for the min_is_revision_valid_and_current tests. + * + * @return array + */ + public function min_is_revision_valid_and_current_provider(): array { + return [ + 'Negative value' => [-1, false], + 'Empty value' => [0, false], + 'A time before the minimum accepted value' => [min_get_minimum_revision() - 1, false], + 'The minimum accepted value' => [min_get_minimum_revision(), true], + 'The current time' => [time(), true], + // Note: We have to be careful using time values because the data provider is run at the start of the phpunit run, + // but the test may not be run for some time. + // On a slower machine and/or database, this could be several hours. + // For a more specific time we must have a specific test function. + 'A time in the future' => [time() + DAYSECS, false] + ]; + } + + + /** + * Test the min_is_revision_valid_and_current function with close times. + * + * Note: These tests are incompatible with data providers. + * + * @covers ::min_is_revision_valid_and_current + */ + public function test_min_is_revision_valid_and_current_close_proximity(): void { + // A time in the near future. + $this->assertTrue(min_is_revision_valid_and_current(time() + 55)); + + // A time in the too-far future. + $this->assertFalse(min_is_revision_valid_and_current(time() + 70)); + + } } diff --git a/theme/font.php b/theme/font.php index 7a8f43a4000..be8a3af130e 100644 --- a/theme/font.php +++ b/theme/font.php @@ -49,6 +49,11 @@ if ($slashargument = min_get_slash_argument()) { $font = min_optional_param('font', '', 'RAW'); } +if (!min_is_revision_valid_and_current($rev)) { + // If the rev is invalid, normalise it to -1 to disable all caching. + $rev = -1; +} + if (!$font) { font_not_found(); } diff --git a/theme/image.php b/theme/image.php index e9cc853f81d..caa81ca9d31 100644 --- a/theme/image.php +++ b/theme/image.php @@ -58,6 +58,11 @@ if ($slashargument = min_get_slash_argument()) { $usesvg = (bool)min_optional_param('svg', '1', 'INT'); } +if (!min_is_revision_valid_and_current($rev)) { + // If the rev is invalid, normalise it to -1 to disable all caching. + $rev = -1; +} + if (empty($component) or $component === 'moodle' or $component === 'core') { $component = 'core'; } diff --git a/theme/javascript.php b/theme/javascript.php index c6bda484fb8..203ca94c08b 100644 --- a/theme/javascript.php +++ b/theme/javascript.php @@ -50,6 +50,11 @@ if ($slashargument = min_get_slash_argument()) { $type = min_optional_param('type', 'head', 'RAW'); } +if (!min_is_revision_valid_and_current($rev)) { + // If the rev is invalid, normalise it to -1 to disable all caching. + $rev = -1; +} + if ($type !== 'head' and $type !== 'footer') { header('HTTP/1.0 404 not found'); die('Theme was not found, sorry.'); diff --git a/theme/styles.php b/theme/styles.php index c64eb6546bc..4a3239aa38e 100644 --- a/theme/styles.php +++ b/theme/styles.php @@ -66,6 +66,13 @@ if (!is_null($themesubrev)) { $themesubrev = min_clean_param($themesubrev, 'INT'); } +// Note: We only check validity of the revision number here, we do not check the theme sub-revision because this is +// not solely based on time. +if (!min_is_revision_valid_and_current($rev)) { + // If the rev is invalid, normalise it to -1 to disable all caching. + $rev = -1; +} + // Check that type fits into the expected values. if (!in_array($type, ['all', 'all-rtl', 'editor', 'editor-rtl'])) { css_send_css_not_found(); diff --git a/theme/yui_combo.php b/theme/yui_combo.php index 5ffe8a07bdc..319faa4e255 100644 --- a/theme/yui_combo.php +++ b/theme/yui_combo.php @@ -231,8 +231,9 @@ while (count($parts)) { continue; } $revision = (int)array_shift($bits); - if ($revision === -1) { - // Revision -1 says please don't cache the JS + if (!min_is_revision_valid_and_current($revision)) { + // A non-current revision means please don't cache the JS + $revision = -1; $cache = false; } $frankenstyle = array_shift($bits); @@ -278,8 +279,9 @@ while (count($parts)) { continue; } $revision = (int)array_shift($bits); - if ($revision === -1) { - // Revision -1 says please don't cache the JS + if (!min_is_revision_valid_and_current($revision)) { + // A non-current revision means please don't cache the JS + $revision = -1; $cache = false; } $contentfile = "$CFG->libdir/yuilib/gallery/" . join('/', $bits);