From a362ac12b119b49519f5af51b92ebb7d6e127b87 Mon Sep 17 00:00:00 2001
From: Simon Rettberg
Date: Thu, 31 Oct 2013 12:38:25 +0100
Subject: Comments, minor refactoring, possiblity to validate configuration
 parameters

---
 config.php                 |  3 ++
 inc/crypto.inc.php         | 29 ++++++++++++++++
 inc/db.inc.php             | 27 ++++++++++++++-
 inc/message.inc.php        | 85 +++++++++++++++++++++++++++++++++++-----------
 inc/user.inc.php           |  2 +-
 inc/util.inc.php           | 44 ++++++++++++++++++++----
 inc/validator.inc.php      | 43 +++++++++++++++++++++++
 index.php                  | 15 ++++++++
 modules/adduser.inc.php    |  5 +--
 modules/baseconfig.inc.php | 69 ++++++++++++++++++++++++-------------
 10 files changed, 269 insertions(+), 53 deletions(-)
 create mode 100644 inc/crypto.inc.php
 create mode 100644 inc/validator.inc.php

diff --git a/config.php b/config.php
index 262ed508..dbe75519 100644
--- a/config.php
+++ b/config.php
@@ -1,5 +1,8 @@
 <?php
 
+// This might leak sensitive information. Never enable in production!
+define('CONFIG_DEBUG', true);
+
 define('CONFIG_SESSION_DIR', '/tmp/openslx');
 define('CONFIG_SESSION_TIMEOUT', 86400);
 
diff --git a/inc/crypto.inc.php b/inc/crypto.inc.php
new file mode 100644
index 00000000..54cdef8a
--- /dev/null
+++ b/inc/crypto.inc.php
@@ -0,0 +1,29 @@
+<?php
+
+class Crypto
+{
+
+	/**
+	 * Hash given string using crypt's $6$,
+	 * which translates to ~130 bit salt
+	 * and 5000 rounds of hashing with SHA-512.
+	 */
+	public static function hash6($password)
+	{
+		$salt = substr(str_replace('+', '.', base64_encode(pack('N4', mt_rand(), mt_rand(), mt_rand(), mt_rand()))), 0, 22);
+		$hash = crypt($password, '$6$' . $salt);
+		if (strlen($hash) < 60) Util::traceError('Error hashing password using SHA-512');
+		return $hash;
+	}
+
+	/**
+	 * Check if the given password matches the given cryp hash.
+	 * Useful for checking a hashed password.
+	 */
+	public static function verify($password, $hash)
+	{
+		return crypt($password, $hash) === $hash;
+	}
+
+}
+
diff --git a/inc/db.inc.php b/inc/db.inc.php
index 09341a07..a797ae93 100644
--- a/inc/db.inc.php
+++ b/inc/db.inc.php
@@ -1,11 +1,18 @@
 <?php
 
+/**
+ * Handle communication with the database
+ * This is a very thin layer between you and PDO.
+ */
 class Database
 {
 	private static $dbh = false;
 	private static $statements = array();
 
-	public static function init()
+	/**
+	 * Connect to the DB if not already connected.
+	 */
+	private static function init()
 	{
 		if (self::$dbh !== false) return;
 		try {
@@ -15,6 +22,10 @@ class Database
 		}
 	}
 
+	/**
+	 * If you just need the first row of a query you can use this.
+	 * Will return an associative array, or false if no row matches the query
+	 */
 	public static function queryFirst($query, $args = array())
 	{
 		$res = self::simpleQuery($query, $args);
@@ -22,6 +33,10 @@ class Database
 		return $res->fetch(PDO::FETCH_ASSOC);
 	}
 
+	/**
+	 * Execute the given query and return the number of rows affected.
+	 * Mostly useful for UPDATEs or INSERTs
+	 */
 	public static function exec($query, $args = array())
 	{
 		$res = self::simpleQuery($query, $args);
@@ -29,6 +44,12 @@ class Database
 		return $res->rowCount();
 	}
 
+	/**
+	 * Execute the given query and return the corresponding PDOStatement object
+	 * Note that this will re-use PDOStatements, so if you run the same
+	 * query again with different params, do not rely on the first PDOStatement
+	 * still being valid. If you need to do something fancy, use Database::prepare
+	 */
 	public static function simpleQuery($query, $args = array())
 	{
 		self::init();
@@ -44,6 +65,10 @@ class Database
 		return self::$statements[$query];
 	}
 
+	/**
+	 * Simply calls PDO::prepare and returns the PDOStatement.
+	 * You must call PDOStatement::execute manually on it.
+	 */
 	public static function prepare($query)
 	{
 		self:init();
diff --git a/inc/message.inc.php b/inc/message.inc.php
index b90ed630..4da277e7 100644
--- a/inc/message.inc.php
+++ b/inc/message.inc.php
@@ -6,10 +6,12 @@ $error_text = array(
 	'token'             => 'Ungültiges Token. CSRF Angriff?',
 	'adduser-disabled'  => 'Keine ausreichenden Rechte, um weitere Benutzer hinzuzufügen',
 	'password-mismatch' => 'Passwort und Passwortbestätigung stimmen nicht überein',
-	'empty-field'       => 'Ein benötigtes Feld wurde nicht ausgefüllt',
+	'empty-field'       => 'Ein Feld wurde nicht ausgefüllt',
 	'adduser-success'   => 'Benutzer erfolgreich hinzugefügt',
 	'no-permission'     => 'Keine ausreichenden Rechte, um auf diese Seite zuzugreifen',
 	'settings-updated'  => 'Einstellungen wurden aktualisiert',
+	'debug-mode'        => 'Der Debug-Modus ist aktiv!',
+	'value-invalid'     => 'Der Wert {{1}} ist ungültig für die Option {{0}} und wurde ignoriert',
 );
 
 class Message
@@ -17,51 +19,96 @@ class Message
 	private static $list = array();
 	private static $flushed = false;
 
+	/**
+	 * Add error message to page. If messages have not been flushed
+	 * yet, it will be added to the queue, otherwise it will be added
+	 * in place during rendering.
+	 */
 	public static function addError($id)
 	{
-		self::$list[] = array(
-			'type' => 'error',
-			'id'    => $id
-		);
-		if (self::$flushed) self::renderList();
+		self::add('error', $id, func_get_args());
 	}
 
 	public static function addWarning($id)
 	{
-		self::$list[] = array(
-			'type' => 'warning',
-			'id'    => $id
-		);
-		if (self::$flushed) self::renderList();
+		self::add('warning', $id, func_get_args());
 	}
 
 	public static function addInfo($id)
 	{
-		self::$list[] = array(
-			'type' => 'info',
-			'id'    => $id
-		);
-		if (self::$flushed) self::renderList();
+		self::add('info', $id, func_get_args());
 	}
 
 	public static function addSuccess($id)
 	{
+		self::add('success', $id, func_get_args());
+	}
+
+	/**
+	 * Internal function that adds a message. Used by
+	 * addError/Success/Info/... above.
+	 */
+	private static function add($type, $id, $params)
+	{
+		global $error_text;
+		if (!isset($error_text[$id])) Util::traceError('Invalid message id: ' . $id);
 		self::$list[] = array(
-			'type' => 'success',
-			'id'    => $id
+			'type'   => $type,
+			'id'     => $id,
+			'params' => array_slice($params, 1)
 		);
 		if (self::$flushed) self::renderList();
 	}
 
+	/**
+	 * Render all currently queued messages, flushing the queue.
+	 * After calling this, any further calls to add* will be rendered in
+	 * place in the current page output.
+	 */
 	public static function renderList()
 	{
 		global $error_text;
 		foreach (self::$list as $item) {
-			Render::addTemplate('messagebox-' . $item['type'], array('message' => $error_text[$item['id']]));
+			$message = $error_text[$item['id']];
+			foreach ($item['params'] as $index => $text) {
+				$message = str_replace('{{' . $index . '}}', $text, $message);
+			}
+			Render::addTemplate('messagebox-' . $item['type'], array('message' => $message));
 		}
 		self::$list = array();
 		self::$flushed = true;
 	}
 
+	/**
+	 * Deserialize any messages from the current HTTP request and
+	 * place them in the message queue.
+	 */
+	public static function fromRequest()
+	{
+		$messages = is_array($_REQUEST['message']) ? $_REQUEST['message'] : array($_REQUEST['message']);
+		foreach ($messages as $message) {
+			$data = explode('|', $message);
+			if (count($data) < 2 || !preg_match('/^(error|warning|info|success)$/', $data[0])) continue;
+			self::add($data[0], $data[1], array_slice($data, 1));
+		}
+	}
+
+	/**
+	 * Turn the current message queue into a serialized version,
+	 * suitable for appending to a GET or POST request
+	 */
+	public static function toRequest()
+	{
+		$parts = array();
+		foreach (self::$list as $item) {
+			$str = 'message[]=' . urlencode($item['type'] . '|' .$item['id']);
+			if (!empty($item['params'])) {
+				$str .= '|' . implode('|', $item['params']);
+			}
+			$parts[] = $str;
+		}
+		return implode('&', $parts);
+	}
+
 }
 
diff --git a/inc/user.inc.php b/inc/user.inc.php
index 38e57e33..b333c7e4 100644
--- a/inc/user.inc.php
+++ b/inc/user.inc.php
@@ -39,7 +39,7 @@ class User
 	{
 		$ret = Database::queryFirst('SELECT userid, passwd FROM user WHERE login = :user LIMIT 1', array(':user' => $user));
 		if ($ret === false) return false;
-		if (crypt($pass, $ret['passwd']) !== $ret['passwd']) return false;
+		if (!Crypto::verify($pass, $ret['passwd'])) return false;
 		Session::create();
 		Session::set('uid', $ret['userid']);
 		Session::set('token', md5(rand() . time() . rand() . $_SERVER['REMOTE_ADDR'] . rand() . $_SERVER['REMOTE_PORT'] . rand() . $_SERVER['HTTP_USER_AGENT']));
diff --git a/inc/util.inc.php b/inc/util.inc.php
index 0d85b989..f456d164 100644
--- a/inc/util.inc.php
+++ b/inc/util.inc.php
@@ -1,30 +1,53 @@
 <?php
 
-$verboseDebug = true;
-
 class Util
 {
+
+	/**
+	 * Displays an error message and stops script execution.
+	 * If CONFIG_DEBUG is true, it will also dump a stack trace
+	 * and all globally defined variables.
+	 * (As this might reveal sensistive data you should never enable it in production)
+	 */
 	public static function traceError($message)
 	{
-		global $verboseDebug;
 		Header('Content-Type: text/plain; charset=utf-8');
 		echo "--------------------\nFlagrant system error:\n$message\n--------------------\n\n";
-		if (isset($verboseDebug) && $verboseDebug) {
+		if (defined('CONFIG_DEBUG') && CONFIG_DEBUG) {
 			debug_print_backtrace();
 			echo "\n\n";
-			$vars = get_defined_vars();
-			print_r($vars);
+			print_r($GLOBALS);
 		}
 		exit(0);
 	}
 
+	/**
+	 * Redirects the user via a '302 Moved' header.
+	 * An active session will be saved, any messages that haven't
+	 * been displayed yet will be appended to the redirect.
+	 */
 	public static function redirect($location)
 	{
 		Session::save();
+		$messages = Message::toRequest();
+		if (!empty($messages)) {
+			if (strpos($location, '?') === false) {
+				$location .= '?' . $messages;
+			} else {
+				$location .= '&' . $messages;
+			}
+		}
 		Header('Location: ' . $location);
 		exit(0);
 	}
 
+	/**
+	 * Verify the user's token that protects agains CSRF.
+	 * If the user is logged in and there is no token variable set in
+	 * the request, or the submitted token does not match the user's
+	 * token, this function will return false and display an error.
+	 * If the token matches, or the user is not logged in, it will return true.
+	 */
 	public static function verifyToken()
 	{
 		if (Session::get('token') === false) return true;
@@ -33,7 +56,14 @@ class Util
 		return false;
 	}
 
-	function markup($string)
+	/**
+	 * Simple markup "rendering":
+	 * *word* is bold
+	 * /word/ is italics
+	 * _word_ is underlined
+	 * \n is line break
+	 */
+	public static function markup($string)
 	{
 		$string = htmlspecialchars($string);
 		$string = preg_replace('#(^|[\n \-_/\.])\*(.+?)\*($|[ \-_/\.\!\?,])#is', '$1<b>$2</b>$3', $string);
diff --git a/inc/validator.inc.php b/inc/validator.inc.php
new file mode 100644
index 00000000..72b7fa0b
--- /dev/null
+++ b/inc/validator.inc.php
@@ -0,0 +1,43 @@
+<?php
+
+/**
+ * This class contains all the helper functions that
+ * can be referenced by a config setting. Every function
+ * here is supposed to validate the given config value
+ * and wither return the validated and possibly sanitized
+ * value, or false to indicate that the given value is invalid.
+ */
+class Validator
+{
+
+	public static function validate($condition, $value)
+	{
+		if (empty($condition)) return $value;
+		$data = explode(':', $condition, 2);
+		switch ($data[0]) {
+		case 'regex':
+			if (preg_match($data[1], $value)) return $value;
+			return false;
+		case 'function':
+			return self::$data[1]($value);
+		default:
+			Util::traceError('Unknown validation method: ' . $data[0]);
+		}
+	}
+
+	/**
+	 * Validate linux password. If already in $6$ hash form,
+	 * the unchanged value will be returned.
+	 * if empty, an empty string will also be returned.
+	 * Otherwise it it assumed that the value is a plain text
+	 * password that is supposed to be hashed.
+	 */
+	private static function linuxPassword($value)
+	{
+		if (empty($value)) return '';
+		if (preg_match('/^\$6\$.+\$./', $value)) return $value;
+		return Crypto::hash6($value);
+	}
+
+}
+
diff --git a/index.php b/index.php
index aa88db53..d693b9e8 100644
--- a/index.php
+++ b/index.php
@@ -9,6 +9,8 @@ require_once('inc/util.inc.php');
 require_once('inc/message.inc.php');
 require_once('inc/db.inc.php');
 require_once('inc/permission.inc.php');
+require_once('inc/crypto.inc.php');
+require_once('inc/validator.inc.php');
 
 if (empty($_REQUEST['do'])) {
 	// No specific module - set default
@@ -23,15 +25,28 @@ if (!file_exists($module)) {
 	Util::traceError('Invalid module: ' . $module);
 }
 
+// Display any messages
+if (isset($_REQUEST['message'])) {
+	Message::fromRequest();
+}
+
+// Load module - it will execute pre-processing, or act upon request parameters
 require_once($module);
 unset($module);
 
+// Main menu
 $menu = new Menu;
 Render::addTemplate('main-menu', $menu);
 
 Message::renderList();
 
+// Render module. If the module wants to output anything, it will be done here
 render_module();
 
+if (defined('CONFIG_DEBUG') && CONFIG_DEBUG) {
+	Message::addWarning('debug-mode');
+}
+
+// Send page to client.
 Render::output();
 
diff --git a/modules/adduser.inc.php b/modules/adduser.inc.php
index 3e49a78a..04b6044f 100644
--- a/modules/adduser.inc.php
+++ b/modules/adduser.inc.php
@@ -6,13 +6,14 @@ if (isset($_POST['action']) && $_POST['action'] === 'adduser') {
 	// Check required fields
 	if (empty($_POST['user']) || empty($_POST['pass1']) || empty($_POST['pass2']) || empty($_POST['fullname']) || empty($_POST['phone']) || empty($_POST['email'])) {
 		Message::addError('empty-field');
+		Util::redirect('?do=adduser');
 	} elseif ($_POST['pass1'] !== $_POST['pass2']) {
 		Message::addError('password-mismatch');
+		Util::redirect('?do=adduser');
 	} else {
-		$salt = substr(str_replace('+', '.', base64_encode(pack('N4', mt_rand(), mt_rand(), mt_rand(), mt_rand()))), 0, 22);
 		$data = array(
 			'user'       => $_POST['user'],
-			'pass'       => crypt($_POST['pass1'], '$6$' . $salt),
+			'pass'       => Crypto::hash6($_POST['pass1']),
 			'fullname'   => $_POST['fullname'],
 			'phone'      => $_POST['phone'],
 			'email'      => $_POST['email'],
diff --git a/modules/baseconfig.inc.php b/modules/baseconfig.inc.php
index 58c6fa01..f6f4188f 100644
--- a/modules/baseconfig.inc.php
+++ b/modules/baseconfig.inc.php
@@ -3,43 +3,60 @@
 User::load();
 
 // Determine if we're setting global, distro or pool
-if (isset($_REQUEST['distro'])) {
+$qry_extra = array();
+if (isset($_REQUEST['distroid'])) {
 	// TODO: Everything
-	$qry_insert = ', distroid';
-	$qry_values = ', :distroid';
-	$qry_distroid = (int)$_REQUEST['distro'];
-	if (isset($_REQUEST['pool'])) {
-		// TODO: Everything
-		$qry_insert .= ', poolid';
-		$qry_values .= ', :poolid';
-		$qry_poolid .= (int)$_REQUEST['pool'];
+	$qry_extra[] = array(
+		'name'  => 'distroid',
+		'value' => (int)$_REQUEST['distroid'],
+		'table' => 'setting_distro',
+	);
+	if (isset($_REQUEST['poolid'])) {
+		$qry_extra[] = array(
+			'name'  => 'poolid',
+			'value' => (int)$_REQUEST['poolid'],
+			'table' => 'setting_pool',
+		);
 	}
-} else {
-	$qry_insert = '';
-	$qry_values = '';
-	$qry_distroid = '';
-	$qry_poolid = '';
 }
 
 
 if (isset($_POST['setting']) && is_array($_POST['setting'])) {
 	if (User::hasPermission('superadmin')) {
 		if (Util::verifyToken()) {
+			// Build variables for specific sub-settings
+			$qry_insert = '';
+			$qry_values = '';
+			foreach ($qry_extra as $item) {
+				$qry_insert = ', ' . $item['name'];
+				$qry_values = ', :' . $item['name'];
+			}
 			// Load all existing config options to validate input
 			$settings = array();
-			$res = Database::simpleQuery('SELECT setting FROM setting');
+			$res = Database::simpleQuery('SELECT setting, validator FROM setting');
 			while ($row = $res->fetch(PDO::FETCH_ASSOC)) {
-				$settings[$row['setting']] = true; // will contain validation regex at some point
+				$settings[$row['setting']] = $row['validator'];
 			}
-			foreach (array_keys($settings) as $key) {
-				$value = (isset($_POST['setting'][$key]) ? $_POST['setting'][$key] : '');
-				// use validation regex here
-				Database::exec("INSERT INTO setting_global (setting, value $qry_insert) VALUES (:key, :value $qry_values) ON DUPLICATE KEY UPDATE value = :value", array(
-					'key'      => $key,
-					'value'    => $value,
-				));
+			foreach ($settings as $key => $validator) {
+				$input = (isset($_POST['setting'][$key]) ? $_POST['setting'][$key] : '');
+				// Validate data first!
+				$value = Validator::validate($validator, $input);
+				if ($value === false) {
+					Message::addWarning('value-invalid', $key, $input);
+					continue;
+				}
+				// Now put into DB
+				Database::exec("INSERT INTO setting_global (setting, value $qry_insert)
+					VALUES (:key, :value $qry_values)
+					ON DUPLICATE KEY UPDATE value = :value",
+					$qry_extra + array(
+						'key'      => $key,
+						'value'    => $value,
+					)
+				);
 			}
 			Message::addSuccess('settings-updated');
+			Util::redirect('?do=baseconfig');
 		}
 	}
 }
@@ -50,6 +67,12 @@ function render_module()
 		Message::addError('no-permission');
 		return;
 	}
+	// Build left joins for specific settings
+	global $qry_extra;
+	$joins = '';
+	foreach ($qry_extra as $item) {
+		$joins .= " LEFT JOIN ${item['table']} ";
+	}
 	// List global config option
 	$settings = array();
 	$res = Database::simpleQuery('SELECT setting.setting, setting.defaultvalue, setting.permissions, setting.description, tbl.value
-- 
cgit v1.2.3-55-g7522