151 lines
5.9 KiB
Diff
151 lines
5.9 KiB
Diff
From 4c491e299c9b800358b16fa4886d8d94f45abe2e Mon Sep 17 00:00:00 2001
|
|
From: Jeff Forcier <jeff@bitprophet.org>
|
|
Date: Fri, 25 Feb 2022 14:50:42 -0500
|
|
Subject: [PATCH] Fix CVE re: PKey.write_private_key chmod race
|
|
|
|
CVE-2022-24302 (see changelog for link)
|
|
|
|
Conflict:NA
|
|
Reference:https://github.com/paramiko/paramiko/commit/4c491e299c9b800358b16fa4886d8d94f45abe2e
|
|
|
|
---
|
|
paramiko/pkey.py | 12 ++++++++-
|
|
sites/www/changelog.rst | 14 ++++++++++
|
|
tests/test_pkey.py | 58 ++++++++++++++++++++++++++++++++++++++++-
|
|
3 files changed, 82 insertions(+), 2 deletions(-)
|
|
|
|
diff --git a/paramiko/pkey.py b/paramiko/pkey.py
|
|
index 5bdfb1d..40afe19 100644
|
|
--- a/paramiko/pkey.py
|
|
+++ b/paramiko/pkey.py
|
|
@@ -551,7 +551,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 c423f5a..5867999 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.8.1 <2021-11-28>`
|
|
- :bug:`985` (via :issue:`992`) Fix listdir failure when server uses a locale.
|
|
Now on Python 2.7 `SFTPAttributes <paramiko.sftp_attr.SFTPAttributes>` will
|
|
diff --git a/tests/test_pkey.py b/tests/test_pkey.py
|
|
index 94b2492..4223544 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
|
|
@@ -686,3 +688,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)
|
|
--
|
|
2.27.0
|
|
|