Fix CVE re: PKey.write_private_key chmod race

CVE-2022-24302 (see changelog for link)

3 files changed, 82 insertions, 2 deletions

diff --git a/paramiko/pkey.py b/paramiko/pkey.py
index 7865a6ea..67945261 100644
--- a/paramiko/pkey.py
+++ b/paramiko/pkey.py
@@ -558,7 +558,17 @@ class PKey(object):
 
         :raises: ``IOError`` -- if there was an error writing the file.
         """
-        with open(filename, "w") as f:
+        # Ensure that we create new key files directly with a user-only mode,
+        # instead of opening, writing, then chmodding, which leaves us open to
+        # CVE-2022-24302.
+        # NOTE: O_TRUNC is a noop on new files, and O_CREAT is a noop on
+        # existing files, so using all 3 in both cases is fine. Ditto the use
+        # of the 'mode' argument; it should be safe to give even for existing
+        # files (though it will not act like a chmod in that case).
+        kwargs = dict(flags=os.O_WRONLY | os.O_TRUNC | os.O_CREAT, mode=o600)
+        # NOTE: yea, you still gotta inform the FLO that it is in "write" mode
+        with os.fdopen(os.open(filename, **kwargs), mode="w") as f:
+            # TODO 3.0: remove the now redundant chmod
             os.chmod(filename, o600)
             self._write_private_key(f, key, format, password=password)
 
diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst
index af648ddc..37d149f2 100644
--- a/sites/www/changelog.rst
+++ b/sites/www/changelog.rst
@@ -2,6 +2,20 @@
 Changelog
 =========
 
+- :bug:`-` (`CVE-2022-24302
+  <https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-24302>`_) Creation
+  of new private key files using `~paramiko.pkey.PKey` subclasses was subject
+  to a race condition between file creation & mode modification, which could be
+  exploited by an attacker with knowledge of where the Paramiko-using code
+  would write out such files.
+
+  This has been patched by using `os.open` and `os.fdopen` to ensure new files
+  are opened with the correct mode immediately. We've left the subsequent
+  explicit ``chmod`` in place to minimize any possible disruption, though it
+  may get removed in future backwards-incompatible updates.
+
+  Thanks to Jan Schejbal for the report & feedback on the solution, and to
+  Jeremy Katz at Tidelift for coordinating the disclosure.
 - :release:`2.10.0 <2022-03-11>`
 - :feature:`1976` Add support for the ``%C`` token when parsing SSH config
   files. Foundational PR submitted by ``@jbrand42``.
diff --git a/tests/test_pkey.py b/tests/test_pkey.py
index 0cc20133..d44a96ac 100644
--- a/tests/test_pkey.py
+++ b/tests/test_pkey.py
@@ -23,6 +23,7 @@ Some unit tests for public/private key objects.
 
 import unittest
 import os
+import stat
 from binascii import hexlify
 from hashlib import md5
 
@@ -36,10 +37,11 @@ from paramiko import (
     SSHException,
 )
 from paramiko.py3compat import StringIO, byte_chr, b, bytes, PY2
+from paramiko.common import o600
 
 from cryptography.exceptions import UnsupportedAlgorithm
 from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateNumbers
-from mock import patch
+from mock import patch, Mock
 import pytest
 
 from .util import _support, is_low_entropy
@@ -696,3 +698,57 @@ class KeyTest(unittest.TestCase):
             key1.load_certificate,
             _support("test_rsa.key-cert.pub"),
         )
+
+    @patch("paramiko.pkey.os")
+    def _test_keyfile_race(self, os_, exists):
+        # Re: CVE-2022-24302
+        password = "television"
+        newpassword = "radio"
+        source = _support("test_ecdsa_384.key")
+        new = source + ".new"
+        # Mock setup
+        os_.path.exists.return_value = exists
+        # Attach os flag values to mock
+        for attr, value in vars(os).items():
+            if attr.startswith("O_"):
+                setattr(os_, attr, value)
+        # Load fixture key
+        key = ECDSAKey(filename=source, password=password)
+        key._write_private_key = Mock()
+        # Write out in new location
+        key.write_private_key_file(new, password=newpassword)
+        # Expected open via os module
+        os_.open.assert_called_once_with(new, flags=os.O_WRONLY | os.O_CREAT | os.O_TRUNC, mode=o600)
+        os_.fdopen.assert_called_once_with(os_.open.return_value, mode="w")
+        # Old chmod still around for backwards compat
+        os_.chmod.assert_called_once_with(new, o600)
+        assert (
+            key._write_private_key.call_args[0][0]
+            == os_.fdopen.return_value.__enter__.return_value
+        )
+
+    def test_new_keyfiles_avoid_file_descriptor_race_on_chmod(self):
+        self._test_keyfile_race(exists=False)
+
+    def test_existing_keyfiles_still_work_ok(self):
+        self._test_keyfile_race(exists=True)
+
+    def test_new_keyfiles_avoid_descriptor_race_integration(self):
+        # Integration-style version of above
+        password = "television"
+        newpassword = "radio"
+        source = _support("test_ecdsa_384.key")
+        new = source + ".new"
+        # Load fixture key
+        key = ECDSAKey(filename=source, password=password)
+        try:
+            # Write out in new location
+            key.write_private_key_file(new, password=newpassword)
+            # Test mode
+            assert stat.S_IMODE(os.stat(new).st_mode) == o600
+            # Prove can open with new password
+            reloaded = ECDSAKey(filename=new, password=newpassword)
+            assert reloaded == key
+        finally:
+            if os.path.exists(new):
+                os.unlink(new)