Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using OpenSSL-license-licensed code. #1827

Open
tisonkun opened this issue Nov 27, 2023 · 8 comments
Open

Stop using OpenSSL-license-licensed code. #1827

tisonkun opened this issue Nov 27, 2023 · 8 comments

Comments

@tisonkun
Copy link

tisonkun commented Nov 27, 2023

Currently, the LICENSE file is large and complicated, while can be vaguely regarded as a combination of MIT, ISC, and OpenSSL’s licenses.

Although the latest OpenSSL license is Apache License 2.0, in this repo, we use a few of sentences refer to the BSD-style licenses with advertising clauses:

ring/LICENSE

Lines 66 to 83 in 6c29bf6

* 3. All advertising materials mentioning features or use of this
* software must display the following acknowledgment:
* "This product includes software developed by the OpenSSL Project
* for use in the OpenSSL Toolkit. (http://www.openssl.org/)"
*
* 4. The names "OpenSSL Toolkit" and "OpenSSL Project" must not be used to
* endorse or promote products derived from this software without
* prior written permission. For written permission, please contact
* [email protected].
*
* 5. Products derived from this software may not be called "OpenSSL"
* nor may "OpenSSL" appear in their names without prior written
* permission of the OpenSSL Project.
*
* 6. Redistributions of any form whatsoever must retain the following
* acknowledgment:
* "This product includes software developed by the OpenSSL Project
* for use in the OpenSSL Toolkit (http://www.openssl.org/)"

ring/LICENSE

Lines 137 to 145 in 6c29bf6

* 3. All advertising materials mentioning features or use of this software
* must display the following acknowledgement:
* "This product includes cryptographic software written by
* Eric Young ([email protected])"
* The word 'cryptographic' can be left out if the rouines from the library
* being used are not cryptographic related :-).
* 4. If you include any Windows specific code (or a derivative thereof) from
* the apps directory (application code) you must include an acknowledgement:
* "This product includes software written by Tim Hudson ([email protected])"

It causes concerns in downstream for using this software in a mindset like so-called "permissive OSS license" or "weak copyleft license": https://lists.apache.org/thread/ptwdv18z4wd9r11nmdwj7wgzwvm3b8l2

@briansmith do you have more background of the license content, or how generally a downstream user use it?

The background of this questions is from https://www.apache.org/legal/resolved.html#category-x that "BSD-4-Clause/BSD-4-Clause (University of California-Specific)" can introduce burden for users to convey this software - they're, be required, to include extra acknowledgement for certern actions. And while if we can either:

  1. use an Apache License 2.0 base, or;
  2. be clear that what part of this crate is not covered by these licenses.
@briansmith
Copy link
Owner

I believe it is already clear which parts of the crate are covered by which license as each source file has a license header at the top. The code in question comes from BoringSSL and they didn't do the relicensing. Unfortunately they (BoringSSL) put some important bits in OpenSSL-license-licensed header files. Our goal is to replace all the OpenSSL-license-licensed code as we move completely away from C to Rust. Once that is done there won't be any issue regarding this anymore.

See also #902 which tracks making the licensing use SPDX.

@briansmith briansmith changed the title Revisit license requirement Stop using OpenSSL-license-licensed code. Nov 28, 2023
@tisonkun
Copy link
Author

@briansmith Thanks for your reply. I have one more question:

it is already clear which parts of the crate are covered by which license

For a takeaway, are those files OpenSSL-license-licensed included in any downstream software that use ring? Or it's optional if certain feature unused.

I did a check and it seems the fileset is:

  • crypto/constant_time_test.c
  • crypto/cpu_intel.c
  • crypto/internal.h
  • crypto/mem.c
  • crypto/fipsmodule/bn/internal.h
  • crypto/fipsmodule/bn/montgomery.c
  • include/ring-core/aes.h
  • include/ring-core/arm_arch.h
  • include/ring-core/base.h
  • include/ring-core/mem.h
  • include/ring-core/type_check.h

You can check if there is other missing, and perhaps we can divide and conquer them :D

@briansmith
Copy link
Owner

crypto/constant_time_test.c

This is test code that shouldn't be linked into the ring library. Fixing this is tracked in #1705. I would appreciate a PR.

crypto/cpu_intel.c

I have sent some patches upstream to BoringSSL which, when they are all merged, will allow us to more easily replace cpu_intel.c with Rust code.

crypto/fipsmodule/bn/internal.h
crypto/fipsmodule/bn/montgomery.c

  • We need to replace crypto/ec/*.c with Rust code. This will eliminate the need for bn/internal.h.
  • We need to replace the Montgomery reduction function in montgomery.c with a Rust version. This will eliminate montgomery.c.

include/ring-core/aes.h

We need to vendor the rust-crypto aes/soft code at https://github.com/RustCrypto/block-ciphers/tree/master/aes/src/soft into src/third_party/rust_crypto/aes/soft, replacing aes_nohw.c. Then we can remove aes_nohw.c and aes.h.

include/ring-core/mem.h

  • Easy: Rename fe_isnonzero in curve25519.c to fe_isnonzero_vartime. Update the caller. Change fe_isnonzero_vartime to do a variable-time comparison instead of CRYPTO_memcmp. Stop including mem.h and remove mem.h.

include/ring-core/base.h
include/ring-core/type_check.h

crypto/mem.c

  • We need to come up with a reliable implementation of value_barrier_w in Rust or assembly. Then we can rewrite crypto/mem.c in Rust, using the value barrier.

crypto/internal.h

  • Do all the above work, which will reduce the amount of code from this header that we use.
  • Delete the dead code from this header after doing the above,
  • Rewrite the remaining C code in Rust, using new constant-time utilities written in Rust,

include/ring-core/arm_arch.h

We probably need to work with BoringSSL upstream to relicense this since it is used by the assembly code.

@briansmith
Copy link
Owner

include/ring-core/aes.h
We need to vendor the rust-crypto aes/soft code at https://github.com/RustCrypto/block-ciphers/tree/master/aes/src/soft into src/third_party/rust_crypto/aes/soft, replacing aes_nohw.c. Then we can remove aes_nohw.c and aes.h.

This is now tracked as #1886 with more details.

@tisonkun
Copy link
Author

Thanks a lot for the updates @briansmith!

For crypto/mem.c, I found src/constant_time.rs uses CRYPTO_memcmp also:

/// Returns `Ok(())` if `a == b` and `Err(error::Unspecified)` otherwise.
/// The comparison of `a` and `b` is done in constant time with respect to the
/// contents of each, but NOT in constant time with respect to the lengths of
/// `a` and `b`.
pub fn verify_slices_are_equal(a: &[u8], b: &[u8]) -> Result<(), error::Unspecified> {
    if a.len() != b.len() {
        return Err(error::Unspecified);
    }
    let result = unsafe { CRYPTO_memcmp(a.as_ptr(), b.as_ptr(), a.len()) };
    match result {
        0 => Ok(()),
        _ => Err(error::Unspecified),
    }
}

prefixed_extern! {
    fn CRYPTO_memcmp(a: *const u8, b: *const u8, len: c::size_t) -> c::int;
}

Maybe we compare a[i] != b[i] iteratively?

Can we replace it with variable-time comparison? And it seems a common use case to do CRYPTO_memcmp, I wonder if we can "reimplement it" in a different way like:

int CRYPTO_memcmp(const void *in_a, const void *in_b, size_t len) {
  const unsigned *a = in_a;
  const unsigned *b = in_b;
  for (unsigned i = 0; i < len; i++) {
    if (a[i] ^ b[i]) return 1;
  }
  return 0;
}

or just use memcmp.

@briansmith
Copy link
Owner

PR #2070 and PR #1899, and @tisonkun's PR #1893, amongst other small changes, are making progress on this.

The refactoring to remove crypto/cpu_intel.c is well underway.

@nyabinary
Copy link

Yeah, it makes using the crate practically impossible in GPL licensed stuff:
https://www.gnu.org/licenses/license-list.en.html#OriginalBSD
Nice to see progress being made on this tho :3

@SDAChess
Copy link

If there is anything I can help with regarding this effort; let me know I would love to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants