Perform signature verification on pickled data transferred over sockets #251

soupytwist · 2018-08-06T19:52:13Z

Before unpickling anything, ensure that it has a valid digital
signature using a randomly-generated shared key. In order for an attacker
to send or tamper with data on the same socket, they must know this key
to compute a valid signature.

Fixes #105 by requiring some authentication before unpickling.

Before unpickling anything, ensure that it has a valid digital signature using a randomly-generated shared key. In order for an attacker to send or tamper with data on the same socket, they must know this key to compute a valid signature.

ssbr · 2018-08-06T22:05:48Z

rope/base/oi/doa.py

@@ -114,24 +118,55 @@ class _SocketReceiver(_MessageReceiver):
    def __init__(self):
        self.server_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        self.data_port = 3037
+        self.key = uuid.uuid4().bytes


could just use os.urandom(16) and it'd be more obviously correct. (It's not obvious without reading its source that uuid4 uses a CSPRNG, although it "should", and does.)

In fact, I think usually people prefer to use the digest size as the key size, so 16 is half as big as it should be. I'd replace with os.urandom(32). (Without making any statement about whether 128 bits of security is too few or whatever). Trying to remember where I heard this but can't find it.

Much nicer, thank you.

ssbr · 2018-08-06T22:06:11Z

rope/base/oi/doa.py

        while self.data_port < 4000:
            try:
-                self.server_socket.bind(('', self.data_port))
+                self.server_socket.bind(('127.0.0.1', self.data_port))


can you use localhost or similar for better ipv6 something?

localhost might work.

ssbr · 2018-08-06T22:06:38Z

rope/base/oi/doa.py

+                    continue
+
+                digest = hmac.new(self.key, buf_data, hashlib.sha256).digest()
+                if buf_digest != digest:


use hmac.compare_digest to avoid timing attacks. Otherwise people can discover the correct hmac for their malicious input by continually sending requests and seeing how long it takes to be rejected.

I see, but this is only available for python 2.7+ (rope supports as low as 2.6). I could use the stronger protection if available, but I don't think there's much possibility of this attack because 1) the attacker doesn't get feedback of whether their attempt is successful and 2) the process is very short-lived so any brute-force style attack like this would take too long.

the attacker doesn't get feedback of whether their attempt is successful

I think this is untrue. The longer it takes for the connection to be closed, the more successful they have been.

the process is very short-lived so any brute-force style attack like this would take too long.

This may be so, but it's a precondition that might change later.

In general I don't like security-critical code that is "delicate" like this. If we ever had a long-lived process, would anyone think to fix this code to restore its security properties? I doubt it. It's better to be straightforward. This is probably how I'd write it:

def _compat_compare_digest(a, b): if len(a) != len(b): return False difference = 0 for (a_char, b_char) in zip(a, b): difference |= ord(a_char) ^ ord(b_char) return difference == 0 try: from hmac import compare_digest except ImportError: compare_digest = _compat_compare_digest

Test case:

def test_eq_empty(self): self.assertTrue(doa._compat_compare_digest('', '')) def test_eq_nonempty(self): self.assertTrue(doa._compat_compare_digest('abc', 'abc')) def test_neq_samelength(self): self.assertFalse(doa._compat_compare_digest('abc', 'abd')) def test_neq_difflength(self): self.assertFalse(doa._compat_compare_digest('abc', 'abcd'))

It's much easier to drop support for 2.6. Who uses 2.6, anyway?

I don't know who uses 2.6, or even that it's entirely supported (but the tests do run and pass).

ssbr · 2018-08-06T22:08:28Z

ropetest/doatest.py

+import multiprocessing
+try:
+    import pickle
+except ImportError:


other way around. pickle will always succeed, it's cPickle that might not depending on Python version / implementation.

So it should be:

try: import cPickle as pickle except ImportError: import pickle

Huh, I copied this from elsewhere without noticing. Yeah, that's not right!

ssbr · 2018-08-06T23:01:25Z

ropetest/doatest.py

+        self.assertEqual(0, len(received_objs))
+
+    def test_CVE_2014_3539_sanity(self):
+        # Tests that sending valid, encrypted data on the socket does work.


signed, not encrypted

Artifact from a previous idea, ty :)

- Bind to localhost instead of 127.0.0.1 - Update key generation method - Import cPickle if available

ssbr · 2018-08-07T02:27:17Z

rope/base/oi/doa.py

+                    continue
+
+                digest = hmac.new(self.key, buf_data, hashlib.sha256).digest()
+                if buf_digest != digest:


the attacker doesn't get feedback of whether their attempt is successful

I think this is untrue. The longer it takes for the connection to be closed, the more successful they have been.

the process is very short-lived so any brute-force style attack like this would take too long.

This may be so, but it's a precondition that might change later.

In general I don't like security-critical code that is "delicate" like this. If we ever had a long-lived process, would anyone think to fix this code to restore its security properties? I doubt it. It's better to be straightforward. This is probably how I'd write it:

def _compat_compare_digest(a, b): if len(a) != len(b): return False difference = 0 for (a_char, b_char) in zip(a, b): difference |= ord(a_char) ^ ord(b_char) return difference == 0 try: from hmac import compare_digest except ImportError: compare_digest = _compat_compare_digest

Test case:

def test_eq_empty(self): self.assertTrue(doa._compat_compare_digest('', '')) def test_eq_nonempty(self): self.assertTrue(doa._compat_compare_digest('abc', 'abc')) def test_neq_samelength(self): self.assertFalse(doa._compat_compare_digest('abc', 'abd')) def test_neq_difflength(self): self.assertFalse(doa._compat_compare_digest('abc', 'abcd'))

It's much easier to drop support for 2.6. Who uses 2.6, anyway?

ssbr · 2018-08-07T02:29:08Z

rope/base/oi/doa.py

@@ -118,11 +117,11 @@ class _SocketReceiver(_MessageReceiver):
    def __init__(self):
        self.server_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        self.data_port = 3037
-        self.key = uuid.uuid4().bytes
+        self.key = bytes(os.urandom(32))


os.urandom always returns bytes in all python versions

>>> import os >>> type(os.urandom(32)) <type 'str'>

ah, I forgot there's no distinction in python2. Done.

- remove redundant call to bytes - use hmac.compare_digest

mcepl

Please consider indicated changes. Otherwise, I really like it and it looks pretty straightforward, so it should work.

mcepl · 2018-08-07T07:51:08Z

rope/base/oi/doa.py

@@ -11,6 +14,20 @@
 import threading


+def _compat_compare_digest(a, b):


I would say that this function would deserve at least some comment explaining its function. Normal humans among us (and most Python programmers are normal humans) are not used to bitwise operations. How is this effectively different from a == b?

Also, do you think support for Python < 2.7.7 is that crucial? (we have hmac.compare_digests since that release) Python 2.6 is in my opinion only part of RHEL-6 which is effectively frozen and on the life support these days. Where else you can meet Python 2.6?

I don't know that it is crucial, but deprecating an old version to support this function doesn't seem worthwhile imo (it's pretty simple). I will document it, though, good point.

FWIW the bit math here is actually pretty standard (I wrote it from memory). It's still not necessarily constant time but what can you do?

mcepl · 2018-08-07T11:38:46Z

rope/base/oi/doa.py

                break
            except socket.error:
                self.data_port += 1
        self.server_socket.listen(1)

    def get_send_info(self):
-        return str(self.data_port)
+        return '%d:%s' % (self.data_port,


If you were using random.SystemRandom(), you could use its methods, for example:

rd = random.SystemRandom() rand_str = {:X}'.format(rd.getrandbits(25))

with slightly more predictable and manageable results.

I don't think you meant this line, but I'm not sure how it's different than urandom that I am using now. That seems just as simple/manageable to me for this case.

I'm not sure I understand the suggestion here. random.SystemRandom is just a wrapper around urandom that lets you get other things like random integers, floats, etc. -- but if you just want a random string, urandom is easier.

- Document _compat_compare_digest

Implemented the documentation change, not sure what github wants me to do to close the review!

soupytwist force-pushed the cve-2014-3539 branch from ad135f8 to b2e0de7 Compare August 6, 2018 19:56

soupytwist force-pushed the cve-2014-3539 branch from b2e0de7 to ea501c6 Compare August 6, 2018 21:44

soupytwist mentioned this pull request Aug 6, 2018

CVE-2014-3539 python-rope: pickle.load of remotely supplied data with no authentication #105

Closed

ssbr suggested changes Aug 6, 2018

View reviewed changes

ssbr reviewed Aug 6, 2018

View reviewed changes

Code review comments

52037a9

- Bind to localhost instead of 127.0.0.1 - Update key generation method - Import cPickle if available

ssbr suggested changes Aug 7, 2018

View reviewed changes

More code review changes

1990049

- remove redundant call to bytes - use hmac.compare_digest

ssbr approved these changes Aug 7, 2018

View reviewed changes

mcepl previously requested changes Aug 7, 2018

View reviewed changes

More code review changes

dc66284

- Document _compat_compare_digest

soupytwist merged commit b01da7a into master Aug 8, 2018

soupytwist deleted the cve-2014-3539 branch August 8, 2018 18:10

This was referenced Aug 8, 2018

critical severity security vulnerability detected in rope <= 0.10.7 #250

Closed

Simple hackish solution to CVE-2014-3539. #107

Closed

terencehonles mentioned this pull request May 3, 2021

Security concerns about pickle for serialization jazzband/django-redis#424

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform signature verification on pickled data transferred over sockets #251

Perform signature verification on pickled data transferred over sockets #251

soupytwist commented Aug 6, 2018 •

edited

ssbr Aug 6, 2018

ssbr Aug 6, 2018

soupytwist Aug 7, 2018

ssbr Aug 6, 2018

soupytwist Aug 7, 2018

ssbr Aug 6, 2018

soupytwist Aug 7, 2018

ssbr Aug 7, 2018 •

edited

soupytwist Aug 7, 2018

ssbr Aug 6, 2018

soupytwist Aug 7, 2018

ssbr Aug 6, 2018 •

edited

soupytwist Aug 7, 2018

ssbr Aug 7, 2018 •

edited

ssbr Aug 7, 2018

soupytwist Aug 7, 2018

mcepl left a comment

mcepl Aug 7, 2018

soupytwist Aug 7, 2018

ssbr Aug 7, 2018

mcepl Aug 7, 2018

soupytwist Aug 7, 2018

ssbr Aug 7, 2018

		@@ -11,6 +14,20 @@
		import threading


		def _compat_compare_digest(a, b):

Perform signature verification on pickled data transferred over sockets #251

Perform signature verification on pickled data transferred over sockets #251

Conversation

soupytwist commented Aug 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssbr Aug 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssbr Aug 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssbr Aug 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcepl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soupytwist commented Aug 6, 2018 •

edited

ssbr Aug 7, 2018 •

edited

ssbr Aug 6, 2018 •

edited

ssbr Aug 7, 2018 •

edited