Strengthen input validation of component names

[pascal-meunier]

The path used with require_once may be invalid or manipulated with dots due to missing or incorrect input validation, or the file may be unreadable.

[zooley]

"... or manipulated with dots due to missing or incorrect input validation ..."

The path is derived from the model's name which is either a set property of the controller or derived by singularizing the controller's name and, thus, no external inputs are accepted or should even involved in the process.

[pascal-meunier]

Hi Shawn, the API infers the component from the URL which results in an observable and demonstrated exception when the URL contains dots at the right place, as it tries to open the component "com_..".

The path is derived from the model's name which is either a set property of the controller or derived by singularizing the controller's name and, thus, no external inputs are accepted or should even involved in the process.

[pascal-meunier]

In case I'm not being clear, this patch fixes an exception triggered by the API when the URL contains '..' where the component name is expected.

[zooley]

That's a problem that should be handled via the component loader. That is, the check shouldn't be needed as the system shouldn't ever allow it to reach that point. The real issue seems to be here:

hubzero-cms/core/libraries/Hubzero/Component/Loader.php

Line 129 in 77136aa

$option = preg_replace('/[^A-Z0-9_\.-]/i', '', $option);

I can't think of any reason why periods would be allowed in component names. Probably not even dashes and should just be [^A-Z0-9_] but that's less an issue. Then check if the resulting option is just 'com_':

	public function canonical($option)
	{
		if (is_array($option))
		{
			$option = implode('', $option);
		}
		$option = preg_replace('/[^A-Z0-9_-]/i', '', $option);
		if (substr($option, 0, strlen('com_')) != 'com_')
		{
			$option = 'com_' . $option;
		}
		if ($option == 'com_')
		{
			$option = '';
		}
		return $option;
	}

There's a secondary issue that these lines:

hubzero-cms/core/libraries/Hubzero/Component/Loader.php

Line 164 in 77136aa

$option = $this->canonical($option);

hubzero-cms/core/libraries/Hubzero/Api/Component/Loader.php

Line 34 in 77136aa

$option = $this->canonical($option);

Need to be moved up to right before the if (empty($option)) checks.

		$option = $this->canonical($option);

		if (empty($option))
		{

That should guard against invalid component names. Then it should fall through to the "is enabled" checks. There is a flag for strict checking (i.e., if it isn't in the database and enabled, throw a 404) but the component loader isn't checking with that flag purely because it was easier to develop without it (drop a component in place with no DB entry and it still loads). That may simply have to be enforced going forward.

if ($this->isEnabled($option)) should probably be if ($this->isEnabled($option, true))

[pascal-meunier]

Thank you Shawn, I made the changes you suggested instead of my initial ones. I'll keep the idea of the strict checking for when we are ready to verify it won't break anything in production hubs.

[zooley]

You may still want to add the following after this if state:

if ($option == 'com_')
{
	$option = '';
}

If I call {host}/api/index.php?option=com_.., the periods will be filtered and the resulting $option is just "com_" which will pass the if (empty($option)) check.

[pascal-meunier]

Would it be wrong to abort inside that function and so reduce complexity and save resources, as in:

if ((strlen($option) == 0) || ($option == 'com_')) $this->app->abort(404, $lang->translate('JLIB_APPLICATION_ERROR_COMPONENT_NOT_FOUND'));
if (substr($option, 0, strlen('com_')) != 'com_')
{
	$option = 'com_' . $option;
}

[zooley]

I wouldn't. That method is used in a variety of places to sanitize $option values and there may be cases where some piece of code is doing checks or whatever and you don't want a 404 getting thrown in the middle of it. A bit of a separation of tasks issue. The canonical method's sole purpose is to sanitize and normalize a given string.

[pascal-meunier]

Thank you Shawn.