Prevent replays of file server requests

changelog/cve-2022-22936.security

@@ -0,0 +1 @@
+Prevent job and fileserver replays

salt/crypt.py

@@ -696,9 +696,17 @@ def _authenticate(self):
                 self._authenticate_future.set_exception(error)
             else:
                 key = self.__key(self.opts)
-                AsyncAuth.creds_map[key] = creds
-                self._creds = creds
-                self._crypticle = Crypticle(self.opts, creds["aes"])
+                if key not in AsyncAuth.creds_map:
+                    log.debug("%s Got new master aes key.", self)
+                    AsyncAuth.creds_map[key] = creds
+                    self._creds = creds
+                    self._crypticle = Crypticle(self.opts, creds["aes"])
+                elif self._creds["aes"] != creds["aes"]:
+                    log.debug("%s The master's aes key has changed.", self)
+                    AsyncAuth.creds_map[key] = creds
+                    self._creds = creds
+                    self._crypticle = Crypticle(self.opts, creds["aes"])
+
                 self._authenticate_future.set_result(
                     True
                 )  # mark the sign-in as complete
@@ -1277,6 +1285,7 @@ def __singleton_init__(self, opts, io_loop=None):
         self.serial = salt.payload.Serial(self.opts)
         self.pub_path = os.path.join(self.opts["pki_dir"], "minion.pub")
         self.rsa_path = os.path.join(self.opts["pki_dir"], "minion.pem")
+        self._creds = None
         if "syndic_master" in self.opts:
             self.mpub = "syndic_master.pub"
         elif "alert_master" in self.opts:
@@ -1346,8 +1355,14 @@ def authenticate(self, _=None):  # TODO: remove unused var
                         )
                     continue
                 break
-            self._creds = creds
-            self._crypticle = Crypticle(self.opts, creds["aes"])
+            if self._creds is None:
+                log.error("%s Got new master aes key.", self)
+                self._creds = creds
+                self._crypticle = Crypticle(self.opts, creds["aes"])
+            elif self._creds["aes"] != creds["aes"]:
+                log.error("%s The master's aes key has changed.", self)
+                self._creds = creds
+                self._crypticle = Crypticle(self.opts, creds["aes"])
 
     def sign_in(self, timeout=60, safe=True, tries=1, channel=None):
         """
@@ -1415,11 +1430,11 @@ class Crypticle:
     AES_BLOCK_SIZE = 16
     SIG_SIZE = hashlib.sha256().digest_size
 
-    def __init__(self, opts, key_string, key_size=192):
+    def __init__(self, opts, key_string, key_size=192, serial=0):
         self.key_string = key_string
         self.keys = self.extract_keys(self.key_string, key_size)
         self.key_size = key_size
-        self.serial = salt.payload.Serial(opts)
+        self.serial = serial
 
     @classmethod
     def generate_key_string(cls, key_size=192):
@@ -1489,19 +1504,45 @@ def decrypt(self, data):
             data = cypher.decrypt(data)
         return data[: -data[-1]]
 
-    def dumps(self, obj):
+    def dumps(self, obj, nonce=None):
         """
         Serialize and encrypt a python object
         """
-        return self.encrypt(self.PICKLE_PAD + self.serial.dumps(obj))
+        if nonce:
+            toencrypt = (
+                self.PICKLE_PAD + nonce.encode() + salt.payload.Serial({}).dumps(obj)
+            )
+        else:
+            toencrypt = self.PICKLE_PAD + salt.payload.Serial({}).dumps(obj)
+        return self.encrypt(toencrypt)
 
-    def loads(self, data, raw=False):
+    def loads(self, data, raw=False, nonce=None):
         """
         Decrypt and un-serialize a python object
         """
         data = self.decrypt(data)
         # simple integrity check to verify that we got meaningful data
         if not data.startswith(self.PICKLE_PAD):
             return {}
-        load = self.serial.loads(data[len(self.PICKLE_PAD) :], raw=raw)
-        return load
+        data = data[len(self.PICKLE_PAD) :]
+        if nonce:
+            ret_nonce = data[:32].decode()
+            data = data[32:]
+            if ret_nonce != nonce:
+                raise SaltClientError("Nonce verification error")
+        payload = salt.payload.Serial({}).loads(data, raw=raw)
+        if isinstance(payload, dict):
+            if "serial" in payload:
+                serial = payload.pop("serial")
+                if serial <= self.serial:
+                    log.critical(
+                        "A message with an invalid serial was received.\n"
+                        "this serial: %d\n"
+                        "last serial: %d\n"
+                        "The minion will not honor this request.",
+                        serial,
+                        self.serial,
+                    )
+                    return {}
+                self.serial = serial
+        return payload

salt/master.py

@@ -142,6 +142,44 @@ def __prep_key(self):
         """
         return salt.daemons.masterapi.access_keys(self.opts)
 
+    @classmethod
+    def get_serial(cls, opts=None, event=None):
+        with cls.secrets["aes"]["secret"].get_lock():
+            if cls.secrets["aes"]["serial"].value == sys.maxsize:
+                cls.rotate_secrets(opts, event, use_lock=False)
+            else:
+                cls.secrets["aes"]["serial"].value += 1
+            return cls.secrets["aes"]["serial"].value
+
+    @classmethod
+    def rotate_secrets(cls, opts=None, event=None, use_lock=True):
+        log.info("Rotating master AES key")
+        if opts is None:
+            opts = {}
+
+        for secret_key, secret_map in cls.secrets.items():
+            # should be unnecessary-- since no one else should be modifying
+            if use_lock:
+                with secret_map["secret"].get_lock():
+                    secret_map["secret"].value = salt.utils.stringutils.to_bytes(
+                        secret_map["reload"]()
+                    )
+                    if "serial" in secret_map:
+                        secret_map["serial"].value = 0
+            else:
+                secret_map["secret"].value = salt.utils.stringutils.to_bytes(
+                    secret_map["reload"]()
+                )
+                if "serial" in secret_map:
+                    secret_map["serial"].value = 0
+            if event:
+                event.fire_event({"rotate_{}_key".format(secret_key): True}, tag="key")
+
+        if opts.get("ping_on_rotate"):
+            # Ping all minions to get them to pick up the new key
+            log.debug("Pinging all connected minions due to key rotation")
+            salt.utils.master.ping_all_connected_minions(opts)
+
 
 class Maintenance(salt.utils.process.SignalHandlingProcess):
     """
@@ -312,21 +350,8 @@ def handle_key_rotate(self, now):
                 to_rotate = True
 
         if to_rotate:
-            log.info("Rotating master AES key")
-            for secret_key, secret_map in SMaster.secrets.items():
-                # should be unnecessary-- since no one else should be modifying
-                with secret_map["secret"].get_lock():
-                    secret_map["secret"].value = salt.utils.stringutils.to_bytes(
-                        secret_map["reload"]()
-                    )
-                self.event.fire_event(
-                    {"rotate_{}_key".format(secret_key): True}, tag="key"
-                )
+            SMaster.rotate_secrets(self.opts, self.event)
             self.rotate = now
-            if self.opts.get("ping_on_rotate"):
-                # Ping all minions to get them to pick up the new key
-                log.debug("Pinging all connected minions " "due to key rotation")
-                salt.utils.master.ping_all_connected_minions(self.opts)
 
     def handle_git_pillar(self):
         """
@@ -712,8 +737,12 @@ def start(self):
                         salt.crypt.Crypticle.generate_key_string()
                     ),
                 ),
+                "serial": multiprocessing.Value(
+                    ctypes.c_longlong, lock=False  # We'll use the lock from 'secret'
+                ),
                 "reload": salt.crypt.Crypticle.generate_key_string,
             }
+
             log.info("Creating master process manager")
             # Since there are children having their own ProcessManager we should wait for kill more time.
             self.process_manager = salt.utils.process.ProcessManager(wait_for_kill=5)

salt/minion.py

@@ -1660,6 +1660,7 @@ def _handle_decoded_payload(self, data):
         Override this method if you wish to handle the decoded data
         differently.
         """
+
         # Ensure payload is unicode. Disregard failure to decode binary blobs.
         if six.PY2:
             data = salt.utils.data.decode(data, keep=True)

salt/transport/mixins/auth.py

@@ -604,6 +604,7 @@ def _auth(self, load, sign_messages=False):
                 ret["aes"] = pub.public_encrypt(aes, RSA.pkcs1_oaep_padding)
             else:
                 ret["aes"] = cipher.encrypt(aes)
+
         # Be aggressive about the signature
         digest = salt.utils.stringutils.to_bytes(hashlib.sha256(aes).hexdigest())
         ret["sig"] = salt.crypt.private_encrypt(self.master_key.key, digest)

salt/transport/tcp.py

@@ -410,6 +410,9 @@ def _crypted_transfer(self, load, tries=3, timeout=60):
         Indeed, we can fail too early in case of a master restart during a
         minion state execution call
         """
+        nonce = uuid.uuid4().hex
+        if load and isinstance(load, dict):
+            load["nonce"] = nonce
 
         @salt.ext.tornado.gen.coroutine
         def _do_transfer():
@@ -421,7 +424,7 @@ def _do_transfer():
             # communication, we do not subscribe to return events, we just
             # upload the results to the master
             if data:
-                data = self.auth.crypticle.loads(data)
+                data = self.auth.crypticle.loads(data, nonce=nonce)
                 data = salt.transport.frame.decode_embedded_strs(data)
             raise salt.ext.tornado.gen.Return(data)
 
@@ -834,7 +837,7 @@ def handle_message(self, stream, header, payload):
             elif req_fun == "send":
                 stream.write(
                     salt.transport.frame.frame_msg(
-                        self.crypticle.dumps(ret), header=header
+                        self.crypticle.dumps(ret, nonce), header=header
                     )
                 )
             elif req_fun == "send_private":
@@ -1672,7 +1675,7 @@ def publish(self, load):
         Publish "load" to minions
         """
         payload = {"enc": "aes"}
-
+        load["serial"] = salt.master.SMaster.get_serial()
         crypticle = salt.crypt.Crypticle(
             self.opts, salt.master.SMaster.secrets["aes"]["secret"].value
         )

salt/transport/zeromq.py

@@ -67,6 +67,7 @@
     except ImportError:
         from Crypto.Cipher import PKCS1_OAEP  # nosec
 
+
 log = logging.getLogger(__name__)
 
 
@@ -78,12 +79,12 @@ def _get_master_uri(master_ip, master_port, source_ip=None, source_port=None):
     rc = zmq_connect(socket, "tcp://192.168.1.17:5555;192.168.1.1:5555"); assert (rc == 0);
     Source: http://api.zeromq.org/4-1:zmq-tcp
     """
+
     from salt.utils.zeromq import ip_bracket
 
     master_uri = "tcp://{master_ip}:{master_port}".format(
         master_ip=ip_bracket(master_ip), master_port=master_port
     )
-
     if source_ip or source_port:
         if LIBZMQ_VERSION_INFO >= (4, 1, 6) and ZMQ_VERSION_INFO >= (16, 0, 1):
             # The source:port syntax for ZeroMQ has been added in libzmq 4.1.6
@@ -416,6 +417,9 @@ def _crypted_transfer(self, load, tries=3, timeout=60, raw=False):
         :param int tries: The number of times to make before failure
         :param int timeout: The number of seconds on a response before failing
         """
+        nonce = uuid.uuid4().hex
+        if load and isinstance(load, dict):
+            load["nonce"] = nonce
 
         @salt.ext.tornado.gen.coroutine
         def _do_transfer():
@@ -430,7 +434,7 @@ def _do_transfer():
             # communication, we do not subscribe to return events, we just
             # upload the results to the master
             if data:
-                data = self.auth.crypticle.loads(data, raw)
+                data = self.auth.crypticle.loads(data, raw, nonce)
             if six.PY3 and not raw:
                 data = salt.transport.frame.decode_embedded_strs(data)
             raise salt.ext.tornado.gen.Return(data)
@@ -919,7 +923,7 @@ def handle_message(self, stream, payload):
         if req_fun == "send_clear":
             stream.send(self.serial.dumps(ret))
         elif req_fun == "send":
-            stream.send(self.serial.dumps(self.crypticle.dumps(ret)))
+            stream.send(self.serial.dumps(self.crypticle.dumps(ret, nonce)))
         elif req_fun == "send_private":
             stream.send(
                 self.serial.dumps(
@@ -1180,6 +1184,7 @@ def publish(self, load):
         :param dict load: A load to be sent across the wire to minions
         """
         payload = {"enc": "aes"}
+        load["serial"] = salt.master.SMaster.get_serial()
         crypticle = salt.crypt.Crypticle(
             self.opts, salt.master.SMaster.secrets["aes"]["secret"].value
         )

tests/pytests/unit/test_crypt.py

@@ -0,0 +1,54 @@
+"""
+tests.pytests.unit.test_crypt
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Unit tests for salt's crypt module
+"""
+
+import uuid
+
+import pytest
+import salt.crypt
+import salt.master
+import salt.utils.files
+
+
+def test_cryptical_dumps_no_nonce():
+    master_crypt = salt.crypt.Crypticle({}, salt.crypt.Crypticle.generate_key_string())
+    data = {"foo": "bar"}
+    ret = master_crypt.dumps(data)
+
+    # Validate message structure
+    assert isinstance(ret, bytes)
+    une = master_crypt.decrypt(ret)
+    une.startswith(master_crypt.PICKLE_PAD)
+    assert salt.payload.Serial({}).loads(une[len(master_crypt.PICKLE_PAD) :]) == data
+
+    # Validate load back to orig data
+    assert master_crypt.loads(ret) == data
+
+
+def test_cryptical_dumps_valid_nonce():
+    nonce = uuid.uuid4().hex
+    master_crypt = salt.crypt.Crypticle({}, salt.crypt.Crypticle.generate_key_string())
+    data = {"foo": "bar"}
+    ret = master_crypt.dumps(data, nonce=nonce)
+
+    assert isinstance(ret, bytes)
+    une = master_crypt.decrypt(ret)
+    une.startswith(master_crypt.PICKLE_PAD)
+    nonce_and_data = une[len(master_crypt.PICKLE_PAD) :]
+    assert nonce_and_data.startswith(nonce.encode())
+    assert salt.payload.Serial({}).loads(nonce_and_data[len(nonce) :]) == data
+
+    assert master_crypt.loads(ret, nonce=nonce) == data
+
+
+def test_cryptical_dumps_invalid_nonce():
+    nonce = uuid.uuid4().hex
+    master_crypt = salt.crypt.Crypticle({}, salt.crypt.Crypticle.generate_key_string())
+    data = {"foo": "bar"}
+    ret = master_crypt.dumps(data, nonce=nonce)
+    assert isinstance(ret, bytes)
+    with pytest.raises(salt.crypt.SaltClientError, match="Nonce verification error"):
+        assert master_crypt.loads(ret, nonce="abcde")

tests/pytests/unit/transport/test_zeromq.py

@@ -17,6 +17,7 @@
 import salt.log.setup
 import salt.transport.client
 import salt.transport.server
+import salt.transport.zeromq
 import salt.utils.platform
 import salt.utils.process
 import salt.utils.stringutils
@@ -214,7 +215,6 @@
 @pytest.fixture
 def pki_dir(tmpdir):
     madir = tmpdir.mkdir("master")
-
     mapriv = madir.join("master.pem")
     mapriv.write(MASTER_PRIV_KEY.strip())
     mapub = madir.join("master.pub")
@@ -225,8 +225,6 @@ def pki_dir(tmpdir):
     maspub = madir.join("master_sign.pub")
     maspub.write(MASTER_SIGNING_PUB.strip())
 
-    mipub = madir.mkdir("minions").join("minion")
-    mipub.write(MINION_PUB_KEY.strip())
     for sdir in [
         "minions_autosign",
         "minions_denied",
@@ -235,6 +233,9 @@ def pki_dir(tmpdir):
     ]:
         madir.mkdir(sdir)
 
+    mipub = madir.mkdir("minions").join("minion")
+    mipub.write(MINION_PUB_KEY.strip())
+
     midir = tmpdir.mkdir("minion")
     mipub = midir.join("minion.pub")
     mipub.write(MINION_PUB_KEY.strip())