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?

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

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.

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.

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)

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 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: https://github.com/rust-lang-nursery/rust-bindgen/issues/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.

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.

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.

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.

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.

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.

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.

UPDATE: ASan has been implemented in https://github.com/google/mundane/commit/0148297bf1c5bb8ccc8acbc7abcc15cb4b53d6ee. 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.

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.