From c3d14e1fa54c7dade7b1b7955575e2991396d7ef Mon Sep 17 00:00:00 2001 From: Andrew Dolgov Date: Mon, 14 Sep 2020 19:46:52 +0300 Subject: [PATCH] - fix multiple vulnerabilities in af_proxy_http - fix vulnerability in rewrite_relative_url() which prevented some URLs from being properly absolutized - fetch_file_contents: validate all URLs before requesting them - validate URLs: explicitly whitelist http and https scheme, forbid everything else - DiskCache/cached_url: only serve whitelisted content types (images, video) - simplify filename/URL handling code, remove and consolidate some less-used functions --- classes/backend.php | 2 +- classes/diskcache.php | 6 +-- classes/feeds.php | 56 ++--------------------- classes/handler/public.php | 2 +- classes/pluginhost.php | 2 +- classes/pref/feeds.php | 2 +- classes/rpc.php | 2 +- include/functions.php | 82 +++++++++++++++++++++++----------- plugins/af_proxy_http/init.php | 13 +++--- 9 files changed, 72 insertions(+), 95 deletions(-) diff --git a/classes/backend.php b/classes/backend.php index 122e28c65..5bd724728 100644 --- a/classes/backend.php +++ b/classes/backend.php @@ -88,7 +88,7 @@ class Backend extends Handler { } function help() { - $topic = clean_filename($_REQUEST["topic"]); // only one for now + $topic = basename(clean($_REQUEST["topic"])); // only one for now if ($topic == "main") { $info = get_hotkeys_info(); diff --git a/classes/diskcache.php b/classes/diskcache.php index 68829b8e3..74415189c 100644 --- a/classes/diskcache.php +++ b/classes/diskcache.php @@ -191,7 +191,7 @@ class DiskCache { ]; public function __construct($dir) { - $this->dir = CACHE_DIR . "/" . clean_filename($dir); + $this->dir = CACHE_DIR . "/" . basename(clean($dir)); } public function getDir() { @@ -227,9 +227,7 @@ class DiskCache { } public function getFullPath($filename) { - $filename = clean_filename($filename); - - return $this->dir . "/" . $filename; + return $this->dir . "/" . basename(clean($filename)); } public function put($filename, $data) { diff --git a/classes/feeds.php b/classes/feeds.php index 55a514cc0..58ba1b6f8 100755 --- a/classes/feeds.php +++ b/classes/feeds.php @@ -1124,9 +1124,9 @@ class Feeds extends Handler_Protected { $pdo = Db::pdo(); - $url = Feeds::fix_url($url); + $url = validate_url($url); - if (!$url || !Feeds::validate_feed_url($url)) return array("code" => 2); + if (!$url) return array("code" => 2); $contents = @fetch_file_contents($url, false, $auth_login, $auth_pass); @@ -1924,7 +1924,7 @@ class Feeds extends Handler_Protected { } static function get_feeds_from_html($url, $content) { - $url = Feeds::fix_url($url); + $url = validate_url($url); $baseUrl = substr($url, 0, strrpos($url, '/') + 1); $feedUrls = []; @@ -1955,56 +1955,6 @@ class Feeds extends Handler_Protected { return preg_match("/ "http://www.example/" - if (strpos($url, '/', strpos($url, ':') + 3) === false) { - $url .= '/'; - } - - //convert IDNA hostname to punycode if possible - if (function_exists("idn_to_ascii")) { - $parts = parse_url($url); - if (mb_detect_encoding($parts['host']) != 'ASCII') - { - $parts['host'] = idn_to_ascii($parts['host']); - $url = build_url($parts); - } - } - - if ($url != "http:///") - return $url; - else - return ''; - } - static function add_feed_category($feed_cat, $parent_cat_id = false, $order_id = 0) { if (!$feed_cat) return false; diff --git a/classes/handler/public.php b/classes/handler/public.php index e6d94e223..135cdcbc7 100755 --- a/classes/handler/public.php +++ b/classes/handler/public.php @@ -1234,7 +1234,7 @@ class Handler_Public extends Handler { public function pluginhandler() { $host = new PluginHost(); - $plugin_name = clean_filename($_REQUEST["plugin"]); + $plugin_name = basename(clean($_REQUEST["plugin"])); $method = clean($_REQUEST["pmethod"]); $host->load($plugin_name, PluginHost::KIND_USER, 0); diff --git a/classes/pluginhost.php b/classes/pluginhost.php index 4fec13000..c6c036783 100755 --- a/classes/pluginhost.php +++ b/classes/pluginhost.php @@ -193,7 +193,7 @@ class PluginHost { foreach ($plugins as $class) { $class = trim($class); - $class_file = strtolower(clean_filename($class)); + $class_file = strtolower(basename(clean($class))); if (!is_dir(__DIR__."/../plugins/$class_file") && !is_dir(__DIR__."/../plugins.local/$class_file")) continue; diff --git a/classes/pref/feeds.php b/classes/pref/feeds.php index 9d29ab478..8cdb1577c 100755 --- a/classes/pref/feeds.php +++ b/classes/pref/feeds.php @@ -1701,7 +1701,7 @@ class Pref_Feeds extends Handler_Protected { foreach ($feeds as $feed) { $feed = trim($feed); - if (Feeds::validate_feed_url($feed)) { + if (validate_url($feed)) { $this->pdo->beginTransaction(); diff --git a/classes/rpc.php b/classes/rpc.php index 208551075..7f809f29b 100755 --- a/classes/rpc.php +++ b/classes/rpc.php @@ -572,7 +572,7 @@ class RPC extends Handler_Protected { function log() { $msg = clean($_REQUEST['msg']); - $file = clean_filename($_REQUEST['file']); + $file = basename(clean($_REQUEST['file'])); $line = (int) clean($_REQUEST['line']); $context = clean($_REQUEST['context']); diff --git a/include/functions.php b/include/functions.php index 16953a0a1..43e9eb8f6 100644 --- a/include/functions.php +++ b/include/functions.php @@ -238,8 +238,9 @@ $url = ltrim($url, ' '); $url = str_replace(' ', '%20', $url); - if (strpos($url, "//") === 0) - $url = 'http:' . $url; + $url = validate_url($url); + + if (!$url) return false; $url_host = parse_url($url, PHP_URL_HOST); $fetch_domain_hits[$url_host] += 1; @@ -623,10 +624,6 @@ } } - function clean_filename($filename) { - return basename(preg_replace("/\.\.|[\/\\\]/", "", clean($filename))); - } - function make_password($length = 12) { $password = ""; $possible = "0123456789abcdfghjkmnpqrstvwxyzABCDFGHJKMNPQRSTVWXYZ*%+^"; @@ -1517,13 +1514,6 @@ return $parts['scheme'] . "://" . $parts['host'] . $parts['path']; } - function cleanup_url_path($path) { - $path = str_replace("/./", "/", $path); - $path = str_replace("//", "/", $path); - - return $path; - } - /** * Converts a (possibly) relative URL to a absolute one. * @@ -1533,33 +1523,35 @@ * @return string Absolute URL */ function rewrite_relative_url($url, $rel_url) { - if (strpos($rel_url, "://") !== false) { + + $rel_parts = parse_url($rel_url); + + if ($rel_parts['host'] && $rel_parts['scheme']) { return $rel_url; } else if (strpos($rel_url, "//") === 0) { # protocol-relative URL (rare but they exist) + return "https:" . $rel_url; + } else if (strpos($rel_url, "magnet:") === 0) { + # allow magnet links return $rel_url; - } else if (preg_match("/^[a-z]+:/i", $rel_url)) { - # magnet:, feed:, etc - return $rel_url; - } else if (strpos($rel_url, "/") === 0) { - $parts = parse_url($url); - $parts['path'] = $rel_url; - $parts['path'] = cleanup_url_path($parts['path']); - - return build_url($parts); - } else { $parts = parse_url($url); + if (!isset($parts['path'])) { $parts['path'] = '/'; } + $dir = $parts['path']; + if (substr($dir, -1) !== '/') { $dir = dirname($parts['path']); $dir !== '/' && $dir .= '/'; } + $parts['path'] = $dir . $rel_url; - $parts['path'] = cleanup_url_path($parts['path']); + + $parts['path'] = str_replace("/./", "/", $parts['path']); + $parts['path'] = str_replace("//", "/", $parts['path']); return build_url($parts); } @@ -1837,6 +1829,15 @@ if ($mimetype == "application/octet-stream") $mimetype = "video/mp4"; + /* only serve video and images */ + if (!preg_match("/(image|video)\//", $mimetype)) { + http_response_code(400); + header("Content-type: text/plain"); + + print "Stored file has disallowed content type ($mimetype)"; + return false; + } + header("Content-type: $mimetype"); $stamp = gmdate("D, d M Y H:i:s", filemtime($filename)) . " GMT"; @@ -1924,3 +1925,34 @@ return $ttrss_version['version']; } + + function validate_url($url) { + + # fix protocol-relative URLs + if (strpos($url, "//") === 0) + $url = "https:" . $url; + + if (filter_var($url, FILTER_VALIDATE_URL) === FALSE) + return false; + + $tokens = parse_url($url); + + if (!$tokens['host']) + return false; + + if (!in_array($tokens['scheme'], ['http', 'https'])) + return false; + + //convert IDNA hostname to punycode if possible + if (function_exists("idn_to_ascii")) { + if (mb_detect_encoding($tokens['host']) != 'ASCII') { + $parts['host'] = idn_to_ascii($tokens['host']); + $url = build_url($tokens); + } + } + + /* if ($tokens['host'] == 'localhost' || $tokens['host'] == '127.0.0.1') + return false; */ + + return $url; + } diff --git a/plugins/af_proxy_http/init.php b/plugins/af_proxy_http/init.php index 80100160d..936942387 100644 --- a/plugins/af_proxy_http/init.php +++ b/plugins/af_proxy_http/init.php @@ -45,8 +45,7 @@ class Af_Proxy_Http extends Plugin { } public function imgproxy() { - - $url = rewrite_relative_url(get_self_url_prefix(), $_REQUEST["url"]); + $url = validate_url(clean($_REQUEST["url"])); // called without user context, let's just redirect to original URL if (!$_SESSION["uid"]) { @@ -59,7 +58,6 @@ class Af_Proxy_Http extends Plugin { if ($this->cache->exists($local_filename)) { header("Location: " . $this->cache->getUrl($local_filename)); return; - //$this->cache->send($local_filename); } else { $data = fetch_file_contents(["url" => $url, "max_size" => MAX_CACHE_FILE_SIZE]); @@ -97,14 +95,13 @@ class Af_Proxy_Http extends Plugin { imagedestroy($img); } else { - header("Content-type: text/html"); + header("Content-type: text/plain"); http_response_code(400); - print "

Proxy request failed.

"; - print "

Fetch error $fetch_last_error ($fetch_last_error_code)

"; - print "

URL: $url

"; - print ""; + print "Proxy request failed.\n". + "Fetch error $fetch_last_error ($fetch_last_error_code)\n". + "Requested URL: $url"; } } }