Avoid side channel leakage when decoding private data

[ctz]

Had this on the back burner for a while.

This introduces a base64 decoder into this crate, which is customised to:

1. exactly match what need for PEM, including skipping whitespace as required by RFC7468.
2. have a side-channel-free mode for processing private data, heavily inspired by the code in boringssl.

(1) means we can remove the whitespace skipping we do beforehand. (2) is bought into play only when parsing sections that contain private keys.

This PR also improves benchmarking, to see what effect switching away from rust-base64 had -- it is approx 60% slower for certificate-only workloads.

Another option would be to retain the rust-base64 dependency, but restore the whitespace stripping just before calling into it.

[djc]

If we're going to use our own base64 implementation, should we inline this directly into pki-types instead of using the separate crate? (We could use an off-by-default Cargo feature depending on the compile-time impact of this code.)

[ctz]

Yeah I think that would be reasonable. We could also publish a final version of this crate which is just pub use pki_types::pem::*; or something?

[djc]

That depends on what the API in pki-types would look like, I guess? Was thinking we might more closely align the API to the new types, but I suppose we might want to close the side channel for current rustls-pemfile 2.x users?

[cpu]

Was thinking we might more closely align the API to the new types, but I suppose we might want to close the side channel for current rustls-pemfile 2.x users?

Maybe something like:

1. Put the base64 decoder code into pki-types
2. Switch the 2.x release here to use it
3. Move the pemfile API/types into pki-types
4. Cut a 3.x release here that cuts over entirely to pki-types

WDYT about also extending this commit-set to add a test that decodes the webpki-root-certs trusted roots with both decoders and asserts equivalence of the decoded DER? (Taking a dev-dependency on webpki-root-certs).

[djc]

1. Put the base64 decoder code into pki-types
2. Switch the 2.x release here to use it

That means rustls-pki-types 1.x would forever be stuck with a sort-of general purpose base64 decoding API, which seems suboptimal?

[cpu]

That means rustls-pki-types 1.x would forever be stuck with a sort-of general purpose base64 decoding API, which seems suboptimal?

Fair point.

In that case I think we probably need to decide between not fixing the side channel in the 2.x release stream or leaving pemfile separate from pki-types. The latter seems more appealing to me.

[ctz]

How about:

1. merge this and cut another 2.x release, which would be the final one
2. move the base64 decoder up to pki-types
3. use it to implement some more idiomatic apis (eg, fn private_key here would be better termed PrivateKeyDer::try_from_pem() or some such)
4. move the remainder of this crate (which i expect would be the more general iterator-based API, but perhaps not the standalone functions) into pki_types::pem
5. move all our callers over

[djc]

I definitely think it's possible to have our cake and eat it, too. I was thinking something like this:

1. Design idiomatic APIs for pki-types first, backed by the new decoder
2. Design additional lower-level surface we need in pki_types::pem to support current rustls-pemfile API
3. "Rebase" current rustls-pemfile on top of the new pki_types::pem API

IMO getting a nice PEM API for pki-types will be most important for the long-term, so IMO that should be first on the list. I think we'll want to retain Item (though I'd probably name it PemObject) anyway, so we can hang a bunch of API off of that in pki_types::pem.

I was thinking something like this:

trait FromPem {
    fn from_pem(rd: &mut impl BufRead) -> Result<Self, Error>;
}

Like in my rustls-native-certs PR, we could opt to implement a custom error type instead of yielding io::Error and wrapping PEM-specific errors in it. Might also add a method with default impl like from_pem_str() into the trait.

[cpu]

@djc I like this plan. Assuming Ctz does too is it something you want to execute yourself or would it be helpful to get the ball rolling? I think I'll have some capacity after finishing a rustls-ffi release.

[djc]

@djc I like this plan. Assuming Ctz does too is it something you want to execute yourself or would it be helpful to get the ball rolling? I think I'll have some capacity after finishing a rustls-ffi release.

Would be helpful if someone else can get the ball rolling!

[ctz]

I will have a go at proposing a starting point on the pki-types repo.

[ctz]

▶️ rustls/pki-types#53