From 864a1971f2579bac79e3e90cfc4e82eeba78cf9c Mon Sep 17 00:00:00 2001 From: Chris Woolum Date: Mon, 11 May 2026 18:43:34 -0700 Subject: [PATCH] ssh-key: fix ssh-dss signature encoding on 64-bit targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since switching from `BigUint::to_bytes_be()` to `BoxedUint::to_be_bytes()` in 07f295e8, `Signer for DsaKeypair::try_sign` produces a 48-byte blob instead of the 40 bytes required by RFC 4253 ยง6.6, panicking the debug-assert at `signature.rs:345` and breaking ssh-dss publickey client auth in release builds. Root cause: `dsa::Signature::r()`/`s()` are constructed at `KeySize::n_aligned() == n.div_ceil(Limb::BITS) * Limb::BITS` precision. For ssh-dss (n = 160) on 64-bit targets that's 192 bits, so `to_be_bytes()` returns 24 bytes per component. Trim the limb-alignment leading zeros down to `DSA_SIGNATURE_SIZE / 2` before left-padding so values shorter than 160 bits still get padded. Also bump dsa to 0.7.0-rc.14 to pick up the verify-side overflow fix (`v1.concatenating_mul(&v2)` in `verifying_key.rs`); without it, `DsaPublicKey::verify` panics with "attempted to multiply with overflow" on 1024-bit p. Reworked the existing `try_sign_and_verify_dsa` test so it asserts on the encoded `signature.data.len() == DSA_SIGNATURE_SIZE` rather than on the underlying `dsa` component byte lengths -- the old assertion (`r.len() == 20`) couldn't hold on 64-bit given the alignment above. Refs Eugeny/russh#705 --- Cargo.lock | 4 +-- ssh-key/Cargo.toml | 2 +- ssh-key/src/signature.rs | 67 ++++++++++------------------------------ 3 files changed, 19 insertions(+), 54 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9377ebd8..065d2a7a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -356,9 +356,9 @@ dependencies = [ [[package]] name = "dsa" -version = "0.7.0-rc.13" +version = "0.7.0-rc.14" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4cc15b5f6b4f28237e692ecf3823bec0aa55aa528a0993a366831dcdcbfeb54c" +checksum = "205b3f37ceb87c2ed5c72dac75f286848c68b19ba3246e9196f3666ce048a14b" dependencies = [ "crypto-bigint", "crypto-primes", diff --git a/ssh-key/Cargo.toml b/ssh-key/Cargo.toml index 3a7b4bd2..d16d8031 100644 --- a/ssh-key/Cargo.toml +++ b/ssh-key/Cargo.toml @@ -28,7 +28,7 @@ crypto-bigint = { version = "0.7.0-rc.28", features = ["alloc"] } argon2 = { version = "0.5", optional = true, default-features = false, features = ["alloc"] } bcrypt-pbkdf = { version = "0.10", optional = true, default-features = false, features = ["alloc"] } bigint = { package = "num-bigint-dig", version = "0.8.4", optional = true, default-features = false, features = ["std"] } -dsa = { version = "0.7.0-rc.13", optional = true, default-features = false, features = ["hazmat"] } +dsa = { version = "0.7.0-rc.14", optional = true, default-features = false, features = ["hazmat"] } ecdsa = { version = "0.17.0-rc.16", optional = true, default-features = false } # only need this explicit dependency due to a minimal-versions fail @ https://github.com/RustCrypto/elliptic-curves/issues/1038 ed25519-dalek = { version = "3.0.0-pre.6", optional = true, default-features = false } hex = { version = "0.4", optional = true } diff --git a/ssh-key/src/signature.rs b/ssh-key/src/signature.rs index 7d0347ba..311f9b36 100644 --- a/ssh-key/src/signature.rs +++ b/ssh-key/src/signature.rs @@ -333,13 +333,19 @@ impl Signer for DsaKeypair { .map_err(|_| signature::Error::new())?; // Encode the format specified in RFC4253 section 6.6: two raw 80-bit integers concatenated - let mut data = Vec::new(); + let mut data = Vec::with_capacity(DSA_SIGNATURE_SIZE); + let half = DSA_SIGNATURE_SIZE / 2; for component in [signature.r(), signature.s()] { - let mut bytes = component.to_be_bytes(); - let pad_len = (DSA_SIGNATURE_SIZE / 2).saturating_sub(bytes.len()); - data.extend(iter::repeat(0).take(pad_len)); - data.extend(&bytes); + // `BoxedUint::to_be_bytes()` is sized to `bits_precision`, which + // `dsa` rounds up to a multiple of `Limb::BITS` (24 bytes on 64-bit + // for n = 160). Since `dsa` enforces `r < q` and `s < q`, any bytes + // above the trailing `half` are zero and can be sliced off. + let bytes = component.to_be_bytes(); + let bytes = &bytes[bytes.len().saturating_sub(half)..]; + let pad_len = half.saturating_sub(bytes.len()); + data.resize(data.len() + pad_len, 0); + data.extend_from_slice(bytes); } debug_assert_eq!(data.len(), DSA_SIGNATURE_SIZE); @@ -887,65 +893,24 @@ mod tests { use encoding::Decode as _; use signature::{Signer as _, Verifier as _}; - fn check_signature_component_lens( - keypair: &DsaKeypair, - data: &[u8], - r_len: usize, - s_len: usize, - ) { - use sha1::{Digest as _, Sha1}; - use signature::DigestSigner as _; - - let signature = dsa::SigningKey::try_from(keypair) - .expect("valid DSA signing key") - .try_sign_digest(|d: &mut Sha1| Ok(d.update(data))) - .expect("valid DSA signature"); - - let r = signature.r().to_be_bytes(); - assert_eq!( - r.len(), - r_len, - "dsa signature component `r` has len {} != {}", - r.len(), - r_len - ); - let s = signature.s().to_be_bytes(); - assert_eq!( - s.len(), - s_len, - "dsa signature component `s` has len {} != {}", - s.len(), - s_len - ); - } - let keypair = hex!("0000008100c161fb30c9e4e3602c8510f93bbd48d813da845dfcc75f3696e440cd019d609809608cd592b8430db901d7b43740740045b547c60fb035d69f9c64d3dfbfb13bb3edd8ccfdd44705739a639eb70f4aed16b0b8355de1b21cd9d442eff250895573a8af7ce2fb71fb062e887482dab5c68139845fb8afafc5f3819dc782920d510000001500f3fb6762430332bd5950edc5cd1ae6f17b88514f0000008061ef1394d864905e8efec3b610b7288a6522893af2a475f910796e0de47c8b065d365e942e80e471d1e6d4abdee1d3d3ede7103c6996432f1a9f9a671a31388672d63555077911fc69e641a997087260d22cdbf4965aa64bb382204f88987890ec225a5a7723a977dc1ecc5e04cf678f994692b20470adbf697489f800817b920000008100a9a6f1b65fc724d65df7441908b34af66489a4a3872cbbba25ea1bcfc83f25c4af1a62e339eefc814907cfaf0cb6d2d16996212a32a27a63013f01c57d0630f0be16c8c69d16fc25438e613b904b98aeb3e7c356fa8e75ee1d474c9f82f1280c5a6c18e9e607fcf7586eefb75ea9399da893b807375ac1396fd586bf277161980000001500ced95f1c7bbb39be4987837ad1f71be31bb7b0d9"); let keypair = DsaKeypair::decode(&mut &keypair[..]).expect("properly encoded DSA keypair"); let data = hex!("F0000040713d5f6fffe0000e6421ab0b3a69774d3da02fd72b107d6b32b6dad7c1660bbf507bf3eac3304cc5058f7e6f81b04239b8471459b1f3b387e2626f7eb8f6bcdd3200000006626c616465320000000e7373682d636f6e6e656374696f6e00000009686f73746261736564000000077373682d647373000001b2000000077373682d6473730000008100c161fb30c9e4e3602c8510f93bbd48d813da845dfcc75f3696e440cd019d609809608cd592b8430db901d7b43740740045b547c60fb035d69f9c64d3dfbfb13bb3edd8ccfdd44705739a639eb70f4aed16b0b8355de1b21cd9d442eff250895573a8af7ce2fb71fb062e887482dab5c68139845fb8afafc5f3819dc782920d510000001500f3fb6762430332bd5950edc5cd1ae6f17b88514f0000008061ef1394d864905e8efec3b610b7288a6522893af2a475f910796e0de47c8b065d365e942e80e471d1e6d4abdee1d3d3ede7103c6996432f1a9f9a671a31388672d63555077911fc69e641a997087260d22cdbf4965aa64bb382204f88987890ec225a5a7723a977dc1ecc5e04cf678f994692b20470adbf697489f800817b920000008100a9a6f1b65fc724d65df7441908b34af66489a4a3872cbbba25ea1bcfc83f25c4af1a62e339eefc814907cfaf0cb6d2d16996212a32a27a63013f01c57d0630f0be16c8c69d16fc25438e613b904b98aeb3e7c356fa8e75ee1d474c9f82f1280c5a6c18e9e607fcf7586eefb75ea9399da893b807375ac1396fd586bf2771619800000015746f6d61746f7373682e6c6f63616c646f6d61696e00000009746f6d61746f737368"); - check_signature_component_lens( - &keypair, - &data, - DSA_SIGNATURE_SIZE / 2, - DSA_SIGNATURE_SIZE / 2, - ); let signature = keypair.try_sign(&data[..]).expect("dsa try_sign is ok"); + assert_eq!(signature.as_bytes().len(), DSA_SIGNATURE_SIZE); keypair .public .verify(&data[..], &signature) .expect("dsa verify is ok"); + // This input produces an `r` shorter than 160 bits, exercising the + // left-pad path for naturally short signature components. let data = hex!("00000040713d5f6fffe0000e6421ab0b3a69774d3da02fd72b107d6b32b6dad7c1660bbf507bf3eac3304cc5058f7e6f81b04239b8471459b1f3b387e2626f7eb8f6bcdd3200000006626c616465320000000e7373682d636f6e6e656374696f6e00000009686f73746261736564000000077373682d647373000001b2000000077373682d6473730000008100c161fb30c9e4e3602c8510f93bbd48d813da845dfcc75f3696e440cd019d609809608cd592b8430db901d7b43740740045b547c60fb035d69f9c64d3dfbfb13bb3edd8ccfdd44705739a639eb70f4aed16b0b8355de1b21cd9d442eff250895573a8af7ce2fb71fb062e887482dab5c68139845fb8afafc5f3819dc782920d510000001500f3fb6762430332bd5950edc5cd1ae6f17b88514f0000008061ef1394d864905e8efec3b610b7288a6522893af2a475f910796e0de47c8b065d365e942e80e471d1e6d4abdee1d3d3ede7103c6996432f1a9f9a671a31388672d63555077911fc69e641a997087260d22cdbf4965aa64bb382204f88987890ec225a5a7723a977dc1ecc5e04cf678f994692b20470adbf697489f800817b920000008100a9a6f1b65fc724d65df7441908b34af66489a4a3872cbbba25ea1bcfc83f25c4af1a62e339eefc814907cfaf0cb6d2d16996212a32a27a63013f01c57d0630f0be16c8c69d16fc25438e613b904b98aeb3e7c356fa8e75ee1d474c9f82f1280c5a6c18e9e607fcf7586eefb75ea9399da893b807375ac1396fd586bf2771619800000015746f6d61746f7373682e6c6f63616c646f6d61696e00000009746f6d61746f737368"); - // verify that this data produces signature with `r` integer component that is less than 160 bits/20 bytes. - check_signature_component_lens( - &keypair, - &data, - DSA_SIGNATURE_SIZE / 2 - 1, - DSA_SIGNATURE_SIZE / 2, - ); let signature = keypair .try_sign(&data[..]) - .expect("dsa try_sign for r.len() == 19 is ok"); + .expect("dsa try_sign for short `r` is ok"); + assert_eq!(signature.as_bytes().len(), DSA_SIGNATURE_SIZE); keypair .public .verify(&data[..], &signature)