Various security enhancements:

* forbid XML with DOCTYPE
* use a common HMAC method for consistency
* Generate a secret key and store it in local config if it does not exist
This commit is contained in:
bohwaz 2022-10-30 00:52:35 +02:00
parent bcaed89caf
commit 7356c8a574
7 changed files with 63 additions and 9 deletions

View file

@ -501,7 +501,8 @@ abstract class NextCloud
{
$uri = trim($uri, '/');
$expire = intval((time() - strtotime('2022-09-01'))/3600) + 8; // 8 hours
$hash = $expire . ':' . hash_hmac('sha1', $user . "\0" . $expire . "\0" . $uri, $this->getDirectDownloadSecret($uri, $user));
$hash = Server::hmac([$user, $expire, $uri], $this->getDirectDownloadSecret($uri, $user));
$hash = $expire . ':' . $hash;
$uri = rawurlencode($uri);
$uri = str_replace('%2F', '/', $uri);
@ -577,7 +578,7 @@ abstract class NextCloud
throw new Exception('Link has expired', 401);
}
$verify = hash_hmac('sha1', $user . "\0" . $expire . "\0" . $uri, $this->getDirectDownloadSecret($uri, $user));
$verify = Server::hmac([$user, $expire, $uri], $this->getDirectDownloadSecret($uri, $user));
// Check if the provided hash is correct
if (!hash_equals($verify, $hash)) {

View file

@ -631,6 +631,10 @@ class Server
$body = file_get_contents('php://input');
if (false !== strpos($body, '<!DOCTYPE ')) {
throw new Exception('Invalid XML', 400);
}
$this->log('Requested depth: %s', $depth);
// We don't really care about having a correct XML string,
@ -822,6 +826,10 @@ class Server
static public function parsePropPatch(string $body): array
{
if (false !== strpos($body, '<!DOCTYPE ')) {
throw new Exception('Invalid XML', 400);
}
$xml = @simplexml_load_string($body);
if (false === $xml) {
@ -1214,4 +1222,16 @@ class Server
printf('<?xml version="1.0" encoding="utf-8"?><d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns"><s:message>%s</s:message></d:error>', htmlspecialchars($e->getMessage(), ENT_XML1));
}
/**
* Utility function to create HMAC hash of data, useful for NextCloud and WOPI
*/
static public function hmac(array $data, string $key = '')
{
// Protect against length attacks by pre-hashing data
$data = array_map('sha1', $data);
$data = implode(':', $details);
return hash_hmac('sha1', $data, sha1($key));
}
}

View file

@ -190,7 +190,11 @@ class WOPI
}
}
$xml = simplexml_load_string($r);
if (false !== strpos($r, '<!DOCTYPE ')) {
throw new \RuntimeException('Invalid XML returned by discovery');
}
$xml = @simplexml_load_string($r);
if (!is_object($xml)) {
throw new \RuntimeException('Invalid XML returned by discovery');

View file

@ -91,7 +91,7 @@ class NextCloud extends WebDAV_NextCloud
throw new WebDAV_Exception('No user with that name', 401);
}
return hash('sha256', $uri . $user->login . $user->password);
return WebDAV::hmac([$uri, $user->login, $user->password]);
}
protected function cleanChunks(): void

View file

@ -481,8 +481,8 @@ class Storage extends AbstractStorage
$ttl = time()+(3600*10);
// Use the user password as a server secret
$hash = hash_hmac('sha1', $ttl . "\0" . $uri, $user->password);
$data = sprintf('%s_%s_%s', $hash, $ttl, $user->login);
$check = WebDAV::hash(compact('ttl', 'uri'), $user->password);
$data = sprintf('%s_%s_%s', $check, $ttl, $user->login);
return [
WOPI::PROP_TOKEN => WOPI::base64_encode_url_safe($data),
@ -509,7 +509,7 @@ class Storage extends AbstractStorage
$user = $this->users->current();
$check = hash_hmac('sha1', $ttl . "\0" . $uri, $user->password);
$check = WebDAV::hash(compact('ttl', 'uri'), $user->password);
if (!hash_equals($check, $hash)) {
return null;

View file

@ -37,4 +37,13 @@ class WebDAV extends WebDAV_Server
{
http_log('DAV: ' . $message, ...$params);
}
/**
* Utility function to create HMAC hash of data, useful for NextCloud and WOPI
*/
static public function hmac(array $data, string $key = '')
{
$key = SECRET_KEY . sha1($key);
return parent::hmac($data, $key);
}
}

View file

@ -12,11 +12,31 @@ spl_autoload_register(function ($class) {
ErrorManager::enable(ErrorManager::DEVELOPMENT);
ErrorManager::setLogFile(__DIR__ . '/../error.log');
if (!file_exists(__DIR__ . '/../config.local.php')) {
$cfg_file = __DIR__ . '/../config.local.php';
if (!file_exists($cfg_file)) {
die('This server is not configured yet. Please copy config.dist.php to config.local.php and edit it.');
}
require __DIR__ . '/../config.local.php';
require $cfg_file;
// Create random secret key
if (!defined('KaraDAV\SECRET_KEY')) {
$cfg = file_get_contents($cfg_file);
if (false == strpos($cfg, 'SECRET_KEY')) {
$secret = base64_encode(random_bytes(16));
define('KaraDAV\SECRET_KEY', $secret);
$c = sprintf("\n\n// Randomly generated secret key, please change only if necessary\nconst SECRET_KEY = %s;\n\n",
var_export($secret, true));
$cfg = preg_replace('/\?>\s*$|$/', $c, $cfg, 1);
file_put_contents($cfg_file, $cfg);
unset($secret, $cfg_file, $cfg);
}
}
// Init database
if (!file_exists(DB_FILE)) {