md: fix return values of EVP_DigestVerify(Final)

[huwcbjones]

Previously they'd always error and never return Ok(false) when the signature failed to validate.

[alex]

This silently ignores errors if they're on teh stack -- digest_verify_final deliberately tried to preserve these if they existed.

[huwcbjones]

Yeah, however you can't use these APIs to power the ECDSA sign/verify API and have the ECDSA tests pass (see huwcbjones@1108ddb).
Also, if we don't clear the error stack in the Ok(true) case, then the assertion that ensures the error stack is clear fails!
Hence me trying to avoid changing the test code (as much as possible)!

[alex]

I'm not sure I understand: if OpenSSL puts an error on the stack, we want to return that to the user, not silently ignore it.

[huwcbjones]

EcdsaSigRef.verify currently uses ECDSA_do_verify. This func is deprecated, however the return values from rust function are as follows:

• Ok(true): sig OK
• Ok(false): sig invalid
• Err(_): some other error occurred
  There are tests for both Ok(true) and Ok(false) cases.

To verify an ECDSA signature in a non-deprecated way, the answer is to use EVP_Digest* APIs. There's already a wrapper for these APIs in md_ctx. The doc string for digest_verify[_final] states:

rust-openssl/openssl/src/md_ctx.rs

Lines 372 to 373 in 6a87a2c

	/// Returns `Ok(true)` if the signature is valid, `Ok(false)` if the signature is invalid, and `Err` if an error
	/// occurred.

So naturally this sounds like an ideal drop-in replacement for ECDSA_do_verify.

The docs for EVP_DigestVerify say that 1=sig OK, 0= sig invalid, else=some other error.
However, when you get a 0 (sig invalid) rc, openssl will also (maybe) put an error on the stack saying invalid sig.
So the current implementation of digest_verify_final can never return Ok(false) as documented because the error stack (at least in the RSA case) is never empty! And for digest_verify, it will also never return Ok(false) because cvt will throw an Err at a non-1 rc, so Ok(rc==1) will always evaulate to true.

The test verify_fail signs the string "Some Crypto Text" with a key, then verifies the signature against "Some Crypto text" (lower case T). From the ossl docs, we should get a return code of 0 because "the signature did not verify successfully (that is, tbs [data to be verified] did not match the original data...)".
If you fix the test

diff --git a/openssl/src/md_ctx.rs b/openssl/src/md_ctx.rs
index 0182907ad..10e0ffe5a 100644
--- a/openssl/src/md_ctx.rs
+++ b/openssl/src/md_ctx.rs
@@ -435,7 +435,7 @@ mod test {
         ctx.digest_verify_update(bad_data).unwrap();
         assert!(matches!(
             ctx.digest_verify_final(&signature),
-            Ok(false) | Err(_)
+            Ok(false)
         ));
         assert!(ErrorStack::get().errors().is_empty());
     }

The test now fails because it gets an error, even though the return code is 0. Case in-point, running the test with the following diff applied:

diff --git a/openssl/src/md_ctx.rs b/openssl/src/md_ctx.rs
index 0182907ad..4a3ddeb22 100644
--- a/openssl/src/md_ctx.rs
+++ b/openssl/src/md_ctx.rs
@@ -317,6 +317,7 @@ impl MdCtxRef {
             if r == 1 {
                 Ok(true)
             } else {
+                println!("r={r}");
                 let errors = ErrorStack::get();
                 if errors.errors().is_empty() {
                     Ok(false)

Gives you

r=0

assertion failed: matches!(ctx.digest_verify_final(&signature), Ok(false))
thread 'md_ctx::test::verify_fail' panicked at openssl/src/md_ctx.rs:437:9:
assertion failed: matches!(ctx.digest_verify_final(&signature), Ok(false))

This change fixes the implementation to match what's documented and as a side-effect clears the error stack in the case that the signature is invalid, because the verify_fail test explicits tests to make sure the error stack is clear after digest_verify_final is called (regardless of the return value).

Going back to how I got here, as digest_verify_final never returns Ok(false) in the case where the signature is not valid for the given data, I cannot use the MdCtx sig verification functions as written to replace the deprecated ECDSA_do_verify because the tests fail because Ok(false) is never returned (the existing API contract would be broken).

[huwcbjones]

Now with that said and all the reading code I've done, there's also another implementation of the EVP_Digest* APIs in sign.rs that does exactly what I need and implements the return value checking from EVP_DigestVerifyFinal correctly (and it older than the MdCtx impl)!

rust-openssl/openssl/src/sign.rs

Lines 530 to 543 in 6a87a2c

	pub fn verify(&self, signature: &[u8]) -> Result<bool, ErrorStack> {
	unsafe {
	let r =
	EVP_DigestVerifyFinal(self.md_ctx, signature.as_ptr() as *mut _, signature.len());
	match r {
	1 => Ok(true),
	0 => {
	ErrorStack::get(); // discard error stack
	Ok(false)
	}
	_ => Err(ErrorStack::get()),
	}
	}
	}

^ that's literally what I wrote in this PR without even seeing it! 🤣 🤦🏻‍♂️

Anyhow, I've got another way to do what I want, so I'll leave it to you whether want to take this change, or want the docs fixing.