Remove pickle.loads in averager #160

justheuristic · 2021-02-28T16:45:49Z

Addresses #155 : require pre-serialized (bytes) metadata in get_current_state, switch to MSGPackSerializer in the application code

borzunov · 2021-02-28T16:51:57Z

hivemind/client/averaging/__init__.py

@@ -42,7 +42,7 @@ class DecentralizedAverager(mp.Process, averaging_pb2_grpc.DecentralizedAveragin
    :param dht: a DHT node that will be used to find groups
    :param start: if True, starts the background process immediately

-    :param prefix: a shared prefix for all group keys
+    :param prefix: a shared prefix for all group keysP


Nit: accidental change?

mryab · 2021-02-28T17:12:18Z

hivemind/client/averaging/__init__.py

@@ -376,7 +384,7 @@ async def _load_state_from_peers(self, future: MPFuture):
                    current_tensor_parts, tensors = [], []
                    async for message in stream:
                        if message.metadata:
-                            metadata = PickleSerializer.loads(message.metadata)


PickleSerializer is no longer required, so the import can be removed

mryab · 2021-02-28T17:14:36Z

hivemind/client/averaging/__init__.py

+        Try to download the latest optimizer state one of the existing peer.
+        :returns: on success, return a 2-tuple with (serialized_metadata, tensors), where
+
+        - serialized_metadata is a small bytestring containing **serialized** metadata (e.g. hyperparameters)


Shouldn't we also deserialize on return? What are the cases where we still need serialized metadata as the output of this method?

mryab

Also bump the version, please

mryab · 2021-02-28T21:14:32Z

tests/test_averaging.py

@@ -277,7 +278,7 @@ def get_current_state(self):
            """
            nonlocal num_calls, super_metadata, super_tensors
            num_calls += 1
-            return super_metadata, super_tensors
+            return serializer.dumps(super_metadata), super_tensors


I believe you can use self.serializer here and avoid declaring the serializer at L271?

* copytree implementation for py37 compatibility (#162) * copytree implementation for py37 compatibility * Running tests for python3.7 * Increment version * Python3.7 notions * Remove pickle.loads in averager (#160) * Security update: remove pickle.loads in averager * add py37 to circleci config Co-authored-by: Alexander Borzunov <[email protected]> Co-authored-by: Max Ryabinin <[email protected]> * Support edge cases for DHT key/subkey/value, add tests, update .gitignore for pb2 (#167) * fix bug with subkey equals zero * add autogenerated protobuf files to .gitignore * test store and get "tricky" values in dht * Fix the remaining tests for py37 (#166) * DecentralizedAverager is now compatible with python37's acyncio exception * the problem was: grpc.aio with python37 raised concurrent.futures.CancelledError in some cases; * we relied on isinstance(asyncio.CancelledError, Exception) == False * but isinstance(concurrent.futures.CancelledError, Exception) == True * DecentralizedAverager now shuts down if dereferenced in the main process * though it won't shutdown if dereferenced in forks for obvious reasons * HIVEMIND_THREADS now actually works * test_averaging now shuts down dht and averager instances to avoid leaking processes Co-authored-by: Max Ryabinin <[email protected]> Co-authored-by: Max Ryabinin <[email protected]> * Move Averager metadata serialization out of user scope (#168) * move metadata serialization outside user scope * test_overcrowded: reduce the default number of peers * Handle edge cases in DecentralizedAverager (#171) * move metadata serialization outside user scope * retry averager.step on network errors * raise AllreduceException on partial tensor * test split/combine tensors, combine corrupted stream Co-authored-by: Max Ryabinin <[email protected]> * Fix a typo in quickstart.md (#174) * Serialize DHTID source with msgpack (#172) * Change DHTID serializer * Remove unused serializers * Add msgpack tuple serialization * Move CLI server launch script to hivemind/hivemind_cli (#173) * Cast environment variables to correct types * Compiling libp2p daemon on setup (#153) * add setup.py prototype * refactor * feat: add p2p daemon (#164) * Add p2p daemon * Test p2p daemon exits correctly * Impose restriction on elapsed time Co-authored-by: Ilya Kobelev <[email protected]> * compare golang versions using packaging.version * fix typo Co-authored-by: justheuristic <[email protected]> * move p2pd executable to hivemind/hivemind_cli Co-authored-by: Alexey Bukhtiyarov <[email protected]> Co-authored-by: justheuristic <[email protected]> Co-authored-by: Alexander Borzunov <[email protected]> Co-authored-by: Max Ryabinin <[email protected]> Co-authored-by: Michael Diskin <[email protected]> Co-authored-by: romakail <[email protected]> Co-authored-by: Ilya <[email protected]> Co-authored-by: Ilya Kobelev <[email protected]>

justheuristic requested review from mryab and borzunov February 28, 2021 16:46

borzunov reviewed Feb 28, 2021

View reviewed changes

borzunov approved these changes Feb 28, 2021

View reviewed changes

mryab requested changes Feb 28, 2021

View reviewed changes

mryab reviewed Feb 28, 2021

View reviewed changes

justheuristic and others added 10 commits March 1, 2021 00:33

Security update: remove pickle.loads in averager

b101f6a

address review changes by borzunov@

680d7f7

address review changes by mryab@

a776467

address review changes by mryab@

7f52dc6

address review changes by mryab@

8e9f036

address review changes by mryab@

7f0ac1a

address review changes by mryab@

f81679b

typo

3480541

use averager's MSGPackSerializer in tests

7c83698

bump

0eeed48

justheuristic force-pushed the security-fix branch from 1291d91 to 0eeed48 Compare February 28, 2021 21:33

add circleci py37

6d31d5c

mryab changed the title ~~Security update: remove pickle.loads in averager~~ Remove pickle.loads in averager Feb 28, 2021

mryab added the security Security issues or improvements label Feb 28, 2021

mryab approved these changes Feb 28, 2021

View reviewed changes

justheuristic merged commit ca4b8c5 into master Feb 28, 2021

justheuristic deleted the security-fix branch February 28, 2021 22:00

justheuristic linked an issue Mar 2, 2021 that may be closed by this pull request

Attack vector: Unpickling data from peers may lead to remote code execution #155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove pickle.loads in averager #160

Remove pickle.loads in averager #160

justheuristic commented Feb 28, 2021 •

edited

Loading

borzunov Feb 28, 2021

mryab Feb 28, 2021

mryab Feb 28, 2021

mryab left a comment

mryab Feb 28, 2021

Remove pickle.loads in averager #160

Remove pickle.loads in averager #160

Conversation

justheuristic commented Feb 28, 2021 • edited Loading

borzunov Feb 28, 2021

Choose a reason for hiding this comment

mryab Feb 28, 2021

Choose a reason for hiding this comment

mryab Feb 28, 2021

Choose a reason for hiding this comment

mryab left a comment

Choose a reason for hiding this comment

mryab Feb 28, 2021

Choose a reason for hiding this comment

justheuristic commented Feb 28, 2021 •

edited

Loading