From 14c59be85e9ec53cb2f0b3e9d0d8ce7569d913c6 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Mon, 25 Mar 2013 18:16:11 +1300 Subject: [PATCH 1/2] API Form::setStrictFormMethodCheck() and strict argument to setFormMethod() Thanks to @sminnee for getting this started --- docs/en/topics/forms.md | 8 ++++ docs/en/topics/security.md | 17 +++------ forms/Form.php | 50 ++++++++++++++++++++++++- tests/forms/FormTest.php | 77 ++++++++++++++++++++++++++------------ 4 files changed, 114 insertions(+), 38 deletions(-) diff --git a/docs/en/topics/forms.md b/docs/en/topics/forms.md index 5ac3189f3..ce149e850 100644 --- a/docs/en/topics/forms.md +++ b/docs/en/topics/forms.md @@ -339,6 +339,14 @@ or set on a form field instance via anyone of these methods: SilverStripe tries to protect users against *Cross-Site Request Forgery (CSRF)* by adding a hidden *SecurityID* parameter to each form. See [secure-development](/topics/security) for details. +In addition, you should limit forms to the intended HTTP verb (mostly `GET` or `POST`) +to further reduce attack surface, by using `[api:Form->setStrictFormMethodCheck()]`. + + :::php + $myForm->setFormMethod('POST'); + $myForm->setStrictFormMethodCheck(true); + $myForm->setFormMethod('POST', true); // alternative short notation + ### Remove existing fields If you want to remove certain fields from your subclass: diff --git a/docs/en/topics/security.md b/docs/en/topics/security.md index 0f449256d..2d711ba80 100644 --- a/docs/en/topics/security.md +++ b/docs/en/topics/security.md @@ -275,21 +275,14 @@ Some rules of thumb: ## Cross-Site Request Forgery (CSRF) -SilverStripe has built-in countermeasures against this type of identity theft for all form submissions. A form object -will automatically contain a *SecurityID* parameter which is generated as a secure hash on the server, connected to the +SilverStripe has built-in countermeasures against [CSRF](http://shiflett.org/articles/cross-site-request-forgeries) identity theft for all form submissions. A form object +will automatically contain a `SecurityID` parameter which is generated as a secure hash on the server, connected to the currently active session of the user. If this form is submitted without this parameter, or if the parameter doesn't match the hash stored in the users session, the request is discarded. +You can disable this behaviour through `[api:Form->disableSecurityToken()]`. -If you know what you're doing, you can disable this behaviour: - - :::php - $myForm->disableSecurityToken(); - - -See -[http://shiflett.org/articles/cross-site-request-forgeries](http://shiflett.org/articles/cross-site-request-forgeries) - - +It is also recommended to limit form submissions to the intended HTTP verb (mostly `GET` or `POST`) +through `[api:Form->setStrictFormMethodCheck()]`. ## Casting user input diff --git a/forms/Form.php b/forms/Form.php index b658756a7..19fabdddc 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -65,6 +65,11 @@ class Form extends RequestHandler { protected $validator; protected $formMethod = "post"; + + /** + * @var boolean + */ + protected $strictFormMethodCheck = false; protected static $current_action; @@ -239,7 +244,22 @@ class Form extends RequestHandler { * if the form is valid. */ public function httpSubmission($request) { - $vars = $request->requestVars(); + // Strict method check + if($this->strictFormMethodCheck) { + + // Throws an error if the method is bad... + if($this->formMethod != strtolower($request->httpMethod())) { + $response = Controller::curr()->getResponse(); + $response->addHeader('Allow', $this->formMethod); + $this->httpError(405, _t("Form.BAD_METHOD", "This form requires a ".$this->formMethod." submission")); + } + + // ...and only uses the vairables corresponding to that method type + $vars = $this->formMethod == 'get' ? $request->getVars() : $request->postVars(); + } else { + $vars = $request->requestVars(); + } + if(isset($funcName)) { Form::set_current_action($funcName); } @@ -794,11 +814,37 @@ class Form extends RequestHandler { * Set the form method: GET, POST, PUT, DELETE. * * @param $method string + * @param $strict If non-null, pass value to {@link setStrictFormMethodCheck()}. */ - public function setFormMethod($method) { + public function setFormMethod($method, $strict = null) { $this->formMethod = strtolower($method); + if($strict !== null) $this->setStrictFormMethodCheck($strict); return $this; } + + /** + * If set to true, enforce the matching of the form method. + * + * This will mean two things: + * - GET vars will be ignored by a POST form, and vice versa + * - A submission where the HTTP method used doesn't match the form will return a 400 error. + * + * If set to false (the default), then the form method is only used to construct the default + * form. + * + * @param $bool boolean + */ + public function setStrictFormMethodCheck($bool) { + $this->strictFormMethodCheck = (bool)$bool; + return $this; + } + + /** + * @return boolean + */ + public function getStrictFormMethodCheck() { + return $this->strictFormMethodCheck; + } /** * Return the form's action attribute. diff --git a/tests/forms/FormTest.php b/tests/forms/FormTest.php index e1fa43048..428a63a13 100644 --- a/tests/forms/FormTest.php +++ b/tests/forms/FormTest.php @@ -330,6 +330,24 @@ class FormTest extends FunctionalTest { ); $this->assertEquals(200, $response->getStatusCode(), 'Submission suceeds with security token'); } + + public function testStrictFormMethodChecking() { + $response = $this->get('FormTest_ControllerWithStrictPostCheck'); + $response = $this->get( + 'FormTest_ControllerWithStrictPostCheck/Form/?Email=test@test.com&action_doSubmit=1' + ); + $this->assertEquals(405, $response->getStatusCode(), 'Submission fails with wrong method'); + + $response = $this->get('FormTest_ControllerWithStrictPostCheck'); + $response = $this->post( + 'FormTest_ControllerWithStrictPostCheck/Form', + array( + 'Email' => 'test@test.com', + 'action_doSubmit' => 1 + ) + ); + $this->assertEquals(200, $response->getStatusCode(), 'Submission succeeds with correct method'); + } public function testEnableSecurityToken() { SecurityToken::disable(); @@ -468,28 +486,11 @@ class FormTest_Controller extends Controller implements TestOnly { 'SomeRequiredField' ) ); - - // Disable CSRF protection for easier form submission handling - $form->disableSecurityToken(); + $form->disableSecurityToken(); // Disable CSRF protection for easier form submission handling return $form; } - public function FormWithSecurityToken() { - $form = new Form( - $this, - 'FormWithSecurityToken', - new FieldList( - new EmailField('Email') - ), - new FieldList( - new FormAction('doSubmit') - ) - ); - - return $form; - } - public function doSubmit($data, $form, $request) { $form->sessionMessage('Test save was successful', 'good'); return $this->redirectBack(); @@ -533,12 +534,40 @@ class FormTest_ControllerWithSecurityToken extends Controller implements TestOnl return $this->redirectBack(); } - public function getViewer($action = null) { - return new SSViewer('BlankPage'); - } } -Config::inst()->update('Director', 'rules', array( - 'FormTest_Controller' => 'FormTest_Controller' -)); +class FormTest_ControllerWithStrictPostCheck extends Controller implements TestOnly { + protected $template = 'BlankPage'; + + public function Link($action = null) { + return Controller::join_links( + 'FormTest_ControllerWithStrictPostCheck', + $this->request->latestParam('Action'), + $this->request->latestParam('ID'), + $action + ); + } + + public function Form() { + $form = new Form( + $this, + 'Form', + new FieldList( + new EmailField('Email') + ), + new FieldList( + new FormAction('doSubmit') + ) + ); + $form->setFormMethod('POST'); + $form->setStrictFormMethodCheck(true); + $form->disableSecurityToken(); // Disable CSRF protection for easier form submission handling + return $form; + } + + public function doSubmit($data, $form, $request) { + $form->sessionMessage('Test save was successful', 'good'); + return $this->redirectBack(); + } +} \ No newline at end of file From 3e88c98ca513880e2b43ed7f27ade17fef5d9170 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 8 May 2013 10:22:52 +0200 Subject: [PATCH 2/2] API Restrict MemberLoginForm to POST requests for increased security CVE-2013-2653 - Thanks to Fara Rustein of Deloitte Argentina for reporting. --- security/MemberLoginForm.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/security/MemberLoginForm.php b/security/MemberLoginForm.php index 1e5e2bceb..cb584539c 100644 --- a/security/MemberLoginForm.php +++ b/security/MemberLoginForm.php @@ -89,6 +89,9 @@ class MemberLoginForm extends LoginForm { $fields->push(new HiddenField('BackURL', 'BackURL', $backURL)); } + // Reduce attack surface by enforcing POST requests + $this->setFormMethod('POST', true); + parent::__construct($controller, $name, $fields, $actions); // Focus on the email input when the page is loaded