Development

#1617 ([PATCH] The yml validator file can be overriden by a remote attacker)

You must first sign up to be able to contribute.

Ticket #1617 (closed defect: fixed)

Opened 2 years ago

Last modified 5 months ago

[PATCH] The yml validator file can be overriden by a remote attacker

Reported by: Danila.Mircea Assigned to: noel
Priority: major Milestone: 1.0.16
Component: validation Version: 1.0.11
Keywords: validation attack Cc: gregoire
Qualification: Ready for core team

Description

if you have an action like this:

public function executeRegister()
{
  if ($this->getRequest()->getMethod() != sfRequest::POST)
  {
    $this->setVariablesForView();
    return sfView::SUCCESS;
  }
  else
  {
    //do something with the data
  }
}

public function handleErrorRegister()
{
  $this->setVariablesForView();
  return sfView::SUCCESS;
}
		

and a validator: register.yml

When you make a GET to myModule/register - shows form

When you make a POST to myModule/register - validation works, if it is not validated, returns you in the first page

When you make a GET to myModule/Register - note the caps R, it says it cannot find RegisteSuccess?.php (normal, as it is called registerSuccess.php)

When you make a POST to myModule/Register - the validator is ignored (because it does not exist with caps R), no template must be rendered, so no error regarding RegisterSuccess?.php, so what it does is going directly to "//do something with the data" section with no validation at all.

All this may lead to an attack on all applications using this kind of validation technique.

Correction suggestion: sfExecutionFilter.class.php modification at line 79

checkConfig() second parameter must not be true or false, but must be configurable.

Attachments

patch_for_1617.patch (1.2 kB) - added by timu on 12/21/07 13:58:16.
Backward compability patch, if you'r action name "actionName" you'r validate file name should be actionName.yml or actionname.yml
sfExecutionFilter.class.php.patch (2.0 kB) - added by Carl.Vondrick on 02/25/08 07:31:27.
Alternative patch that eliminates naming conflict

Change History

08/29/07 11:25:00 changed by gregoire

  • owner changed from Fabien to noel.
  • milestone changed from 1.1.0 to 1.0.8.

08/29/07 11:25:24 changed by gregoire

  • cc set to gregoire.

08/30/07 14:03:08 changed by Rihad.Haciyev

Oh no, another month to wait for 1.0.8... I had hoped this bug would be fixed in the upcoming 1.1 by the end of August, as planned previously. Until then, users are forced to e.g. hack $actionName through strtolower:

        $validationConfig = $moduleName.'/'.sfConfig::get('sf_app_module_validate_dir_name').'/'.strtolower($actionName).'.yml';

09/27/07 00:26:15 changed by gregoire

  • status changed from new to closed.
  • version deleted.
  • resolution set to fixed.
  • qualification set to Unreviewed.
  • milestone changed from 1.0.8 to post-1.0.

The problem here is the backward compatibility. As it was permitted until now to have action beginning with an uppercase, this patch will break existing applications. I m setting this ticket for the future 1.x release.

09/27/07 00:26:51 changed by gregoire

  • status changed from closed to reopened.
  • resolution deleted.

12/21/07 13:58:16 changed by timu

  • attachment patch_for_1617.patch added.

Backward compability patch, if you'r action name "actionName" you'r validate file name should be actionName.yml or actionname.yml

02/25/08 07:31:27 changed by Carl.Vondrick

  • attachment sfExecutionFilter.class.php.patch added.

Alternative patch that eliminates naming conflict

02/25/08 07:37:09 changed by Carl.Vondrick

  • version set to 1.0.11.
  • qualification set to Ready for core team.
  • milestone deleted.

I attached an alternative patch that should resolve this in an even more backwards-compatible way. The problem with timu's patch is that it was possible for two actions to share the same validation file: executeRegister would use register.yml and executeREgister would also use register.yml.

The attached patch eliminates this possibility and should be good-to-go for symfony 1.0. It uses the assumption that the case of the first character of the action name is irrelevant to handle both routing cases: /module/register and /module/Register.

To recap:

before:

  • /module/register --> fails validation, skips action, displays errors
  • /module/Register --> ignores validation, executes action, dies on display
  • /module/REgister --> an entirely different action

after:

  • /module/register --> fails validation, skips action, displays errors
  • /module/Register --> jumps to /module/register validation routine: fails validation, skips action, displays error
  • /module/REgister --> an entirely different action

02/25/08 07:42:00 changed by Carl.Vondrick

  • summary changed from The yml validator file can be overriden by a remote attacker to [PATCH] The yml validator file can be overriden by a remote attacker.

05/13/08 10:20:09 changed by fabien

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [8922]) fixed yml validator file can be overriden by a remote attacker (closes #1617 - based on a patch from Carl.Vondrick)

05/13/08 10:21:32 changed by fabien

  • milestone set to 1.0.16.

05/13/08 11:03:02 changed by fabien

(In [8925]) fixed yml validator file can be overriden by a remote attacker (closes #1617 - based on a patch from Carl.Vondrick)