Moodle: moodle: formula injection allows arbitrary formula execution via unescaped data export

@@ -832,27 +832,27 @@ public function test_set_and_render_caption_for_table(): void {
     public function test_table_exports_escaped_formulas(): void {
         $table = new flexible_table('tablelib_test_export');
         $table->define_baseurl('/invalid.php');
-        $table->define_columns(['c1', 'c2', 'c3', 'c4', 'c5', 'c6', 'c7', 'c8']);
+        $table->define_columns(['c0', 'c1', 'c2', 'c3', 'c4', 'c5', 'c6']);
 
         ob_start();
         $table->is_downloadable(true);
         $table->is_downloading('csv');
 
         $table->setup();
         $table->add_data([
-            'column0' => "\t=SUM(1+1)", // With tab.
-            'column1' => "\r=SUM(1+1)", // With carriage return.
-            'column2' => "\n=SUM(1+1)", // With new line.
-            'column3' => "=SUM(1+1)",
-            'column4' => "=1+1",
-            'column5' => "+1+1",
-            'column6' => "-1+1",
-            'column7' => "@A1",
-            'column8' => "-", // Single dash (should not be escaped).
+            'column0' => "  =SUM(1+1)", // With spaces.
+            'column1' => "=SUM(1+1)",
+            'column2' => "=1+1",
+            'column3' => "+1+1",
+            'column4' => "-1+1",
+            'column5' => "@A1",
+            'column6' => "-", // Single dash (should not be escaped).
         ]);
+
         $output = ob_get_contents();
         ob_end_clean();
 
-        $this->assertEquals("\n'=SUM(1+1),'=SUM(1+1),'=SUM(1+1),'=SUM(1+1),'=1+1,'+1+1,'-1+1,'@A1,-\n", substr($output, 3));
+        $matchregex = "/\"?'  =SUM\(1\+1\)\"?,'=SUM\(1\+1\),'=1\+1,'\+1\+1,'-1\+1,'@A1,-/";
+        $this->assertMatchesRegularExpression($matchregex, $output);
     }
 }

@@ -449,6 +449,9 @@ public function add_data($row) {
             }
         }
         $delimiter = csv_import_reader::get_delimiter($this->delimiter);
+        foreach ($row as $key => $value) {
+            $row[$key] = \core\dataformat::escape_spreadsheet_formula($value);
+        }
         fputcsv($this->fp, $row, $delimiter, $this->csvenclosure, '\\');
     }
 

@@ -207,6 +207,7 @@ public function write_string($row, $col, $str, $format = null) {
         $col += 1;
 
         $celladdress = CellAddress::fromColumnAndRow($col, $row + 1);
+        $str = \core\dataformat::escape_spreadsheet_formula($str);
 
         $this->worksheet->getStyle($celladdress)->getNumberFormat()->setFormatCode(NumberFormat::FORMAT_TEXT);
         $this->worksheet->getCell($celladdress)->setValueExplicit($str, DataType::TYPE_STRING);

@@ -164,6 +164,8 @@ public function write_string($row, $col, $str, $format = null) {
         if (is_array($format)) {
             $format = new MoodleODSFormat($format);
         }
+        $str = \core\dataformat::escape_spreadsheet_formula($str);
+
         $this->data[$row][$col]->value = $str;
         $this->data[$row][$col]->type = 'string';
         $this->data[$row][$col]->format = $format;

@@ -168,4 +168,39 @@ public static function write_data_to_filearea(array $filerecord, string $datafor
 
         return get_file_storage()->create_file_from_pathname($filerecord, $filepath);
     }
+
+    /**
+     * Escape formula spreadsheet values.
+     *
+     * Check values being used in spreadsheets and make them safe for inclusion.
+     * Following OWASP recommendations {@link https://owasp.org/www-community/attacks/CSV_Injection}.
+     *
+     * @param mixed $value Value to check.
+     * @return string|null Return escaped formula if detected.
+     */
+    public static function escape_spreadsheet_formula(mixed $value): ?string {
+        // Allow mixed input; only process strings.
+        if (!is_string($value)) {
+            return $value;
+        }
+
+        // Moodle's null placeholder: exactly one dash.
+        if ($value === '-') {
+            return $value;
+        }
+
+        // Trim only for checking, not for modifying output.
+        $trimmed = ltrim($value);
+        if ($trimmed === '') {
+            return $value;
+        }
+
+        $formulacharacters = ['=', '+', '-', '@'];
+        // If trimmed version starts with formula character, escape it.
+        if (in_array($trimmed[0], $formulacharacters, true)) {
+            // Prepend single quote if value starts with a formula character.
+            return "'" . $value;
+        }
+        return $value;
+    }
 }

@@ -123,7 +123,11 @@ public function start_sheet($columns) {
      * @param int $rownum
      */
     public function write_record($record, $rownum) {
-        $row = Row::fromValues($this->format_record($record));
+        $rowvalues = $this->format_record($record);
+        foreach ($rowvalues as $key => $value) {
+            $rowvalues[$key] = \core\dataformat::escape_spreadsheet_formula($value);
+        }
+        $row = Row::fromValues($rowvalues);
         $this->writer->addRow($row);
     }
 

@@ -826,4 +826,33 @@ public function test_set_and_render_caption_for_table(): void {
         $this->expectOutputRegex('/' . '<caption class="inline">' . $caption . '<\/caption>' . '/');
     }
 
+    /**
+     * Test formulas are escaped in exported tables.
+     */
+    public function test_table_exports_escaped_formulas(): void {
+        $table = new flexible_table('tablelib_test_export');
+        $table->define_baseurl('/invalid.php');
+        $table->define_columns(['c1', 'c2', 'c3', 'c4', 'c5', 'c6', 'c7', 'c8']);
+
+        ob_start();
+        $table->is_downloadable(true);
+        $table->is_downloading('csv');
+
+        $table->setup();
+        $table->add_data([
+            'column0' => "\t=SUM(1+1)", // With tab.
+            'column1' => "\r=SUM(1+1)", // With carriage return.
+            'column2' => "\n=SUM(1+1)", // With new line.
+            'column3' => "=SUM(1+1)",
+            'column4' => "=1+1",
+            'column5' => "+1+1",
+            'column6' => "-1+1",
+            'column7' => "@A1",
+            'column8' => "-", // Single dash (should not be escaped).
+        ]);
+        $output = ob_get_contents();
+        ob_end_clean();
+
+        $this->assertEquals("\n'=SUM(1+1),'=SUM(1+1),'=SUM(1+1),'=SUM(1+1),'=1+1,'+1+1,'-1+1,'@A1,-\n", substr($output, 3));
+    }
 }

@@ -122,4 +122,80 @@ public function test_write_data_to_filearea(string $dataformat): void {
         $this->assertStringStartsWith($filerecord['filename'], $file->get_filename());
         $this->assertGreaterThan(0, $file->get_filesize());
     }
+
+    /**
+     * Data provider for test_escape_spreadsheet_formula.
+     *
+     * @return array
+     */
+    public static function escape_spreadsheet_formula_provider(): array {
+        return [
+            'null stays null' => [
+                null,
+                null,
+            ],
+            'empty string stays empty' => [
+                '',
+                '',
+            ],
+            'Formula with tab' => [
+                'value' => "\t=SUM(1+1)",
+                'expected' => "'\t=SUM(1+1)",
+            ],
+            'Formula with carriage return' => [
+                'value' => "\r=SUM(1+1)",
+                'expected' => "'\r=SUM(1+1)",
+            ],
+            'Formula with new line' => [
+                'value' => "\n=SUM(1+1)",
+                'expected' => "'\n=SUM(1+1)",
+            ],
+            'Formula starting with "="' => [
+                'value' => "=SUM(1+1)",
+                'expected' => "'=SUM(1+1)",
+            ],
+            'Formula starting with "+"' => [
+                'value' => "+1+1",
+                'expected' => "'+1+1",
+            ],
+            'Formula starting with "-"' => [
+                'value' => "-1+1",
+                'expected' => "'-1+1",
+            ],
+            'Formula starting with "@"' => [
+                'value' => "@A5",
+                'expected' => "'@A5",
+            ],
+            'Null placeholder' => [
+                'value' => "-",
+                'expected' => "-",
+            ],
+            'dash with leading space is not placeholder, so escaped' => [
+                ' -',
+                "' -",
+            ],
+            'dash with trailing space is not placeholder, so escaped' => [
+                '- ',
+                "'- ",
+            ],
+            'Non-formula' => [
+                'value' => "Hello there",
+                'expected' => "Hello there",
+            ],
+        ];
+    }
+
+    /**
+     * Test escape_spreadsheet_formula.
+     *
+     * @dataProvider escape_spreadsheet_formula_provider
+     * @param string|null $value The value to test.
+     * @param string|null $expected The expected value after escaping.
+     */
+    public function test_escape_spreadsheet_formula(?string $value, ?string $expected): void {
+        $this->resetAfterTest();
+
+        $escapedvalue = dataformat::escape_spreadsheet_formula($value);
+        $this->assertEquals($expected, $escapedvalue);
+    }
 }