From 64ac84fdb5665732b32172e8765dd68ebf040822 Mon Sep 17 00:00:00 2001 From: David Woloszyn Date: Wed, 6 Sep 2023 15:58:43 +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/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 +++-- 8 files changed, 160 insertions(+), 7 deletions(-) diff --git a/lib/configonlylib.php b/lib/configonlylib.php index 670d69b8ede..114b2f95385 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/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 2dc10cca40c..8942c65993c 100644 --- a/lib/tests/configonlylib_test.php +++ b/lib/tests/configonlylib_test.php @@ -28,7 +28,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. * @@ -151,4 +150,76 @@ class core_configonlylib_testcase 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 = 1592179200; // Equivalent to 20200615 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 18d0f3198b0..4227b4e1abc 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 2b25d142401..d18d073241f 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'])) { css_send_css_not_found(); diff --git a/theme/yui_combo.php b/theme/yui_combo.php index b74f3bd62c9..af8d7b6d8b2 100644 --- a/theme/yui_combo.php +++ b/theme/yui_combo.php @@ -234,8 +234,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); @@ -281,8 +282,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);