Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSH certificate encoding/parsing incompatibility with OpenSSH #9207

Closed
lkubb opened this issue Jul 10, 2023 · 4 comments · Fixed by #9208
Closed

SSH certificate encoding/parsing incompatibility with OpenSSH #9207

lkubb opened this issue Jul 10, 2023 · 4 comments · Fixed by #9208

Comments

@lkubb
Copy link
Contributor

lkubb commented Jul 10, 2023

Description
There is an encoding mismatch regarding critical options with values between ssh-keygen and cryptography:

  1. Loading an SSH certificate generated by ssh-keygen with cryptography does not yield expected values.
  2. Reading an SSH certificates generated by SSHCertificateBuilder with ssh-keygen fails.

Steps
Re 1:

  1. Generate a key to be signed: ssh-keygen -t ed25519
  2. Generate a CA signing key: ssh-keygen -t ecdsa
  3. Sign a certificate: ssh-keygen -s id_ed25519 -n cryptouser,testuser -I [email protected] -O "critical:force-command=echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" -O "critical:verify-required" -V 0x63b2b264:0x767eb5c0 id_ecdsa.pub
  4. Load the certificate with cryptography:
from cryptography.hazmat.primitives.serialization import (
    load_ssh_public_identity,
)

cert = load_ssh_public_identity(
    b"[email protected] AAAAKGVjZHNhLXNoYTItbm"
    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"
)

print(cert.critical_options[b"force-command"])
assert cert.critical_options == {
    b"force-command": b"echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
    b"verify-required": b"",
}
b'\x00\x00\x00(echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
Traceback (most recent call last):
  File "/home/user/test.py", line 21, in <module>
    assert cert.critical_options == {b"force-command": b"echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", b"verify-required": b""}
AssertionError

Re 2:

  1. Generate an SSH certificate with cryptography
from datetime import datetime, timedelta

from cryptography.hazmat.primitives.asymmetric import ed25519
from cryptography.hazmat.primitives.serialization import (
    SSHCertificateBuilder,
    SSHCertificateType,
)

ca_key = ed25519.Ed25519PrivateKey.generate()
sign_key = ed25519.Ed25519PrivateKey.generate()


cert = (
    SSHCertificateBuilder()
    .public_key(sign_key.public_key())
    .type(SSHCertificateType.USER)
    .valid_for_all_principals()
    .valid_before((datetime.now() + timedelta(days=1)).timestamp())
    .valid_after(datetime.now().timestamp())
    .add_critical_option(
        b"force-command", b"echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
    )
    .sign(ca_key)
)

with open("/tmp/sshcert.pub", "wb") as f:
    f.write(cert.public_bytes())
  1. Try to read it using ssh-keygen
$ cat /tmp/sshcert.pub
[email protected] AAAAIHNzaC1lZDI1NTE5LWNlcnQtdjAxQG9wZW5zc2guY29tAAAAICgEmbEZIZTs/K0UREvoo+eBXJGJhfZFzm+Q2CeCdBQbAAAAIJ5oypqQoA7gt+MJER8tlWwJgdpocFnOcdJHT2rPXvdwAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAZKv6HwAAAABkrUufAAAAPQAAAA1mb3JjZS1jb21tYW5kAAAAKGVjaG8gYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWEAAAAAAAAAAAAAADMAAAALc3NoLWVkMjU1MTkAAAAgGaQ+IQWx7X2AbbbWE4Ua2N7aW/uT16yt8n1bcr2BytwAAABTAAAAC3NzaC1lZDI1NTE5AAAAQGCWJZWlpr6/m/xwxQOcCfg73CUgjvqD1jsTDeepemxZRWhByJ44qdASQ0ykiuQjzGrigYaynuRbA4tuap0K3A4=
$ ssh-keygen -Lf /tmp/sshcert.pub
/tmp/sshcert.pub:
        Type: [email protected] user certificate
        Public key: ED25519-CERT SHA256:oxxO0zve78LqnyWIf3DRrbIzVoWXDK/FuBRerotDrHY
        Signing CA: ED25519 SHA256:HHnlyz4SqRKnCKwUVgN3hvJyKA8BQ5Hmy0SOsjgfnGQ (using ssh-ed25519)
        Key ID: ""
        Serial: 0
        Valid: from 2023-07-10T12:31:27 to 2023-07-11T12:31:27
        Principals: (none)
        Critical Options:
show_options: parse critical: string is too large

Background
The specification is a bit confusing in regards to how critical options/extensions with values should be encoded:

The critical options section of the certificate specifies zero or more
options on the certificates validity. The format of this field
is a sequence of zero or more tuples:

string       name
string       data

So that's the first header for the data. You then have to look at the
table to determine the format of data's contents. E.g.

force-command string Specifies a command that is executed
...

So, for name=force-command, the contents of the data buffer are a string.
That's the second header.

The intent is to support options/extensions with data types other
than string, e.g. integers or arrays of string.

Source

golang/crypto had the same issue: golang/go#10569

lkubb added a commit to lkubb/pyca-cryptography that referenced this issue Jul 10, 2023
reaperhulk pushed a commit that referenced this issue Jul 10, 2023
* Add tests for issue #9207

* Fix encoding of SSH certs with critical options

* Test unexpected additional values for crit opts/exts
@reaperhulk
Copy link
Member

reaperhulk commented Jul 10, 2023

A few more steps before we close this:

  • Backport to 41.0.x with warning for parsing the incorrect encoding (Backport ssh cert fix #9211)
  • Changelog entry to 42 stating we've dropped support (concurrent with forward porting the 41.0.2 changelog)

@reaperhulk reaperhulk reopened this Jul 10, 2023
reaperhulk pushed a commit to reaperhulk/cryptography that referenced this issue Jul 10, 2023
* Add tests for issue pyca#9207

* Fix encoding of SSH certs with critical options

* Test unexpected additional values for crit opts/exts
alex pushed a commit that referenced this issue Jul 11, 2023
* 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 <[email protected]>
@reaperhulk
Copy link
Member

Okay this is now released in 41.0.2 😄

s0undt3ch added a commit to s0undt3ch/salt that referenced this issue Jul 16, 2023
@michael-cfchan
Copy link

Is it correct that this issue affects cryptography>=40.0? The patched code appears to be introduced only in 40.0. Thanks!

@alex
Copy link
Member

alex commented Jul 29, 2023 via email

Shortfinga added a commit to Shortfinga/advisory-database that referenced this issue Aug 21, 2023
According to the CVE (https://nvd.nist.gov/vuln/detail/CVE-2023-38325)
the vulnerability only affects >=40.0. The GHSA
(GHSA-cf7p-gm2m-833m) also says this.  In
the issues this was asked and answered explicitly
(pyca/cryptography#9207 (comment)).
sethmlarson pushed a commit to pypa/advisory-database that referenced this issue Aug 21, 2023
According to the CVE (https://nvd.nist.gov/vuln/detail/CVE-2023-38325) the vulnerability only affects >=40.0. The GHSA (GHSA-cf7p-gm2m-833m) also says this.  In the issues this was asked and answered explicitly (pyca/cryptography#9207 (comment)).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants