From: Christian Flothmann Date: Fri, 1 Sep 2017 11:39:51 +0200 Subject: ensure that submitted data are uploaded files [CVE-2017-16790] https://symfony.com/blog/cve-2017-16790-ensure-that-submitted-data-are-uploaded-files Origin: backport, https://github.com/symfony/symfony/commit/f9e210cc7bbaa52adc2a6f695cc343854fa43df6 --- UPGRADE-2.7.md | 2 + src/Symfony/Component/Form/CHANGELOG.md | 5 + .../Form/Extension/Core/Type/FileType.php | 31 +++-- .../HttpFoundationRequestHandler.php | 9 +- .../Component/Form/NativeRequestHandler.php | 21 ++-- .../Component/Form/RequestHandlerInterface.php | 7 ++ .../Form/Tests/AbstractRequestHandlerTest.php | 12 ++ .../Tests/Extension/Core/Type/FileTypeTest.php | 132 ++++++++++++++------- .../HttpFoundationRequestHandlerTest.php | 5 + .../Form/Tests/NativeRequestHandlerTest.php | 11 ++ 10 files changed, 175 insertions(+), 60 deletions(-) diff --git a/UPGRADE-2.7.md b/UPGRADE-2.7.md index 5de67eb..432c036 100644 --- a/UPGRADE-2.7.md +++ b/UPGRADE-2.7.md @@ -39,6 +39,8 @@ Router Form ---- + * the `isFileUpload()` method was added to the `RequestHandlerInterface` + * In form types and extension overriding the "setDefaultOptions" of the AbstractType or AbstractExtensionType has been deprecated in favor of overriding the new "configureOptions" method. diff --git a/src/Symfony/Component/Form/CHANGELOG.md b/src/Symfony/Component/Form/CHANGELOG.md index e79eec4..31e991d 100644 --- a/src/Symfony/Component/Form/CHANGELOG.md +++ b/src/Symfony/Component/Form/CHANGELOG.md @@ -12,6 +12,11 @@ CHANGELOG * moved data trimming logic of TrimListener into StringUtil * [BC BREAK] When registering a type extension through the DI extension, the tag alias has to match the actual extended type. +2.7.38 +------ + + * [BC BREAK] the `isFileUpload()` method was added to the `RequestHandlerInterface` + 2.7.0 ----- diff --git a/src/Symfony/Component/Form/Extension/Core/Type/FileType.php b/src/Symfony/Component/Form/Extension/Core/Type/FileType.php index 40da512..b761f14 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/FileType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/FileType.php @@ -27,20 +27,35 @@ class FileType extends AbstractType */ public function buildForm(FormBuilderInterface $builder, array $options) { - if ($options['multiple']) { - $builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $event) { - $form = $event->getForm(); - $data = $event->getData(); + $builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $event) use ($options) { + $form = $event->getForm(); + $requestHandler = $form->getConfig()->getRequestHandler(); + $data = null; + + if ($options['multiple']) { + $data = array(); + + foreach ($event->getData() as $file) { + if ($requestHandler->isFileUpload($file)) { + $data[] = $file; + } + } // submitted data for an input file (not required) without choosing any file - if (array(null) === $data) { + if (array(null) === $data || array() === $data) { $emptyData = $form->getConfig()->getEmptyData(); $data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData; - $event->setData($data); } - }); - } + + $event->setData($data); + } elseif (!$requestHandler->isFileUpload($event->getData())) { + $emptyData = $form->getConfig()->getEmptyData(); + + $data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData; + $event->setData($data); + } + }); } /** diff --git a/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php b/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php index 98bbd4b..d30adaf 100644 --- a/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php +++ b/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php @@ -16,6 +16,7 @@ use Symfony\Component\Form\FormError; use Symfony\Component\Form\FormInterface; use Symfony\Component\Form\RequestHandlerInterface; use Symfony\Component\Form\Util\ServerParams; +use Symfony\Component\HttpFoundation\File\File; use Symfony\Component\HttpFoundation\Request; /** @@ -31,9 +32,6 @@ class HttpFoundationRequestHandler implements RequestHandlerInterface */ private $serverParams; - /** - * {@inheritdoc} - */ public function __construct(ServerParams $serverParams = null) { $this->serverParams = $serverParams ?: new ServerParams(); @@ -115,4 +113,9 @@ class HttpFoundationRequestHandler implements RequestHandlerInterface $form->submit($data, 'PATCH' !== $method); } + + public function isFileUpload($data) + { + return $data instanceof File; + } } diff --git a/src/Symfony/Component/Form/NativeRequestHandler.php b/src/Symfony/Component/Form/NativeRequestHandler.php index c9a7685..dfff480 100644 --- a/src/Symfony/Component/Form/NativeRequestHandler.php +++ b/src/Symfony/Component/Form/NativeRequestHandler.php @@ -26,14 +26,6 @@ class NativeRequestHandler implements RequestHandlerInterface */ private $serverParams; - /** - * {@inheritdoc} - */ - public function __construct(ServerParams $params = null) - { - $this->serverParams = $params ?: new ServerParams(); - } - /** * The allowed keys of the $_FILES array. * @@ -47,6 +39,11 @@ class NativeRequestHandler implements RequestHandlerInterface 'type', ); + public function __construct(ServerParams $params = null) + { + $this->serverParams = $params ?: new ServerParams(); + } + /** * {@inheritdoc} */ @@ -129,6 +126,14 @@ class NativeRequestHandler implements RequestHandlerInterface $form->submit($data, 'PATCH' !== $method); } + public function isFileUpload($data) + { + // POST data will always be strings or arrays of strings. Thus, we can be sure + // that the submitted data is a file upload if the "error" value is an integer + // (this value must have been injected by PHP itself). + return is_array($data) && isset($data['error']) && is_int($data['error']); + } + /** * Returns the method used to submit the request to the server. * diff --git a/src/Symfony/Component/Form/RequestHandlerInterface.php b/src/Symfony/Component/Form/RequestHandlerInterface.php index d0a58e6..94c7bf3 100644 --- a/src/Symfony/Component/Form/RequestHandlerInterface.php +++ b/src/Symfony/Component/Form/RequestHandlerInterface.php @@ -25,4 +25,11 @@ interface RequestHandlerInterface * @param mixed $request The current request. */ public function handleRequest(FormInterface $form, $request = null); + + /** + * @param mixed $data + * + * @return bool + */ + public function isFileUpload($data); } diff --git a/src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php b/src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php index 022b514..0a4bcf2 100644 --- a/src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php +++ b/src/Symfony/Component/Form/Tests/AbstractRequestHandlerTest.php @@ -355,12 +355,24 @@ abstract class AbstractRequestHandlerTest extends \PHPUnit_Framework_TestCase ); } + public function testUploadedFilesAreAccepted() + { + $this->assertTrue($this->requestHandler->isFileUpload($this->getMockFile())); + } + + public function testInvalidFilesAreRejected() + { + $this->assertFalse($this->requestHandler->isFileUpload($this->getInvalidFile())); + } + abstract protected function setRequestData($method, $data, $files = array()); abstract protected function getRequestHandler(); abstract protected function getMockFile($suffix = ''); + abstract protected function getInvalidFile(); + protected function getMockForm($name, $method = null, $compound = true) { $config = $this->getMock('Symfony\Component\Form\FormConfigInterface'); diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php index cbeeb46..5156806 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php @@ -11,8 +11,15 @@ namespace Symfony\Component\Form\Tests\Extension\Core\Type; +use Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationRequestHandler; +use Symfony\Component\Form\NativeRequestHandler; +use Symfony\Component\Form\RequestHandlerInterface; +use Symfony\Component\HttpFoundation\File\UploadedFile; + class FileTypeTest extends \Symfony\Component\Form\Test\TypeTestCase { + const TESTED_TYPE = 'Symfony\Component\Form\Extension\Core\Type\FileType'; + /** * @group legacy */ @@ -26,18 +33,23 @@ class FileTypeTest extends \Symfony\Component\Form\Test\TypeTestCase // https://github.com/symfony/symfony/pull/5028 public function testSetData() { - $form = $this->factory->createBuilder('Symfony\Component\Form\Extension\Core\Type\FileType')->getForm(); - $data = $this->createUploadedFileMock('abcdef', 'original.jpg', true); + $form = $this->factory->createBuilder(static::TESTED_TYPE)->getForm(); + $data = $this->getMockBuilder('Symfony\Component\HttpFoundation\File\File') + ->setConstructorArgs(array(__DIR__.'/../../../Fixtures/foo', 'foo')) + ->getMock(); $form->setData($data); $this->assertSame($data, $form->getData()); } - public function testSubmit() + /** + * @dataProvider requestHandlerProvider + */ + public function testSubmit(RequestHandlerInterface $requestHandler) { - $form = $this->factory->createBuilder('Symfony\Component\Form\Extension\Core\Type\FileType')->getForm(); - $data = $this->createUploadedFileMock('abcdef', 'original.jpg', true); + $form = $this->factory->createBuilder(static::TESTED_TYPE)->setRequestHandler($requestHandler)->getForm(); + $data = $this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo', 'foo.jpg'); $form->submit($data); @@ -66,30 +78,36 @@ class FileTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertSame(array(), $form->getData()); } - public function testSetDataMultiple() + /** + * @dataProvider requestHandlerProvider + */ + public function testSetDataMultiple(RequestHandlerInterface $requestHandler) { $form = $this->factory->createBuilder('file', null, array( 'multiple' => true, - ))->getForm(); + ))->setRequestHandler($requestHandler)->getForm(); $data = array( - $this->createUploadedFileMock('abcdef', 'first.jpg', true), - $this->createUploadedFileMock('zyxwvu', 'second.jpg', true), + $this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo', 'foo.jpg'), + $this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo2', 'foo2.jpg'), ); $form->setData($data); $this->assertSame($data, $form->getData()); } - public function testSubmitMultiple() + /** + * @dataProvider requestHandlerProvider + */ + public function testSubmitMultiple(RequestHandlerInterface $requestHandler) { $form = $this->factory->createBuilder('Symfony\Component\Form\Extension\Core\Type\FileType', null, array( 'multiple' => true, - ))->getForm(); + ))->setRequestHandler($requestHandler)->getForm(); $data = array( - $this->createUploadedFileMock('abcdef', 'first.jpg', true), - $this->createUploadedFileMock('zyxwvu', 'second.jpg', true), + $this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo', 'foo.jpg'), + $this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo2', 'foo2.jpg'), ); $form->submit($data); @@ -100,40 +118,72 @@ class FileTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertArrayHasKey('multiple', $view->vars['attr']); } - public function testDontPassValueToView() + /** + * @dataProvider requestHandlerProvider + */ + public function testDontPassValueToView(RequestHandlerInterface $requestHandler) { - $form = $this->factory->create('Symfony\Component\Form\Extension\Core\Type\FileType'); + $form = $this->factory->createBuilder(static::TESTED_TYPE)->setRequestHandler($requestHandler)->getForm(); $form->submit(array( - 'Symfony\Component\Form\Extension\Core\Type\FileType' => $this->createUploadedFileMock('abcdef', 'original.jpg', true), + 'file' => $this->createUploadedFileMock($requestHandler, __DIR__.'/../../../Fixtures/foo', 'foo.jpg'), )); - $view = $form->createView(); - $this->assertEquals('', $view->vars['value']); + $this->assertEquals('', $form->createView()->vars['value']); } - private function createUploadedFileMock($name, $originalName, $valid) + /** + * @dataProvider requestHandlerProvider + */ + public function testSubmittedFilePathsAreDropped(RequestHandlerInterface $requestHandler) { - $file = $this - ->getMockBuilder('Symfony\Component\HttpFoundation\File\UploadedFile') - ->setConstructorArgs(array(__DIR__.'/../../../Fixtures/foo', 'foo')) - ->getMock() - ; - $file - ->expects($this->any()) - ->method('getBasename') - ->will($this->returnValue($name)) - ; - $file - ->expects($this->any()) - ->method('getClientOriginalName') - ->will($this->returnValue($originalName)) - ; - $file - ->expects($this->any()) - ->method('isValid') - ->will($this->returnValue($valid)) - ; - - return $file; + $form = $this->factory->createBuilder(static::TESTED_TYPE)->setRequestHandler($requestHandler)->getForm(); + $form->submit('file:///etc/passwd'); + + $this->assertNull($form->getData()); + $this->assertNull($form->getNormData()); + $this->assertSame('', $form->getViewData()); + } + + /** + * @dataProvider requestHandlerProvider + */ + public function testMultipleSubmittedFilePathsAreDropped(RequestHandlerInterface $requestHandler) + { + $form = $this->factory + ->createBuilder(static::TESTED_TYPE, null, array( + 'multiple' => true, + )) + ->setRequestHandler($requestHandler) + ->getForm(); + $form->submit(array( + 'file:///etc/passwd', + $this->createUploadedFileMock(new HttpFoundationRequestHandler(), __DIR__.'/../../../Fixtures/foo', 'foo.jpg'), + $this->createUploadedFileMock(new NativeRequestHandler(), __DIR__.'/../../../Fixtures/foo2', 'foo2.jpg'), + )); + + $this->assertCount(1, $form->getData()); + } + + public function requestHandlerProvider() + { + return array( + array(new HttpFoundationRequestHandler()), + array(new NativeRequestHandler()), + ); + } + + private function createUploadedFileMock(RequestHandlerInterface $requestHandler, $path, $originalName) + { + if ($requestHandler instanceof HttpFoundationRequestHandler) { + return new UploadedFile($path, $originalName, null, 10, null, true); + } + + return array( + 'name' => $originalName, + 'error' => 0, + 'type' => 'text/plain', + 'tmp_name' => $path, + 'size' => 10, + ); } } diff --git a/src/Symfony/Component/Form/Tests/Extension/HttpFoundation/HttpFoundationRequestHandlerTest.php b/src/Symfony/Component/Form/Tests/Extension/HttpFoundation/HttpFoundationRequestHandlerTest.php index e71a1fd..9fbe122 100644 --- a/src/Symfony/Component/Form/Tests/Extension/HttpFoundation/HttpFoundationRequestHandlerTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/HttpFoundation/HttpFoundationRequestHandlerTest.php @@ -51,4 +51,9 @@ class HttpFoundationRequestHandlerTest extends AbstractRequestHandlerTest { return new UploadedFile(__DIR__.'/../../Fixtures/foo'.$suffix, 'foo'.$suffix); } + + protected function getInvalidFile() + { + return 'file:///etc/passwd'; + } } diff --git a/src/Symfony/Component/Form/Tests/NativeRequestHandlerTest.php b/src/Symfony/Component/Form/Tests/NativeRequestHandlerTest.php index 3858006..aba4172 100644 --- a/src/Symfony/Component/Form/Tests/NativeRequestHandlerTest.php +++ b/src/Symfony/Component/Form/Tests/NativeRequestHandlerTest.php @@ -216,4 +216,15 @@ class NativeRequestHandlerTest extends AbstractRequestHandlerTest 'size' => 100, ); } + + protected function getInvalidFile() + { + return array( + 'name' => 'upload.txt', + 'type' => 'text/plain', + 'tmp_name' => 'owfdskjasdfsa', + 'error' => '0', + 'size' => '100', + ); + } }