Skip to content

Commit ab5a337

Browse files
achamayouachamayou
authored
Tidy crypto (#7009)
1 parent 75cd4b3 commit ab5a337

33 files changed

+592
-445
lines changed

‎.clang-tidy

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,24 @@ Checks: >
1919
-readability-function-cognitive-complexity,
2020
-misc-const-correctness,
2121
-misc-include-cleaner,
22-
-readability-identifier-length
22+
-readability-identifier-length,
23+
-cppcoreguidelines-pro-type-union-access,
24+
-cppcoreguidelines-pro-type-reinterpret-cast,
25+
-cppcoreguidelines-avoid-non-const-global-variables,
26+
-misc-use-anonymous-namespace,
27+
-readability-avoid-nested-conditional-operator,
28+
-cppcoreguidelines-pro-bounds-constant-array-index,
29+
-cppcoreguidelines-narrowing-conversions,
30+
-bugprone-narrowing-conversions,
31+
-bugprone-easily-swappable-parameters,
32+
-cppcoreguidelines-pro-bounds-pointer-arithmetic,
33+
-cppcoreguidelines-avoid-c-arrays,
34+
-modernize-avoid-c-arrays,
35+
-cppcoreguidelines-pro-type-const-cast,
36+
-bugprone-assignment-in-if-condition,
37+
-performance-enum-size,
38+
-bugprone-casting-through-void
2339
2440
WarningsAsErrors: '*'
25-
HeaderFilterRegex: '.*/3rdparty/.*'
41+
HeaderFilterRegex: '?!(3rdparty)'
2642
FormatStyle: 'file'

‎cmake/crypto.cmake

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ set(CCFCRYPTO_SRC
3333
if(COMPILE_TARGET STREQUAL "snp")
3434
add_library(ccfcrypto.snp ${CCFCRYPTO_SRC})
3535
add_san(ccfcrypto.snp)
36+
add_tidy(ccfcrypto.snp)
3637
target_compile_options(ccfcrypto.snp PUBLIC ${COMPILE_LIBCXX})
3738
target_link_options(ccfcrypto.snp PUBLIC ${LINK_LIBCXX})
3839
target_link_libraries(ccfcrypto.snp PUBLIC qcbor.snp)
@@ -56,6 +57,7 @@ find_library(TLS_LIBRARY ssl)
5657

5758
add_library(ccfcrypto.host STATIC ${CCFCRYPTO_SRC})
5859
add_san(ccfcrypto.host)
60+
add_tidy(ccfcrypto.host)
5961
target_compile_options(ccfcrypto.host PUBLIC ${COMPILE_LIBCXX})
6062
target_link_options(ccfcrypto.host PUBLIC ${LINK_LIBCXX})
6163

‎include/ccf/crypto/key_pair.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ namespace ccf::crypto
183183
* @param pkey PEM key to load
184184
* @return Key pair
185185
*/
186-
KeyPairPtr make_key_pair(const Pem& pkey);
186+
KeyPairPtr make_key_pair(const Pem& pem);
187187

188188
/**
189189
* Construct a new public / private ECDSA key pair from a JsonWebKeyECPrivate

‎include/ccf/crypto/openssl/openssl_wrappers.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ namespace ccf::crypto
3232
*/
3333

3434
/// Returns the error string from an error code
35-
inline std::string error_string(int ec)
35+
inline std::string error_string(unsigned long ec)
3636
{
3737
// ERR_error_string doesn't really expect the code could actually be zero
3838
// and uses the `static char buf[256]` which is NOT cleaned nor checked
@@ -42,7 +42,7 @@ namespace ccf::crypto
4242
std::string err(256, '\0');
4343
ERR_load_crypto_strings();
4444
SSL_load_error_strings();
45-
ERR_error_string_n((unsigned long)ec, err.data(), err.size());
45+
ERR_error_string_n(ec, err.data(), err.size());
4646
ERR_free_strings();
4747
// Remove any trailing NULs before returning
4848
err.resize(std::strlen(err.c_str()));
@@ -166,9 +166,9 @@ namespace ccf::crypto
166166
Unique_SSL_OBJECT(
167167
BIO_new_mem_buf(buf, len), [](auto x) { BIO_free(x); })
168168
{}
169-
Unique_BIO(const std::vector<uint8_t>& d) :
169+
Unique_BIO(std::span<const uint8_t> s) :
170170
Unique_SSL_OBJECT(
171-
BIO_new_mem_buf(d.data(), d.size()), [](auto x) { BIO_free(x); })
171+
BIO_new_mem_buf(s.data(), s.size()), [](auto x) { BIO_free(x); })
172172
{}
173173
Unique_BIO(const Pem& pem) :
174174
Unique_SSL_OBJECT(

‎include/ccf/crypto/pem.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ namespace ccf::crypto
2222

2323
public:
2424
Pem() = default;
25-
Pem(const std::string& s_);
25+
Pem(std::string pem_string);
2626
Pem(const uint8_t* data, size_t size);
2727

2828
explicit Pem(std::span<const uint8_t> s) : Pem(s.data(), s.size()) {}

‎src/crypto/base64.cpp

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,18 @@ namespace ccf::crypto
1717
std::vector<uint8_t> raw_from_b64url(const std::string_view& b64url_string)
1818
{
1919
std::string b64_string = std::string(b64url_string);
20-
for (size_t i = 0; i < b64_string.size(); i++)
20+
for (char& c : b64_string)
2121
{
22-
switch (b64_string[i])
22+
switch (c)
2323
{
2424
case '-':
25-
b64_string[i] = '+';
25+
c = '+';
2626
break;
2727
case '_':
28-
b64_string[i] = '/';
28+
c = '/';
2929
break;
30+
default:
31+
continue;
3032
}
3133
}
3234
auto padding = b64_string.size() % 4 == 2 ? 2 :
@@ -51,16 +53,18 @@ namespace ccf::crypto
5153
{
5254
auto r = Base64Impl::b64_from_raw(data, size);
5355

54-
for (size_t i = 0; i < r.size(); i++)
56+
for (char& c : r)
5557
{
56-
switch (r[i])
58+
switch (c)
5759
{
5860
case '+':
59-
r[i] = '-';
61+
c = '-';
6062
break;
6163
case '/':
62-
r[i] = '_';
64+
c = '_';
6365
break;
66+
default:
67+
continue;
6468
}
6569
}
6670

‎src/crypto/cose.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ namespace ccf::cose::edit
1616
{
1717
UsefulBufC buf{cose_input.data(), cose_input.size()};
1818

19-
QCBORError err;
19+
QCBORError err = QCBOR_SUCCESS;
2020
QCBORDecodeContext ctx;
2121
QCBORDecode_Init(&ctx, buf, QCBOR_DECODE_MODE_NORMAL);
2222

@@ -88,7 +88,7 @@ namespace ccf::cose::edit
8888
}
8989
else if (std::holds_alternative<desc::Value>(descriptor))
9090
{
91-
auto& [pos, key, value] = std::get<desc::Value>(descriptor);
91+
const auto& [pos, key, value] = std::get<desc::Value>(descriptor);
9292

9393
// Maximum expected size of the additional map, sub-map is the
9494
// worst-case scenario
@@ -126,7 +126,7 @@ namespace ccf::cose::edit
126126
}
127127
else if (std::holds_alternative<desc::Value>(descriptor))
128128
{
129-
auto& [pos, key, value] = std::get<desc::Value>(descriptor);
129+
const auto& [pos, key, value] = std::get<desc::Value>(descriptor);
130130

131131
if (std::holds_alternative<pos::InArray>(pos))
132132
{

‎src/crypto/ecdsa.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ namespace ccf::crypto
3939
auto der_size = i2d_ECDSA_SIG(sig, nullptr);
4040
OpenSSL::CHECK0(der_size);
4141
std::vector<uint8_t> der_sig(der_size);
42-
auto der_sig_buf = der_sig.data();
42+
auto* der_sig_buf = der_sig.data();
4343
OpenSSL::CHECK0(i2d_ECDSA_SIG(sig, &der_sig_buf));
4444
return der_sig;
4545
}
@@ -55,9 +55,9 @@ namespace ccf::crypto
5555
std::vector<uint8_t> ecdsa_sig_der_to_p1363(
5656
const std::vector<uint8_t>& signature, CurveID curveId)
5757
{
58-
auto sig_ptr = signature.data();
58+
const auto* sig_ptr = signature.data();
5959
OpenSSL::Unique_ECDSA_SIG ecdsa_sig(
60-
d2i_ECDSA_SIG(NULL, &sig_ptr, signature.size()));
60+
d2i_ECDSA_SIG(nullptr, &sig_ptr, signature.size()));
6161
// r and s are managed by Unique_ECDSA_SIG object, so we shouldn't use
6262
// Unique_BIGNUM for them
6363
const BIGNUM* r = ECDSA_SIG_get0_r(ecdsa_sig);

‎src/crypto/hash.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ namespace ccf::crypto
1010
{
1111
void default_sha256(const std::span<const uint8_t>& data, uint8_t* h)
1212
{
13-
return openssl_sha256(data, h);
13+
openssl_sha256(data, h);
1414
}
1515

1616
std::vector<uint8_t> sha256(const std::vector<uint8_t>& data)

‎src/crypto/hmac.cpp

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
#include "crypto/openssl/hash.h"
66

7+
#include <limits>
78
#include <openssl/hmac.h>
89

910
namespace ccf::crypto
@@ -15,18 +16,23 @@ namespace ccf::crypto
1516
const std::vector<uint8_t>& key,
1617
const std::vector<uint8_t>& data)
1718
{
18-
auto o_md_type = OpenSSL::get_md_type(type);
19+
const auto* o_md_type = OpenSSL::get_md_type(type);
1920
HashBytes r(EVP_MD_size(o_md_type));
2021
unsigned int len = 0;
21-
auto rc = HMAC(
22+
if (key.size() > std::numeric_limits<int>::max())
23+
{
24+
throw std::logic_error("Key size is too large");
25+
}
26+
int key_size = static_cast<int>(key.size());
27+
auto* rc = HMAC(
2228
o_md_type,
2329
key.data(),
24-
key.size(),
30+
key_size,
2531
data.data(),
2632
data.size(),
2733
r.data(),
2834
&len);
29-
if (rc == 0)
35+
if (rc == nullptr)
3036
{
3137
throw std::logic_error("HMAC Failed");
3238
}

‎src/crypto/key_wrap.cpp

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "ccf/crypto/symmetric_key.h"
99
#include "openssl/symmetric_key.h"
1010

11+
#include <climits>
1112
#include <cstdint>
1213
#include <openssl/rand.h>
1314
#include <stdexcept>
@@ -19,7 +20,7 @@ namespace ccf::crypto
1920
// http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/cos01/pkcs11-curr-v2.40-cos01.html
2021

2122
std::vector<uint8_t> ckm_rsa_pkcs_oaep_wrap(
22-
RSAPublicKeyPtr wrapping_key,
23+
RSAPublicKeyPtr wrapping_key, // NOLINT(performance-unnecessary-value-param)
2324
const std::vector<uint8_t>& unwrapped,
2425
const std::optional<std::vector<uint8_t>>& label)
2526
{
@@ -36,7 +37,7 @@ namespace ccf::crypto
3637
}
3738

3839
std::vector<uint8_t> ckm_rsa_pkcs_oaep_unwrap(
39-
RSAKeyPairPtr wrapping_key,
40+
RSAKeyPairPtr wrapping_key, // NOLINT(performance-unnecessary-value-param)
4041
const std::vector<uint8_t>& wrapped,
4142
const std::optional<std::vector<uint8_t>>& label)
4243
{
@@ -68,18 +69,30 @@ namespace ccf::crypto
6869
return ossl->ckm_aes_key_unwrap_pad(wrapped);
6970
}
7071

72+
static constexpr size_t AES_KEY_SIZE_128 = 128;
73+
static constexpr size_t AES_KEY_SIZE_192 = 192;
74+
static constexpr size_t AES_KEY_SIZE_256 = 256;
75+
76+
static constexpr size_t AES_KEY_SIZE_128_BYTES = 16;
77+
static constexpr size_t AES_KEY_SIZE_192_BYTES = 24;
78+
static constexpr size_t AES_KEY_SIZE_256_BYTES = 32;
79+
7180
std::vector<uint8_t> ckm_rsa_aes_key_wrap(
7281
size_t aes_key_size,
73-
RSAPublicKeyPtr wrapping_key,
82+
RSAPublicKeyPtr wrapping_key, // NOLINT(performance-unnecessary-value-param)
7483
const std::vector<uint8_t>& unwrapped,
7584
const std::optional<std::vector<uint8_t>>& label)
7685
{
77-
if (aes_key_size != 128 && aes_key_size != 192 && aes_key_size != 256)
86+
if (
87+
aes_key_size != AES_KEY_SIZE_128 && aes_key_size != AES_KEY_SIZE_192 &&
88+
aes_key_size != AES_KEY_SIZE_256)
89+
{
7890
throw std::runtime_error("invalid key size");
91+
}
7992

8093
// - Generates temporary random AES key of ulAESKeyBits length. This key is
8194
// not accessible to the user - no handle is returned.
82-
std::vector<uint8_t> taeskey(aes_key_size / 8);
95+
std::vector<uint8_t> taeskey(aes_key_size / CHAR_BIT);
8396
RAND_bytes(taeskey.data(), taeskey.size());
8497

8598
// - Wraps the AES key with the wrapping RSA key using CKM_RSA_PKCS_OAEP
@@ -112,18 +125,20 @@ namespace ccf::crypto
112125
}
113126

114127
std::vector<uint8_t> ckm_rsa_aes_key_unwrap(
115-
RSAKeyPairPtr wrapping_key,
128+
RSAKeyPairPtr wrapping_key, // NOLINT(performance-unnecessary-value-param)
116129
const std::vector<uint8_t>& wrapped,
117130
const std::optional<std::vector<uint8_t>>& label)
118131
{
119132
// - Splits the input into two parts. The first is the wrapped AES key, and
120133
// the second is the wrapped target key. The length of the first part is
121134
// equal to the length of the unwrapping RSA key.
122135

123-
size_t w_aes_sz = wrapping_key->key_size() / 8;
136+
size_t w_aes_sz = wrapping_key->key_size() / CHAR_BIT;
124137

125138
if (wrapped.size() <= w_aes_sz)
139+
{
126140
throw std::runtime_error("not enough ciphertext");
141+
}
127142

128143
std::vector<uint8_t> w_aeskey(wrapped.begin(), wrapped.begin() + w_aes_sz);
129144
std::vector<uint8_t> w_target(wrapped.begin() + w_aes_sz, wrapped.end());
@@ -134,8 +149,13 @@ namespace ccf::crypto
134149
std::vector<uint8_t> t_aeskey =
135150
wrapping_key->rsa_oaep_unwrap(w_aeskey, label);
136151

137-
if (t_aeskey.size() != 16 && t_aeskey.size() != 24 && t_aeskey.size() != 32)
152+
if (
153+
t_aeskey.size() != AES_KEY_SIZE_128_BYTES &&
154+
t_aeskey.size() != AES_KEY_SIZE_192_BYTES &&
155+
t_aeskey.size() != AES_KEY_SIZE_256_BYTES)
156+
{
138157
throw std::runtime_error("invalid key size");
158+
}
139159

140160
// - Un-wraps the target key from the second part with the temporary AES key
141161
// using CKM_AES_KEY_WRAP_PAD (RFC5649).

0 commit comments

Comments
 (0)