Attachment 931720 Details for Bug 1134676 – patch

[patch] patch

s3ql_tip_fix.diff (text/plain), 10.96 KB, created by Murray McAllister on 2014-08-28 05:04:18 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Murray McAllister

Created: 2014-08-28 05:04:18 UTC

Size: 10.96 KB

patch

obsolete

>diff --git a/Changes.txt b/Changes.txt
>--- a/Changes.txt
>+++ b/Changes.txt
>@@ -1,5 +1,15 @@
> UNRELEASED, S3QL tip
> 
>+  * SECURITY UPDATE (CVE-2014-0485).
>+
>+    A remote code execution vulnerability was fixed.
>+
>+    An attacker with control over the communication with the storage
>+    backend or the ability to manipulate the data stored in the
>+    backend was able to trigger execution of arbitrary code by
>+    mount.s3ql, fsck.s3ql, mkfs.s3ql, s3qladm and s3ql_verify. Both
>+    encrypted and unencrypted file systems were vulnerable.
>+
>   * Fixed a crash when using Google OAuth2 and the first request after
>     the access token has expired is a write request.
> 
>diff --git a/src/s3ql/backends/common.py b/src/s3ql/backends/common.py
>--- a/src/s3ql/backends/common.py
>+++ b/src/s3ql/backends/common.py
>@@ -10,11 +10,15 @@
> from abc import abstractmethod, ABCMeta
> from functools import wraps
> import time
>+import codecs
>+import io
> import textwrap
> import inspect
> import ssl
> import os
> import re
>+import pickletools
>+import pickle
> 
> log = logging.getLogger(__name__)
> 
>@@ -559,3 +563,64 @@
>         proxy = None
> 
>     return proxy
>+
>+
>+SAFE_UNPICKLE_OPCODES = {'BININT', 'BININT1', 'BININT2', 'LONG1', 'LONG4',
>+                         'BINSTRING', 'SHORT_BINSTRING', 'GLOBAL',
>+                         'NONE', 'NEWTRUE', 'NEWFALSE', 'BINUNICODE',
>+                         'BINFLOAT', 'EMPTY_LIST', 'APPEND', 'APPENDS',
>+                         'LIST', 'EMPTY_TUPLE', 'TUPLE', 'TUPLE1', 'TUPLE2',
>+                         'TUPLE3', 'EMPTY_DICT', 'DICT', 'SETITEM',
>+                         'SETITEMS', 'POP', 'DUP', 'MARK', 'POP_MARK',
>+                         'BINGET', 'LONG_BINGET', 'BINPUT', 'LONG_BINPUT',
>+                         'PROTO', 'STOP', 'REDUCE'}
>+
>+SAFE_UNPICKLE_GLOBAL_NAMES = { ('__builtin__', 'bytearray'),
>+                               ('__builtin__', 'set'),
>+                               ('__builtin__', 'frozenset'),
>+                               ('_codecs', 'encode') }
>+SAFE_UNPICKLE_GLOBAL_OBJS = { bytearray, set, frozenset, codecs.encode }
>+
>+class SafeUnpickler(pickle.Unpickler):
>+    def find_class(self, module, name):
>+        if (module, name) not in SAFE_UNPICKLE_GLOBAL_NAMES:
>+            raise pickle.UnpicklingError("global '%s.%s' is unsafe" %
>+                                         (module, name))
>+        ret = super().find_class(module, name)
>+        if ret not in SAFE_UNPICKLE_GLOBAL_OBJS:
>+            raise pickle.UnpicklingError("global '%s.%s' is unsafe" %
>+                                         (module, name))
>+        return ret
>+
>+
>+def safe_unpickle_fh(fh, fix_imports=True, encoding="ASCII",
>+                  errors="strict"):
>+    '''Safely unpickle untrusted data from *fh*
>+
>+    *fh* must be seekable.
>+    '''
>+
>+    if not fh.seekable():
>+        raise TypeError('*fh* must be seekable')
>+    pos = fh.tell()
>+
>+    # First make sure that we know all used opcodes
>+    for (opcode, arg, _) in pickletools.genops(fh):
>+        if opcode.proto > 2 or opcode.name not in SAFE_UNPICKLE_OPCODES:
>+            raise pickle.UnpicklingError('opcode %s is unsafe' % opcode.name)
>+
>+    fh.seek(pos)
>+
>+    # Then use a custom Unpickler to ensure that we only give access to
>+    # specific, whitelisted globals. Note that with the above opcodes, there is
>+    # no way to trigger attribute access, so "brachiating" from a white listed
>+    # object to __builtins__ is not possible.
>+    return SafeUnpickler(fh, fix_imports=fix_imports,
>+                         encoding=encoding, errors=errors).load()
>+
>+def safe_unpickle(buf, fix_imports=True, encoding="ASCII",
>+                  errors="strict"):
>+    '''Safely unpickle untrusted data in *buf*'''
>+
>+    return safe_unpickle_fh(io.BytesIO(buf), fix_imports=fix_imports,
>+                            encoding=encoding, errors=errors)
>diff --git a/src/s3ql/backends/comprenc.py b/src/s3ql/backends/comprenc.py
>--- a/src/s3ql/backends/comprenc.py
>+++ b/src/s3ql/backends/comprenc.py
>@@ -8,7 +8,7 @@
> 
> from ..logging import logging # Ensure use of custom logger class
> from .. import BUFSIZE, PICKLE_PROTOCOL
>-from .common import AbstractBackend, ChecksumError
>+from .common import AbstractBackend, ChecksumError, safe_unpickle
> from ..inherit_docstrings import (copy_ancestor_docstring, prepend_ancestor_docstring,
>                                   ABCDocstMeta)
> from Crypto.Cipher import AES
>@@ -112,7 +112,7 @@
> 
>         if not encrypted:
>             try:
>-                return (None, pickle.loads(buf, encoding='latin1'))
>+                return (None, safe_unpickle(buf, encoding='latin1'))
>             except pickle.UnpicklingError:
>                 raise ChecksumError('Invalid metadata')
> 
>@@ -134,8 +134,8 @@
>                                 % (stored_key, key))
> 
>         buf = b64decode(metadata['data'])
>-        return (nonce, pickle.loads(aes_cipher(meta_key).decrypt(buf),
>-                                    encoding='latin1'))
>+        return (nonce, safe_unpickle(aes_cipher(meta_key).decrypt(buf),
>+                                     encoding='latin1'))
> 
>     @prepend_ancestor_docstring
>     def open_read(self, key):
>diff --git a/src/s3ql/backends/local.py b/src/s3ql/backends/local.py
>--- a/src/s3ql/backends/local.py
>+++ b/src/s3ql/backends/local.py
>@@ -9,7 +9,8 @@
> from ..logging import logging # Ensure use of custom logger class
> from .. import BUFSIZE, PICKLE_PROTOCOL
> from ..inherit_docstrings import (copy_ancestor_docstring, ABCDocstMeta)
>-from .common import AbstractBackend, DanglingStorageURLError, NoSuchObject, ChecksumError
>+from .common import (AbstractBackend, DanglingStorageURLError, NoSuchObject,
>+                     ChecksumError, safe_unpickle_fh)
> import _thread
> import io
> import os
>@@ -58,14 +59,11 @@
>         path = self._key_to_path(key)
>         try:
>             with open(path, 'rb') as src:
>-                return pickle.load(src, encoding='latin1')
>+                return safe_unpickle_fh(src, encoding='latin1')
>         except FileNotFoundError:
>             raise NoSuchObject(key)
>         except pickle.UnpicklingError as exc:
>-            if (isinstance(exc.args[0], str)
>-                and exc.args[0].startswith('invalid load key')):
>-                raise ChecksumError('Invalid metadata')
>-            raise
>+            raise ChecksumError('Invalid metadata, pickle says: %s' % exc)
> 
>     @copy_ancestor_docstring
>     def get_size(self, key):
>@@ -80,12 +78,9 @@
>             raise NoSuchObject(key)
> 
>         try:
>-            fh.metadata = pickle.load(fh, encoding='latin1')
>+            fh.metadata = safe_unpickle_fh(fh, encoding='latin1')
>         except pickle.UnpicklingError as exc:
>-            if (isinstance(exc.args[0], str)
>-                and exc.args[0].startswith('invalid load key')):
>-                raise ChecksumError('Invalid metadata')
>-            raise
>+            raise ChecksumError('Invalid metadata, pickle says: %s' % exc)
>         return fh
> 
>     @copy_ancestor_docstring
>@@ -192,9 +187,9 @@
> 
>             if metadata is not None:
>                 try:
>-                    pickle.load(src, encoding='latin1')
>-                except pickle.UnpicklingError:
>-                    raise ChecksumError('Invalid metadata')
>+                    safe_unpickle_fh(src, encoding='latin1')
>+                except pickle.UnpicklingError as exc:
>+                    raise ChecksumError('Invalid metadata, pickle says: %s' % exc)
>                 pickle.dump(metadata, dest, PICKLE_PROTOCOL)
>             shutil.copyfileobj(src, dest, BUFSIZE)
>         except:
>diff --git a/src/s3ql/backends/s3c.py b/src/s3ql/backends/s3c.py
>--- a/src/s3ql/backends/s3c.py
>+++ b/src/s3ql/backends/s3c.py
>@@ -10,7 +10,7 @@
> from .. import PICKLE_PROTOCOL, BUFSIZE
> from .common import (AbstractBackend, NoSuchObject, retry, AuthorizationError,
>                      AuthenticationError, DanglingStorageURLError, retry_generator,
>-                     get_proxy, get_ssl_context)
>+                     get_proxy, get_ssl_context, ChecksumError, safe_unpickle)
> from ..inherit_docstrings import (copy_ancestor_docstring, prepend_ancestor_docstring,
>                                   ABCDocstMeta)
> from io import BytesIO
>@@ -698,7 +698,10 @@
>                 log.warning('MD5 mismatch in metadata for %s', key)
>                 raise BadDigestError('BadDigest',
>                                      'Meta MD5 for %s does not match' % key)
>-            return pickle.loads(b64decode(buf), encoding='latin1')
>+            try:
>+                return safe_unpickle(b64decode(buf), encoding='latin1')
>+            except pickle.UnpicklingError as exc:
>+                raise ChecksumError('Corrupted metadata, pickle says: %s' % exc)
>         elif format_ == 'raw': # No MD5 available
>             return meta
>         else:
>diff --git a/tests/t1_safe_unpickle.py b/tests/t1_safe_unpickle.py
>new file mode 100644
>--- /dev/null
>+++ b/tests/t1_safe_unpickle.py
>@@ -0,0 +1,75 @@
>+'''
>+t1_safe_unpickle.py - this file is part of S3QL (http://s3ql.googlecode.com)
>+
>+Copyright (C) 2014 Nikolaus Rath <Nikolaus@rath.org>
>+
>+This program can be distributed under the terms of the GNU GPLv3.
>+'''
>+
>+if __name__ == '__main__':
>+    import pytest
>+    import sys
>+    sys.exit(pytest.main([__file__] + sys.argv[1:]))
>+
>+from s3ql.backends.common import safe_unpickle
>+from s3ql import PICKLE_PROTOCOL
>+from pytest import raises as assert_raises
>+import pickle
>+import os
>+import pytest
>+import subprocess
>+
>+class CustomClass(object):
>+    pass
>+
>+def test_elementary():
>+    for obj in ('a string', b'bytes', 42, 23.222,
>+                bytearray(b'ascii rules')):
>+        buf = pickle.dumps(obj, PICKLE_PROTOCOL)
>+        safe_unpickle(buf)
>+
>+def test_composite():
>+    elems = ('a string', b'bytes', 42, 23.222)
>+
>+    # Tuple
>+    buf = pickle.dumps(elems, PICKLE_PROTOCOL)
>+    safe_unpickle(buf)
>+
>+    # Dict
>+    buf = pickle.dumps(dict(x for x in enumerate(elems)),
>+                       PICKLE_PROTOCOL)
>+    safe_unpickle(buf)
>+
>+    # List
>+    buf = pickle.dumps(list(elems), PICKLE_PROTOCOL)
>+    safe_unpickle(buf)
>+
>+    # Set
>+    buf = pickle.dumps(set(elems), PICKLE_PROTOCOL)
>+    safe_unpickle(buf)
>+
>+    # Frozenset
>+    buf = pickle.dumps(frozenset(elems), PICKLE_PROTOCOL)
>+    safe_unpickle(buf)
>+
>+def test_python2():
>+    py2_prog = ('import cPickle as pickle\n'
>+                'print(pickle.dumps([42, "bytes", u"unicode", '
>+                'b"more bytes", u"more unicode"], 2))')
>+    try:
>+        buf = subprocess.check_output(['python', '-c', py2_prog])
>+    except subprocess.CalledProcessError:
>+        pytest.skip('failed to execute python2')
>+
>+    assert safe_unpickle(buf, encoding='latin1') == \
>+        [ 42, 'bytes', 'unicode', 'more bytes', 'more unicode']
>+
>+@pytest.mark.parametrize("obj", [CustomClass, CustomClass(), pickle.Pickler,
>+                                  os.remove])
>+def test_bad(obj):
>+    buf = pickle.dumps(obj, PICKLE_PROTOCOL)
>+    assert_raises(pickle.UnpicklingError, safe_unpickle, buf)
>+
>+def test_wrong_proto():
>+    buf = pickle.dumps(b'foo', 3)
>+    assert_raises(pickle.UnpicklingError, safe_unpickle, buf)

Actions: View | Diff

Attachments on bug 1134676: 931720