8000 Fix CVE re: PKey.write_private_key chmod race · paramiko/paramiko@4c491e2 · GitHub 8000
[go: up one dir, main page]

Skip to content

Commit 4c491e2

Browse files
committed
Fix CVE re: PKey.write_private_key chmod race
CVE-2022-24302 (see changelog for link)
1 parent aa3cc6f commit 4c491e2

File tree

3 files changed

+82
-2
lines changed

3 files changed

+82
-2
lines changed

paramiko/pkey.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -558,7 +558,17 @@ def _write_private_key_file(self, filename, key, format, password=None):
558558
559559
:raises: ``IOError`` -- if there was an error writing the file.
560560
"""
561-
with open(filename, "w") as f:
561+
# Ensure that we create new key files directly with a user-only mode,
562+
# instead of opening, writing, then chmodding, which leaves us open to
563+
# CVE-2022-24302.
564+
# NOTE: O_TRUNC is a noop on new files, and O_CREAT is a noop on
565+
# existing files, so using all 3 in both cases is fine. Ditto the use
566+
# of the 'mode' argument; it should be safe to give even for existing
567+
# files (though it will not act like a chmod in that case).
568+
kwargs = dict(flags=os.O_WRONLY | os.O_TRUNC | os.O_CREAT, mode=o600)
569+
# NOTE: yea, you still gotta inform the FLO that it is in "write" mode
570+
with os.fdopen(os.open(filename, **kwargs), mode="w") as f:
571+
# TODO 3.0: remove the now redundant chmod
562572
os.chmod(filename, o600)
563573
self._write_private_key(f, key, format, password=password)
564574

sites/www/changelog.rst

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,20 @@
22
Changelog
33
=========
44

5+
- :bug:`-` (`CVE-2022-24302
6+
<https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-24302>`_) Creation
7+
of new private key files using `~paramiko.pkey.PKey` subclasses was subject
8+
to a race condition between file creation & mode modification, which could be
9+
exploited by an attacker with knowledge of where the Paramiko-using code
10+
would write out such files.
11+
12+
This has been patched by using `os.open` and `os.fdopen` to ensure new files
13+
are opened with the correct mode immediately. We've left the subsequent
14+
explicit ``chmod`` in place to minimize any possible disruption, though it
15+
may get removed in future backwards-incompatible updates.
16+
17+
Thanks to Jan Schejbal for the report & feedback on the solution, and to
18+
Jeremy Katz at Tidelift for coordinating the disclosure.
519
- :release:`2.10.0 <2022-03-11>`
620
- :feature:`1976` Add support for the ``%C`` token when parsing SSH config
721
files. Foundational PR submitted by ``@jbrand42``.

tests/test_pkey.py

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import unittest
2525
import os
26+
import stat
2627
from binascii import hexlify
2728
from hashlib import md5
2829

@@ -36,10 +37,11 @@
3637
SSHException,
3738
)
3839
from paramiko.py3compat import StringIO, byte_chr, b, bytes, PY2
40+
from paramiko.common import o600
3941

4042
from cryptography.exceptions import UnsupportedAlgorithm
4143
from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateNumbers
42-
from mock import patch
44+
from mock import patch, Mock
4345
import pytest
4446

4547
from .util import _support, is_low_entropy
@@ -696,3 +698,57 @@ def test_certificates(self):
696698
key1.load_certificate,
697699
_support("test_rsa.key-cert.pub"),
698700
)
701+
702+
@patch("paramiko.pkey.os")
703+
def _test_keyfile_race(self, os_, exists):
704+
# Re: CVE-2022-24302
705+
password = "television"
706+
newpassword = "radio"
707+
source = _support("test_ecdsa_384.key")
708+
new = source + ".new"
709+
# Mock setup
710+
os_.path.exists.return_value = exists
711+
# Attach os flag values to mock
712+
for attr, value in vars(os).items():
713+
if attr.startswith("O_"):
714+
setattr(os_, attr, value)
715+
# Load fixture key
716+
key = ECDSAKey(filename=source, password=password)
717+
key._write_private_key = Mock()
718+
# Write out in new location
719+
key.write_private_key_file(new, password=newpassword)
720+
# Expected open via os module
721+
os_.open.assert_called_once_with(new, flags=os.O_WRONLY | os.O_CREAT | os.O_TRUNC, mode=o600)
722+
os_.fdopen.assert_called_once_with(os_.open.return_value, mode="w")
723+
# Old chmod still around for backwards compat
724+
os_.chmod.assert_called_once_with(new, o600)
725+
assert (
726+
key._write_private_key.call_args[0][0]
727+
== os_.fdopen.return_value.__enter__.return_value
728+
)
729+
730+
def test_new_keyfiles_avoid_file_descriptor_race_on_chmod(self):
731+
self._test_keyfile_race(exists=False)
732+
733+
def test_existing_keyfiles_still_work_ok(self):
734+
self._test_keyfile_race(exists=True)
735+
736+
def test_new_keyfiles_avoid_descriptor_race_integration(self):
737+
# Integration-style version of above
738+
password = "television"
739+
newpassword = "radio"
740+
source = _support("test_ecdsa_384.key")
741+
new = source + ".new"
742+
# Load fixture key
743+
key = ECDSAKey(filename=source, password=password)
744+
try:
745+
# Write out in new location
746+
key.write_private_key_file(new, password=newpassword)
747+
# Test mode
748+
assert stat.S_IMODE(os.stat(new).st_mode) == o600
749+
# Prove can open with new password
750+
reloaded = ECDSAKey(filename=new, password=newpassword)
751+
assert reloaded == key
752+
finally:
753+
if os.path.exists(new):
754+
os.unlink(new)

0 commit comments

Comments
 (0)
0