Merge pull request #733 from Zankaria/improve-auth

Improve authentication security

composer.json

@@ -33,6 +33,7 @@
             "inc/lock.php",
             "inc/queue.php",
             "inc/functions.php",
+            "inc/functions/net.php",
             "inc/functions/num.php",
             "inc/functions/format.php",
             "inc/driver/http-driver.php",

inc/config.php

@@ -186,7 +186,7 @@
 
 	// How long should the cookies last (in seconds). Defines how long should moderators should remain logged
 	// in (0 = browser session).
-	$config['cookies']['expire'] = 60 * 60 * 24 * 30 * 6; // ~6 months
+	$config['cookies']['expire'] = 60 * 60 * 24 * 7; // 1 week.
 
 	// Make this something long and random for security.
 	$config['cookies']['salt'] = 'abcdefghijklmnopqrstuvwxyz09123456789!@#$%^&*()';
@@ -194,6 +194,10 @@
 	// Whether or not you can access the mod cookie in JavaScript. Most users should not need to change this.
 	$config['cookies']['httponly'] = true;
 
+	// Do not allow logins via unencrypted HTTP. Should only be changed in testing environments or if you connect to a
+	// load-balancer without encryption.
+	$config['cookies']['secure_login_only'] = true;
+
 	// Used to salt secure tripcodes ("##trip") and poster IDs (if enabled).
 	$config['secure_trip_salt'] = ')(*&^%$#@!98765432190zyxwvutsrqponmlkjihgfedcba';
 
@@ -1252,6 +1256,7 @@
 	// Moderator errors
 	$config['error']['toomanyunban']	= _('You are only allowed to unban %s users at a time. You tried to unban %u users.');
 	$config['error']['invalid']		= _('Invalid username and/or password.');
+	$config['error']['insecure']		= _('Login on insecure connections is disabled.');
 	$config['error']['notamod']		= _('You are not a mod…');
 	$config['error']['invalidafter']	= _('Invalid username and/or password. Your user may have been deleted or changed.');
 	$config['error']['malformed']		= _('Invalid/malformed cookies.');

inc/functions/net.php

@@ -0,0 +1,10 @@
+<?php
+namespace Vichan\Functions\Net;
+
+
+/**
+ * @return bool Returns if the client-server connection is an encrypted one (HTTPS).
+ */
+function is_connection_secure(): bool {
+	return !empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off';
+}

inc/mod/auth.php

@@ -4,19 +4,21 @@
  *  Copyright (c) 2010-2013 Tinyboard Development Group
  */
 
+use Vichan\Functions\Net;
+
 defined('TINYBOARD') or exit;
 
 // create a hash/salt pair for validate logins
-function mkhash($username, $password, $salt = false) {
+function mkhash(string $username, string $password, bool $salt = false): array|string {
 	global $config;
-	
+
 	if (!$salt) {
 		// create some sort of salt for the hash
 		$salt = substr(base64_encode(sha1(rand() . time(), true) . $config['cookies']['salt']), 0, 15);
-		
+
 		$generated_salt = true;
 	}
-	
+
 	// generate hash (method is not important as long as it's strong)
 	$hash = substr(
 		base64_encode(
@@ -30,62 +32,59 @@ function mkhash($username, $password, $salt = false) {
 			)
 		), 0, 20
 	);
-	
+
-	if (isset($generated_salt))
+	if (isset($generated_salt)) {
-		return array($hash, $salt);
+		return [ $hash, $salt ];
-	else
+	} else {
 		return $hash;
+	}
 }
 
-function crypt_password_old($password) {
+function crypt_password(string $password): array {
-	$salt = generate_salt();
-	$password = hash('sha256', $salt . sha1($password));
-	return array($salt, $password);
-}
-
-function crypt_password($password) {
 	global $config;
 	// `salt` database field is reused as a version value. We don't want it to be 0.
 	$version = $config['password_crypt_version'] ? $config['password_crypt_version'] : 1;
 	$new_salt = generate_salt();
 	$password = crypt($password, $config['password_crypt'] . $new_salt . "$");
-	return array($version, $password);
+	return [ $version, $password ];
 }
 
-function test_password($password, $salt, $test) {
+function test_password(string $password, string $salt, string $test): array {
-	global $config;
-
 	// Version = 0 denotes an old password hashing schema. In the same column, the
 	// password hash was kept previously
-	$version = (strlen($salt) <= 8) ? (int) $salt : 0;
+	$version = strlen($salt) <= 8 ? (int)$salt : 0;
 
 	if ($version == 0) {
 		$comp = hash('sha256', $salt . sha1($test));
-	}
+	} else {
-	else {
 		$comp = crypt($test, $password);
 	}
-	return array($version, hash_equals($password, $comp));
+	return [ $version, hash_equals($password, $comp) ];
 }
 
-function generate_salt() {
+function generate_salt(): string {
-	// mcrypt_create_iv() was deprecated in PHP 7.1.0, only use it if we're below that version number.
-	if (PHP_VERSION_ID < 70100) {
-		// 128 bits of entropy
-		return strtr(base64_encode(mcrypt_create_iv(16, MCRYPT_DEV_URANDOM)), '+', '.');
-	}
-
-	// Otherwise, use random_bytes()
 	return strtr(base64_encode(random_bytes(16)), '+', '.');
 }
 
-function login($username, $password) {
+function calc_cookie_name(bool $is_https, bool $is_path_jailed, string $base_name): string {
+	if ($is_https) {
+		if ($is_path_jailed) {
+			return "__Host-$base_name";
+		} else {
+			return "__Secure-$base_name";
+		}
+	} else {
+		return $base_name;
+	}
+}
+
+function login(string $username, string $password): array|false {
 	global $mod, $config;
-	
+
 	$query = prepare("SELECT `id`, `type`, `boards`, `password`, `version` FROM ``mods`` WHERE BINARY `username` = :username");
 	$query->bindValue(':username', $username);
 	$query->execute() or error(db_error($query));
-	
+
 	if ($user = $query->fetch(PDO::FETCH_ASSOC)) {
 		list($version, $ok) = test_password($user['password'], $user['version'], $password);
 
@@ -100,40 +99,83 @@ function login($username, $password) {
 				$query->execute() or error(db_error($query));
 			}
 
-			return $mod = array(
+			return $mod = [
 				'id' => $user['id'],
 				'type' => $user['type'],
 				'username' => $username,
 				'hash' => mkhash($username, $user['password']),
 				'boards' => explode(',', $user['boards'])
-			);
+			];
 		}
 	}
-	
+
 	return false;
 }
 
-function setCookies() {
+function setCookies(): void {
 	global $mod, $config;
-	if (!$mod)
+	if (!$mod) {
 		error('setCookies() was called for a non-moderator!');
-	
+	}
-	setcookie($config['cookies']['mod'],
+
-			$mod['username'] . // username
+	$is_https = Net\is_connection_secure();
-			':' . 
+	$is_path_jailed = $config['cookies']['jail'];
-			$mod['hash'][0] . // password
+	$name = calc_cookie_name($is_https, $is_path_jailed, $config['cookies']['mod']);
-			':' .
+
-			$mod['hash'][1], // salt
+	// <username>:<password>:<salt>
-		time() + $config['cookies']['expire'], $config['cookies']['jail'] ? $config['cookies']['path'] : '/', null, !empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off', $config['cookies']['httponly']);
+	$value = "{$mod['username']}:{$mod['hash'][0]}:{$mod['hash'][1]}";
+
+	$options = [
+		'expires' => time() + $config['cookies']['expire'],
+		'path' => $is_path_jailed ? $config['cookies']['path'] : '/',
+		'secure' => $is_https,
+		'httponly' => $config['cookies']['httponly'],
+		'samesite' => 'Strict'
+	];
+
+	setcookie($name, $value, $options);
 }
 
-function destroyCookies() {
+function destroyCookies(): void {
 	global $config;
-	// Delete the cookies
+	$base_name = $config['cookies']['mod'];
-	setcookie($config['cookies']['mod'], 'deleted', time() - $config['cookies']['expire'], $config['cookies']['jail']?$config['cookies']['path'] : '/', null, !empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off', true);
+	$del_time = time() - 60 * 60 * 24 * 365; // 1 year.
+	$jailed_path = $config['cookies']['jail'] ? $config['cookies']['path'] : '/';
+	$http_only = $config['cookies']['httponly'];
+
+	$options_multi = [
+		$base_name => [
+			'expires' => $del_time,
+			'path' => $jailed_path ,
+			'secure' => false,
+			'httponly' => $http_only,
+			'samesite' => 'Strict'
+		],
+		"__Host-$base_name" => [
+			'expires' => $del_time,
+			'path' => $jailed_path,
+			'secure' => true,
+			'httponly' => $http_only,
+			'samesite' => 'Strict'
+		],
+		"__Secure-$base_name" => [
+			'expires' => $del_time,
+			'path' => '/',
+			'secure' => true,
+			'httponly' => $http_only,
+			'samesite' => 'Strict'
+		]
+	];
+
+	foreach ($options_multi as $name => $options) {
+		if (isset($_COOKIE[$name])) {
+			setcookie($name, 'deleted', $options);
+			unset($_COOKIE[$name]);
+		}
+	}
 }
 
-function modLog($action, $_board=null) {
+function modLog(string $action, ?string $_board = null): void {
 	global $mod, $board, $config;
 	$query = prepare("INSERT INTO ``modlogs`` VALUES (:id, :ip, :board, :time, :text)");
 	$query->bindValue(':id', (isset($mod['id']) ? $mod['id'] : -1), PDO::PARAM_INT);
@@ -147,62 +189,72 @@ function modLog($action, $_board=null) {
 	else
 		$query->bindValue(':board', null, PDO::PARAM_NULL);
 	$query->execute() or error(db_error($query));
-	
+
-	if ($config['syslog'])
+	if ($config['syslog']) {
 		_syslog(LOG_INFO, '[mod/' . $mod['username'] . ']: ' . $action);
+	}
 }
 
-function create_pm_header() {
+function create_pm_header(): mixed {
 	global $mod, $config;
-	
+
 	if ($config['cache']['enabled'] && ($header = cache::get('pm_unread_' . $mod['id'])) != false) {
-		if ($header === true)
+		if ($header === true) {
 			return false;
-	
+		}
+
 		return $header;
 	}
-	
+
 	$query = prepare("SELECT `id` FROM ``pms`` WHERE `to` = :id AND `unread` = 1");
 	$query->bindValue(':id', $mod['id'], PDO::PARAM_INT);
 	$query->execute() or error(db_error($query));
-	
+
-	if ($pm = $query->fetch(PDO::FETCH_ASSOC))
+	if ($pm = $query->fetch(PDO::FETCH_ASSOC)) {
-		$header = array('id' => $pm['id'], 'waiting' => $query->rowCount() - 1);
+		$header = [ 'id' => $pm['id'], 'waiting' => $query->rowCount() - 1 ];
-	else
+	} else {
 		$header = true;
-	
+	}
-	if ($config['cache']['enabled'])
+
+	if ($config['cache']['enabled']) {
 		cache::set('pm_unread_' . $mod['id'], $header);
-	
+	}
-	if ($header === true)
+
+	if ($header === true) {
 		return false;
-	
+	}
+
 	return $header;
 }
 
-function make_secure_link_token($uri) {
+function make_secure_link_token(string $uri): string {
 	global $mod, $config;
 	return substr(sha1($config['cookies']['salt'] . '-' . $uri . '-' . $mod['id']), 0, 8);
 }
 
-function check_login($prompt = false) {
+function check_login(bool $prompt = false): void {
 	global $config, $mod;
+
+	$is_https = Net\is_connection_secure();
+	$is_path_jailed = $config['cookies']['jail'];
+	$expected_cookie_name = calc_cookie_name($is_https, $is_path_jailed, $config['cookies']['mod']);
+
 	// Validate session
-	if (isset($_COOKIE[$config['cookies']['mod']])) {
+	if (isset($expected_cookie_name)) {
 		// Should be username:hash:salt
-		$cookie = explode(':', $_COOKIE[$config['cookies']['mod']]);
+		$cookie = explode(':', $_COOKIE[$expected_cookie_name]);
 		if (count($cookie) != 3) {
 			// Malformed cookies
 			destroyCookies();
 			if ($prompt) mod_login();
 			exit;
 		}
-		
+
 		$query = prepare("SELECT `id`, `type`, `boards`, `password` FROM ``mods`` WHERE `username` = :username");
 		$query->bindValue(':username', $cookie[0]);
 		$query->execute() or error(db_error($query));
 		$user = $query->fetch(PDO::FETCH_ASSOC);
-		
+
 		// validate password hash
 		if ($cookie[1] !== mkhash($cookie[0], $user['password'], $cookie[2])) {
 			// Malformed cookies
@@ -210,7 +262,7 @@ function check_login($prompt = false) {
 			if ($prompt) mod_login();
 			exit;
 		}
-		
+
 		$mod = array(
 			'id' => (int)$user['id'],
 			'type' => (int)$user['type'],

inc/mod/pages.php

@@ -5,6 +5,8 @@
  */
 use Vichan\Functions\Format;
 
+use Vichan\Functions\Net;
+
 defined('TINYBOARD') or exit;
 
 
@@ -31,9 +33,11 @@ function mod_page($title, $template, $args, $subtitle = false) {
 function mod_login($redirect = false) {
 	global $config;
 
-	$args = array();
+	$args = [];
 
-	if (isset($_POST['login'])) {
+	if ($config['cookies']['secure_login_only'] && !Net\is_connection_secure()) {
+		$args['error'] = $config['error']['insecure'];
+	} elseif (isset($_POST['login'])) {
 		// Check if inputs are set and not empty
 		if (!isset($_POST['username'], $_POST['password']) || $_POST['username'] == '' || $_POST['password'] == '') {
 			$args['error'] = $config['error']['invalid'];