Merge pull request #49 from silverstripe-security/pulls/3.5/ss-2017-007

[ss-2017-007] Ensure xls formulae are safely sanitised on output (3.5)

dev/CSVParser.php

@@ -247,7 +247,9 @@ class CSVParser extends Object implements Iterator {
 					array($this->enclosure, $this->delimiter),
 					$value
 				);
-
+				// Trim leading tab
+				// [SS-2017-007] Ensure all cells with leading [@=+] have a leading tab
+				$value = ltrim($value, "\t");
 				if(array_key_exists($i, $this->headerRow)) {
 					if($this->headerRow[$i]) {
 						$row[$this->headerRow[$i]] = $value;

forms/gridfield/GridFieldExportButton.php

@@ -30,6 +30,15 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP
 	 */
 	protected $targetFragment;
 
+	/**
+	 * Set to true to disable XLS sanitisation
+	 * [SS-2017-007] Ensure all cells with leading [@=+] have a leading tab
+	 *
+	 * @config
+	 * @var bool
+	 */
+	private static $xls_export_disabled = false;
+
 	/**
 	 * @param string $targetFragment The HTML fragment to write the button into
 	 * @param array $exportColumns The columns to include in the export
@@ -174,6 +183,13 @@ class GridFieldExportButton implements GridField_HTMLProvider, GridField_ActionP
 					}
 
 					$value = str_replace(array("\r", "\n"), "\n", $value);
+
+					// [SS-2017-007] Sanitise XLS executable column values with a leading tab
+					if (!Config::inst()->get(get_class($this), 'xls_export_disabled')
+						&& preg_match('/^[-@=+].*/', $value)
+					) {
+						$value = "\t" . $value;
+					}
 					$columnData[] = '"' . str_replace('"', '""', $value) . '"';
 				}
 

tests/dev/CSVParserTest.php

@@ -21,16 +21,18 @@ class CSVParserTest extends SapphireTest {
 			$registered[] = $record['IsRegistered'];
 		}
 
-		$this->assertEquals(array('John','Jane','Jamie','Järg'), $firstNames);
+		$this->assertEquals(array('John','Jane','Jamie','Järg','Jacob'), $firstNames);
 
 		$this->assertEquals(array(
 			"He's a good guy",
 			"She is awesome." . PHP_EOL
 				. "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
 			"Pretty old, with an escaped comma",
-			"Unicode FTW"), $biographies);
+			"Unicode FTW",
-		$this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays);
+			"Likes leading tabs in his biography",
-		$this->assertEquals(array('1', '0', '1', '1'), $registered);
+		), $biographies);
+		$this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982","31/4/2000"), $birthdays);
+		$this->assertEquals(array('1', '0', '1', '1', '0'), $registered);
 	}
 
 	public function testParsingWithHeadersAndColumnMap() {
@@ -54,15 +56,16 @@ class CSVParserTest extends SapphireTest {
 			$registered[] = $record['IsRegistered'];
 		}
 
-		$this->assertEquals(array('John','Jane','Jamie','Järg'), $firstNames);
+		$this->assertEquals(array('John','Jane','Jamie','Järg','Jacob'), $firstNames);
 		$this->assertEquals(array(
 			"He's a good guy",
 			"She is awesome."
 				. PHP_EOL . "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
 			"Pretty old, with an escaped comma",
-			"Unicode FTW"), $biographies);
+			"Unicode FTW",
-		$this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays);
+			"Likes leading tabs in his biography"), $biographies);
-		$this->assertEquals(array('1', '0', '1', '1'), $registered);
+		$this->assertEquals(array("31/01/1988","31/01/1982","31/01/1882","31/06/1982","31/4/2000"), $birthdays);
+		$this->assertEquals(array('1', '0', '1', '1', '0'), $registered);
 	}
 
 	public function testParsingWithExplicitHeaderRow() {
@@ -82,15 +85,16 @@ class CSVParserTest extends SapphireTest {
 		}
 
 		/* And the first row will be returned in the data */
-		$this->assertEquals(array('FirstName','John','Jane','Jamie','Järg'), $firstNames);
+		$this->assertEquals(array('FirstName','John','Jane','Jamie','Järg','Jacob'), $firstNames);
 		$this->assertEquals(array(
 			'Biography',
 			"He's a good guy",
 			"She is awesome." . PHP_EOL
 				. "So awesome that she gets multiple rows and \"escaped\" strings in her biography",
 			"Pretty old, with an escaped comma",
-			"Unicode FTW"), $biographies);
+			"Unicode FTW",
-		$this->assertEquals(array("Birthday","31/01/1988","31/01/1982","31/01/1882","31/06/1982"), $birthdays);
+			"Likes leading tabs in his biography"), $biographies);
-		$this->assertEquals(array('IsRegistered', '1', '0', '1', '1'), $registered);
+		$this->assertEquals(array("Birthday","31/01/1988","31/01/1982","31/01/1882","31/06/1982","31/4/2000"), $birthdays);
+		$this->assertEquals(array('IsRegistered', '1', '0', '1', '1', '0'), $registered);
 	}
 }

tests/dev/CsvBulkLoaderTest.php

@@ -27,7 +27,7 @@ class CsvBulkLoaderTest extends SapphireTest {
 		$results = $loader->load($filepath);
 
 		// Test that right amount of columns was imported
-		$this->assertEquals(4, $results->Count(), 'Test correct count of imported data');
+		$this->assertEquals(5, $results->Count(), 'Test correct count of imported data');
 
 		// Test that columns were correctly imported
 		$obj = DataObject::get_one("CsvBulkLoaderTest_Player", array(
@@ -49,14 +49,14 @@ class CsvBulkLoaderTest extends SapphireTest {
 		$filepath = $this->getCurrentAbsolutePath() . '/CsvBulkLoaderTest_PlayersWithHeader.csv';
 		$loader->deleteExistingRecords = true;
 		$results1 = $loader->load($filepath);
-		$this->assertEquals(4, $results1->Count(), 'Test correct count of imported data on first load');
+		$this->assertEquals(5, $results1->Count(), 'Test correct count of imported data on first load');
 
 		//delete existing data before doing second CSV import
 		$results2 = $loader->load($filepath, '512MB', true);
 		//get all instances of the loaded DataObject from the database and count them
 		$resultDataObject = DataObject::get('CsvBulkLoaderTest_Player');
 
-		$this->assertEquals(4, $resultDataObject->Count(),
+		$this->assertEquals(5, $resultDataObject->Count(),
 			'Test if existing data is deleted before new data is added');
 		}
 

tests/dev/CsvBulkLoaderTest_PlayersWithHeader.csv

@@ -4,3 +4,4 @@
 So awesome that she gets multiple rows and \"escaped\" strings in her biography","31/01/1982","0"
 "Jamie","Pretty old\, with an escaped comma","31/01/1882","1"
 "Järg","Unicode FTW","31/06/1982","1"
+"Jacob","	Likes leading tabs in his biography","31/4/2000","0"

tests/forms/gridfield/GridFieldExportButtonTest.php

@@ -53,6 +53,22 @@ class GridFieldExportButtonTest extends SapphireTest {
 		);
 	}
 
+	public function testXLSSanitisation() {
+		// Create risky object
+		$object = new GridFieldExportButtonTest_Team();
+		$object->Name = '=SUM(1, 2)';
+		$object->write();
+
+		// Export
+		$button = new GridFieldExportButton();
+		$button->setExportColumns(array('Name' => 'My Name'));
+
+		$this->assertEquals(
+			"\"My Name\"\n\"\t=SUM(1, 2)\"\n\"Test\"\n\"Test2\"\n",
+			$button->generateExportFileData($this->gridField)
+		);
+	}
+
 	public function testGenerateFileDataAnonymousFunctionField() {
 		$button = new GridFieldExportButton();
 		$button->setExportColumns(array(