Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-9764

Empty value for CONFIG_TABLE config_name= 'mime_triggers' causes functions_fileupload.php->fileupload->check_content() to be too restrictive

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.7-PL1
    • Fix Version/s: 3.0.9-RC1
    • Component/s: None
    • Labels:
      None
    • Environment:
      PHP 5.3.1, MySQL 5.1.41, Opera 10.60 / Firefox 3.0.5

      Description

      If

      SELECT config_value FROM *_config WHERE config_name= 'mime_triggers';

      gives an empty result or an empty string file uploads will be tested too restrictive for possible attack vectors. In /includes/functions_upload.php function check_content() will then search for a < only (with no additional text appended to it) in the 256 first bytes of the file, since we have an array of one element with an empty string! Of course this results in denying way too many files - < alone may appear very well in these files.

      This happens because in /includes/functions_user.php function avatar_upload($data, &$error) constructs fileupload blindly with the parameter explode('|', $config['mime_triggers']) - which of course gives a valid array having one element (with an empty string).

      To fix this I suggest to extend function set_disallowed_content() in /includes/functions_upload.php, which is now:

      	function set_disallowed_content($disallowed_content)
      	{
      		if ($disallowed_content !== false && is_array($disallowed_content))
      		{
      			$this->disallowed_content = $disallowed_content;
      		}
      	}

      ...to a more precise like:

      	function set_disallowed_content($disallowed_content)
      	{
      		if ($disallowed_content !== false && is_array($disallowed_content))
      		{
      			foreach ($disallowed_content as $k1 => $v1)  // Check each element
      			{
      				if ($v1== '')  // Element value is an empty string?
      				{
      					unset($disallowed_content[$k1]);  // Remove it from array
      				}
      			}
       
      			// Assigning an empty array is not a problem
      			$this->disallowed_content = $disallowed_content;
      		}
      	}

      This way empty array elements would be killed.

        Issue Links

          Activity

          Hide
          ToonArmy Chris Smith added a comment -

          There is a flaw here, you should be able to disable the MIME triggers by changing or removing the config option say for debugging purposes. Failing secure is a complete accident thanks to explodes() behaviour when passing in an empty string.

          I've seen users who've messed this up more than once.

          Show
          ToonArmy Chris Smith added a comment - There is a flaw here, you should be able to disable the MIME triggers by changing or removing the config option say for debugging purposes. Failing secure is a complete accident thanks to explodes() behaviour when passing in an empty string. I've seen users who've messed this up more than once.
          Hide
          ToonArmy Chris Smith added a comment -

          Certainly not a major issue

          Show
          ToonArmy Chris Smith added a comment - Certainly not a major issue
          Hide
          bantu Andreas Fischer added a comment -

          It looks like the implementation doesn't support the checking for PDF and Flash exploits by just editing $config['mime_triggers'] because of the hardcoded '<' sign in:

          			foreach ($disallowed_content as $forbidden)
          			{
          				if (stripos($ie_mime_relevant, '<' . $forbidden) !== false)
          				{
          					return false;
          				}
          			}
          

          Show
          bantu Andreas Fischer added a comment - It looks like the implementation doesn't support the checking for PDF and Flash exploits by just editing $config ['mime_triggers'] because of the hardcoded '<' sign in: foreach ($disallowed_content as $forbidden) { if (stripos($ie_mime_relevant, '<' . $forbidden) !== false) { return false; } }
          Hide
          bantu Andreas Fischer added a comment -

          Ok, so I think I have a patch for this. But unless the majority thinks that it should go into 3.0.8-RC1, I'd rather put it into 3.0.9-RC1 so it can be tested a bit longer.

          Show
          bantu Andreas Fischer added a comment - Ok, so I think I have a patch for this. But unless the majority thinks that it should go into 3.0.8-RC1, I'd rather put it into 3.0.9-RC1 so it can be tested a bit longer.
          Hide
          bantu Andreas Fischer added a comment -

          By the way: I used array_diff(..., array('')) since array_filter() would also filter out "0".

          Show
          bantu Andreas Fischer added a comment - By the way: I used array_diff(..., array('')) since array_filter() would also filter out "0".

            People

            • Assignee:
              bantu Andreas Fischer
              Reporter:
              AmigoJack AmigoJack
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development