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

Connection::dangerous_extract_secrets returns ConnectionTrafficSecrets::Aes128Gcm even when AES-256-GCM is negotiated #1833

Closed
1 task done
Arnavion opened this issue Mar 2, 2024 · 0 comments · Fixed by #1834

Comments

@Arnavion
Copy link
Contributor

Arnavion commented Mar 2, 2024

Checklist

  • I've searched the issue tracker for similar bugs.

Describe the bug
With the ring provider, when a connection is negotiated with TLS 1.2 and ciphersuite using AES-256-GCM, conn.dangerous_extract_secrets() should return tx and rx that contain ConnectionTrafficSecrets::Aes256Gcm { ... }. Instead they contain ConnectionTrafficSecrets::Aes128Gcm { ... } (with the correct 32-bit AeadKey).

Bug is at

Ok(ConnectionTrafficSecrets::Aes128Gcm {

... introduced in 0.22 with the switch to the AeadKey type.

To Reproduce
Steps to reproduce the behavior:

rustls = { version = "0.23", features = ["ring"] }
webpki-roots = "0.26"
fn main() {
    use rustls::{ClientConfig, ClientConnection, ConnectionTrafficSecrets, ExtractedSecrets, RootCertStore, Stream, crypto::{ring as rustls_ring, CryptoProvider}};

    let hostname = "www.google.com";

    let cipher_suites = vec![
        rustls_ring::cipher_suite::TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
        rustls_ring::cipher_suite::TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
    ];

    let crypto_provider = CryptoProvider {
        cipher_suites,
        ..rustls_ring::default_provider()
    };
    crypto_provider.install_default().unwrap();

    let root_store = RootCertStore {
        roots: webpki_roots::TLS_SERVER_ROOTS.to_owned(),
    };

    let mut config =
        ClientConfig::builder_with_protocol_versions(&[&rustls::version::TLS12])
        .with_root_certificates(root_store)
        .with_no_client_auth();
    config.enable_secret_extraction = true;
    let config = std::sync::Arc::new(config);

    let name = hostname.try_into().unwrap();

    let mut conn = ClientConnection::new(config, name).unwrap();
    let mut sock = std::net::TcpStream::connect("www.google.com:443").unwrap();
    let mut tls = Stream::new(&mut conn, &mut sock);
    std::io::Write::write_all(&mut tls, b"\r\n").unwrap();
    let protocol_version = conn.protocol_version().unwrap();
    println!(
        "using {protocol_version:?} {:?}",
        conn.negotiated_cipher_suite(),
    );
    let ExtractedSecrets { tx, rx } = conn.dangerous_extract_secrets().unwrap();
    match tx.1 {
        ConnectionTrafficSecrets::Aes256Gcm { key, .. } => {
            assert_eq!(key.as_ref().len(), 32);
            println!("tx is ConnectionTrafficSecrets::Aes256Gcm with key {:?}B key", key.as_ref().len());
        },

        ConnectionTrafficSecrets::Aes128Gcm { key, .. } => panic!("tx is ConnectionTrafficSecrets::Aes128Gcm with key {:?}B key", key.as_ref().len()),

        ConnectionTrafficSecrets::Chacha20Poly1305 { key, .. } => panic!("tx is ConnectionTrafficSecrets::Chacha20Poly1305 with key {:?}B key", key.as_ref().len()),

        _ => unreachable!(),
    }
    match rx.1 {
        ConnectionTrafficSecrets::Aes256Gcm { key, .. } => {
            assert_eq!(key.as_ref().len(), 32);
            println!("rx is ConnectionTrafficSecrets::Aes256Gcm with key {:?}B key", key.as_ref().len());
        },

        ConnectionTrafficSecrets::Aes128Gcm { key, .. } => panic!("rx is ConnectionTrafficSecrets::Aes128Gcm with key {:?}B key", key.as_ref().len()),

        ConnectionTrafficSecrets::Chacha20Poly1305 { key, .. } => panic!("rx is ConnectionTrafficSecrets::Chacha20Poly1305 with key {:?}B key", key.as_ref().len()),

        _ => unreachable!(),
    }
}

Expected:

using TLSv1_2 Some(TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384)
tx is ConnectionTrafficSecrets::Aes256Gcm with key 32B key
rx is ConnectionTrafficSecrets::Aes256Gcm with key 32B key

Actual:

using TLSv1_2 Some(TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384)
thread 'main' panicked at src/main.rs:46:60:
tx is ConnectionTrafficSecrets::Aes128Gcm with key 32B key

Applicable Version(s)
rustls v0.23.1

Expected behavior
Explained above.

Additional context
Workaround is to ignore the enum type and branch on key.as_ref().len()

github-merge-queue bot pushed a commit that referenced this issue Mar 3, 2024
… negotiated.

55bb279 inadvertently changed `extract_keys`
to always return `ConnectionTrafficSecrets::Aes128Gcm`, even when AES-256-GCM
was negotiated. This change fixes it by restoring the key length check.

Fixes #1833
@ctz ctz closed this as completed in #1834 Mar 3, 2024
cpu pushed a commit to cpu/rustls that referenced this issue Mar 11, 2024
… negotiated.

55bb279 inadvertently changed `extract_keys`
to always return `ConnectionTrafficSecrets::Aes128Gcm`, even when AES-256-GCM
was negotiated. This change fixes it by restoring the key length check.

Fixes rustls#1833
ctz pushed a commit that referenced this issue Mar 25, 2024
… negotiated.

55bb279 inadvertently changed `extract_keys`
to always return `ConnectionTrafficSecrets::Aes128Gcm`, even when AES-256-GCM
was negotiated. This change fixes it by restoring the key length check.

Fixes #1833
ctz pushed a commit that referenced this issue Mar 25, 2024
… negotiated.

55bb279 inadvertently changed `extract_keys`
to always return `ConnectionTrafficSecrets::Aes128Gcm`, even when AES-256-GCM
was negotiated. This change fixes it by restoring the key length check.

Fixes #1833
github-merge-queue bot pushed a commit that referenced this issue Mar 25, 2024
… negotiated.

55bb279 inadvertently changed `extract_keys`
to always return `ConnectionTrafficSecrets::Aes128Gcm`, even when AES-256-GCM
was negotiated. This change fixes it by restoring the key length check.

Fixes #1833
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant