Remove SHA1 support from RSA key handling

Author: bitprophet

paramiko/auth_handler.py

@@ -614,7 +614,13 @@ def _parse_userauth_request(self, m):
             # NOTE: server never wants to guess a client's algo, they're
             # telling us directly. No need for _finalize_pubkey_algorithm
             # anywhere in this flow.
+            # TODO: ok is this a spot where it can say a SHA2 dealie in some
+            # fields but still ssh-rsa within the pubkey blob part?
+            # TODO: ok so this would be rsa-sha2-256 or w/e, if this field says
+            # ssh-rsa the request can get stuffed.
             algorithm = m.get_text()
+            # TODO: This part would, if deconstructed, still be allowed to have
+            # "ssh-rsa" in its first field.
             keyblob = m.get_binary()
             try:
                 key = self._generate_key_from_request(algorithm, keyblob)

paramiko/client.py

@@ -439,10 +439,32 @@ def connect(
 
         our_server_keys = self._system_host_keys.get(server_hostkey_name)
         if our_server_keys is None:
+            # TODO: this is getting us the test suite _test_connection host key
+            # setup by virtue of that code doing tc.get_host_keys().add() (that
+            # method wraps self._host_keys)
+            # TODO: it should be analogous to running paramiko w/ a
+            # ~/.ssh/known_hosts file
             our_server_keys = self._host_keys.get(server_hostkey_name)
         if our_server_keys is not None:
+            # TODO: keytype needs to turn into one of the rsa-sha2 keys if it's
+            # an RSAKey.
+            # TODO: where is the equivalent on our server-side? hopefully the
+            # tests exercise that lol
+            # TODO: also, is it just me or does this [0] mean we literally do
+            # not actually implement real HostKeyAlgorithms agreement like
+            # ssh.c does?! eesh
             keytype = our_server_keys.keys()[0]
             sec_opts = t.get_security_options()
+            # TODO: clean this up a bit, but it does help some tests pass!
+            if keytype == "ssh-rsa":
+                if "rsa-sha2-512" in sec_opts.key_types:
+                    keytype = "rsa-sha2-512"
+                elif "rsa-sha2-256" in sec_opts.key_types:
+                    keytype = "rsa-sha2-256"
+                else:
+                    raise Exception(
+                        "TODO: REPLACEME with appropriate exception for 'what even is this key type in your known_hosts files?'"
+                    )
             other_types = [x for x in sec_opts.key_types if x != keytype]
             sec_opts.key_types = [keytype] + other_types
 
@@ -459,6 +481,10 @@ def connect(
                     self, server_hostkey_name, server_key
                 )
             else:
+                # TODO: this should 'just work' but dblcheck (HostKeys will be
+                # offering an ssh-rsa-via-sha2 key as "ssh-rsa" still, and the
+                # key would be RSAKey whose .get_name would still say
+                # "ssh-rsa")
                 our_key = our_server_keys.get(server_key.get_name())
                 if our_key != server_key:
                     if our_key is None:

paramiko/config.py

@@ -445,6 +445,7 @@ def _tokenize(self, config, target_hostname, key, value):
         # The actual tokens!
         replacements = {
             # TODO: %%???
+            # TODO: sha1 bad / this is offspec from rfc/openssh
             "%C": sha1(tohash.encode()).hexdigest(),
             "%d": homedir,
             "%h": configured_hostname,

paramiko/rsakey.py

@@ -38,8 +38,6 @@ class RSAKey(PKey):
 
     name = "ssh-rsa"
     HASHES = {
-        "ssh-rsa": hashes.SHA1,
-        "ssh-rsa-cert-v01@openssh.com": hashes.SHA1,
         "rsa-sha2-256": hashes.SHA256,
         "rsa-sha2-256-cert-v01@openssh.com": hashes.SHA256,
         "rsa-sha2-512": hashes.SHA512,
@@ -81,7 +79,14 @@ def __init__(
 
     @classmethod
     def identifiers(cls):
-        return list(cls.HASHES.keys())
+        # NOTE: we no longer want to have ssh-rsa+SHA1 in HASHES but we still
+        # need to advertise we can be used to read ssh-rsa keys (w/ assumption
+        # other parts of system will enforce the use of SHA2 signing algos).
+        # Thus, just say so here.
+        return list(cls.HASHES.keys()) + [
+            "ssh-rsa",
+            "ssh-rsa-cert-v01@openssh.com",
+        ]
 
     @property
     def size(self):
@@ -126,8 +131,8 @@ def sign_ssh_data(self, data, algorithm=None):
         sig = self.key.sign(
             data,
             padding=padding.PKCS1v15(),
-            # HASHES being just a map from long identifier to either SHA1 or
-            # SHA256 - cert'ness is not truly relevant.
+            # HASHES being just a map from long identifier to algo; cert'ness
+            # is not truly relevant.
             algorithm=self.HASHES[algorithm](),
         )
         m = Message()

paramiko/sftp_server.py

@@ -79,6 +79,7 @@
 from paramiko.sftp_si import SFTPServerInterface
 from paramiko.util import b
 
+# TODO: nuke or update w/ newer algs, see below
 _hash_class = {"sha1": sha1, "md5": md5}
 
 
@@ -302,6 +303,9 @@ def _check_file(self, request_number, msg):
             return
         f = self.file_table[handle]
         for x in alg_list:
+            # TODO: this only contains sha1 and md5 so uh, is this extension
+            # actually supported anymore? do we need to update the map for
+            # newer algos instead? other?
             if x in _hash_class:
                 algname = x
                 alg = _hash_class[x]

paramiko/transport.py

@@ -204,7 +204,6 @@ class Transport(threading.Thread, ClosingContextManager):
         "ecdsa-sha2-nistp521",
         "rsa-sha2-512",
         "rsa-sha2-256",
-        "ssh-rsa",
     )
     # ~= PubkeyAcceptedAlgorithms
     _preferred_pubkeys = (
@@ -214,7 +213,6 @@ class Transport(threading.Thread, ClosingContextManager):
         "ecdsa-sha2-nistp521",
         "rsa-sha2-512",
         "rsa-sha2-256",
-        "ssh-rsa",
     )
     _preferred_kex = (
         "ecdh-sha2-nistp256",
@@ -307,12 +305,18 @@ class Transport(threading.Thread, ClosingContextManager):
     }
 
     _key_info = {
-        # TODO: at some point we will want to drop this as it's no longer
-        # considered secure due to using SHA-1 for signatures. OpenSSH 8.8 no
-        # longer supports it. Question becomes at what point do we want to
-        # prevent users with older setups from using this?
-        "ssh-rsa": RSAKey,
-        "ssh-rsa-cert-v01@openssh.com": RSAKey,
+        # TODO: do some downstream uses of this need to be able to 'see'
+        # ssh-rsa in not-using-SHA1 contexts?
+        # TODO: NO!!! good.
+        # TODO: it's used in:
+        # - Transport._verify_key - verification - do not want ssh-rsa
+        # - SecurityOptions - only really uses this as a filter for what's
+        # allowed to be overwritten into its .key_types (which ==
+        # transport._preferred_keys), and since the latter doesn't want ssh-rsa
+        # in it, this use case doesn't require that string in here either.
+        # - AuthHandler._generate_key_from_request - server-side auth
+        # support - is looking at the 'algorithm' field in the request when it
+        # references this structure, so yup, do not want ssh-rsa
         "rsa-sha2-256": RSAKey,
         "rsa-sha2-256-cert-v01@openssh.com": RSAKey,
         "rsa-sha2-512": RSAKey,
@@ -1396,11 +1400,13 @@ def connect(
             # TODO: a more robust implementation would be to ask each key class
             # for its nameS plural, and just use that.
             # TODO: that could be used in a bunch of other spots too
+            # TODO: don't we have that now, lol
+            # TODO: either way this is ~= like using SecurityOptions.key_types
+            # = xxx, but different, which sucks sigh
             if isinstance(hostkey, RSAKey):
                 self._preferred_keys = [
                     "rsa-sha2-512",
                     "rsa-sha2-256",
-                    "ssh-rsa",
                 ]
             else:
                 self._preferred_keys = [hostkey.get_name()]
@@ -2002,6 +2008,8 @@ def _verify_key(self, host_key, sig):
         key: PKey = self._key_info[self.host_key_type](Message(host_key))
         if key is None:
             raise SSHException("Unknown host key type")
+        # TODO: like, here, can a host offer "ssh-rsa" but request SHA2, or are
+        # those baked in?
         if not key.verify_ssh_sig(self.H, Message(sig)):
             raise SSHException(
                 "Signature verification ({}) failed.".format(
@@ -3232,6 +3240,14 @@ def key_types(self):
 
     @key_types.setter
     def key_types(self, x):
+        # TODO: so this reads Transport._key_info.keys(), yells if any values
+        # in `x` /aren't/ in that list, then overwrites
+        # Transport._preferred_keys with `x`...
+        # TODO: so you can read this pretty simply as "replace
+        # transport._preferred_keys with x".
+        # TODO: which is...bad...in cases where SSHClient is trying to simply
+        # load up known_hosts or system known hosts, and use those to determine
+        # which hostkey /algorithms/ it is willing to accept
         self._set("_preferred_keys", "_key_info", x)
 
     @property

sites/www/changelog.rst

@@ -2,6 +2,18 @@
 Changelog
 =========
 
+- :support:`-` Removed support for verifying/signing with RSA keys using SHA-1
+  hashing. Generally, this means most cases where ``"ssh-rsa"`` was used as an
+  algorithm identifier (as opposed to a key material identifier) will no longer
+  accept that string as valid, and the relevant code that actually used eg
+  `hashes.SHA1` no longer does.
+
+  .. warning::
+    This change is backwards incompatible if you are stuck supporting legacy
+    systems with Paramiko that are unable to use SHA2-based signatures with RSA
+    keys (or other workarounds, such as switching from RSA keys to Ed25519
+    ones).
+
 - :bug:`- major` Added a ``password`` kwarg to `PKey.from_type_string
   <paramiko.pkey.PKey.from_type_string>` so it can handle encrypted keys like
   most other PKey constructors already could.

tests/_util.py

@@ -167,42 +167,9 @@ def is_low_entropy():
     return is_32bit and os.environ.get("PYTHONHASHSEED", None) == "0"
 
 
-def sha1_signing_unsupported():
-    """
-    This is used to skip tests in environments where SHA-1 signing is
-    not supported by the backend.
-    """
-    private_key = rsa.generate_private_key(
-        public_exponent=65537, key_size=2048, backend=default_backend()
-    )
-    message = b"Some dummy text"
-    try:
-        private_key.sign(
-            message,
-            padding.PSS(
-                mgf=padding.MGF1(hashes.SHA1()),
-                salt_length=padding.PSS.MAX_LENGTH,
-            ),
-            hashes.SHA1(),
-        )
-        return False
-    except UnsupportedAlgorithm as e:
-        return e._reason == _Reasons.UNSUPPORTED_HASH
-
-
-requires_sha1_signing = unittest.skipIf(
-    sha1_signing_unsupported(), "SHA-1 signing not supported"
-)
-
-_disable_sha2 = dict(
-    disabled_algorithms=dict(keys=["rsa-sha2-256", "rsa-sha2-512"])
-)
-_disable_sha1 = dict(disabled_algorithms=dict(keys=["ssh-rsa"]))
-_disable_sha2_pubkey = dict(
-    disabled_algorithms=dict(pubkeys=["rsa-sha2-256", "rsa-sha2-512"])
-)
-_disable_sha1_pubkey = dict(disabled_algorithms=dict(pubkeys=["ssh-rsa"]))
-
+# TODO: doublecheck where _disable_xxx helpers were in use; disabling sha1 no
+# longer makes any sense, and presumably neither does en/dis abling sha2 but
+# that's the question innit
 
 unicodey = "\u2022"