Skip to content

Commit 750f844

Browse files
xonatiusxonatius
authored
Fix missing GUARDs after s2n_pkey_size calls (#2517)
1 parent 7ad1905 commit 750f844

File tree

11 files changed

+110
-71
lines changed

11 files changed

+110
-71
lines changed

‎crypto/s2n_ecdsa.c‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "utils/s2n_blob.h"
2323
#include "utils/s2n_mem.h"
2424
#include "utils/s2n_random.h"
25+
#include "utils/s2n_result.h"
2526
#include "utils/s2n_safety.h"
2627

2728
#include "crypto/s2n_ecdsa.h"
@@ -30,12 +31,19 @@
3031
#include "crypto/s2n_openssl.h"
3132
#include "crypto/s2n_pkey.h"
3233

33-
int s2n_ecdsa_der_signature_size(const struct s2n_pkey *pkey)
34+
S2N_RESULT s2n_ecdsa_der_signature_size(const struct s2n_pkey *pkey, uint32_t *size_out)
3435
{
36+
ENSURE_REF(pkey);
37+
ENSURE_REF(size_out);
38+
3539
const struct s2n_ecdsa_key *ecdsa_key = &pkey->key.ecdsa_key;
36-
notnull_check(ecdsa_key->ec_key);
40+
ENSURE_REF(ecdsa_key->ec_key);
41+
42+
const int size = ECDSA_size(ecdsa_key->ec_key);
43+
GUARD_AS_RESULT(size);
44+
*size_out = size;
3745

38-
return ECDSA_size(ecdsa_key->ec_key);
46+
return S2N_RESULT_OK;
3947
}
4048

4149
static int s2n_ecdsa_sign(const struct s2n_pkey *priv, s2n_signature_algorithm sig_alg,
@@ -102,7 +110,9 @@ static int s2n_ecdsa_keys_match(const struct s2n_pkey *pub, const struct s2n_pke
102110
GUARD(s2n_hash_update(&state_in, input, sizeof(input)));
103111
GUARD(s2n_hash_update(&state_out, input, sizeof(input)));
104112

105-
GUARD(s2n_alloc(&signature, s2n_ecdsa_der_signature_size(priv)));
113+
uint32_t size = 0;
114+
GUARD_AS_POSIX(s2n_ecdsa_der_signature_size(priv, &size));
115+
GUARD(s2n_alloc(&signature, size));
106116

107117
GUARD(s2n_ecdsa_sign(priv, S2N_SIGNATURE_ECDSA, &state_in, &signature));
108118
GUARD(s2n_ecdsa_verify(pub, S2N_SIGNATURE_ECDSA, &state_out, &signature));

‎crypto/s2n_pkey.c‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include "crypto/s2n_rsa_pss.h"
2222
#include "crypto/s2n_pkey.h"
2323

24+
#include "utils/s2n_result.h"
2425
#include "utils/s2n_safety.h"
2526

2627
int s2n_pkey_zero_init(struct s2n_pkey *pkey)
@@ -61,11 +62,15 @@ int s2n_pkey_check_key_exists(const struct s2n_pkey *pkey)
6162
return pkey->check_key(pkey);
6263
}
6364

64-
int s2n_pkey_size(const struct s2n_pkey *pkey)
65+
S2N_RESULT s2n_pkey_size(const struct s2n_pkey *pkey, uint32_t *size_out)
6566
{
66-
notnull_check(pkey->size);
67+
ENSURE_REF(pkey);
68+
ENSURE_REF(pkey->size);
69+
ENSURE_REF(size_out);
6770

68-
return pkey->size(pkey);
71+
GUARD_RESULT(pkey->size(pkey, size_out));
72+
73+
return S2N_RESULT_OK;
6974
}
7075

7176
int s2n_pkey_sign(const struct s2n_pkey *pkey, s2n_signature_algorithm sig_alg,

‎crypto/s2n_pkey.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "crypto/s2n_rsa.h"
2424

2525
#include "utils/s2n_blob.h"
26+
#include "utils/s2n_result.h"
2627

2728
/* Public/Private Key Type */
2829
typedef enum {
@@ -43,7 +44,7 @@ struct s2n_pkey {
4344
} key;
4445
EVP_PKEY *pkey;
4546

46-
int (*size)(const struct s2n_pkey *key);
47+
S2N_RESULT (*size)(const struct s2n_pkey *key, uint32_t *size_out);
4748
int (*sign)(const struct s2n_pkey *priv_key, s2n_signature_algorithm sig_alg,
4849
struct s2n_hash_state *digest, struct s2n_blob *signature);
4950
int (*verify)(const struct s2n_pkey *pub_key, s2n_signature_algorithm sig_alg,
@@ -59,7 +60,7 @@ int s2n_pkey_zero_init(struct s2n_pkey *pkey);
5960
int s2n_pkey_setup_for_type(struct s2n_pkey *pkey, s2n_pkey_type pkey_type);
6061
int s2n_pkey_check_key_exists(const struct s2n_pkey *pkey);
6162

62-
int s2n_pkey_size(const struct s2n_pkey *pkey);
63+
S2N_RESULT s2n_pkey_size(const struct s2n_pkey *pkey, uint32_t *size_out);
6364
int s2n_pkey_sign(const struct s2n_pkey *pkey, s2n_signature_algorithm sig_alg,
6465
struct s2n_hash_state *digest, struct s2n_blob *signature);
6566
int s2n_pkey_verify(const struct s2n_pkey *pkey, s2n_signature_algorithm sig_alg,

‎crypto/s2n_rsa.c‎

Lines changed: 57 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -13,51 +13,57 @@
1313
* permissions and limitations under the License.
1414
*/
1515

16+
#include "crypto/s2n_rsa.h"
17+
1618
#include <openssl/evp.h>
1719
#include <openssl/rsa.h>
1820
#include <stdint.h>
1921

20-
#include "error/s2n_errno.h"
21-
22-
#include "stuffer/s2n_stuffer.h"
23-
24-
#include "crypto/s2n_hash.h"
2522
#include "crypto/s2n_drbg.h"
26-
#include "crypto/s2n_rsa.h"
27-
#include "crypto/s2n_rsa_signing.h"
23+
#include "crypto/s2n_hash.h"
2824
#include "crypto/s2n_pkey.h"
29-
30-
#include "utils/s2n_safety.h"
31-
#include "utils/s2n_random.h"
25+
#include "crypto/s2n_rsa_signing.h"
26+
#include "error/s2n_errno.h"
27+
#include "stuffer/s2n_stuffer.h"
3228
#include "utils/s2n_blob.h"
29+
#include "utils/s2n_random.h"
30+
#include "utils/s2n_result.h"
31+
#include "utils/s2n_safety.h"
3332

34-
static int s2n_rsa_modulus_check(RSA *rsa)
33+
static S2N_RESULT s2n_rsa_modulus_check(RSA *rsa)
3534
{
36-
/* RSA was made opaque starting in Openssl 1.1.0 */
37-
#if S2N_OPENSSL_VERSION_AT_LEAST(1,1,0) && !defined(LIBRESSL_VERSION_NUMBER)
38-
const BIGNUM *n = NULL;
39-
/* RSA still owns the memory for n */
40-
RSA_get0_key(rsa, &n, NULL, NULL);
41-
notnull_check(n);
42-
#else
43-
notnull_check(rsa->n);
44-
#endif
45-
return 0;
35+
/* RSA was made opaque starting in Openssl 1.1.0 */
36+
#if S2N_OPENSSL_VERSION_AT_LEAST(1, 1, 0) && !defined(LIBRESSL_VERSION_NUMBER)
37+
const BIGNUM *n = NULL;
38+
/* RSA still owns the memory for n */
39+
RSA_get0_key(rsa, &n, NULL, NULL);
40+
ENSURE_REF(n);
41+
#else
42+
ENSURE_REF(rsa->n);
43+
#endif
44+
return S2N_RESULT_OK;
4645
}
4746

48-
static int s2n_rsa_encrypted_size(const struct s2n_pkey *key)
47+
static S2N_RESULT s2n_rsa_encrypted_size(const struct s2n_pkey *key, uint32_t *size_out)
4948
{
49+
ENSURE_REF(key);
50+
ENSURE_REF(size_out);
51+
5052
const struct s2n_rsa_key *rsa_key = &key->key.rsa_key;
51-
notnull_check(rsa_key->rsa);
52-
GUARD(s2n_rsa_modulus_check(rsa_key->rsa));
53+
ENSURE_REF(rsa_key->rsa);
54+
GUARD_RESULT(s2n_rsa_modulus_check(rsa_key->rsa));
5355

54-
return RSA_size(rsa_key->rsa);
56+
const int size = RSA_size(rsa_key->rsa);
57+
GUARD_AS_RESULT(size);
58+
*size_out = size;
59+
60+
return S2N_RESULT_OK;
5561
}
5662

57-
static int s2n_rsa_sign(const struct s2n_pkey *priv, s2n_signature_algorithm sig_alg,
58-
struct s2n_hash_state *digest, struct s2n_blob *signature)
63+
static int s2n_rsa_sign(const struct s2n_pkey *priv, s2n_signature_algorithm sig_alg, struct s2n_hash_state *digest,
64+
struct s2n_blob *signature)
5965
{
60-
switch(sig_alg) {
66+
switch (sig_alg) {
6167
case S2N_SIGNATURE_RSA:
6268
return s2n_rsa_pkcs1v15_sign(priv, digest, signature);
6369
case S2N_SIGNATURE_RSA_PSS_RSAE:
@@ -69,10 +75,10 @@ static int s2n_rsa_sign(const struct s2n_pkey *priv, s2n_signature_algorithm sig
6975
return S2N_SUCCESS;
7076
}
7177

72-
static int s2n_rsa_verify(const struct s2n_pkey *pub, s2n_signature_algorithm sig_alg,
73-
struct s2n_hash_state *digest, struct s2n_blob *signature)
78+
static int s2n_rsa_verify(const struct s2n_pkey *pub, s2n_signature_algorithm sig_alg, struct s2n_hash_state *digest,
79+
struct s2n_blob *signature)
7480
{
75-
switch(sig_alg) {
81+
switch (sig_alg) {
7682
case S2N_SIGNATURE_RSA:
7783
return s2n_rsa_pkcs1v15_verify(pub, digest, signature);
7884
case S2N_SIGNATURE_RSA_PSS_RSAE:
@@ -86,28 +92,32 @@ static int s2n_rsa_verify(const struct s2n_pkey *pub, s2n_signature_algorithm si
8692

8793
static int s2n_rsa_encrypt(const struct s2n_pkey *pub, struct s2n_blob *in, struct s2n_blob *out)
8894
{
89-
S2N_ERROR_IF(out->size < s2n_rsa_encrypted_size(pub), S2N_ERR_NOMEM);
95+
uint32_t size = 0;
96+
GUARD_AS_POSIX(s2n_rsa_encrypted_size(pub, &size));
97+
S2N_ERROR_IF(out->size < size, S2N_ERR_NOMEM);
9098

9199
const s2n_rsa_public_key *key = &pub->key.rsa_key;
92-
int r = RSA_public_encrypt(in->size, (unsigned char *)in->data, (unsigned char *)out->data, key->rsa, RSA_PKCS1_PADDING);
100+
int r = RSA_public_encrypt(in->size, ( unsigned char * )in->data, ( unsigned char * )out->data, key->rsa,
101+
RSA_PKCS1_PADDING);
93102
S2N_ERROR_IF(r != out->size, S2N_ERR_SIZE_MISMATCH);
94103

95104
return 0;
96105
}
97106

98107
static int s2n_rsa_decrypt(const struct s2n_pkey *priv, struct s2n_blob *in, struct s2n_blob *out)
99108
{
100-
unsigned char intermediate[4096];
101-
const int expected_size = s2n_rsa_encrypted_size(priv);
109+
unsigned char intermediate[ 4096 ];
110+
uint32_t expected_size = 0;
111+
112+
GUARD_AS_POSIX(s2n_rsa_encrypted_size(priv, &expected_size));
102113

103-
GUARD(expected_size);
104114
S2N_ERROR_IF(expected_size > sizeof(intermediate), S2N_ERR_NOMEM);
105115
S2N_ERROR_IF(out->size > sizeof(intermediate), S2N_ERR_NOMEM);
106116

107117
GUARD_AS_POSIX(s2n_get_public_random_data(out));
108118

109119
const s2n_rsa_private_key *key = &priv->key.rsa_key;
110-
int r = RSA_private_decrypt(in->size, (unsigned char *)in->data, intermediate, key->rsa, RSA_NO_PADDING);
120+
int r = RSA_private_decrypt(in->size, ( unsigned char * )in->data, intermediate, key->rsa, RSA_NO_PADDING);
111121
S2N_ERROR_IF(r != expected_size, S2N_ERR_SIZE_MISMATCH);
112122

113123
s2n_constant_time_pkcs1_unpad_or_dont(out->data, intermediate, r, out->size);
@@ -117,14 +127,14 @@ static int s2n_rsa_decrypt(const struct s2n_pkey *priv, struct s2n_blob *in, str
117127

118128
static int s2n_rsa_keys_match(const struct s2n_pkey *pub, const struct s2n_pkey *priv)
119129
{
120-
uint8_t plain_inpad[36] = {1}, plain_outpad[36] = {0}, encpad[8192];
130+
uint8_t plain_inpad[ 36 ] = { 1 }, plain_outpad[ 36 ] = { 0 }, encpad[ 8192 ];
121131
struct s2n_blob plain_in = { 0 }, plain_out = { 0 }, enc = { 0 };
122132

123133
plain_in.data = plain_inpad;
124134
plain_in.size = sizeof(plain_inpad);
125135

126136
enc.data = encpad;
127-
enc.size = s2n_rsa_encrypted_size(pub);
137+
GUARD_AS_POSIX(s2n_rsa_encrypted_size(pub, &enc.size));
128138
lte_check(enc.size, sizeof(encpad));
129139
GUARD(s2n_rsa_encrypt(pub, &plain_in, &enc));
130140

@@ -140,9 +150,7 @@ static int s2n_rsa_keys_match(const struct s2n_pkey *pub, const struct s2n_pkey
140150
static int s2n_rsa_key_free(struct s2n_pkey *pkey)
141151
{
142152
struct s2n_rsa_key *rsa_key = &pkey->key.rsa_key;
143-
if (rsa_key->rsa == NULL) {
144-
return 0;
145-
}
153+
if (rsa_key->rsa == NULL) { return 0; }
146154

147155
RSA_free(rsa_key->rsa);
148156
rsa_key->rsa = NULL;
@@ -177,13 +185,13 @@ int s2n_evp_pkey_to_rsa_private_key(s2n_rsa_private_key *rsa_key, EVP_PKEY *evp_
177185

178186
int s2n_rsa_pkey_init(struct s2n_pkey *pkey)
179187
{
180-
pkey->size = &s2n_rsa_encrypted_size;
181-
pkey->sign = &s2n_rsa_sign;
182-
pkey->verify = &s2n_rsa_verify;
183-
pkey->encrypt = &s2n_rsa_encrypt;
184-
pkey->decrypt = &s2n_rsa_decrypt;
185-
pkey->match = &s2n_rsa_keys_match;
186-
pkey->free = &s2n_rsa_key_free;
188+
pkey->size = &s2n_rsa_encrypted_size;
189+
pkey->sign = &s2n_rsa_sign;
190+
pkey->verify = &s2n_rsa_verify;
191+
pkey->encrypt = &s2n_rsa_encrypt;
192+
pkey->decrypt = &s2n_rsa_decrypt;
193+
pkey->match = &s2n_rsa_keys_match;
194+
pkey->free = &s2n_rsa_key_free;
187195
pkey->check_key = &s2n_rsa_check_key_exists;
188196
return 0;
189197
}

‎crypto/s2n_rsa_pss.c‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,17 @@ int s2n_is_rsa_pss_certs_supported()
4141

4242
#if RSA_PSS_CERTS_SUPPORTED
4343

44-
static int s2n_rsa_pss_size(const struct s2n_pkey *key)
44+
static S2N_RESULT s2n_rsa_pss_size(const struct s2n_pkey *key, uint32_t *size_out)
4545
{
46-
notnull_check(key);
46+
ENSURE_REF(key);
47+
ENSURE_REF(size_out);
4748

4849
/* For more info, see: https://www.openssl.org/docs/man1.1.0/man3/EVP_PKEY_size.html */
49-
return EVP_PKEY_size(key->pkey);
50+
const int size = EVP_PKEY_size(key->pkey);
51+
GUARD_AS_RESULT(size);
52+
*size_out = size;
53+
54+
return S2N_RESULT_OK;
5055
}
5156

5257
static int s2n_rsa_is_private_key(RSA *rsa_key)

‎tests/unit/s2n_client_cert_verify_test.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,17 @@
2121

2222
#include "tls/s2n_tls.h"
2323
#include "tls/s2n_connection.h"
24+
#include "utils/s2n_result.h"
2425
#include "utils/s2n_safety.h"
2526

2627
const uint8_t test_signature_data[] = "I signed this";
2728
const uint32_t test_signature_size = sizeof(test_signature_data);
2829
const uint32_t test_max_signature_size = 2 * sizeof(test_signature_data);
2930

30-
static int test_size(const struct s2n_pkey *pkey)
31+
static S2N_RESULT test_size(const struct s2n_pkey *pkey, uint32_t *size_out)
3132
{
32-
return test_max_signature_size;
33+
*size_out = test_max_signature_size;
34+
return S2N_RESULT_OK;
3335
}
3436

3537
static int test_sign(const struct s2n_pkey *priv_key, s2n_signature_algorithm sig_alg,

‎tests/unit/s2n_ecdsa_test.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,9 @@ int main(int argc, char **argv)
141141
uint8_t inputpad[] = "Hello world!";
142142
struct s2n_blob signature = {0}, bad_signature = {0};
143143
struct s2n_hash_state hash_one = {0}, hash_two = {0};
144-
uint32_t maximum_signature_length = s2n_pkey_size(&priv_key);
145144

145+
uint32_t maximum_signature_length = 0;
146+
EXPECT_OK(s2n_pkey_size(&priv_key, &maximum_signature_length));
146147
EXPECT_SUCCESS(s2n_alloc(&signature, maximum_signature_length));
147148

148149
EXPECT_SUCCESS(s2n_hash_new(&hash_one));
@@ -173,7 +174,8 @@ int main(int argc, char **argv)
173174
}
174175

175176
/* Mismatched public/private key should fail verification */
176-
EXPECT_SUCCESS(s2n_alloc(&bad_signature, s2n_pkey_size(&unmatched_priv_key)));
177+
EXPECT_OK(s2n_pkey_size(&unmatched_priv_key, &maximum_signature_length));
178+
EXPECT_SUCCESS(s2n_alloc(&bad_signature, maximum_signature_length));
177179

178180
EXPECT_FAILURE(s2n_pkey_match(&pub_key, &unmatched_priv_key));
179181

‎tests/unit/s2n_pem_rsa_dhe_test.c‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,9 @@ int main(int argc, char **argv)
136136
struct s2n_hash_state tls12_one = {0};
137137
struct s2n_hash_state tls12_two = {0};
138138

139-
EXPECT_SUCCESS(s2n_alloc(&signature, s2n_pkey_size(&pub_key)));
139+
uint32_t maximum_signature_length = 0;
140+
EXPECT_OK(s2n_pkey_size(&pub_key, &maximum_signature_length));
141+
EXPECT_SUCCESS(s2n_alloc(&signature, maximum_signature_length));
140142

141143
if (s2n_hash_is_available(S2N_HASH_MD5_SHA1)) {
142144
/* TLS 1.0 use of RSA with DHE is not permitted when FIPS mode is set */

‎tls/s2n_async_pkey.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,8 @@ S2N_RESULT s2n_async_pkey_sign_sync(struct s2n_connection *conn, s2n_signature_a
262262
const struct s2n_pkey *pkey = conn->handshake_params.our_chain_and_key->private_key;
263263
DEFER_CLEANUP(struct s2n_blob signed_content = { 0 }, s2n_free);
264264

265-
uint32_t maximum_signature_length = s2n_pkey_size(pkey);
265+
uint32_t maximum_signature_length = 0;
266+
GUARD_RESULT(s2n_pkey_size(pkey, &maximum_signature_length));
266267
GUARD_AS_RESULT(s2n_alloc(&signed_content, maximum_signature_length));
267268

268269
GUARD_AS_RESULT(s2n_pkey_sign(pkey, sig_alg, digest, &signed_content));
@@ -378,7 +379,8 @@ S2N_RESULT s2n_async_pkey_sign_perform(struct s2n_async_pkey_op *op, s2n_cert_pr
378379

379380
struct s2n_async_pkey_sign_data *sign = &op->op.sign;
380381

381-
uint32_t maximum_signature_length = s2n_pkey_size(pkey);
382+
uint32_t maximum_signature_length = 0;
383+
GUARD_RESULT(s2n_pkey_size(pkey, &maximum_signature_length));
382384
GUARD_AS_RESULT(s2n_alloc(&sign->signature, maximum_signature_length));
383385

384386
GUARD_AS_RESULT(s2n_pkey_sign(pkey, sign->sig_alg, &sign->digest, &sign->signature));

‎tls/s2n_client_cert_verify.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ int s2n_client_cert_verify_send(struct s2n_connection *conn)
7676
struct s2n_cert_chain_and_key *cert_chain_and_key = conn->handshake_params.our_chain_and_key;
7777

7878
DEFER_CLEANUP(struct s2n_blob signature = {0}, s2n_free);
79-
uint32_t max_signature_size = s2n_pkey_size(cert_chain_and_key->private_key);
79+
uint32_t max_signature_size = 0;
80+
GUARD_AS_POSIX(s2n_pkey_size(cert_chain_and_key->private_key, &max_signature_size));
8081
GUARD(s2n_alloc(&signature, max_signature_size));
8182

8283
GUARD(s2n_pkey_sign(cert_chain_and_key->private_key, chosen_sig_scheme.sig_alg, &conn->handshake.ccv_hash_copy, &signature));

0 commit comments

Comments
 (0)