Merge pull request #636 from basbodart/session-fixation

Regenerate session id only on impossible level

dvwa/includes/dvwaPage.inc.php

@@ -34,12 +34,14 @@
  * This function is called after login and when you change the security level.
  * It gets the security level and sets the httponly and samesite cookie flags
  * appropriately.
- * You can only change the flags by calling session_regenerate_id(), just
- * setting them and doing a session_start() does not change anything.
- * session_regenerate_id() keeps the existing session values, so nothing is lost
- * it just causes a new Set-Cookie header to be sent with the new right flags.
+ *
+ * To force an update of the cookie flags we need to update the session id,
+ * just setting the flags and doing a session_start() does not change anything.
+ * For this, session_id() or session_regenerate_id() can be used.
+ * Both keep the existing session values, so nothing is lost,
+ * it will just cause a new Set-Cookie header to be sent with the new right
+ * flags and the new id (or the same one if we wish to keep it).
 */
-
 function dvwa_start_session() {
 	// This will setup the session cookie based on
 	// the security level.
@@ -63,8 +65,7 @@ function dvwa_start_session() {
 	 * while it is open. So check if one is open, close it if needed
 	 * then update the values and start it again.
 	*/
-
-	if (session_id()) {
+	if (session_status() == PHP_SESSION_ACTIVE) {
 		session_write_close();
 	}
 
@@ -77,10 +78,31 @@ function dvwa_start_session() {
 		'samesite' => $samesite
 	]);
 
-	session_start();
-
-	// This is the call that will force a new Set-Cookie header with the right flags
-	session_regenerate_id();
+	/*
+	 * We need to force a new Set-Cookie header with the updated flags by updating
+	 * the session id, either regenerating it or setting it to a value, because
+	 * session_start() might not generate a Set-Cookie header if a cookie already
+	 * exists.
+	 *
+	 * For impossible security level, we regenerate the session id, PHP will
+	 * generate a new random id. This is good security practice because it
+	 * prevents the reuse of a previous unauthenticated id that an attacker
+	 * might have knowledge of (aka session fixation attack).
+   *
+	 * For lower levels, we want to allow session fixation attacks, so if an id
+	 * already exists, we don't want it to change after authentication. We thus
+	 * set the id to its previous value using session_id(), which will force
+	 * the Set-Cookie header.
+	*/
+	if ($security_level == 'impossible') {
+		session_start();
+		session_regenerate_id(); // force a new id to be generated
+	}
+	else {
+		if (isset($_COOKIE[session_name()])) // if a session id already exists
+			session_id($_COOKIE[session_name()]); // we keep the same id
+		session_start(); // otherwise a new one will be generated here
+	}
 }
 
 if (array_key_exists ("Login", $_POST) && $_POST['Login'] == "Login") {
@@ -198,7 +220,7 @@ function dvwaSecurityLevelSet( $pSecurityLevel ) {
 	$_COOKIE['security'] = $pSecurityLevel;
 }
 
-function dvwaLocaleGet() {	
+function dvwaLocaleGet() {
 	$dvwaSession =& dvwaSessionGrab();
 	return $dvwaSession[ 'locale' ];
 }
@@ -331,15 +353,15 @@ function dvwaHtmlEcho( $pPage ) {
 	$securityLevelHtml = "<em>Security Level:</em> {$securityLevelHtml}";
 	$localeHtml = '<em>Locale:</em> ' . ( dvwaLocaleGet() );
 	$sqliDbHtml = '<em>SQLi DB:</em> ' . ( dvwaSQLiDBGet() );
-	
+
 
 	$messagesHtml = messagesPopAllToHtml();
 	if( $messagesHtml ) {
 		$messagesHtml = "<div class=\"body_padded\">{$messagesHtml}</div>";
 	}
 
 	$systemInfoHtml = "";
-	if( dvwaIsLoggedIn() ) 
+	if( dvwaIsLoggedIn() )
 		$systemInfoHtml = "<div align=\"left\">{$userInfoHtml}<br />{$securityLevelHtml}<br />{$localeHtml}<br />{$sqliDbHtml}</div>";
 	if( $pPage[ 'source_button' ] ) {
 		$systemInfoHtml = dvwaButtonSourceHtmlGet( $pPage[ 'source_button' ] ) . " $systemInfoHtml";