ssh-key: fix ssh-dss signature encoding on 64-bit targets#4
Open
cwoolum wants to merge 1 commit into
Open
Conversation
Since switching from `BigUint::to_bytes_be()` to `BoxedUint::to_be_bytes()` in 07f295e, `Signer<Signature> 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
86ce6fd to
864a197
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs Eugeny/russh#705
Problem
Since
07f295e8("bump crypto packages"),Signer<Signature> for DsaKeypair::try_signproduces a 48-byte signature blob instead of the 40 bytes required by RFC 4253 §6.6 on 64-bit targets. Thedebug_assert_eq!(data.len(), DSA_SIGNATURE_SIZE)atsignature.rs:345panics in debug, and release builds emit an invalid signature that the peer rejects. This breaks ssh-dss publickey client auth in anyrussh0.60.x consumer (russh 0.60 pins=0.6.18).Cause
That commit migrated
r/sencoding fromBigUint::to_bytes_be()(minimum-length) toBoxedUint::to_be_bytes()(sized tobits_precision).dsaconstructsr/satKeySize::n_aligned()precision:For an ssh-dss key (
n = 160) on a 64-bit target (Limb::BITS = 64), this rounds up to192bits → 24 bytes per component, 48 bytes total.Fix
dsaenforcesr < qands < q, so the bytes above the trailing 20 are always zero — slice them off, and left-pad the rare case where the natural representation is shorter than 20 bytes:Also bumps
dsato0.7.0-rc.14, which fixes a verify-sideattempted to multiply with overflowpanic inverifying_key.rs:105for 1024-bitp(rc.14 usesconcatenating_mulinstead of*). Without that bump,DsaPublicKey::verifystill panics even with the encoding fix.Test
Reworked
try_sign_and_verify_dsato assert onsignature.as_bytes().len() == DSA_SIGNATURE_SIZEand roundtrip throughverify, instead of asserting on the underlyingdsacomponent byte lengths (the priorr.len() == 20assertion can't hold on 64-bit given the alignment above).Both data points from the original test (one where
ris naturally 160 bits, one where it's shorter and needs left-padding) are kept.