google / mundane

Mundane is a Rust cryptography library backed by BoringSSL that is difficult to misuse, ergonomic, and performant (in that order).
MIT License
1.07k stars 46 forks source link

Add BoringSSL exponent and modulus getters #28

Closed silvestrst closed 3 years ago

silvestrst commented 3 years ago

Background

We are planning on using mundane for OpenTitan image signing host tooling. One of the things that would be helpful to us is being able to retrieve a key public exponent and modulus from an RSA key.

It seems that mundane doesn't directly expose these fields, so additional BoringSSL API should be exposed: RSA_get0_n and RSA_get0_e

These functions could potentially be useful for other users of mundane.

Questions

  1. Would the upstream be happy with adding these getters (in which case I can put together a patch)?
  2. bindgen.sh expects version number as an input, what is the policy when updating the bindings? Should the version be bumped-up?
silvestrst commented 3 years ago

@joshlf , Miguel Young suggested to add you to this issue, thanks!

joshlf commented 3 years ago

Hi @silvestrst , in principle this seems fine to me. However, I have one question: How do you propose exposing the BIGNUM type in Rust? My immediate thought would be to convert to a type from the num-bigint crate, although I'm not sure how much work that would be.

Assuming we answer this question, I'd be happy to mentor on this!

silvestrst commented 3 years ago

It seems that BIGNUM is just a type alias for

boringssl/boringssl.rs:
...
pub struct bignum_st {
    pub d: *mut u64,
    pub width: ::std::os::raw::c_int,
    pub dmax: ::std::os::raw::c_int,
    pub neg: ::std::os::raw::c_int,
    pub flags: ::std::os::raw::c_int,
}
...

I was thinking on having a getter that returns d as &[u8] or something similar. Does this sound reasonable (I might be missing something)?

joshlf commented 3 years ago

Ah, I see. @davidben, is that structure API stable? Or should we be using getters instead?

davidben commented 3 years ago

Absolutely do not reach into that structure! You should treat it as opaque and not copy-paste it into Rust like that. BIGNUM especially is very sensitive w.r.t. constant-time. We've historically intentionally renamed fields (top to width) in the past alongside subtle semantics changes to intentionally flag and break anyone who was reaching into those fields.

If you need to convert the BIGNUM into some Rust bigint, use BN_bin2bn and BN_bn2bin / BN_bn2bin_padded to convert to and from byte strings. Note, however, that general-purpose integer types won't be safe for any private components of the key, so rope me in before extracting secrets like this.

silvestrst commented 3 years ago

Great, thank you @davidben and @joshlf! I will try to put something together next week.

silvestrst commented 3 years ago

A quick update: I have added RSA_get0_n, RSA_get0_e, BN_bn2bin and BN_num_bytes to Mundane. I have minimally tested it locally by building my changes and using the path to the local version of Mundane in the tool that I am building.

I have checked the modulus returned by the getters against the output that openssl bash command produce and they match.

I need to tidy-up (using newer version of bindgen which seems to add some unrelated changes, etc...) my implementation, and am planning to submit this change for a review in Gerrit in the first half of the next week.

For the reference, this is the change that I will tidy up and put-up in Gerrit: https://github.com/google/mundane/commit/30f76939c5e8d5ef454f9a69cfd97df55b20608d

joshlf commented 3 years ago

Hey @silvestrst , apparently our changes on Gerrit aren't getting propagated to GitHub. I'll investigate, but in the meantime, could you base your changes on the tip of master from Gerrit? https://fuchsia.googlesource.com/mundane

silvestrst commented 3 years ago

Hey @joshlf, thank you for the heads-up, will do! It is likely I will get around it at some point next week.

joshlf commented 3 years ago

@silvestrst has finished this in f03652687ab69a7db094bace501fb738053899db.

silvestrst commented 3 years ago

@joshlf thank you for all your thorough help on that PR!

joshlf commented 3 years ago

Of course!