Merge pull request #49 from phpbb/ticket/security/244

[security/244] Add form token check to plupload

phpBB/assets/javascript/plupload.js

@@ -90,6 +90,12 @@ phpbb.plupload.getSerializedData = function() {
 			obj['attachment_data[' + i + '][' + key + ']'] = datum[key];
 		}
 	}
+
+	// Insert form data
+	var $pluploadForm = $(phpbb.plupload.config.form_hook).first();
+	obj.creation_time = $pluploadForm.find('input[type=hidden][name="creation_time"]').val();
+	obj.form_token = $pluploadForm.find('input[type=hidden][name="form_token"]').val();
+
 	return obj;
 };
 
@@ -264,6 +270,17 @@ phpbb.plupload.deleteFile = function(row, attachId) {
 
 			return;
 		}
+
+		// Handle errors while deleting file
+		if (typeof response.error !== 'undefined') {
+			phpbb.alert(phpbb.plupload.lang.ERROR, response.error.message);
+
+			// We will have to assume that the deletion failed. So leave the file status as uploaded.
+			row.find('.file-status').toggleClass('file-uploaded');
+
+			return;
+		}
+
 		phpbb.plupload.update(response, 'removal', index);
 		// Check if the user can upload files now if he had reached the max files limit.
 		phpbb.plupload.handleMaxFilesReached();

phpBB/includes/message_parser.php

@@ -1524,6 +1524,35 @@ class parse_message extends bbcode_firstpass
 		}
 	}
 
+	/**
+	 * Check attachment form token depending on submit type
+	 *
+	 * @param \phpbb\language\language $language Language
+	 * @param \phpbb\request\request_interface $request Request
+	 * @param string $form_name Form name for checking form key
+	 *
+	 * @return bool True if form token is not needed or valid, false if needed and invalid
+	 */
+	function check_attachment_form_token(\phpbb\language\language $language, \phpbb\request\request_interface $request, $form_name)
+	{
+		$add_file = $request->is_set_post('add_file');
+		$delete_file = $request->is_set_post('delete_file');
+
+		if (($add_file || $delete_file) && !check_form_key($form_name))
+		{
+			$this->warn_msg[] = $language->lang('FORM_INVALID');
+
+			if ($request->is_ajax() && $this->plupload)
+			{
+				$this->plupload->emit_error(-400, 'FORM_INVALID');
+			}
+
+			return false;
+		}
+
+		return true;
+	}
+
 	/**
 	* Parse Attachments
 	*/

phpBB/includes/ucp/ucp_pm_compose.php

@@ -26,7 +26,7 @@ if (!defined('IN_PHPBB'))
 function compose_pm($id, $mode, $action, $user_folders = array())
 {
 	global $template, $db, $auth, $user, $cache;
-	global $phpbb_root_path, $phpEx, $config;
+	global $phpbb_root_path, $phpEx, $config, $language;
 	global $request, $phpbb_dispatcher, $phpbb_container;
 
 	// Damn php and globals - i know, this is horrible
@@ -799,7 +799,10 @@ function compose_pm($id, $mode, $action, $user_folders = array())
 		extract($phpbb_dispatcher->trigger_event('core.ucp_pm_compose_modify_parse_before', compact($vars)));
 
 		// Parse Attachments - before checksum is calculated
+		if ($message_parser->check_attachment_form_token($language, $request, 'ucp_pm_compose'))
+		{
 			$message_parser->parse_attachments('fileupload', $action, 0, $submit, $preview, $refresh, true);
+		}
 
 		if (count($message_parser->warn_msg) && !($remove_u || $remove_g || $add_to || $add_bcc))
 		{

phpBB/posting.php

@@ -974,7 +974,10 @@ if ($submit || $preview || $refresh)
 	}
 
 	// Parse Attachments - before checksum is calculated
+	if ($message_parser->check_attachment_form_token($language, $request, 'posting'))
+	{
 		$message_parser->parse_attachments('fileupload', $mode, $forum_id, $submit, $preview, $refresh);
+	}
 
 	/**
 	* This event allows you to modify message text before parsing

phpBB/styles/prosilver/template/plupload.html

@@ -57,6 +57,7 @@ phpbb.plupload = {
 	lang: {
 		ERROR: '{LA_ERROR}',
 		TOO_MANY_ATTACHMENTS: '{LA_TOO_MANY_ATTACHMENTS}',
+		FORM_INVALID: '{LA_FORM_INVALID}',
 	},
 	order: '{ATTACH_ORDER}',
 	maxFiles: {MAX_ATTACHMENTS},

tests/functional/fileupload_form_test.php

@@ -46,6 +46,13 @@ class phpbb_functional_fileupload_form_test extends phpbb_functional_test_case
 
 	private function upload_file($filename, $mimetype)
 	{
+		$crawler = self::$client->request(
+			'GET',
+			'posting.php?mode=reply&f=2&t=1&sid=' . $this->sid
+		);
+
+		$file_form_data = array_merge(['add_file' => $this->lang('ADD_FILE')], $this->get_hidden_fields($crawler, 'posting.php?mode=reply&f=2&t=1&sid=' . $this->sid));
+
 		$file = array(
 			'tmp_name' => $this->path . $filename,
 			'name' => $filename,
@@ -57,7 +64,7 @@ class phpbb_functional_fileupload_form_test extends phpbb_functional_test_case
 		$crawler = self::$client->request(
 			'POST',
 			'posting.php?mode=reply&f=2&t=1&sid=' . $this->sid,
-			array('add_file' => $this->lang('ADD_FILE')),
+			$file_form_data,
 			array('fileupload' => $file)
 		);
 

tests/functional/plupload_test.php

@@ -76,6 +76,10 @@ class phpbb_functional_plupload_test extends phpbb_functional_test_case
 		$chunk_size = ceil(filesize($this->path . 'valid.jpg') / self::CHUNKS);
 		$handle = fopen($this->path . 'valid.jpg', 'rb');
 
+		$crawler = self::$client->request('POST', $url . '&sid=' . $this->sid);
+
+		$file_form_data = $this->get_hidden_fields($crawler, $url);
+
 		for ($i = 0; $i < self::CHUNKS; $i++)
 		{
 			$chunk = fread($handle, $chunk_size);
@@ -94,13 +98,13 @@ class phpbb_functional_plupload_test extends phpbb_functional_test_case
 			$crawler = self::$client->request(
 				'POST',
 				$url . '&sid=' . $this->sid,
-				array(
+				array_merge(array(
 					'chunk' => $i,
 					'chunks' => self::CHUNKS,
 					'name' => md5('valid') . '.jpg',
 					'real_filename' => 'valid.jpg',
 					'add_file' => $this->lang('ADD_FILE'),
-				),
+				), $file_form_data),
 				array('fileupload' => $file),
 				array('X-PHPBB-USING-PLUPLOAD' => '1')
 			);
@@ -134,17 +138,19 @@ class phpbb_functional_plupload_test extends phpbb_functional_test_case
 			'error' => UPLOAD_ERR_OK,
 		);
 
+		$file_form_data = $this->get_hidden_fields(null, $url);
+
 		self::$client->setServerParameter('HTTP_X_PHPBB_USING_PLUPLOAD', '1');
 		self::$client->request(
 			'POST',
 			$url . '&sid=' . $this->sid,
-			array(
+			array_merge(array(
 				'chunk' => '0',
 				'chunks' => '1',
 				'name' => md5('valid') . '.jpg',
 				'real_filename' => 'valid.jpg',
 				'add_file' => $this->lang('ADD_FILE'),
-			),
+			), $file_form_data),
 			array('fileupload' => $file)
 		);
 

tests/test_framework/phpbb_functional_test_case.php

@@ -1166,24 +1166,14 @@ class phpbb_functional_test_case extends phpbb_test_case
 					'error'		=> UPLOAD_ERR_OK,
 				);
 
-				$crawler = self::$client->request('POST', $posting_url, array('add_file' => $this->lang('ADD_FILE')), array('fileupload' => $file));
+				$file_form_data = array_merge(['add_file' => $this->lang('ADD_FILE')], $this->get_hidden_fields($crawler, $posting_url));
+
+				$crawler = self::$client->request('POST', $posting_url, $file_form_data, array('fileupload' => $file));
 			}
 			unset($form_data['upload_files']);
 		}
 
-		$hidden_fields = array(
+		$form_data = array_merge($form_data, $this->get_hidden_fields($crawler, $posting_url));
-			$crawler->filter('[type="hidden"]')->each(function ($node, $i) {
-				return array('name' => $node->attr('name'), 'value' => $node->attr('value'));
-			}),
-		);
-
-		foreach ($hidden_fields as $fields)
-		{
-			foreach($fields as $field)
-			{
-				$form_data[$field['name']] = $field['value'];
-			}
-		}
 
 		// I use a request because the form submission method does not allow you to send data that is not
 		// contained in one of the actual form fields that the browser sees (i.e. it ignores "hidden" inputs)
@@ -1314,4 +1304,37 @@ class phpbb_functional_test_case extends phpbb_test_case
 
 		return self::request('GET', substr($link, strpos($link, 'mcp.')));
 	}
+
+	/**
+	 * Get hidden fields for URL
+	 *
+	 * @param Symfony\Component\DomCrawler\Crawler|null $crawler Crawler instance or null
+	 * @param string $url Request URL
+	 *
+	 * @return array Hidden form fields array
+	 */
+	protected function get_hidden_fields($crawler, $url)
+	{
+		if (!$crawler)
+		{
+			$crawler = self::$client->request('GET', $url);
+		}
+		$hidden_fields = [
+			$crawler->filter('[type="hidden"]')->each(function ($node, $i) {
+				return ['name' => $node->attr('name'), 'value' => $node->attr('value')];
+			}),
+		];
+
+		$file_form_data = [];
+
+		foreach ($hidden_fields as $fields)
+		{
+			foreach($fields as $field)
+			{
+				$file_form_data[$field['name']] = $field['value'];
+			}
+		}
+
+		return $file_form_data;
+	}
 }