Backport ssh cert fix (#9211)

* Fix encoding of SSH certs with critical options (#9208)

* Add tests for issue #9207

* Fix encoding of SSH certs with critical options

* Test unexpected additional values for crit opts/exts

* temporarily allow invalid ssh cert encoding

---------

Co-authored-by: jeanluc <2163936+lkubb@users.noreply.github.com>

Author: reaperhulk

CHANGELOG.rst

@@ -1,6 +1,17 @@
 Changelog
 =========
 
+.. _v41-0-2:
+
+41.0.2 - 2023-07-10
+~~~~~~~~~~~~~~~~~~~
+
+* Fixed bugs in creating and parsing SSH certificates where critical options
+  with values were handled incorrectly. Certificates are now created correctly
+  and parsing accepts correct values as well as the previously generated
+  invalid forms with a warning. In the next release, support for parsing these
+  invalid forms will be removed.
+
 .. _v41-0-1:
 
 41.0.1 - 2023-06-01

docs/development/test-vectors.rst

@@ -866,6 +866,10 @@ Custom OpenSSH Certificate Test Vectors
   critical option.
 * ``p256-p256-non-lexical-crit-opts.pub`` - A certificate with critical
   options in non-lexical order.
+* ``p256-ed25519-non-singular-crit-opt-val.pub`` - A certificate with
+  a critical option that contains more than one value.
+* ``p256-ed25519-non-singular-ext-val.pub`` - A certificate with
+  an extension that contains more than one value.
 * ``dsa-p256.pub`` - A certificate with a DSA public key signed by a P256
   CA.
 * ``p256-dsa.pub`` - A certificate with a P256 public key signed by a DSA

src/cryptography/hazmat/primitives/serialization/ssh.py

@@ -1063,6 +1063,20 @@ def _parse_exts_opts(exts_opts: memoryview) -> typing.Dict[bytes, bytes]:
         if last_name is not None and bname < last_name:
             raise ValueError("Fields not lexically sorted")
         value, exts_opts = _get_sshstr(exts_opts)
+        if len(value) > 0:
+            try:
+                value, extra = _get_sshstr(value)
+            except ValueError:
+                warnings.warn(
+                    "This certificate has an incorrect encoding for critical "
+                    "options or extensions. This will be an exception in "
+                    "cryptography 42",
+                    utils.DeprecatedIn41,
+                    stacklevel=4,
+                )
+            else:
+                if len(extra) > 0:
+                    raise ValueError("Unexpected extra data after value")
         result[bname] = bytes(value)
         last_name = bname
     return result
@@ -1450,12 +1464,22 @@ def sign(self, private_key: SSHCertPrivateKeyTypes) -> SSHCertificate:
         fcrit = _FragList()
         for name, value in self._critical_options:
             fcrit.put_sshstr(name)
-            fcrit.put_sshstr(value)
+            if len(value) > 0:
+                foptval = _FragList()
+                foptval.put_sshstr(value)
+                fcrit.put_sshstr(foptval.tobytes())
+            else:
+                fcrit.put_sshstr(value)
         f.put_sshstr(fcrit.tobytes())
         fext = _FragList()
         for name, value in self._extensions:
             fext.put_sshstr(name)
-            fext.put_sshstr(value)
+            if len(value) > 0:
+                fextval = _FragList()
+                fextval.put_sshstr(value)
+                fext.put_sshstr(fextval.tobytes())
+            else:
+                fext.put_sshstr(value)
         f.put_sshstr(fext.tobytes())
         f.put_sshstr(b"")  # RESERVED FIELD
         # encode CA public key

tests/hazmat/primitives/test_ssh.py

@@ -1115,26 +1115,28 @@ def test_loads_ssh_cert(self, backend):
         # secp256r1 public key, ed25519 signing key
         cert = load_ssh_public_identity(
             b"ecdsa-sha2-nistp256-cert-v01@openssh.com AAAAKGVjZHNhLXNoYTItbm"
-            b"lzdHAyNTYtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgtdU+dl9vD4xPi8afxERYo"
-            b"s0c0d9/3m7XGY6fGeSkqn0AAAAIbmlzdHAyNTYAAABBBAsuVFNNj/mMyFm2xB99"
-            b"G4xiaUJE1lZNjcp+S2tXYW5KorcHpusSlSqOkUPZ2l0644dgiNPDKR/R+BtYENC"
-            b"8aq8AAAAAAAAAAAAAAAEAAAAUdGVzdEBjcnlwdG9ncmFwaHkuaW8AAAAaAAAACm"
-            b"NyeXB0b3VzZXIAAAAIdGVzdHVzZXIAAAAAY7KyZAAAAAB2frXAAAAAAAAAAIIAA"
-            b"AAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9y"
-            b"d2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGV"
-            b"ybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAADMAAAALc3"
-            b"NoLWVkMjU1MTkAAAAg3P0eyGf2crKGwSlnChbLzTVOFKwQELE1Ve+EZ6rXF18AA"
-            b"ABTAAAAC3NzaC1lZDI1NTE5AAAAQKoij8BsPj/XLb45+wHmRWKNqXeZYXyDIj8J"
-            b"IE6dIymjEqq0TP6ntu5t59hTmWlDO85GnMXAVGBjFbeikBMfAQc= reaperhulk"
-            b"@despoina.local"
+            b"lzdHAyNTYtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgLfsFv9Gbc6LZSiJFWdYQl"
+            b"IMNI50GExXW0fBpgGVf+Y4AAAAIbmlzdHAyNTYAAABBBIzVyRgVLR4F38bIOLBN"
+            b"8CNm8Nf+eBHCVkKDKb9WDyLLD61CEmzjK/ORwFuSE4N60eIGbFidBf0D0xh7G6o"
+            b"TNxsAAAAAAAAAAAAAAAEAAAAUdGVzdEBjcnlwdG9ncmFwaHkuaW8AAAAaAAAACm"
+            b"NyeXB0b3VzZXIAAAAIdGVzdHVzZXIAAAAAY7KyZAAAAAB2frXAAAAAWAAAAA1mb"
+            b"3JjZS1jb21tYW5kAAAALAAAAChlY2hvIGFhYWFhYWFhYWFhYWFhYWFhYWFhYWFh"
+            b"YWFhYWFhYWFhYWFhAAAAD3ZlcmlmeS1yZXF1aXJlZAAAAAAAAACCAAAAFXBlcm1"
+            b"pdC1YMTEtZm9yd2FyZGluZwAAAAAAAAAXcGVybWl0LWFnZW50LWZvcndhcmRpbm"
+            b"cAAAAAAAAAFnBlcm1pdC1wb3J0LWZvcndhcmRpbmcAAAAAAAAACnBlcm1pdC1wd"
+            b"HkAAAAAAAAADnBlcm1pdC11c2VyLXJjAAAAAAAAAAAAAAAzAAAAC3NzaC1lZDI1"
+            b"NTE5AAAAICH6csEOmGbOfT2B/S/FJg3uyPsaPSZUZk2SVYlfs0KLAAAAUwAAAAt"
+            b"zc2gtZWQyNTUxOQAAAEDz2u7X5/TFbN7Ms7DP4yArhz1oWWYKkdAk7FGFkHfjtY"
+            b"/YfNQ8Oky3dCZRi7PnSzScEEjos7723dhF8/y99WwH reaperhulk@despoina."
+            b"local"
         )
         assert isinstance(cert, SSHCertificate)
         cert.verify_cert_signature()
         signature_key = cert.signature_key()
         assert isinstance(signature_key, ed25519.Ed25519PublicKey)
         assert cert.nonce == (
-            b"\xb5\xd5>v_o\x0f\x8cO\x8b\xc6\x9f\xc4DX\xa2\xcd\x1c\xd1\xdf"
-            b"\x7f\xden\xd7\x19\x8e\x9f\x19\xe4\xa4\xaa}"
+            b'-\xfb\x05\xbf\xd1\x9bs\xa2\xd9J"EY\xd6\x10\x94\x83\r#\x9d'
+            b"\x06\x13\x15\xd6\xd1\xf0i\x80e_\xf9\x8e"
         )
         public_key = cert.public_key()
         assert isinstance(public_key, ec.EllipticCurvePublicKey)
@@ -1145,7 +1147,10 @@ def test_loads_ssh_cert(self, backend):
         assert cert.valid_principals == [b"cryptouser", b"testuser"]
         assert cert.valid_before == 1988015552
         assert cert.valid_after == 1672655460
-        assert cert.critical_options == {}
+        assert cert.critical_options == {
+            b"force-command": b"echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+            b"verify-required": b"",
+        }
         assert cert.extensions == {
             b"permit-X11-forwarding": b"",
             b"permit-agent-forwarding": b"",
@@ -1154,6 +1159,31 @@ def test_loads_ssh_cert(self, backend):
             b"permit-user-rc": b"",
         }
 
+    def test_loads_deprecated_invalid_encoding_cert(self, backend):
+        with pytest.warns(utils.DeprecatedIn41):
+            cert = load_ssh_public_identity(
+                b"ecdsa-sha2-nistp256-cert-v01@openssh.com AAAAKGVjZHNhLXNoYT"
+                b"ItbmlzdHAyNTYtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgXE7sJ+xDVVNCO"
+                b"cEvpZS+SXIbc0nJdny/KqVbnwHslMIAAAAIbmlzdHAyNTYAAABBBI/qcLq8"
+                b"iiErpAhOWRqdMkpFSCNv7TVUcXCIfAl01JXbe2MvS4V7lFtiyrBjLSV7Iyw"
+                b"3TrulrWLibjPzZvLwmQcAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAA//"
+                b"////////8AAABUAAAADWZvcmNlLWNvbW1hbmQAAAAoZWNobyBhYWFhYWFhY"
+                b"WFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYQAAAA92ZXJpZnktcmVxdWly"
+                b"ZWQAAAAAAAAAEgAAAApwZXJtaXQtcHR5AAAAAAAAAAAAAABoAAAAE2VjZHN"
+                b"hLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBI/qcLq8iiErpAhOWR"
+                b"qdMkpFSCNv7TVUcXCIfAl01JXbe2MvS4V7lFtiyrBjLSV7Iyw3TrulrWLib"
+                b"jPzZvLwmQcAAABlAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAABKAAAAIQCi"
+                b"eCsIhGKrZdkE1+zY5EBucrLzxFpwnm/onIT/6rapvQAAACEAuVQ1yQjlPKr"
+                b"kfsGfjeG+2umZrOS5Ycx85BQhYf0RgsA="
+            )
+        assert isinstance(cert, SSHCertificate)
+        cert.verify_cert_signature()
+        assert cert.extensions == {b"permit-pty": b""}
+        assert cert.critical_options == {
+            b"force-command": b"echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+            b"verify-required": b"",
+        }
+
     @pytest.mark.parametrize(
         "filename",
         [
@@ -1267,6 +1297,8 @@ def test_invalid_cert_type(self):
             "p256-p256-non-lexical-extensions.pub",
             "p256-p256-duplicate-crit-opts.pub",
             "p256-p256-non-lexical-crit-opts.pub",
+            "p256-ed25519-non-singular-crit-opt-val.pub",
+            "p256-ed25519-non-singular-ext-val.pub",
         ],
     )
     def test_invalid_encodings(self, filename):
@@ -1693,6 +1725,11 @@ def test_sign_and_byte_compare_rsa(self, monkeypatch):
             .valid_after(1672531200)
             .valid_before(1672617600)
             .type(SSHCertificateType.USER)
+            .add_extension(b"permit-pty", b"")
+            .add_critical_option(
+                b"force-command", b"echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+            )
+            .add_critical_option(b"verify-required", b"")
         )
         cert = builder.sign(private_key)
         sig_key = cert.signature_key()
@@ -1707,19 +1744,21 @@ def test_sign_and_byte_compare_rsa(self, monkeypatch):
             b"4kyHpbLEIVloBjzetoqXK6u8Hjz/APuagONypNDCySDR6M7jM85HDcLoFFrbBb8"
             b"pruHSTxQejMeEmJxYf8b7rNl58/IWPB1ymbNlvHL/4oSOlnrtHkjcxRWzpQ7U3g"
             b"T9BThGyhCiI7EMyEHMgP3r7kTzEUwT6IavWDAAAAAAAAAAAAAAABAAAAAAAAAAA"
-            b"AAAAAY7DNAAAAAABjsh6AAAAAAAAAAAAAAAAAAAABFwAAAAdzc2gtcnNhAAAAAw"
-            b"EAAQAAAQEAwXr8fndHTKpaqDA2FYo/+/e1IWhRuiIw5dar/MHGz+9Z6SPqEzC8W"
-            b"TtzgCq2CKbkozBlI6MRa6WqOWYUUXThO2xJ6beAYuRJ1y77EP1J6R+gi5bQUeeC"
-            b"6fWrxbWm95hIJ6245z2gDyKy79zbduq0btrZjtZWYnQ/3GwOM2pdDNuqfcKeU2N"
-            b"eJMh6WyxCFZaAY83raKlyurvB48/wD7moDjcqTQwskg0ejO4zPORw3C6BRa2wW/"
-            b"Ka7h0k8UHozHhJicWH/G+6zZefPyFjwdcpmzZbxy/+KEjpZ67R5I3MUVs6UO1N4"
-            b"E/QU4RsoQoiOxDMhBzID96+5E8xFME+iGr1gwAAARQAAAAMcnNhLXNoYTItNTEy"
-            b"AAABAKCRnfhn6MZs3jRgIDICUpUyWrDCbpStEbdzhmoxF8w2m8klR7owRH/rxOf"
-            b"nWhKMGnXnoERS+az3Zh9ckiQPujkuEToORKpzu6CEWlzHSzyK1o2X548KkW76HJ"
-            b"gqzwMas94HY7UOJUgKSFUI0S3jAgqXAKSa1DxvJBu5/n57aUqPq+BmAtoI8uNBo"
-            b"x4F1pNEop38+oD7rUt8bZ8K0VcrubJZz806K8UNiK0mOahaEIkvZXBfzPGvSNRj"
-            b"0OjDl1dLUZaP8C1o5lVRomEm7pLcgE9i+ZDq5iz+mvQrSBStlpQ5hPGuUOrZ/oY"
-            b"ZLZ1G30R5tWj212MHoNZjxFxM8+f2OT4="
+            b"AAAAAY7DNAAAAAABjsh6AAAAAWAAAAA1mb3JjZS1jb21tYW5kAAAALAAAAChlY2"
+            b"hvIGFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhAAAAD3Zlcmlme"
+            b"S1yZXF1aXJlZAAAAAAAAAASAAAACnBlcm1pdC1wdHkAAAAAAAAAAAAAARcAAAAH"
+            b"c3NoLXJzYQAAAAMBAAEAAAEBAMF6/H53R0yqWqgwNhWKP/v3tSFoUboiMOXWq/z"
+            b"Bxs/vWekj6hMwvFk7c4Aqtgim5KMwZSOjEWulqjlmFFF04TtsSem3gGLkSdcu+x"
+            b"D9SekfoIuW0FHngun1q8W1pveYSCetuOc9oA8isu/c23bqtG7a2Y7WVmJ0P9xsD"
+            b"jNqXQzbqn3CnlNjXiTIelssQhWWgGPN62ipcrq7wePP8A+5qA43Kk0MLJINHozu"
+            b"MzzkcNwugUWtsFvymu4dJPFB6Mx4SYnFh/xvus2Xnz8hY8HXKZs2W8cv/ihI6We"
+            b"u0eSNzFFbOlDtTeBP0FOEbKEKIjsQzIQcyA/evuRPMRTBPohq9YMAAAEUAAAADH"
+            b"JzYS1zaGEyLTUxMgAAAQCYbbNzhflDqZAxyBpdLIX0nLAdnTeFNBudMqgo3KGND"
+            b"WlU9N17hqBEmcvIOrtNi+JKuKZW89zZrbORHvdjv6NjGSKzJD/XA25YrX1KgMEO"
+            b"wt5pzMZX+100drwrjQo+vZqeIN3FJNmT3wssge73v+JsxQrdIAz7YM2OZrFr5HM"
+            b"qZEZ5tMvAf/s5YEMDttEU4zMtmjubQyDM5KyYnZdoDT4sKi2rB8gfaigc4IdI/K"
+            b"8oXL/3Y7rHuOtejl3lUK4v6DxeRl4aqGYWmhUJc++Rh0cbDgC2S6Cq7gAfG2tND"
+            b"zbwL217Q93R08bJn1hDWuiTiaHGauSy2gPUI+cnkvlEocHM"
         )
 
     @pytest.mark.supported(
@@ -1745,6 +1784,11 @@ def test_sign_and_byte_compare_ed25519(self, monkeypatch, backend):
             .valid_after(1672531200)
             .valid_before(1672617600)
             .type(SSHCertificateType.USER)
+            .add_extension(b"permit-pty", b"")
+            .add_critical_option(
+                b"force-command", b"echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
+            )
+            .add_critical_option(b"verify-required", b"")
         )
         cert = builder.sign(private_key)
         sig_key = cert.signature_key()
@@ -1754,8 +1798,11 @@ def test_sign_and_byte_compare_ed25519(self, monkeypatch, backend):
             b"ssh-ed25519-cert-v01@openssh.com AAAAIHNzaC1lZDI1NTE5LWNlcnQtdj"
             b"AxQG9wZW5zc2guY29tAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
             b"AAAAAAAINdamAGCsQq31Uv+08lkBzoO4XLz2qYjJa8CGmj3B1EaAAAAAAAAAAAA"
-            b"AAABAAAAAAAAAAAAAAAAY7DNAAAAAABjsh6AAAAAAAAAAAAAAAAAAAAAMwAAAAt"
-            b"zc2gtZWQyNTUxOQAAACDXWpgBgrEKt9VL/tPJZAc6DuFy89qmIyWvAhpo9wdRGg"
-            b"AAAFMAAAALc3NoLWVkMjU1MTkAAABAAlF6Lxabxs+8fkOr7KjKYei9konIG13cQ"
-            b"gJ2tWf3yFcg3OuV5s/AkRmKdwHlQfTUrhRdOmDnGxeLEB0mvkVFCw=="
+            b"AAABAAAAAAAAAAAAAAAAY7DNAAAAAABjsh6AAAAAWAAAAA1mb3JjZS1jb21tYW5"
+            b"kAAAALAAAAChlY2hvIGFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYW"
+            b"FhAAAAD3ZlcmlmeS1yZXF1aXJlZAAAAAAAAAASAAAACnBlcm1pdC1wdHkAAAAAA"
+            b"AAAAAAAADMAAAALc3NoLWVkMjU1MTkAAAAg11qYAYKxCrfVS/7TyWQHOg7hcvPa"
+            b"piMlrwIaaPcHURoAAABTAAAAC3NzaC1lZDI1NTE5AAAAQL2aUjeD60C2FrbgHcN"
+            b"t8yRa8IRbxvOyA9TZYDGG1dRE3DiR0fuudU20v6vqfTd1gx0S5QyEdECXLl9ZI3"
+            b"AwZgc="
         )

vectors/cryptography_vectors/asymmetric/OpenSSH/certs/p256-ed25519-non-singular-crit-opt-val.pub

@@ -0,0 +1 @@
+ecdsa-sha2-nistp256-cert-v01@openssh.com AAAAKGVjZHNhLXNoYTItbmlzdHAyNTYtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAIbmlzdHAyNTYAAABBBCZWRs4GYIHGJpyXuqvfFGWN49dnJRkZJLDkFrHf6mNHhIMI3vtrLfCZwxPSfnCYWK6YofssZ1FYA6TkVJq8Xi8AAAAAAAAAAAAAAAEAAAAAAAAAAAAAAABjsM0AAAAAAGOyHoAAAABtAAAADWZvcmNlLWNvbW1hbmQAAABBAAAAKGVjaG8gYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWEAAAARaW52YWxpZF9taXNjX2RhdGEAAAAPdmVyaWZ5LXJlcXVpcmVkAAAAAAAAABIAAAAKcGVybWl0LXB0eQAAAAAAAAAAAAAAMwAAAAtzc2gtZWQyNTUxOQAAACDdZgztgAFFC7T5PifrUy/kMu0Pnwq1au3vStKHe7FFMAAAAFMAAAALc3NoLWVkMjU1MTkAAABAt/0pBSDBFy1crBPHOBoKFoxRjKd1tKVdOrD3QVgbBfpaHfxi4vrgYe6JfQ54+vu5P+8yrMyACekT8H6hhvxHDw==

vectors/cryptography_vectors/asymmetric/OpenSSH/certs/p256-ed25519-non-singular-ext-val.pub

@@ -0,0 +1 @@
+ecdsa-sha2-nistp256-cert-v01@openssh.com AAAAKGVjZHNhLXNoYTItbmlzdHAyNTYtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAIbmlzdHAyNTYAAABBBCZWRs4GYIHGJpyXuqvfFGWN49dnJRkZJLDkFrHf6mNHhIMI3vtrLfCZwxPSfnCYWK6YofssZ1FYA6TkVJq8Xi8AAAAAAAAAAAAAAAEAAAAAAAAAAAAAAABjsM0AAAAAAGOyHoAAAAAXAAAAD3ZlcmlmeS1yZXF1aXJlZAAAAAAAAAAvAAAAFGNvbnRhaW5zLWV4dHJhLXZhbHVlAAAAEwAAAAVoZWxsbwAAAAYgd29ybGQAAAAAAAAAMwAAAAtzc2gtZWQyNTUxOQAAACDdZgztgAFFC7T5PifrUy/kMu0Pnwq1au3vStKHe7FFMAAAAFMAAAALc3NoLWVkMjU1MTkAAABAY80oIEvooz/k3x9a+yVkjSNRfi4y/q87wVYiT7keTpP4n9JV/Vlc0u7O2QYOHfb4DUkcrvbsksKVsiqoQu5qDg==