Replace OpenSSL dependency with pinned BoringSSL #1599

spoonincode · 2023-09-06T01:38:28Z

Historically leap has used OpenSSL for TLS support and various crypto primitives (such as SHA256 and R1 key math). TLS support was removed a while ago leaving just the crypto primitives. Still, even with the rather limited usage, we'd like to avoid depending on system provided libraries for consensus bits, or at least "mathy" consensus bits, as these libraries may unexpectedly change some nuance out from under us resulting in consensus failure. A good example of such a change occurred in openssl 3.0.4 (that did not affect Leap).

Because we no longer directly depend on a TLS library as part of our build, we can pin these crypto primitives to a specific point in time without fear of TLS security flaws in the future. Using BoringSSL as a pinned drop in replacement appears to be a great fit. BoringSSL is designed to be simply submoduled in and static linked against. Also, BoringSSL is a fork of OpenSSL so its behavior and API is likely to be the same as existing OpenSSL usage making it less risky than wholesale replacement and refactor with a different library.

This PR builds on #1233 with some changes, notably:

We now maintain our own fork of BoringSSL. While using upstream directly is nice, the trade off of that approach is that it's harder to audit the changes to the Python generation script. Since #1277 + #1255 will require modifying BoringSSL anyways, the fork seems inevitable. Keep in mind that,

BoringSSL usage typically follows a “live at head” model. Projects pin to whatever the current latest of BoringSSL is at the time of update, and regularly update it to pick up new changes.

While the BoringSSL repository may contain project-specific branches, e.g. chromium-2214, those are not supported release branches and must not as such.

So we are simply using yesterday's HEAD for the fork.

While BoringSSL really can simply be "dropped in" as a submodule that comes with a catch that a recent version of Go is required to build. We don't want that. Thus we end up with something I consider really ugly: committing post processed files in to our repository. We will simply have to tolerate this, as it's just how BoringSSL works,

Generally one checks in these generated files alongside the hand-written build files. Periodically an engineer updates the BoringSSL revision, regenerates these files and checks in the updated result. As an example, see how this is done in Chromium.

To make it worse, the generated files must be placed directly outside of the src submodule directory. I would have liked to place these files in to a generated directory to more cleanly delineate the generated files from non-generated files, but alas without hacking up the generated_build_files.py script more that doesn't seem possible.

I've tried to mitigate this ugliness a little by adding a small test in the workflow that regenerates the intermediate files and will fail if the files don't match what is committed. An example of failure can be found, https://github.com/AntelopeIO/leap/actions/runs/6091633748/job/16528533086

Also, as a reviewer note, the first commit in the PR contains the changes before running the make regenerate_boringssl target, and the third commit contains those generated files added to the repo. Ultimately the changes outside that third commit are quite minimal.

Co-authored-by: 766C6164 <valterdaw@gmail.com>

spoonincode · 2023-09-06T01:39:12Z

libraries/libfc/src/crypto/elliptic_common.cpp

-            FC_ASSERT( BN_num_bytes( order ) == static_cast<int>(bin.data_size()) );
-            FC_ASSERT( BN_bn2bin( order, (unsigned char*) bin.data() ) == static_cast<int>(bin.data_size()) );
+            FC_ASSERT( BN_num_bytes( order ) == bin.data_size() );
+            FC_ASSERT( BN_bn2bin( order, (unsigned char*) bin.data() ) == bin.data_size() );


These are a good example of while BoringSSL is a drop in replacement, it isn't without slight differences. A warning was introduced here because BoringSSL returns a size_t while OpenSSL returned an int.

spoonincode · 2023-09-06T01:48:12Z

libraries/libfc/src/crypto/elliptic_secp256k1.cpp

+         RAND_bytes((uint8_t*)ret.my->_key.data(), sizeof(ret.my->_key.data()));
+       } while(!secp256k1_ec_seckey_verify(detail::_get_context(), (const uint8_t*)ret.my->_key.data()));
+       return ret;
+    }


This change requires special attention during review. BoringSSL does not support the K1 curve. We've previously transitioned to using libsecp256k1 for all things K1... except for private key generation as performed here.

_key is a private_key_secret,

leap/libraries/libfc/src/crypto/_elliptic_impl_priv.hpp

Line 22 in 409eb59

private_key_secret _key;

and private_key_secret is a fc::sha256,

leap/libraries/libfc/include/fc/crypto/elliptic.hpp

Line 23 in 409eb59

typedef fc::sha256 private_key_secret;

The original code in this PR is probably incorrect and does not generate 32 random bytes, which would be a disaster.

spoonincode · 2023-09-06T01:50:45Z

libraries/CMakeLists.txt

@@ -7,6 +7,8 @@ set(BN256_INSTALL_COMPONENT "dev")

 set( Boost_USE_MULTITHREADED      ON )
 set( Boost_USE_STATIC_LIBS ON CACHE STRING "ON or OFF" )
+# don't include boost mysql library as it does a find_package(OpenSSL) thus finding the system openssl which could conflict with the bundled boringssl
+set( BOOST_EXCLUDE_LIBRARIES "mysql" )


This is something we'll need to be mindful of on boost bumps (or really any bundled dependency).

greg7mdp · 2023-09-06T12:36:51Z

While BoringSSL really can simply be "dropped in" as a submodule that comes with a catch that a recent version of Go is required to build. We don't want that. Thus we end up with something I consider really ugly: committing post processed files in to our repository.

I think it would be nicer to check in these generated files into our fork of BoringSSL (at least keeping the workflow test that checked-in files match newly generated ones), and to use BoringSSL as a submodule in Leap.
Maybe the checkin of the generated files could be done in an automated fashion via ci/cd.

spoonincode · 2023-09-06T13:01:38Z

That seems reasonable, let me play with that and see if there are any downsides.

spoonincode · 2023-09-07T14:29:01Z

@greg7mdp I'm not sure how to best accomplish that. The primary problem here is that the generate_build_files.py is very inflexible with paths: the boringssl source directory (which is the submodule here) must be at src, and the generated files must be in the parent directory of src. That means placing the generated files in the boringssl submodule would require moving all the files around; that's not good.

Obviously we could change generate_build_files.py to be more flexible, but I like keeping the changes as minimal as possible (maybe they can even be upstreamed at some point).

One possibility is to utilize two submodules. We keep the close-to-upstream boringssl as it is now, but then have a new "boringssl-generated" repo that contains the generated files with the src directory pointing to the boringssl repo. Then leap only includes that "boringssl-generated" repo as a submodule. This certainly has some positives, but I'm unsure how others feel about that approach?

greg7mdp · 2023-09-07T15:06:05Z

One possibility is to utilize two submodules.

This is a little awkward, but I think this is preferable to adding all the generated files to leap itself, and I can't think of a better way to do it. I'm curious to hear what others think.

linh2931 · 2023-09-11T12:41:43Z

One possibility is to utilize two submodules. We keep the close-to-upstream boringssl as it is now, but then have a new "boringssl-generated" repo that contains the generated files with the src directory pointing to the boringssl repo. Then leap only includes that "boringssl-generated" repo as a submodule.

I think main goals would be simple to upgrade in the future and fast to check out. This approach seems to achieve those.

required minor fixup to tester.cmakes

spoonincode · 2023-09-22T19:42:47Z

Okay, I've migrated the generated build files to
https://github.com/AntelopeIO/boringssl-build/
There is a workflow there that checks the committed files match. Notice though that I changed the mechanism to git ls-files. A failure example was committed in the failure_example branch, https://github.com/AntelopeIO/boringssl-build/actions/runs/6269480194/job/17025961921

spoonincode and others added 3 commits September 5, 2023 21:28

replace openssl with boringssl

2ff2b9c

Co-authored-by: 766C6164 <valterdaw@gmail.com>

warning fixes

636117f

add generated boringssl files

fffa8f9

spoonincode commented Sep 6, 2023

View reviewed changes

use data_size() for sha256 length

b832c7e

spoonincode commented Sep 6, 2023

View reviewed changes

spoonincode mentioned this pull request Sep 6, 2023

Replace OpenSSL usage with BoringSSL #1233

Closed

spoonincode linked an issue Sep 6, 2023 that may be closed by this pull request

Replace OpenSSL usage with BoringSSL #644

Closed

1 task

spoonincode added 4 commits September 13, 2023 16:37

prevent anything from find_package(OpenSSL)ing

e2be85a

Merge remote-tracking branch 'origin/main' into HEAD

d755b72

Merge remote-tracking branch 'origin/main' into HEAD

f409cc7

required minor fixup to tester.cmakes

replace committed boringssl files with boringssl-build submodule

9979695

greg7mdp self-requested a review September 25, 2023 21:01

BenjaminGormanPMP requested a review from linh2931 September 25, 2023 21:02

greg7mdp approved these changes Sep 25, 2023

View reviewed changes

linh2931 approved these changes Sep 27, 2023

View reviewed changes

use fc::rand_bytes

705fd14

linh2931 approved these changes Sep 28, 2023

View reviewed changes

greg7mdp approved these changes Sep 28, 2023

View reviewed changes

spoonincode merged commit 2384713 into main Sep 28, 2023

spoonincode deleted the boringssl branch September 28, 2023 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace OpenSSL dependency with pinned BoringSSL #1599

Replace OpenSSL dependency with pinned BoringSSL #1599

spoonincode commented Sep 6, 2023 •

edited

Loading

spoonincode Sep 6, 2023

spoonincode Sep 6, 2023

spoonincode Sep 6, 2023

greg7mdp commented Sep 6, 2023 •

edited

Loading

spoonincode commented Sep 6, 2023

spoonincode commented Sep 7, 2023

greg7mdp commented Sep 7, 2023

linh2931 commented Sep 11, 2023

spoonincode commented Sep 22, 2023

Replace OpenSSL dependency with pinned BoringSSL #1599

Replace OpenSSL dependency with pinned BoringSSL #1599

Conversation

spoonincode commented Sep 6, 2023 • edited Loading

spoonincode Sep 6, 2023

Choose a reason for hiding this comment

spoonincode Sep 6, 2023

Choose a reason for hiding this comment

spoonincode Sep 6, 2023

Choose a reason for hiding this comment

greg7mdp commented Sep 6, 2023 • edited Loading

spoonincode commented Sep 6, 2023

spoonincode commented Sep 7, 2023

greg7mdp commented Sep 7, 2023

linh2931 commented Sep 11, 2023

spoonincode commented Sep 22, 2023

spoonincode commented Sep 6, 2023 •

edited

Loading

greg7mdp commented Sep 6, 2023 •

edited

Loading