Merge pull request #343 from Unrud/paths

Secure path handling

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"
@@ -177,12 +176,9 @@ 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
+        return pathutils.sanitize_path(uri)
 
     def collect_allowed_items(self, items, user):
         """Get items from request that user is allowed to access."""
@@ -249,20 +245,24 @@ 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
+            status, headers, _ = NOT_ALLOWED
+            start_response(status, list(headers.items()))
+            return []
+
+        # 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"]
 

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 []

radicale/pathutils.py

@@ -0,0 +1,84 @@
+# -*- 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 <http://www.gnu.org/licenses/>.
+
+"""
+Helper functions for working with paths
+
+"""
+
+import os
+import posixpath
+
+from . import log
+
+
+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
+
+
+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"""
+    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

radicale/storage/filesystem.py

@@ -28,7 +28,8 @@
 import time
 import sys
 from contextlib import contextmanager
-from .. import config, ical
+
+from .. import config, ical, log, pathutils
 
 
 FOLDER = os.path.expanduser(config.get("storage", "filesystem_folder"))
@@ -63,59 +64,67 @@ 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:
+            # 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)
 
     @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

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,32 +53,43 @@ 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:
+            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)
 
     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))
+        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)
 
     @property
     def text(self):
         components = (
             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 +102,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))