#941 - Security flaw: SS prone to CSRF attack

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@43876 467b73ca-7a2a-4603-9d3b-597d59a354a9

core/control/Controller.php

@@ -155,6 +155,15 @@ class Controller extends ViewableData {
 			if(!isset($funcName)) {
 				user_error("No action button has been clicked in this form executon, and no default has been allowed", E_USER_ERROR);
 			}
+			
+			// Protection against CSRF attacks
+			if($form->securityEnabled()) {
+				$securityID = Session::get('SecurityID');
+
+				if(!$securityID || !isset($this->requestParams['SecurityID']) || $securityID != $this->requestParams['SecurityID']) {
+					trigger_error("Security ID doesn't match, possible CRSF attack.", E_USER_ERROR);
+				}
+			}
 
 
 			// First, try a handler method on the controller

forms/Form.php

@@ -174,7 +174,21 @@ class Form extends ViewableData {
 	 * @return FieldSet The form fields
 	 */
 	function Fields() {
-	  return $this->fields;
+		if($this->securityEnabled()) {
+			if(Session::get('SecurityID')) {
+				$securityID = Session::get('SecurityID');
+			} else {
+				$securityID = rand();
+				Session::set('SecurityID', $securityID);
+			}
+			
+			$fieldsClone = clone $this->fields;
+			$fieldsClone->push(new HiddenField('SecurityID', 'SecurityID', $securityID));
+			
+			return $fieldsClone;
+		} else {
+			return $this->fields;
+		}
 	}
 	
 	/**
@@ -664,6 +678,27 @@ class Form extends ViewableData {
 	function disableDefaultAction() {
 		$this->hasDefaultAction = false;
 	}
+	
+	private $security = true;
+	
+	/**
+	 * Disable the requirement of a SecurityID in the Form. This security protects
+	 * against CSRF attacks, but you should disable this if you don't want to tie 
+	 * a form to a session - eg a search form.
+	 */
+	function disableSecurity() {
+		$this->security = false;
+	}
+	
+	/**
+	 * Returns true if security is enabled - that is if the SecurityID
+	 * should be included and checked on this form.
+	 *
+	 * @return bool
+	 */
+	function securityEnabled() {
+		return $this->security;
+	}
 
 	/**
 	 * Returns the name of a field, if that's the only field that the current controller is interested in.
@@ -689,6 +724,7 @@ class Form extends ViewableData {
 		self::$current_action = $action;
 	}
 	
+	
 	//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 	// TESTING HELPERS
 	//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

javascript/ComplexTableField.js

@@ -109,6 +109,10 @@ ComplexTableField.prototype = {
 				return false;
 			}
 		}
+		
+		if($('SecurityID')) {
+			popupLink = popupLink + '&SecurityID=' + $('SecurityID').value;
+		}
 
 		GB_OpenerObj = this;
 		// use same url to refresh the table after saving the popup, but use a generic rendering method