Skip to content

md: fix return values of EVP_DigestVerify(Final) #2449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

md: fix return values of EVP_DigestVerify(Final) #2449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a5ac8c
error: add clear function
huwcbjones Aug 26, 2025
58abb9b
md: add helper to covnert result of EVP_DigestVerifyFinal and EVP_Dig…
huwcbjones Aug 26, 2025
58b5c5b
md: fix return values of EVP_DigestVerifyFinal and EVP_DigestVerify
huwcbjones Aug 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions openssl/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ impl ErrorStack {
error.put();
}
}

/// Clear the error stack
pub(crate) fn clear() {
unsafe { ffi::ERR_clear_error() }
}
}

impl ErrorStack {
Expand Down
50 changes: 24 additions & 26 deletions openssl/src/md_ctx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,30 +301,32 @@ impl MdCtxRef {
Ok(len)
}

fn convert_verify_result(r: ffi::c_int) -> Result<bool, ErrorStack> {
// https://docs.openssl.org/3.2/man3/EVP_DigestVerifyInit/#return-values
match r {
// EVP_DigestVerifyFinal() and EVP_DigestVerify() return 1 for success...
1 => Ok(true),
// A return value of zero indicates that the signature did not verify successfully
0 => {
// Clear the error stack so debugging any subsequent errors is not confusing
ErrorStack::clear();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Ok(false)
}
// while other values indicate a more serious error
_ => Err(ErrorStack::get()),
}
}

/// Verifies the provided signature.
///
/// Returns `Ok(true)` if the signature is valid, `Ok(false)` if the signature is invalid, and `Err` if an error
/// occurred.
#[corresponds(EVP_DigestVerifyFinal)]
#[inline]
pub fn digest_verify_final(&mut self, signature: &[u8]) -> Result<bool, ErrorStack> {
unsafe {
let r = ffi::EVP_DigestVerifyFinal(
self.as_ptr(),
signature.as_ptr() as *mut _,
signature.len(),
);
if r == 1 {
Ok(true)
} else {
let errors = ErrorStack::get();
if errors.errors().is_empty() {
Ok(false)
} else {
Err(errors)
}
}
}
Self::convert_verify_result(unsafe {
ffi::EVP_DigestVerifyFinal(self.as_ptr(), signature.as_ptr() as *mut _, signature.len())
})
}

/// Computes the signature of the data in `from`.
Expand Down Expand Up @@ -377,16 +379,15 @@ impl MdCtxRef {
#[cfg(ossl111)]
#[inline]
pub fn digest_verify(&mut self, data: &[u8], signature: &[u8]) -> Result<bool, ErrorStack> {
unsafe {
let r = cvt(ffi::EVP_DigestVerify(
Self::convert_verify_result(unsafe {
ffi::EVP_DigestVerify(
self.as_ptr(),
signature.as_ptr(),
signature.len(),
data.as_ptr(),
data.len(),
))?;
Ok(r == 1)
}
)
})
}

/// Returns the size of the message digest, i.e. the size of the hash
Expand Down Expand Up @@ -433,10 +434,7 @@ mod test {

ctx.digest_verify_init(Some(md), &key1).unwrap();
ctx.digest_verify_update(bad_data).unwrap();
assert!(matches!(
ctx.digest_verify_final(&signature),
Ok(false) | Err(_)
));
assert!(!ctx.digest_verify_final(&signature).unwrap());
assert!(ErrorStack::get().errors().is_empty());
}

Expand Down
Loading