On Thu 2023-11-16 20:49:49 -0500, Daniel Kahn Gillmor wrote: > The attached patch runs the valgrind tests during build, but i also note > that it causes a build failure on amd64 platforms, because of what > appears to be data-dependent branching during RSA decryption. I've > raised that concern on the upstream nettle-bugs mailing list (and Cc'ed > Magnus) to try to figure out what we should do to avoid this negative > result. So it's probably not safe yet to just apply this patch > unilaterally (at least not in unstable -- maybe in experimental for now > to get records from the various build daemons that build experimental?)
After discussion on the nettle-bugs mailing list, It looks like the most
straightforward way to avoid the valgrind failure in
rsa-sec-decrypt-test is just to not mark the ciphertext as
undefined. This allows the test code to pass even though there's a
ciphertext-dependent branching path based on ensuring that the
ciphertext is well-formed.
The patch below allows the build to complete, with the valgrind tests,
on the amd64 arch. I haven't tested it on all the other architectures,
but one approach would be to make an upload to debian experimental, with
just these changes, to see how the experimental build daemons deal with
it.
Magnus, If you're OK with my doing that, please let me know, and i can
do an NMU.
--dkg
From 3adecbf7df68e64cfcf2a85224a008d9c33adfd5 Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor <[email protected]> Date: Wed, 15 Nov 2023 12:45:19 -0500 Subject: [PATCH] Run tests under valgrind during build (Closes: #1056107) To make sure we don't introduce build failures, we also avoid valgrind-derived confusion over branch predictions based on RSA ciphertext. --- debian/control | 2 +- ...failure-when-branching-on-ciphertext.patch | 54 +++++++++++++++++++ debian/patches/series | 1 + debian/rules | 3 ++ 4 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 debian/patches/0002-Avoid-valgrind-failure-when-branching-on-ciphertext.patch diff --git a/debian/control b/debian/control index 766e4110..b27ae5ab 100644 --- a/debian/control +++ b/debian/control @@ -3,7 +3,7 @@ Section: libs Priority: optional Maintainer: Magnus Holmgren <[email protected]> Build-Depends: dpkg-dev (>= 1.15.7), debhelper-compat (= 12), - libgmp-dev, m4, texinfo + libgmp-dev, m4, texinfo, valgrind <!nocheck> Standards-Version: 4.6.2 Vcs-Git: https://salsa.debian.org/holmgren/nettle.git Vcs-Browser: https://salsa.debian.org/holmgren/nettle diff --git a/debian/patches/0002-Avoid-valgrind-failure-when-branching-on-ciphertext.patch b/debian/patches/0002-Avoid-valgrind-failure-when-branching-on-ciphertext.patch new file mode 100644 index 00000000..a9e8bf72 --- /dev/null +++ b/debian/patches/0002-Avoid-valgrind-failure-when-branching-on-ciphertext.patch @@ -0,0 +1,54 @@ +From: Daniel Kahn Gillmor <[email protected]> +Date: Mon, 20 Nov 2023 19:21:42 -0500 +Subject: Avoid valgrind failure when branching on ciphertext +MIME-Version: 1.0 +Content-Type: text/plain; charset="utf-8" +Content-Transfer-Encoding: 8bit + +In Message-ID: [email protected], Niels Möller +acknowledged that this was a bug due to evolution of the tooling after +the tests were written: + +> Hi, that's a bug, let me give some background. +> +> Valgrind can be used to test for side channel silence, or more +> precisely, branches or memory addresses depending on secret data, by +> telling valgrind to treat the secret data as "undefined". I added logic +> to do that to some tests, including rsa-sec-decrypt-test, automatically +> enabled if the test is run under valgrind. +> +> But then that test was was broken in a later fix to add more input +> validation. +[…] +> For the input validation in rsa_sec_decrypt, since the cryptotext c +> is presumably known by the attacker, it should not be a problem if +> the comparison c < n leaks information about it. But then maybe the +> side-channel test shouldn't mark the cryptotext input as secret at +> all, only the private key? + +This modification simply avoids marking the ciphertext as secret to +avoid triggering an error on the valgrind tests. +--- + testsuite/rsa-sec-decrypt-test.c | 2 -- + 1 file changed, 2 deletions(-) + +diff --git a/testsuite/rsa-sec-decrypt-test.c b/testsuite/rsa-sec-decrypt-test.c +index 3419322..ade6161 100644 +--- a/testsuite/rsa-sec-decrypt-test.c ++++ b/testsuite/rsa-sec-decrypt-test.c +@@ -28,7 +28,6 @@ rsa_decrypt_for_test(const struct rsa_public_key *pub, + mpn_sec_powm may leak information about the least significant + bits of p and q, due to table lookup in binvert_limb. */ + VALGRIND_MAKE_MEM_UNDEFINED (message, length); +- MARK_MPZ_LIMBS_UNDEFINED(gibberish); + MARK_MPZ_LIMBS_UNDEFINED(key->a); + MARK_MPZ_LIMBS_UNDEFINED(key->b); + MARK_MPZ_LIMBS_UNDEFINED(key->c); +@@ -41,7 +40,6 @@ rsa_decrypt_for_test(const struct rsa_public_key *pub, + + VALGRIND_MAKE_MEM_DEFINED (message, length); + VALGRIND_MAKE_MEM_DEFINED (&ret, sizeof(ret)); +- MARK_MPZ_LIMBS_DEFINED(gibberish); + MARK_MPZ_LIMBS_DEFINED(key->a); + MARK_MPZ_LIMBS_DEFINED(key->b); + MARK_MPZ_LIMBS_DEFINED(key->c); diff --git a/debian/patches/series b/debian/patches/series index f492a836..4e8e004b 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -1 +1,2 @@ fPIC.patch +0002-Avoid-valgrind-failure-when-branching-on-ciphertext.patch diff --git a/debian/rules b/debian/rules index 3a59ce2b..ff708b22 100755 --- a/debian/rules +++ b/debian/rules @@ -11,3 +11,6 @@ override_dh_installdocs: override_dh_auto_configure: dh_auto_configure -- --enable-fat + +execute_after_dh_auto_test: + dh_auto_test -- EMULATOR='$$(VALGRIND)' -- 2.42.0
signature.asc
Description: PGP signature
