From 65b8f37c34cd80680693e813e0081cdafaf58324 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 28 Mar 2026 19:25:24 +0000 Subject: [PATCH] Remove bogus assertion in ecc_weierstrass_add. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment next to the assertion says that it's checking for either identical inputs or mutually inverse ones: that is, input curve points with the same affine x-coordinate. But in that case we should have checked lambda_n, the _numerator_ of the slope of the line through the input points. Instead we were accidentally checking lambda_d, the denominator. ecc_weierstrass_add_general gets this right: it checks lambda_n to decide whether to use the output of the unequal-point addition formula or the doubling formula. The obvious fix is to replace the assertion with one checking lambda_n. But that doesn't work, because it's possible during legitimate operation to cause a call to ecc_weierstrass_add that would fail the fixed version of the assertion! Because of constant-time considerations, every time ecc_weierstrass_multiply consumes a bit of the exponent, it calculates both of two possible outputs of that iteration step, and then selects between them. And sometimes one of those outputs _does_ involve passing mutually inverse values to ecc_weierstrass_add. That's harmless if the result of that calculation is thrown away by the caller – except that it's not harmless if you fail an assertion and crash before getting back _to_ the caller! (With the current style of exponentiation loop, the case that would fail the 'right' assertion involves raising a point P to its own order minus 1, because we're consuming the exponent from the MSB downwards, so at every stage we choose between P^(2k) and P^(2k+1). But switching to the LSB-upwards style of repeatedly squaring to make P^{2^n} and conditionally multiplying in each of those, you'd still have the same failure, just in a different place – the failing case would involve raising P to the power of its own order with the leading 1 bit removed.) So that's why we can't replace the bogus assertion with a better one. Instead, the only thing to do is remove the assertion completely, and rely on callers to have already checked that exponents are in range (hence the precautionary fix in ecdsa_public in the previous commit). What about triggering the _actual_ wrong assertion? Guido Vranken reported this issue and included a test case that demonstrates that it's possible to trigger the bug deliberately. His example case is a P256 curve point P such that 10P and 11P have the same y-coordinate, and therefore trying to compute 21P will add those values together and fail the previous assertion. Worse, it's easy to construct a malicious P256 public key and signature such that attempting to verify that signature provokes the same crash – no matter what the message is. So this is a DoS vector for an on-path attacker, who can make PuTTY crash by substituting this fixed P256 key and signature in the cleartext initial key exchange of any SSH connection. (Only a DoS, though: PuTTY is always compiled with assertions enabled, so it won't do anything _worse_ than crash with an unhelpful message.) Upstream: https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=65b8f37c34cd80680693e813e0081cdafaf58324 CVE: CVE-2026-48852 [thomas: backport and remove tests] Signed-off-by: Thomas Perale --- crypto/ecc-arithmetic.c | 6 ------ 1 files changed, 6 deletions(-) diff --git a/crypto/ecc-arithmetic.c b/crypto/ecc-arithmetic.c index ed2abf39..23695369 100644 --- a/crypto/ecc-arithmetic.c +++ b/crypto/ecc-arithmetic.c @@ -308,12 +308,6 @@ WeierstrassPoint *ecc_weierstrass_add(WeierstrassPoint *P, WeierstrassPoint *Q) ecc_weierstrass_add_prologue( P, Q, &Px, &Py, &Qx, &denom, &lambda_n, &lambda_d); - /* Never expect to have received two mutually inverse inputs, or - * two identical ones (which would make this a doubling). In other - * words, the two input x-coordinates (after putting over a common - * denominator) should never have been equal. */ - assert(!mp_eq_integer(lambda_n, 0)); - /* Now go to the common epilogue code. */ ecc_weierstrass_epilogue(Px, Qx, Py, denom, lambda_n, lambda_d, S); -- 2.54.0