Skip to content

ssh-encoding: normalize non-canonical mpint encodings when decoding - #593

Open
feigeCode wants to merge 1 commit into
RustCrypto:masterfrom
feigeCode:feat/lenient-mpint-decoding
Open

feigeCode wants to merge 1 commit into
RustCrypto:masterfrom
feigeCode:feat/lenient-mpint-decoding

Conversation

@feigeCode

Copy link
Copy Markdown

Decode for Mpint normalizes redundant leading zero bytes instead of rejecting them.

RFC 4251 § 5 requires redundant leading 0x00 bytes to be omitted from mpint
encodings, but some SSH implementations send them anyway. One observed case: an older
Huawei VRP switch puts a redundant leading zero into the RSA exponent and modulus of its
ssh-rsa host key, which makes the whole host key unparseable — the connection cannot be
established at all, even after negotiating a legacy KEX.

OpenSSH tolerates this on the receiving side. sshbuf_get_bignum2_bytes_direct()
(sshbuf-getput-basic.c) trims leading zeros rather than rejecting the message:

	/* Refuse negative (MSB set) bignums */
	if ((len != 0 && (*d & 0x80) != 0))
		return SSH_ERR_BIGNUM_IS_NEGATIVE;
	/* Refuse overlong bignums, allow prepended \0 to avoid MSB set */
	if (len > SSHBUF_MAX_BIGNUM + 1 ||
	    (len == SSHBUF_MAX_BIGNUM + 1 && *d != 0))
		return SSH_ERR_BIGNUM_TOO_LARGE;
	/* Trim leading zeros */
	while (len > 0 && *d == 0x00) {
		d++;
		len--;
	}

Note that the "negative" check inspects the first byte before trimming, so any value
that starts with 0x00 is accepted regardless of what follows.

A redundant leading zero cannot change the interpretation of the value, so how strict a
reader is here is a policy question rather than a correctness one. #290/#291 already
accepted the mirror image of this — "Leading zeros must be stripped from mpint values
(per RFC 4251)" — so tolerating extra leading zeros on the receiving side seems
consistent.

Changes:

  • Decode for Mpint normalizes the value: redundant leading 0x00 bytes are stripped,
    and a single 0x00 is re-added when the remaining value is positive with its MSB set,
    i.e. the result is always canonically encoded. Negative values and already-canonical
    encodings are preserved byte for byte.
  • TryFrom<Box<[u8]>> (and therefore Mpint::from_bytes) is intentionally left strict,
    so RFC 4251 still governs values constructed locally, and the existing
    reject_extra_leading_zeroes contract is unchanged. Only the wire parsing path is
    lenient.
  • Tests: four new cases in ssh-encoding (normalization, zero, preservation of
    canonical/negative encodings, normalized value equals the canonical value) plus an
    end-to-end case in ssh-key that parses a non-canonical ssh-rsa public key blob.

Reproducer note: we could not capture the exact bytes from the device, but building an
ssh-rsa blob with e = 00 01 00 01 and n = 00 00 80 01 reproduces the reported
failure verbatim (ssh_key::Error::Encoding(MpintEncoding)), which is exactly what the
ssh-key regression test covers.

Checks run locally, all passing:

cd ssh-encoding && cargo test --all-features --release
cargo test --workspace --all-features
cargo clippy --all-features --all-targets
cargo fmt --all -- --check
cargo doc --workspace --all-features --no-deps

RFC 4251 § 5 requires redundant leading `0x00` bytes to be omitted, but some
SSH implementations send them anyway: older Huawei network devices, for
example, place a redundant leading zero in the RSA exponent or modulus of their
`ssh-rsa` host key, which previously made the whole key unparseable.

OpenSSH tolerates this on the receiving side (`sshbuf_get_bignum2_bytes_direct`
trims leading zeros rather than rejecting the message), so normalize the value
in `Decode for Mpint` instead of failing. Negative values and already-canonical
encodings are preserved verbatim.

`Mpint::from_bytes` still rejects redundant leading zeros, so locally
constructed values keep being held to RFC 4251.

Signed-off-by: 胡飞 <1835698775@qq.com>

This branch has not been deployed

No deployments
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 this pull request may close these issues.

1 participant