Skip to content

Commit d95bc29

Browse files
oleibmanoleibman
committed
Backport Security Patch
1 parent 9c90ed6 commit d95bc29

File tree

13 files changed

+214
-42
lines changed

13 files changed

+214
-42
lines changed

‎phpstan-baseline.neon

+1-1
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ parameters:
287287

288288
-
289289
message: "#^Offset 'mime' does not exist on array\\{\\}\\|array\\{0\\: int\\<0, max\\>, 1\\: int\\<0, max\\>, 2\\: int, 3\\: string, mime\\: string, channels\\?\\: int, bits\\?\\: int\\}\\.$#"
290-
count: 2
290+
count: 1
291291
path: src/PhpSpreadsheet/Writer/Html.php
292292

293293
-

‎src/PhpSpreadsheet/Reader/Xlsx.php

+11-3
Original file line numberDiff line numberDiff line change
@@ -1291,7 +1291,7 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
12911291
$hfImages[$shapeId]->setName((string) $imageData['title']);
12921292
}
12931293

1294-
$hfImages[$shapeId]->setPath('zip://' . File::realpath($filename) . '#' . $drawings[(string) $imageData['relid']], false);
1294+
$hfImages[$shapeId]->setPath('zip://' . File::realpath($filename) . '#' . $drawings[(string) $imageData['relid']], false, $zip);
12951295
$hfImages[$shapeId]->setResizeProportional(false);
12961296
$hfImages[$shapeId]->setWidth($style['width']);
12971297
$hfImages[$shapeId]->setHeight($style['height']);
@@ -1401,7 +1401,8 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
14011401
$objDrawing->setPath(
14021402
'zip://' . File::realpath($filename) . '#' .
14031403
$images[$embedImageKey],
1404-
false
1404+
false,
1405+
$zip
14051406
);
14061407
} else {
14071408
$linkImageKey = (string) self::getArrayItem(
@@ -1412,6 +1413,9 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
14121413
$url = str_replace('xl/drawings/', '', $images[$linkImageKey]);
14131414
$objDrawing->setPath($url);
14141415
}
1416+
if ($objDrawing->getPath() === '') {
1417+
continue;
1418+
}
14151419
}
14161420
$objDrawing->setCoordinates(Coordinate::stringFromColumnIndex(((int) $oneCellAnchor->from->col) + 1) . ($oneCellAnchor->from->row + 1));
14171421

@@ -1486,7 +1490,8 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
14861490
$objDrawing->setPath(
14871491
'zip://' . File::realpath($filename) . '#' .
14881492
$images[$embedImageKey],
1489-
false
1493+
false,
1494+
$zip
14901495
);
14911496
} else {
14921497
$linkImageKey = (string) self::getArrayItem(
@@ -1497,6 +1502,9 @@ protected function loadSpreadsheetFromFile(string $filename): Spreadsheet
14971502
$url = str_replace('xl/drawings/', '', $images[$linkImageKey]);
14981503
$objDrawing->setPath($url);
14991504
}
1505+
if ($objDrawing->getPath() === '') {
1506+
continue;
1507+
}
15001508
}
15011509
$objDrawing->setCoordinates(Coordinate::stringFromColumnIndex(((int) $twoCellAnchor->from->col) + 1) . ($twoCellAnchor->from->row + 1));
15021510

‎src/PhpSpreadsheet/Worksheet/BaseDrawing.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ public function setWorksheet(?Worksheet $worksheet = null, bool $overrideOld = f
220220
{
221221
if ($this->worksheet === null) {
222222
// Add drawing to \PhpOffice\PhpSpreadsheet\Worksheet\Worksheet
223-
if ($worksheet !== null) {
223+
if ($worksheet !== null && !($this instanceof Drawing && $this->getPath() === '')) {
224224
$this->worksheet = $worksheet;
225225
$this->worksheet->getCell($this->coordinates);
226226
$this->worksheet->getDrawingCollection()->append($this);

‎src/PhpSpreadsheet/Worksheet/Drawing.php

+51-19
Original file line numberDiff line numberDiff line change
@@ -106,40 +106,72 @@ public function getPath()
106106
*/
107107
public function setPath($path, $verifyFile = true, $zip = null)
108108
{
109-
if ($verifyFile && preg_match('~^data:image/[a-z]+;base64,~', $path) !== 1) {
110-
// Check if a URL has been passed. https://stackoverflow.com/a/2058596/1252979
111-
if (filter_var($path, FILTER_VALIDATE_URL)) {
112-
$this->path = $path;
113-
// Implicit that it is a URL, rather store info than running check above on value in other places.
114-
$this->isUrl = true;
115-
$imageContents = file_get_contents($path);
109+
$this->isUrl = false;
110+
if (preg_match('~^data:image/[a-z]+;base64,~', $path) === 1) {
111+
$this->path = $path;
112+
113+
return $this;
114+
}
115+
116+
$this->path = '';
117+
// Check if a URL has been passed. https://stackoverflow.com/a/2058596/1252979
118+
if (filter_var($path, FILTER_VALIDATE_URL)) {
119+
if (!preg_match('/^(http|https|file|ftp|s3):/', $path)) {
120+
throw new PhpSpreadsheetException('Invalid protocol for linked drawing');
121+
}
122+
// Implicit that it is a URL, rather store info than running check above on value in other places.
123+
$this->isUrl = true;
124+
$imageContents = @file_get_contents($path);
125+
if ($imageContents !== false) {
116126
$filePath = tempnam(sys_get_temp_dir(), 'Drawing');
117127
if ($filePath) {
118-
file_put_contents($filePath, $imageContents);
119-
if (file_exists($filePath)) {
120-
$this->setSizesAndType($filePath);
128+
$put = @file_put_contents($filePath, $imageContents);
129+
if ($put !== false) {
130+
if ($this->isImage($filePath)) {
131+
$this->path = $path;
132+
$this->setSizesAndType($filePath);
133+
}
121134
unlink($filePath);
122135
}
123136
}
124-
} elseif (file_exists($path)) {
125-
$this->path = $path;
126-
$this->setSizesAndType($path);
127-
} elseif ($zip instanceof ZipArchive) {
128-
$zipPath = explode('#', $path)[1];
129-
if ($zip->locateName($zipPath) !== false) {
137+
}
138+
} elseif ($zip instanceof ZipArchive) {
139+
$zipPath = explode('#', $path)[1];
140+
$locate = @$zip->locateName($zipPath);
141+
if ($locate !== false) {
142+
if ($this->isImage($path)) {
130143
$this->path = $path;
131144
$this->setSizesAndType($path);
132145
}
133-
} else {
134-
throw new PhpSpreadsheetException("File $path not found!");
135146
}
136147
} else {
137-
$this->path = $path;
148+
$exists = @file_exists($path);
149+
if ($exists !== false && $this->isImage($path)) {
150+
$this->path = $path;
151+
$this->setSizesAndType($path);
152+
}
153+
}
154+
if ($this->path === '' && $verifyFile) {
155+
throw new PhpSpreadsheetException("File $path not found!");
138156
}
139157

140158
return $this;
141159
}
142160

161+
private function isImage(string $path): bool
162+
{
163+
$mime = (string) @mime_content_type($path);
164+
$retVal = false;
165+
if (str_starts_with($mime, 'image/')) {
166+
$retVal = true;
167+
} elseif ($mime === 'application/octet-stream') {
168+
$extension = pathinfo($path, PATHINFO_EXTENSION);
169+
$retVal = in_array($extension, ['bin', 'emf'], true);
170+
}
171+
172+
return $retVal;
173+
}
174+
143175
/**
144176
* Get isURL.
145177
*/

‎src/PhpSpreadsheet/Writer/Html.php

+11-5
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,9 @@ private function extendRowsForChartsAndImages(Worksheet $worksheet, int $row): s
612612
[$rowMax, $colMax, $anyfound] = $this->extendRowsForCharts($worksheet, $row);
613613

614614
foreach ($worksheet->getDrawingCollection() as $drawing) {
615+
if ($drawing instanceof Drawing && $drawing->getPath() === '') {
616+
continue;
617+
}
615618
$anyfound = true;
616619
$imageTL = Coordinate::coordinateFromString($drawing->getCoordinates());
617620
$imageCol = Coordinate::columnIndexFromString($imageTL[0]);
@@ -687,7 +690,7 @@ private function writeImageInCell(Worksheet $worksheet, $coordinates)
687690
}
688691
$filedesc = $drawing->getDescription();
689692
$filedesc = $filedesc ? htmlspecialchars($filedesc, ENT_QUOTES) : 'Embedded image';
690-
if ($drawing instanceof Drawing) {
693+
if ($drawing instanceof Drawing && $drawing->getPath() !== '') {
691694
$filename = $drawing->getPath();
692695

693696
// Strip off eventual '.'
@@ -706,12 +709,15 @@ private function writeImageInCell(Worksheet $worksheet, $coordinates)
706709
$imageData = self::winFileToUrl($filename, $this->isMPdf);
707710

708711
if ($this->embedImages || substr($imageData, 0, 6) === 'zip://') {
712+
$imageData = 'data:,';
709713
$picture = @file_get_contents($filename);
710714
if ($picture !== false) {
711-
$imageDetails = getimagesize($filename) ?: [];
712-
// base64 encode the binary data
713-
$base64 = base64_encode($picture);
714-
$imageData = 'data:' . $imageDetails['mime'] . ';base64,' . $base64;
715+
$mimeContentType = (string) @mime_content_type($filename);
716+
if (substr($mimeContentType, 0, 6) === 'image/') {
717+
// base64 encode the binary data
718+
$base64 = base64_encode($picture);
719+
$imageData = 'data:' . $mimeContentType . ';base64,' . $base64;
720+
}
715721
}
716722
}
717723

‎src/PhpSpreadsheet/Writer/Xls.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -486,7 +486,7 @@ private function processDrawing(BstoreContainer &$bstoreContainer, Drawing $draw
486486

487487
private function processBaseDrawing(BstoreContainer &$bstoreContainer, BaseDrawing $drawing): void
488488
{
489-
if ($drawing instanceof Drawing) {
489+
if ($drawing instanceof Drawing && $drawing->getPath() !== '') {
490490
$this->processDrawing($bstoreContainer, $drawing);
491491
} elseif ($drawing instanceof MemoryDrawing) {
492492
$this->processMemoryDrawing($bstoreContainer, $drawing, $drawing->getRenderingFunction());

‎src/PhpSpreadsheet/Writer/Xlsx.php

+9-1
Original file line numberDiff line numberDiff line change
@@ -495,7 +495,9 @@ public function save($filename, int $flags = 0): void
495495

496496
// Media
497497
foreach ($this->spreadSheet->getSheet($i)->getHeaderFooter()->getImages() as $image) {
498-
$zipContent['xl/media/' . $image->getIndexedFilename()] = file_get_contents($image->getPath());
498+
if ($image->getPath() !== '') {
499+
$zipContent['xl/media/' . $image->getIndexedFilename()] = file_get_contents($image->getPath());
500+
}
499501
}
500502
}
501503

@@ -511,6 +513,9 @@ public function save($filename, int $flags = 0): void
511513
if ($this->getDrawingHashTable()->getByIndex($i) instanceof WorksheetDrawing) {
512514
$imageContents = null;
513515
$imagePath = $this->getDrawingHashTable()->getByIndex($i)->getPath();
516+
if ($imagePath === '') {
517+
continue;
518+
}
514519
if (strpos($imagePath, 'zip://') !== false) {
515520
$imagePath = substr($imagePath, 6);
516521
$imagePathSplitted = explode('#', $imagePath);
@@ -712,6 +717,9 @@ private function processDrawing(WorksheetDrawing $drawing)
712717
{
713718
$data = null;
714719
$filename = $drawing->getPath();
720+
if ($filename === '') {
721+
return null;
722+
}
715723
$imageData = getimagesize($filename);
716724

717725
if (!empty($imageData)) {

‎src/PhpSpreadsheet/Writer/Xlsx/ContentTypes.php

+14-8
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use PhpOffice\PhpSpreadsheet\Shared\File;
77
use PhpOffice\PhpSpreadsheet\Shared\XMLWriter;
88
use PhpOffice\PhpSpreadsheet\Spreadsheet;
9+
use PhpOffice\PhpSpreadsheet\Worksheet\Drawing as WorksheetDrawing;
910
use PhpOffice\PhpSpreadsheet\Worksheet\MemoryDrawing;
1011
use PhpOffice\PhpSpreadsheet\Writer\Exception as WriterException;
1112

@@ -132,18 +133,23 @@ public function writeContentTypes(Spreadsheet $spreadsheet, $includeCharts = fal
132133
$extension = '';
133134
$mimeType = '';
134135

135-
if ($this->getParentWriter()->getDrawingHashTable()->getByIndex($i) instanceof \PhpOffice\PhpSpreadsheet\Worksheet\Drawing) {
136-
$extension = strtolower($this->getParentWriter()->getDrawingHashTable()->getByIndex($i)->getExtension());
137-
$mimeType = $this->getImageMimeType($this->getParentWriter()->getDrawingHashTable()->getByIndex($i)->getPath());
138-
} elseif ($this->getParentWriter()->getDrawingHashTable()->getByIndex($i) instanceof MemoryDrawing) {
139-
$extension = strtolower($this->getParentWriter()->getDrawingHashTable()->getByIndex($i)->getMimeType());
136+
$drawing = $this->getParentWriter()->getDrawingHashTable()->getByIndex($i);
137+
if ($drawing instanceof WorksheetDrawing && $drawing->getPath() !== '') {
138+
$extension = strtolower($drawing->getExtension());
139+
if ($drawing->getIsUrl()) {
140+
$mimeType = image_type_to_mime_type($drawing->getType());
141+
} else {
142+
$mimeType = $this->getImageMimeType($drawing->getPath());
143+
}
144+
} elseif ($drawing instanceof MemoryDrawing) {
145+
$extension = strtolower($drawing->getMimeType());
140146
$extension = explode('/', $extension);
141147
$extension = $extension[1];
142148

143-
$mimeType = $this->getParentWriter()->getDrawingHashTable()->getByIndex($i)->getMimeType();
149+
$mimeType = $drawing->getMimeType();
144150
}
145151

146-
if (!isset($aMediaContentTypes[$extension])) {
152+
if ($mimeType !== '' && !isset($aMediaContentTypes[$extension])) {
147153
$aMediaContentTypes[$extension] = $mimeType;
148154

149155
$this->writeDefaultContentType($objWriter, $extension, $mimeType);
@@ -162,7 +168,7 @@ public function writeContentTypes(Spreadsheet $spreadsheet, $includeCharts = fal
162168
for ($i = 0; $i < $sheetCount; ++$i) {
163169
if (count($spreadsheet->getSheet($i)->getHeaderFooter()->getImages()) > 0) {
164170
foreach ($spreadsheet->getSheet($i)->getHeaderFooter()->getImages() as $image) {
165-
if (!isset($aMediaContentTypes[strtolower($image->getExtension())])) {
171+
if ($image->getPath() !== '' && !isset($aMediaContentTypes[strtolower($image->getExtension())])) {
166172
$aMediaContentTypes[strtolower($image->getExtension())] = $this->getImageMimeType($image->getPath());
167173

168174
$this->writeDefaultContentType($objWriter, strtolower($image->getExtension()), $aMediaContentTypes[strtolower($image->getExtension())]);

‎tests/PhpSpreadsheetTests/Reader/Xlsx/URLImageTest.php

+27-1
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,16 @@
22

33
namespace PhpOffice\PhpSpreadsheetTests\Reader\Xlsx;
44

5+
use PhpOffice\PhpSpreadsheet\Exception as SpreadsheetException;
56
use PhpOffice\PhpSpreadsheet\IOFactory;
67
use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;
78
use PhpOffice\PhpSpreadsheetTests\Reader\Utility\File;
89
use PHPUnit\Framework\TestCase;
910

1011
class URLImageTest extends TestCase
1112
{
12-
public function testURLImageSource(): void
13+
// https://github.com/readthedocs/readthedocs.org/issues/11615
14+
public function xtestURLImageSource(): void
1315
{
1416
if (getenv('SKIP_URL_IMAGE_TEST') === '1') {
1517
self::markTestSkipped('Skipped due to setting of environment variable');
@@ -39,4 +41,28 @@ public function testURLImageSource(): void
3941
self::assertSame('png', $extension);
4042
}
4143
}
44+
45+
public function xtestURLImageSourceNotFound(): void
46+
{
47+
if (getenv('SKIP_URL_IMAGE_TEST') === '1') {
48+
self::markTestSkipped('Skipped due to setting of environment variable');
49+
}
50+
$filename = realpath(__DIR__ . '/../../../data/Reader/XLSX/urlImage.notfound.xlsx');
51+
self::assertNotFalse($filename);
52+
$reader = IOFactory::createReader('Xlsx');
53+
$spreadsheet = $reader->load($filename);
54+
$worksheet = $spreadsheet->getActiveSheet();
55+
$collection = $worksheet->getDrawingCollection();
56+
self::assertCount(0, $collection);
57+
}
58+
59+
public function testURLImageSourceBadProtocol(): void
60+
{
61+
$filename = realpath(__DIR__ . '/../../../data/Reader/XLSX/urlImage.bad.dontuse');
62+
self::assertNotFalse($filename);
63+
$this->expectException(SpreadsheetException::class);
64+
$this->expectExceptionMessage('Invalid protocol for linked drawing');
65+
$reader = IOFactory::createReader('Xlsx');
66+
$reader->load($filename);
67+
}
4268
}

0 commit comments

Comments
 (0)