From ee095a463d997eed30232e4a8e25ca3ef2acea1a Mon Sep 17 00:00:00 2001 From: Unrud Date: Thu, 24 Dec 2015 07:48:14 +0100 Subject: [PATCH 1/8] Improve URI sanitation The old implementation failed to sanitize URIs like ".", "..", "../.." or "//" --- radicale/__init__.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/radicale/__init__.py b/radicale/__init__.py index df3ca9d4b..7ff04d8b0 100644 --- a/radicale/__init__.py +++ b/radicale/__init__.py @@ -177,12 +177,17 @@ def decode(self, text, environ): @staticmethod def sanitize_uri(uri): - """Unquote and remove /../ to prevent access to other data.""" + """Unquote and make absolute to prevent access to other data.""" uri = unquote(uri) trailing_slash = "/" if uri.endswith("/") else "" uri = posixpath.normpath(uri) - trailing_slash = "" if uri == "/" else trailing_slash - return uri + trailing_slash + new_uri = "/" + for part in uri.split("/"): + if not part or part in (".", ".."): + continue + new_uri = posixpath.join(new_uri, part) + trailing_slash = "" if new_uri.endswith("/") else trailing_slash + return new_uri + trailing_slash def collect_allowed_items(self, items, user): """Get items from request that user is allowed to access.""" From 780cecc0f2f91116bc71852888061874fd5b1277 Mon Sep 17 00:00:00 2001 From: Unrud Date: Thu, 24 Dec 2015 08:19:12 +0100 Subject: [PATCH 2/8] Always sanitize request URI Do no rely on the HTTP server --- radicale/__init__.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/radicale/__init__.py b/radicale/__init__.py index 7ff04d8b0..97be760d0 100644 --- a/radicale/__init__.py +++ b/radicale/__init__.py @@ -254,21 +254,23 @@ def __call__(self, environ, start_response): headers = pprint.pformat(self.headers_log(environ)) log.LOGGER.debug("Request headers:\n%s" % headers) + # Strip base_prefix from request URI base_prefix = config.get("server", "base_prefix") if environ["PATH_INFO"].startswith(base_prefix): - # Sanitize request URI - environ["PATH_INFO"] = self.sanitize_uri( - "/%s" % environ["PATH_INFO"][len(base_prefix):]) - log.LOGGER.debug("Sanitized path: %s", environ["PATH_INFO"]) + environ["PATH_INFO"] = environ["PATH_INFO"][len(base_prefix):] elif config.get("server", "can_skip_base_prefix"): log.LOGGER.debug( - "Skipped already sanitized path: %s", environ["PATH_INFO"]) + "Prefix already stripped from path: %s", environ["PATH_INFO"]) else: # Request path not starting with base_prefix, not allowed log.LOGGER.debug( "Path not starting with prefix: %s", environ["PATH_INFO"]) environ["PATH_INFO"] = None + # Sanitize request URI + environ["PATH_INFO"] = self.sanitize_uri(environ["PATH_INFO"]) + log.LOGGER.debug("Sanitized path: %s", environ["PATH_INFO"]) + path = environ["PATH_INFO"] # Get function corresponding to method From ed44830447224d13fb2d2639cf1297e41ec5c511 Mon Sep 17 00:00:00 2001 From: Unrud Date: Thu, 24 Dec 2015 08:24:55 +0100 Subject: [PATCH 3/8] Error message if path not starting with prefix Before the program crashed implicitly --- radicale/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/radicale/__init__.py b/radicale/__init__.py index 97be760d0..12932e81b 100644 --- a/radicale/__init__.py +++ b/radicale/__init__.py @@ -265,7 +265,9 @@ def __call__(self, environ, start_response): # Request path not starting with base_prefix, not allowed log.LOGGER.debug( "Path not starting with prefix: %s", environ["PATH_INFO"]) - environ["PATH_INFO"] = None + status, headers, _ = NOT_ALLOWED + start_response(status, list(headers.items())) + return [] # Sanitize request URI environ["PATH_INFO"] = self.sanitize_uri(environ["PATH_INFO"]) From 1ad994cadf8a339863c1a8f5e405b76a83fd7ba5 Mon Sep 17 00:00:00 2001 From: Unrud Date: Thu, 24 Dec 2015 09:41:10 +0100 Subject: [PATCH 4/8] Move sanitize_path into pathutils.py --- radicale/__init__.py | 13 ++----------- radicale/pathutils.py | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 11 deletions(-) create mode 100644 radicale/pathutils.py diff --git a/radicale/__init__.py b/radicale/__init__.py index 12932e81b..8ded6d1a5 100644 --- a/radicale/__init__.py +++ b/radicale/__init__.py @@ -32,7 +32,6 @@ import sys import pprint import base64 -import posixpath import socket import ssl import wsgiref.simple_server @@ -48,7 +47,7 @@ from urlparse import urlparse # pylint: enable=F0401,E0611 -from . import auth, config, ical, log, rights, storage, xmlutils +from . import auth, config, ical, log, pathutils, rights, storage, xmlutils VERSION = "1.0.1" @@ -179,15 +178,7 @@ def decode(self, text, environ): def sanitize_uri(uri): """Unquote and make absolute to prevent access to other data.""" uri = unquote(uri) - trailing_slash = "/" if uri.endswith("/") else "" - uri = posixpath.normpath(uri) - new_uri = "/" - for part in uri.split("/"): - if not part or part in (".", ".."): - continue - new_uri = posixpath.join(new_uri, part) - trailing_slash = "" if new_uri.endswith("/") else trailing_slash - return new_uri + trailing_slash + return pathutils.sanitize_path(uri) def collect_allowed_items(self, items, user): """Get items from request that user is allowed to access.""" diff --git a/radicale/pathutils.py b/radicale/pathutils.py new file mode 100644 index 000000000..5a0e0c8a8 --- /dev/null +++ b/radicale/pathutils.py @@ -0,0 +1,37 @@ +# -*- coding: utf-8 -*- +# +# This file is part of Radicale Server - Calendar Server +# +# This library is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Radicale. If not, see . + +""" +Helper functions for working with paths + +""" + +import posixpath + + +def sanitize_path(path): + """Make absolute (with leading slash) to prevent access to other data. + Preserves an potential trailing slash.""" + trailing_slash = "/" if path.endswith("/") else "" + path = posixpath.normpath(path) + new_path = "/" + for part in path.split("/"): + if not part or part in (".", ".."): + continue + new_path = posixpath.join(new_path, part) + trailing_slash = "" if new_path.endswith("/") else trailing_slash + return new_path + trailing_slash \ No newline at end of file From 6b7e79a368be86d266c51cf50915324490a1b728 Mon Sep 17 00:00:00 2001 From: Unrud Date: Thu, 24 Dec 2015 10:07:04 +0100 Subject: [PATCH 5/8] Use sanitize_path instead of normpath See a7b47f075499a1e1b40539bc1fa872a3ab77a204 The check for "." is now needless because the sane path is always absolute. ```path.replace(os.sep, "/")``` is only relevant for the (multi)filesystem backend and should be there. --- radicale/ical.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/radicale/ical.py b/radicale/ical.py index 46fb4196d..e6cb1eeef 100644 --- a/radicale/ical.py +++ b/radicale/ical.py @@ -26,13 +26,14 @@ """ import os -import posixpath import hashlib import re from uuid import uuid4 from random import randint from contextlib import contextmanager +from . import pathutils + def serialize(tag, headers=(), items=()): """Return a text corresponding to given collection ``tag``. @@ -183,8 +184,9 @@ def __init__(self, path, principal=False): """ self.encoding = "utf-8" - split_path = path.split("/") - self.path = path if path != "." else "" + # path should already be sanitized + self.path = pathutils.sanitize_path(path).strip("/") + split_path = self.path.split("/") if principal and split_path and self.is_node(self.path): # Already existing principal collection self.owner = split_path[0] @@ -215,8 +217,8 @@ def from_path(cls, path, depth="1", include_container=True): if path is None: return [] - # First do normpath and then strip, to prevent access to FOLDER/../ - sane_path = posixpath.normpath(path.replace(os.sep, "/")).strip("/") + # path should already be sanitized + sane_path = pathutils.sanitize_path(path).strip("/") attributes = sane_path.split("/") if not attributes: return [] From b4b3d51f33c7623d312f289252dd7bbb8f58bbe6 Mon Sep 17 00:00:00 2001 From: Unrud Date: Thu, 24 Dec 2015 12:37:11 +0100 Subject: [PATCH 6/8] Convert paths safely to file system paths With the old implementation on Windows a path like "/c:/file/ignore" got converted to "c:\file" and allowed access to files outside of FOLDER --- radicale/pathutils.py | 37 +++++++++++++++++++++++++++- radicale/storage/filesystem.py | 35 ++++++++++++++------------ radicale/storage/multifilesystem.py | 38 +++++++++++++++++------------ 3 files changed, 77 insertions(+), 33 deletions(-) diff --git a/radicale/pathutils.py b/radicale/pathutils.py index 5a0e0c8a8..c3e623b95 100644 --- a/radicale/pathutils.py +++ b/radicale/pathutils.py @@ -20,8 +20,11 @@ """ +import os import posixpath +from . import log + def sanitize_path(path): """Make absolute (with leading slash) to prevent access to other data. @@ -34,4 +37,36 @@ def sanitize_path(path): continue new_path = posixpath.join(new_path, part) trailing_slash = "" if new_path.endswith("/") else trailing_slash - return new_path + trailing_slash \ No newline at end of file + return new_path + trailing_slash + + +def is_safe_filesystem_path_component(path): + """Checks if path is a single component of a local filesystem path + and is safe to join""" + if not path: + return False + drive, _ = os.path.splitdrive(path) + if drive: + return False + head, _ = os.path.split(path) + if head: + return False + if path in (os.curdir, os.pardir): + return False + return True + + +def path_to_filesystem(path, base_folder): + """Converts path to a local filesystem path relative to base_folder + in a secure manner or raises ValueError.""" + sane_path = sanitize_path(path).strip("/") + safe_path = base_folder + if not sane_path: + return safe_path + for part in sane_path.split("/"): + if not is_safe_filesystem_path_component(part): + log.LOGGER.debug("Can't translate path safely to filesystem: %s", + path) + raise ValueError("Unsafe path") + safe_path = os.path.join(safe_path, part) + return safe_path \ No newline at end of file diff --git a/radicale/storage/filesystem.py b/radicale/storage/filesystem.py index a45e8eba1..b3e06d213 100644 --- a/radicale/storage/filesystem.py +++ b/radicale/storage/filesystem.py @@ -28,7 +28,8 @@ import time import sys from contextlib import contextmanager -from .. import config, ical + +from .. import config, ical, pathutils FOLDER = os.path.expanduser(config.get("storage", "filesystem_folder")) @@ -63,41 +64,41 @@ def open(path, mode="r"): class Collection(ical.Collection): """Collection stored in a flat ical file.""" @property - def _path(self): + def _filesystem_path(self): """Absolute path of the file at local ``path``.""" - return os.path.join(FOLDER, self.path.replace("/", os.sep)) + return pathutils.path_to_filesystem(self.path, FOLDER) @property def _props_path(self): """Absolute path of the file storing the collection properties.""" - return self._path + ".props" + return self._filesystem_path + ".props" def _create_dirs(self): """Create folder storing the collection if absent.""" - if not os.path.exists(os.path.dirname(self._path)): - os.makedirs(os.path.dirname(self._path)) + if not os.path.exists(os.path.dirname(self._filesystem_path)): + os.makedirs(os.path.dirname(self._filesystem_path)) def save(self, text): self._create_dirs() - with open(self._path, "w") as fd: + with open(self._filesystem_path, "w") as fd: fd.write(text) def delete(self): - os.remove(self._path) + os.remove(self._filesystem_path) os.remove(self._props_path) @property def text(self): try: - with open(self._path) as fd: + with open(self._filesystem_path) as fd: return fd.read() except IOError: return "" @classmethod def children(cls, path): - abs_path = os.path.join(FOLDER, path.replace("/", os.sep)) - _, directories, files = next(os.walk(abs_path)) + filesystem_path = pathutils.path_to_filesystem(path, FOLDER) + _, directories, files = next(os.walk(filesystem_path)) for filename in directories + files: rel_filename = posixpath.join(path, filename) if cls.is_node(rel_filename) or cls.is_leaf(rel_filename): @@ -105,17 +106,19 @@ def children(cls, path): @classmethod def is_node(cls, path): - abs_path = os.path.join(FOLDER, path.replace("/", os.sep)) - return os.path.isdir(abs_path) + filesystem_path = pathutils.path_to_filesystem(path, FOLDER) + return os.path.isdir(filesystem_path) @classmethod def is_leaf(cls, path): - abs_path = os.path.join(FOLDER, path.replace("/", os.sep)) - return os.path.isfile(abs_path) and not abs_path.endswith(".props") + filesystem_path = pathutils.path_to_filesystem(path, FOLDER) + return (os.path.isfile(filesystem_path) and not + filesystem_path.endswith(".props")) @property def last_modified(self): - modification_time = time.gmtime(os.path.getmtime(self._path)) + modification_time = \ + time.gmtime(os.path.getmtime(self._filesystem_path)) return time.strftime("%a, %d %b %Y %H:%M:%S +0000", modification_time) @property diff --git a/radicale/storage/multifilesystem.py b/radicale/storage/multifilesystem.py index 30f8c2dc2..fe5637d4c 100644 --- a/radicale/storage/multifilesystem.py +++ b/radicale/storage/multifilesystem.py @@ -30,13 +30,14 @@ from . import filesystem from .. import ical from .. import log +from .. import pathutils class Collection(filesystem.Collection): """Collection stored in several files per calendar.""" def _create_dirs(self): - if not os.path.exists(self._path): - os.makedirs(self._path) + if not os.path.exists(self._filesystem_path): + os.makedirs(self._filesystem_path) @property def headers(self): @@ -52,17 +53,18 @@ def write(self): name = ( component.name if sys.version_info[0] >= 3 else component.name.encode(filesystem.FILESYSTEM_ENCODING)) - path = os.path.join(self._path, name) - with filesystem.open(path, "w") as fd: + filesystem_path = os.path.join(self._filesystem_path, name) + with filesystem.open(filesystem_path, "w") as fd: fd.write(text) def delete(self): - shutil.rmtree(self._path) + shutil.rmtree(self._filesystem_path) os.remove(self._props_path) def remove(self, name): - if os.path.exists(os.path.join(self._path, name)): - os.remove(os.path.join(self._path, name)) + filesystem_path = os.path.join(self._filesystem_path, name) + if os.path.exists(filesystem_path): + os.remove(filesystem_path) @property def text(self): @@ -70,14 +72,14 @@ def text(self): ical.Timezone, ical.Event, ical.Todo, ical.Journal, ical.Card) items = set() try: - filenames = os.listdir(self._path) + filenames = os.listdir(self._filesystem_path) except (OSError, IOError) as e: log.LOGGER.info('Error while reading collection %r: %r' - % (self._path, e)) + % (self._filesystem_path, e)) return "" for filename in filenames: - path = os.path.join(self._path, filename) + path = os.path.join(self._filesystem_path, filename) try: with filesystem.open(path) as fd: items.update(self._parse(fd.read(), components)) @@ -90,17 +92,21 @@ def text(self): @classmethod def is_node(cls, path): - path = os.path.join(filesystem.FOLDER, path.replace("/", os.sep)) - return os.path.isdir(path) and not os.path.exists(path + ".props") + filesystem_path = pathutils.path_to_filesystem(path, + filesystem.FOLDER) + return (os.path.isdir(filesystem_path) and + not os.path.exists(filesystem_path + ".props")) @classmethod def is_leaf(cls, path): - path = os.path.join(filesystem.FOLDER, path.replace("/", os.sep)) - return os.path.isdir(path) and os.path.exists(path + ".props") + filesystem_path = pathutils.path_to_filesystem(path, + filesystem.FOLDER) + return (os.path.isdir(filesystem_path) and + os.path.exists(path + ".props")) @property def last_modified(self): last = max([ - os.path.getmtime(os.path.join(self._path, filename)) - for filename in os.listdir(self._path)] or [0]) + os.path.getmtime(os.path.join(self._filesystem_path, filename)) + for filename in os.listdir(self._filesystem_path)] or [0]) return time.strftime("%a, %d %b %Y %H:%M:%S +0000", time.gmtime(last)) From bcaf452e516c02c9bed584a73736431c5e8831f1 Mon Sep 17 00:00:00 2001 From: Unrud Date: Thu, 24 Dec 2015 13:32:30 +0100 Subject: [PATCH 7/8] Convert component names safely to filenames Component names are controlled by the user and without this checks access to arbitrary files is possible if the multifilesystem backend is used. --- radicale/storage/multifilesystem.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/radicale/storage/multifilesystem.py b/radicale/storage/multifilesystem.py index fe5637d4c..93cec8743 100644 --- a/radicale/storage/multifilesystem.py +++ b/radicale/storage/multifilesystem.py @@ -53,6 +53,11 @@ def write(self): name = ( component.name if sys.version_info[0] >= 3 else component.name.encode(filesystem.FILESYSTEM_ENCODING)) + if not pathutils.is_safe_filesystem_path_component(name): + log.LOGGER.debug( + "Can't tranlate name safely to filesystem, " + "skipping component: %s", name) + continue filesystem_path = os.path.join(self._filesystem_path, name) with filesystem.open(filesystem_path, "w") as fd: fd.write(text) @@ -62,6 +67,11 @@ def delete(self): os.remove(self._props_path) def remove(self, name): + if not pathutils.is_safe_filesystem_path_component(name): + log.LOGGER.debug( + "Can't tranlate name safely to filesystem, " + "skipping component: %s", name) + return filesystem_path = os.path.join(self._filesystem_path, name) if os.path.exists(filesystem_path): os.remove(filesystem_path) From eed37792aef835a18cf1413814adf37a3394b4d1 Mon Sep 17 00:00:00 2001 From: Unrud Date: Thu, 24 Dec 2015 14:25:34 +0100 Subject: [PATCH 8/8] Convert filesystem paths safely to paths This only becomes a problem if the OS/filesystem allows / in filenames or . respectively .. as filenames. --- radicale/pathutils.py | 12 ++++++++++++ radicale/storage/filesystem.py | 8 +++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/radicale/pathutils.py b/radicale/pathutils.py index c3e623b95..2aa13aff2 100644 --- a/radicale/pathutils.py +++ b/radicale/pathutils.py @@ -40,6 +40,18 @@ def sanitize_path(path): return new_path + trailing_slash +def is_safe_path_component(path): + """Checks if path is a single component of a path and is safe to join""" + if not path: + return False + head, _ = posixpath.split(path) + if head: + return False + if path in (".", ".."): + return False + return True + + def is_safe_filesystem_path_component(path): """Checks if path is a single component of a local filesystem path and is safe to join""" diff --git a/radicale/storage/filesystem.py b/radicale/storage/filesystem.py index b3e06d213..7b5e1b686 100644 --- a/radicale/storage/filesystem.py +++ b/radicale/storage/filesystem.py @@ -29,7 +29,7 @@ import sys from contextlib import contextmanager -from .. import config, ical, pathutils +from .. import config, ical, log, pathutils FOLDER = os.path.expanduser(config.get("storage", "filesystem_folder")) @@ -100,6 +100,12 @@ def children(cls, path): filesystem_path = pathutils.path_to_filesystem(path, FOLDER) _, directories, files = next(os.walk(filesystem_path)) for filename in directories + files: + # make sure that the local filename can be translated + # into an internal path + if not pathutils.is_safe_path_component(filename): + log.LOGGER.debug("Skipping unsupported filename: %s", + filename) + continue rel_filename = posixpath.join(path, filename) if cls.is_node(rel_filename) or cls.is_leaf(rel_filename): yield cls(rel_filename)