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?

To err on the safe side, we initially made our key objects neither Send nor Sync. However, BoringSSL provides concurrency semantics for key objects that allow us to relax these restrictions. In particular, key objects' reference counts are incremented and decremented atomically, and all BoringSSL operations on keys have well-defined concurrency safety semantics.

I propose that we implement Sync on key objects, and use BoringSSL's definition of which functions are mutating to decide which methods to have take a &mut self vs a &self receiver.

An aside on Clone and reference counting

Currently, keys are reference counted, and cloning a Mundane key object obtains a new reference-counted reference to the same underlying object. Unfortunately, we cannot implement either Send or Sync so long as it's possible to obtain multiple references to the same underlying object.

If reference-counted key objects are Send, then different clones could be sent to different threads, and we'd have no way of preventing those two separate clones from being operated on using &mut self methods concurrently, which would be unsound.

If reference-counted key objects are Sync, then different clones owned by one thread could be accessed concurrently from different threads, and we'd have no way of preventing those two separate clones from being operated on using &mut self methods concurrently, which would be unsound.

Thus, I conclude that we must remove the ability to clone key objects. The primary benefit to reference counting in BoringSSL is to be able to use keys concurrently from multiple threads at once. Since Rust's lifetime system allows us to share references across threads safely, we get the same advantage even without reference counting. Even if reference counting were desired, we could put a Mundane key object inside of an Rc or an Arc and get the same effect.

Thus, the concrete tasks are:

• Remove the ability to Clone key objects
• Audit the codebase to ensure that all mutating BoringSSL functions are exposed via &mut self methods, and document our strategy and reasoning in code comments
• Implement Send and Sync on key objects

Old (incorrect) text:

BoringSSL key types are reference-counted, and use reference counting to implement Clone. While the reference counting itself is thread-safe (see CRYPTO_refcount_xxx, crypto/internal.h), it's not clear that all operations on keys are also thread-safe. In other words, having two key objects in different threads which are both references to the same underlying BoringSSL object may mean that calling methods on those objects concurrently is unsound. As a result, our key objects do not implement Send.

Eventually, we will want to identify which methods are thread-safe and which are not. This is not only a prerequisite for making our key objects Send, it's also a prerequisite for making them Sync. However, we can much more easily unblock making our key objects Send by just not implementing Clone so that a given key object is always the only reference to its underlying BoringSSL object.

Per @davidben's comment here:

I think you should probably think about what things you want to support and build an API around that.

• What flavors of RSA private keys do you believe in? Just CRT? n/e/d as well?
• If you support multiple, do you want to jam them into one type or separate types? (Separate might make more sense given only CRT is serializable.)

Read the entire comment thread for more context.

We should answer these questions, document them clearly in our RSA code, and then update the organization of our RSA implementation as appropriate.

The submodule at boringssl/boringssl should pin to a particular commit hash. The fact that it doesn't is a bug.

This may have been a conscious choice to avoid but I wanted to see if it makes sense to use some type that implements AsRef<[u8]> in place of &[u8]? This would make it easy for users to supply &str or String in addition to &[u8] when calling functions. I thought this would be in line with the idea that mundane is intended to be easy to use.

UPDATE: ASan has been implemented in 0148297. We should still run tests with MSan, although it's less important than ASan.

=== old text ===

We should enable ASan and MSan when running cargo test in order to catch issues with our use of the BoringSSL API. ASan should help catch issues with reference counting, allocation, and freeing, while MSan should help catch issues with memory initialization.

It would be useful to be able to use Hashers and Hmacs in order to hash values that implement the core's Hash trait. In order to do that, Mundane's Hasher trait and Hmac type should implement the core's Hasher trait.

I suspect that, since our Hasher trait is not Clone and its finish method takes self by value, we can't just do a blanket impl - impl<H: Hasher> core::hash::Hasher for H { ... }. There would be no way to implement that trait's finish method, which takes self by immutable reference. Instead, we'll likely need to add a core::hash::Hasher bound to our Hasher trait.

For the Hmac type, it should be more straightforward.

Now that we're using docs.rs to host our documentation, we need to add metadata to the Cargo.toml file to instruct docs.rs to use all build features when building the documentation. By default, it just uses the default features, meaning that we get no documentation for things like the insecure feature.

For details on how to do this, see the "Metadata for custom builds" section here. In particular, we'll want a section like this:

[package.metadata.docs.rs]
features = [ "bytes", "insecure", "..." ]

In total, we'll want the following features documented:

• bytes
• insecure
• kdf
• rsa-pkcs1v15

We'll also want to add a comment on the [features] line in Cargo.toml reminding developers to update the [package.metadata.docs.rs] section when they update the [features] section.

Currently, we have a few small smoke tests to make sure that we don't decrement BoringSSL refcounts too far. However, we have no tests to ensure that we decrement refcounts far enough (in other words, that we don't leak resources by leaving unused objects allocated and constructed).

We should also test the latter. The way to do this is probably to figure out how to inspect the refcount on a BoringSSL object and use that mechanism to ensure that the refcount on these objects is 1 when we only hold a single reference. That said, perhaps there's a better way to do this.

EDIT: Let's first try to see if we can enable ASan (#15), which should address this.

Currently, in the public module, marshaling and parsing of public and private keys is handled by bare functions which take DerPublicKey or DerPrivateKey trait bounds. The API evolved this way from an older version in which all public and private keys were assumed to be DER-encodable. Given the current API, it probably makes more sense to have parsing and marshaling be methods on those traits rather than bare functions.

Our CONTRIBUTING.md documentation on submitting code was copied from BoringSSL's. As such, it assumes that you know to pull from the googlesource.com repo. Since Mundane is also available on GitHub, it's less obvious in our case.

Concretely, we should document that submissions should be pushed to https://fuchsia.googlesource.com/mundane. It's probably easiest to document that by suggesting the following command to clone: git clone --recursive https://fuchsia.googlesource.com/mundane.

Hello, I recently had the pleasure of building your crate when I noticed that the dependencies may be out of date. It would appear that mundane currently requires cmake 3.3 or higher:

(Paths redacted below)

Compiling mundane v0.4.4
error: failed to run custom build command for `mundane v0.4.4`

Caused by:
  process didn't exit successfully: `....release/build/mundane-dd369fe2607dde56/build-script-main` (exit code: 101)
  --- stdout
  cargo:rerun-if-env-changed=....github.com-1ecc6299db9ec823/mundane-0.4.4
  cargo:rerun-if-env-changed=..../release/build/mundane-453aa1aaa20c68ee/out
  cargo:rerun-if-env-changed=0
  cargo:rerun-if-env-changed=4
  cargo:rerun-if-env-changed=4
  -- Configuring incomplete, errors occurred!

  --- stderr
  CMake Error at CMakeLists.txt:1 (cmake_minimum_required):
    CMake 3.3 or higher is required.  You are running version 2.8.12.2


  thread 'main' panicked at 'cmake failed with status exit code: 1', .../cargo/registry/src/github.com-1ecc6299db9ec823/mundane-0.4.4/build/main.rs:219:9
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Hello,

it looks like mundane is being actively developed, so I wonder whether are there any plans to support also TLS? If so, could you share some planned timelines?

Thanks,

Bartosz.

When compiling on Linux, I get the following linker error:

/usr/bin/ld: /home/vagrant/tmp/mundane/target/debug/build/mundane-9145b5e5c96cb8ea/out/boringssl/build_2/crypto/libcrypto_0_2_0.a(cbb.c.o): relocation R_X86_64_32 against `.rodata' can not be used when making a shared object; recompile with -fPIC
          /home/vagrant/tmp/mundane/target/debug/build/mundane-9145b5e5c96cb8ea/out/boringssl/build_2/crypto/libcrypto_0_2_0.a: error adding symbols: Bad value
          collect2: error: ld returned 1 exit status

This is surprising because, if I'm not mistaken, @erickt has successfully compiled on Linux before, implying that this is either a nondeterministic issue or has to do with my setup in particular.

Both Mundane and the better-established ring are Rust cryptographic libraries that have difficulty-of-misuse as an explicit goal and are based on BoringSSL. I would like to suggest that any significant differences between them be documented such that developers in the target audience (whichever that may be) can make an informed choice between these two libraries.

cc @briansmith

The docs.rs build for 0.4.4 failed (logs). I believe this is caused by rust-lang/docs.rs#1303.

Currently, our bindgen.sh script does not automatically generate the link_name attributes that we need in order to perform symbol renaming as described in boringssl/README.md. Instead, we do this manually, which is annoying.

We should do this automatically. One good option would be to implement the bindgen feature described in this issue: rust-lang/rust-bindgen#1375. It could also be good to have a simpler workaround until that feature is implemented.

Signature::verify takes a public key and a signature and returns a boolean. @zarvox has pointed out that it's not 100% obvious to somebody coming across code like if sig.verify(&pub) { ... } which return value means that the signature is valid and which means that it's invalid.

In order to make it more obvious, we should rename this method to is_valid.

Currently, there's no way to automatically test that the version number in link and link_name attributes in boringssl/boringssl.rs match the version number in Cargo.toml. This has already led to an issue: we published 0.2.1 with the version numbers in boringssl/boringssl.rs still at 0.2.0, forcing us to fix and release 0.2.2, and yank 0.2.1. We should add an automated test so this sort of issue can't slip through in the future.

Introduction

In later GCC versions some warning semantics have changed, causing build to fail (due to -Werror).

One example:

  /home/svt/.cargo/registry/src/github.com-1ecc6299db9ec823/mundane-0.4.4/boringssl/boringssl/crypto/fipsmodule/sha/sha512.c: In function ‘SHA512_256_Final’:
  /home/svt/.cargo/registry/src/github.com-1ecc6299db9ec823/mundane-0.4.4/boringssl/boringssl/crypto/fipsmodule/sha/sha512.c:179:10: error: ‘SHA512_Final’ accessing 64 bytes in a region of size 32 [-Werror=stringop-overflow=]
    179 |   return SHA512_Final(out, sha);

This could be unpleasant for people with rolling distros like archlinux that usually have significantly newer compiler versions.

How to reproduce

I have experienced the problem with GCC 11:

Using built-in specs.
COLLECT_GCC=/usr/bin/gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/11.1.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure --prefix=/usr --libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/ --enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++,d --with-isl --with-linker-hash-style=gnu --with-system-zlib --enable-__cxa_atexit --enable-cet=auto --enable-checking=release --enable-clocale=gnu --enable-default-pie --enable-default-ssp --enable-gnu-indirect-function --enable-gnu-unique-object --enable-install-libiberty --enable-linker-build-id --enable-lto --enable-multilib --enable-plugin --enable-shared --enable-threads=posix --disable-libssp --disable-libstdcxx-pch --disable-libunwind-exceptions --disable-werror gdc_include_dir=/usr/include/dlang/gdc
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.1.0 (GCC)

However, judging by the GCC change log - it is likely that GCC 10 would trigger same errors.

Some additional thoughts

There several easy short term solutions of course:

• Downgrade GCC
• Build an older version of GCC
• Check out Mundane locally and do something like "CFLAGS=-Wno-error ./configure" on the BoringSSL (hacky)

Open questions:

• Presumably individual block operations should be behind a feature flag since they're low level, but we still want to be able to expose high-level cipher modes (e.g., AES-GCM) via composition. How do we do that?
• What block cipher modes do we have to support?
• Do we want to build in padding?
  • Do we want to expose a non-padding variant as well?
  • How do we ensure that padding is only with safe block cipher modes (e.g., don't allow PKCS11 padding w/ CBC, which is vulnerable to a padding oracle attack)

I noticed this comment, the function sign() will both hashing and signing the message.

how can i sign the message without hashing ？

Looking forward to your reply ~

This is a low-level operation, and so should not be enabled by default. It probably makes sense to group it under a bytes module along with the existing rand_bytes function.

Open questions:

• What padding modes should we expose? Only PSS would be preferable, but that's probably not feasible
• If we expose different padding modes, how do we expose those in the API in a composable way?

https://docs.rs/crate/mundane/0.4.2/builds

[INFO] [stderr]  Documenting mundane v0.4.2 (/opt/rustwide/workdir)
[INFO] [stderr] error: `[RsaPkcs1v15]` cannot be resolved, ignoring it...
[INFO] [stderr]    --> src/public/rsa/mod.rs:363:26
[INFO] [stderr]     |
[INFO] [stderr] 363 | /// feature is enabled, [`RsaPkcs1v15`].
[INFO] [stderr]     |                          ^^^^^^^^^^^^^ cannot be resolved, ignoring
[INFO] [stderr]     |

========

Edit by @joshlf : This issue is now about both fixing this issue and also about documenting all items behind feature flags in general; see this comment below.

Currently, our error type can be printed as a string, which provides a lot of information about the error. Cryptography errors are a notorious source of information leak, as programmers often misuse the errors and either compute on them or expose them in ways that they shouldn't. Ring, for example, takes the much more aggressive approach of having an error type that contains no information at all.

I'm not sure what the right thing for us to do here is, since it seems useful to provide some error information, such as about parsing failures. But we need to strike the right balance, and exposing as much as we do now may be too much. This is also compounded by the fact that a lot of the errors we would want to expose (like parsing errors) come from BoringSSL, and BoringSSL's errors cannot be inspected programmatically.

In boringssl/bindgen.sh, we use the term "whitelist" to refer to lists of items that should be included by bindgen when generating bindings. For inclusivity reasons, we should use the term "allowlist" instead.

Currently, building Mundane requires Go 1.11+. Utilities in Go are used for three tasks:

1. Generating the interned error message bundle and its index (crypto/err_data.c in the build tree, from crypto/err/*.errordata in the source tree).

2. Extracting the names of public symbols from the archive created in the first library build (crypto/libcrypto.a) and saving the result in symbols.txt above the two build directories.

3. Generating the headers with substitutions from regular to versioned symbol names, using the result of the previous utility.

Those are rather simple programs and I have reimplemented them in Rust without much difficulty. Integrating them into build.rs could lead to elimination of Go as a build dependency of Mundane. While having any single dependency is not particularly onerous, I believe that minimizing their number eases the acceptance of a package, both for developers and end users. Even more so since Go 1.11 is not available on current LTS versions of popular distros.

Would there be any interest of pursuing this further?