rust-openssl is vulnerable to the Marvin Attack

[tomato42]

I've reviewed the code responsible for doing RSA PKCS#1v1.5 decryption:

rust-openssl/openssl/src/encrypt.rs

Lines 440 to 453 in a644ec2

	pub fn decrypt(&self, from: &[u8], to: &mut [u8]) -> Result<usize, ErrorStack> {
	let mut written = to.len();
	unsafe {
	cvt(ffi::EVP_PKEY_decrypt(
	self.pctx,
	to.as_mut_ptr(),
	&mut written,
	from.as_ptr(),
	from.len(),
	))?;
	}
	Ok(written)
	}

and I'm pretty sure that it is vulnerable to the Marvin Attack as it will perform a jump/branch based on the error value returned from OpenSSL.

If you'd like to perform actual test for the leakage (to confirm the review and measure the size of the side-channel), I can run the test, but I'd like to ask for help in writing a test harness. Example test harnesses are available in the marvin-toolkit repo, the one for rust-crypto would most likely be the easiest one to adapt for this package.

May I also ask for assigning a CVE to this issue? As a repo owner you can create a security issue here in github and ask for a CVE assignment.

[alex]

This crate is a fairly minimal wrapper around OpenSSL. I'm not sure there's any way to branchless-ly handle the OpenSSL error stack.

As a result, I'm not sure it makes sense to consider this project as impacted, or not. Rather the underlying OpenSSL that is used is either impacted or not.

[tomato42]

This crate is a fairly minimal wrapper around OpenSSL. I'm not sure there's any way to branchless-ly handle the OpenSSL error stack.

there is:
https://github.com/tomato42/marvin-toolkit/blob/master/example/openssl/time_decrypt_legacy.c#L235-L240
you simply ignore the error stack

As a result, I'm not sure it makes sense to consider this project as impacted, or not. Rather the underlying OpenSSL that is used is either impacted or not.

unfortunately no, I've tested openssl extensively, and the OpenSSL API can be used safely, it's just very hard and unintuitive to do

[alex]

ERR_pop_to_mark is variable time in the number of errors on the stack.

[tomato42]

ERR_pop_to_mark is variable time in the number of errors on the stack.

and you base this statement on what? because I have tested it and verified that it is not

[alex]

Reading it: https://github.com/openssl/openssl/blob/master/crypto/err/err_mark.c#L42-L60

[tomato42]

If you think this comment in openssl is incorrect, feel free to raise an issue in openssl project: https://github.com/openssl/openssl/blob/master/crypto/rsa/rsa_pk1.c#L266-L272

but I have verified that the ERR_pop_to_mark doesn't have a side-channel leakage that depends on PKCS#1 padding being correct or not. If you don't believe me, feel free to either test, or debug it yourself. I don't have time to do it myself and explain line by line why it's not leaky, sorry.

[tomato42]

CVE-2024-3296 has been assigned for this issue.

[Shnatsel]

I understand a proof of concept has not yet been demonstrated for this issue, so its existence is still disputed. As such no version with a fix has been published.

I believe the best next step is to wire up this function to the marvin-toolkit test suite, and the test harness for rust-crypto could serve as an example.

[tomato42]

@Shnatsel yes, if somebody would create a test harness for rust-openssl, I should be able to rather quickly run it and provide conclusive evidence for existence of leakage.

[sfackler]

This is unambiguously a problem in OpenSSL, and has apparently been fixed since 3.2.0:

Since version 3.2.0, the EVP_PKEY_decrypt() method when used with PKCS#1 v1.5 padding doesn't return an error in case it detects an error in padding, instead it returns a pseudo-randomly generated message, removing the need of side-channel secure code from applications using OpenSSL.

[tomato42]

@sfackler it's not a problem in OpenSSL. If you use OpenSSL 3.2 with a PKCS#11 module you still have a problem

[alex]

Surely that's a problem in the PKCS#11 module then?

As I've expressed before, this problem cannot be properly solved external to OpenSSL, because all of these APIs involving manipulating the error stack, for which there are no constant time APIs.

[tomato42]

because all of these APIs involving manipulating the error stack

You have provided no evidence what so ever that this is actually happening. I have tested it in practice, and have found that openssl does manipulate error stack in a way that is safe.

So, it being an issue in applications that call the API is not just my opinion about it, it's also OpenSSL's official stance.

[alex]

Notwithstanding your inability to measure it, the branches exist.

If the standard is "can it be measured", that's fine. But no one has measured this issue with rust-openssl!

[simo5]

@alex in fairness every other user like rust-openssl we measured does show a leak, I do not think there are rel doubts we can show it.
Do you insist on having a measurement first?

[tomato42]

Notwithstanding your inability to measure it, the branches exist.

if a branch does not depend on secret data, then it does not matter.

The standard is not "can it be measured", as I'm measuring it to precision of individual clock cycles. If I see that there is no data dependence larger than 0.2 ns and I'm running the test on a machine with a 2GHz CPU, then there is no side-channel leakage.

[alex]

My view is that the point of fixing this in OpenSSL was to avoid every single consumer needing to attempt to mitigate it.

That's the approach we took in pyca/cryptography, and IMO it's the only reasonable approach for a general purpose cryptography library.

I do not understand why that's insufficient.

[tomato42]

My view is that the point of fixing this in OpenSSL was to avoid every single consumer needing to attempt to mitigate it.

Then your view is incorrect. That's not why I proposed the fix. The reason why I proposed the fix was to protect as many users as possible with reasonable amount of effort.

That's the approach we took in pyca/cryptography, and IMO it's the only reasonable approach for a general purpose cryptography library.

I do not understand why that's insufficient.

pyca/cryptography recognised it as a bug in pyca/cryptography, with the design of the API being the cause of the issue, and documented the API as insecure

I see no attempt to do the same here.

[alex]

I'd personally be fine documenting that PKCS1v15 decryption may be vulnerable to timing variability, and that no one should use PKCS1v15 encryption/decryption in 2024.

[tomato42]

I'd be ok with the fix being "document the API as insecure, with recommendation against its use, and recommendation to use OpenSSL with implicit rejection as a workaround for deployments that absolutely require support for PKCS#1v1.5 decryption still"

[Shnatsel]

The API can also be marked as deprecated so that a warning about it is surfaced to the API users.

[sfackler]

We are not deprecating EVP_Decrypt. That is what literally all EVP-based decryption with any key/padding type use.

[tomato42]

@Shnatsel if you use it for OAEP then it's fine, it's the PKCS#1v1.5 decryption that's problematic

[Shnatsel]

Is there a way to warn users of the PKCS#1v1.5 alone? Perhaps a #[deprecated] attribute on the Padding enum variant? Rust reference states that #[deprecated] can be applied to enum variants.

[tomato42]

I'm pretty sure that PKCS#1v1.5 is the default (it is for the openssl itself)

[alex]

There's a small subtly, which is that PKCS1v15 is usable for signing and encryption, and this issue is only relevant to the encryption use case.

…

On Fri, Apr 5, 2024 at 1:29 PM Shnatsel ***@***.***> wrote: Is there a way to warn users of the PKCS#1v1.5 alone? Perhaps a #[deprecated] attribute on the Padding enum variant? Rust reference <https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute> states that #[deprecated] can be applied to enum variants. — Reply to this email directly, view it on GitHub <#2171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBCM7BYJX2OSVXLJHLDY33NQJAVCNFSM6AAAAABDIHJKUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQGMYDKNRQHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

[Shnatsel]

I suppose a #[deprecated] attribute could be attached to it only if the enums used for encryption and decryption are different types. But I believe that would be a semver-breaking change, so it would not be picked up by existing users.

[sfackler]

The goal of this crate is to implement direct, memory safe bindings to OpenSSL APIs. Some of those APIs are poorly designed, but that is frankly OpenSSL's problem, not my problem.

[tomato42]

I've proposed changes to the OpenSSL documentation to make some of my statements here part of official documentation: openssl/openssl#24159