-
Notifications
You must be signed in to change notification settings - Fork 73
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
Replace OpenSSL dependency with pinned BoringSSL #1599
Changes from all commits
2ff2b9c
636117f
fffa8f9
b832c7e
e2be85a
d755b72
f409cc7
9979695
705fd14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,6 @@ pkg update && pkg install \ | |
curl \ | ||
boost-all \ | ||
python3 \ | ||
openssl \ | ||
llvm11 \ | ||
pkgconf | ||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
add_subdirectory(boringssl EXCLUDE_FROM_ALL) | ||
target_compile_options(fipsmodule PRIVATE -Wno-error) | ||
target_compile_options(crypto PRIVATE -Wno-error) | ||
target_compile_options(decrepit PRIVATE -Wno-error) | ||
|
||
#paranoia for when a dependent library depends on openssl (such as libcurl) | ||
set_target_properties(fipsmodule PROPERTIES C_VISIBILITY_PRESET hidden) | ||
set_target_properties(crypto PROPERTIES C_VISIBILITY_PRESET hidden) | ||
set_target_properties(decrepit PROPERTIES C_VISIBILITY_PRESET hidden) | ||
|
||
add_library(boringssl INTERFACE) | ||
target_link_libraries(boringssl INTERFACE crypto decrepit) | ||
target_include_directories(boringssl INTERFACE boringssl/src/include) | ||
|
||
# avoid conflict with system lib | ||
set_target_properties(crypto PROPERTIES PREFIX libbs) | ||
|
||
install( TARGETS crypto | ||
LIBRARY DESTINATION ${CMAKE_INSTALL_FULL_LIBDIR} COMPONENT dev EXCLUDE_FROM_ALL | ||
ARCHIVE DESTINATION ${CMAKE_INSTALL_FULL_LIBDIR} COMPONENT dev EXCLUDE_FROM_ALL | ||
) | ||
|
||
install( TARGETS decrepit | ||
LIBRARY DESTINATION ${CMAKE_INSTALL_FULL_LIBDIR} COMPONENT dev EXCLUDE_FROM_ALL | ||
ARCHIVE DESTINATION ${CMAKE_INSTALL_FULL_LIBDIR} COMPONENT dev EXCLUDE_FROM_ALL | ||
) | ||
|
||
install( DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/boringssl/src/include/" DESTINATION "${CMAKE_INSTALL_FULL_INCLUDEDIR}/leapboringssl" COMPONENT dev EXCLUDE_FROM_ALL ) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,8 +75,8 @@ namespace fc { namespace ecc { | |
ssl_bignum order; | ||
FC_ASSERT( EC_GROUP_get_order( group, order, ctx ) ); | ||
private_key_secret bin; | ||
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() ); | ||
return bin; | ||
} | ||
|
||
|
@@ -94,8 +94,8 @@ namespace fc { namespace ecc { | |
FC_ASSERT( EC_GROUP_get_order( group, order, ctx ) ); | ||
BN_rshift1( order, order ); | ||
private_key_secret bin; | ||
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() ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
return bin; | ||
} | ||
|
||
|
@@ -196,21 +196,6 @@ namespace fc { namespace ecc { | |
BN_bn2bin(bn, &((unsigned char*)&sec)[32-nbytes] ); | ||
return sec; | ||
} | ||
|
||
private_key private_key::generate() | ||
{ | ||
EC_KEY* k = EC_KEY_new_by_curve_name( NID_secp256k1 ); | ||
if( !k ) FC_THROW_EXCEPTION( exception, "Unable to generate EC key" ); | ||
if( !EC_KEY_generate_key( k ) ) | ||
{ | ||
FC_THROW_EXCEPTION( exception, "ecc key generation error" ); | ||
|
||
} | ||
|
||
return private_key( k ); | ||
} | ||
|
||
|
||
} | ||
|
||
void to_variant( const ecc::private_key& var, variant& vo ) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,6 +4,7 @@ | |||||
#include <fc/crypto/hmac.hpp> | ||||||
#include <fc/crypto/openssl.hpp> | ||||||
#include <fc/crypto/sha512.hpp> | ||||||
#include <fc/crypto/rand.hpp> | ||||||
|
||||||
#include <fc/fwd_impl.hpp> | ||||||
#include <fc/exception/exception.hpp> | ||||||
|
@@ -12,6 +13,8 @@ | |||||
#include <secp256k1.h> | ||||||
#include <secp256k1_recovery.h> | ||||||
|
||||||
#include <openssl/rand.h> | ||||||
|
||||||
#if _WIN32 | ||||||
# include <malloc.h> | ||||||
#elif defined(__FreeBSD__) | ||||||
|
@@ -79,6 +82,14 @@ namespace fc { namespace ecc { | |||||
return fc::sha512::hash( serialized_result.begin() + 1, serialized_result.size() - 1 ); | ||||||
} | ||||||
|
||||||
private_key private_key::generate() | ||||||
{ | ||||||
private_key ret; | ||||||
do { | ||||||
rand_bytes(ret.my->_key.data(), ret.my->_key.data_size()); | ||||||
} while(!secp256k1_ec_seckey_verify(detail::_get_context(), (const uint8_t*)ret.my->_key.data())); | ||||||
return ret; | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
and
The original code in this PR is probably incorrect and does not generate 32 random bytes, which would be a disaster. |
||||||
|
||||||
public_key::public_key() {} | ||||||
|
||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something we'll need to be mindful of on boost bumps (or really any bundled dependency).