Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Secure path handling #343

Merged
merged 8 commits into from
Dec 24, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions radicale/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import sys
import pprint
import base64
import posixpath
import socket
import ssl
import wsgiref.simple_server
Expand All @@ -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"
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -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"]

Expand Down
12 changes: 7 additions & 5 deletions radicale/ical.py
Original file line number Diff line number Diff line change
Expand Up @@ -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``.
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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 []
Expand Down
84 changes: 84 additions & 0 deletions radicale/pathutils.py
Original file line number Diff line number Diff line change
@@ -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
41 changes: 25 additions & 16 deletions radicale/storage/filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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
Expand Down
48 changes: 32 additions & 16 deletions radicale/storage/multifilesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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))
Expand All @@ -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))