digest_verify and digest_verify_final return Err with empty error stack, instead of Ok(false) as documented, on invalid signature

[reivilibre]

https://www.openssl.org/docs/man1.1.1/man3/EVP_DigestVerifyFinal.html says

EVP_DigestVerifyFinal() and EVP_DigestVerify() return 1 for success; any other value indicates failure. *A return value of zero indicates that the signature did not verify successfully (that is, tbs did not match the original data or the signature had an invalid form), while other values indicate a more serious error (and sometimes also indicate an invalid signature form).

However this crate, using the cvt function, maps a return code of zero to Err(ErrorStack::get())

• callsite of cvt:

  rust-openssl/openssl/src/md_ctx.rs

  Line 372 in 9f29412

  let r = cvt(ffi::EVP_DigestVerify(

• cvt:

  rust-openssl/openssl/src/lib.rs

  Lines 220 to 227 in 9f29412

  	#[inline]
  	fn cvt(r: c_int) -> Result<c_int, ErrorStack> {
  	if r <= 0 {
  	Err(ErrorStack::get())
  	} else {
  	Ok(r)
  	}
  	}

I could certainly be wrong, but it looks to me that this should be cvt_n instead, which only treats negative values as errors.

[sfackler]

Yeah, that sounds right, though the Ok(false) case is extremely unfortunate to have to remember to check for :(

From a usability perspective I might say we want to diverge from OpenSSL a bit and just return a Result<(), ErrorStack>, even for the 0 return case.

[reivilibre]

I can see that being desirable, but I also think that ErrorStack([]) is not a very good error message.
For diagnostic purposes it seems somewhat nice to know that the signature is of the correct shape but invalid, rather than e.g. the signature wasn't of the right length.
(I imagine the error stack would be non-empty in that case, but I have also seen at least one issue report on here where someone got a mysterious empty error stack for something, in their case due to an old OpenSSL version, but it therefore makes me less confident about assuming that an empty ErrorStack is the same as an invalid signature.)

From my perspective it might be nice to funnel the return value into ErrorStack or some sort of wrapper for ErrorStack so I can manually check for 0. The error messages would still not be human-grade, but at least accessible to a developer then.

[reivilibre]

heh, case in point: my error was passing in a base64-encoded signature to the verify function, so it seems that case causes an empty ErrorStack as well. Not sure what the return value is in that case.

EDIT: I added a debug line, it returned -1. So it is unfortunate to not be able to distinguish the two as a caller :-)

[sfackler]

I'd honestly be pretty surprised if OpenSSL would commonly if ever expose information about why a signature was invalid, since information leaks like that can be exploited. I might expect that the Err case is actually only for internal errors like a failure to allocate memory etc.

[reivilibre]

I suppose you're right; other than the length of the signature being wrong (i.e. the signature can't possibly be right and it's not worth trying to even check it), there are probably no other errors that can't be exploited.
So I may as well just check the length manually and then just assume any error is an invalid signature. This is probably sufficient for my case.