Skip to content

Commit 0430b7d

Browse files
dkosticdkostic
authored
Refactor PQDSA_KEY set_raw functions to use goto-err cleanup (#2993)
Consolidates error handling, fixes memory leaks on reuse, and ensures atomic key assignment (no partial state on failure).
1 parent 167015c commit 0430b7d

1 file changed

Lines changed: 86 additions & 97 deletions

File tree

‎crypto/fipsmodule/pqdsa/pqdsa.c‎

Lines changed: 86 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -93,50 +93,42 @@ int PQDSA_KEY_set_raw_keypair_from_seed(PQDSA_KEY *key, CBS *in) {
9393
return 0;
9494
}
9595

96-
//allocate buffers to store key pair
96+
int ret = 0;
9797
uint8_t *public_key = OPENSSL_malloc(key->pqdsa->public_key_len);
98-
if (public_key == NULL) {
99-
return 0;
100-
}
101-
10298
uint8_t *private_key = OPENSSL_malloc(key->pqdsa->private_key_len);
103-
if (private_key == NULL) {
104-
OPENSSL_free(public_key);
105-
return 0;
106-
}
107-
10899
uint8_t *seed = OPENSSL_malloc(key->pqdsa->keygen_seed_len);
109-
if (seed == NULL) {
110-
OPENSSL_free(private_key);
111-
OPENSSL_free(public_key);
112-
return 0;
100+
if (public_key == NULL || private_key == NULL || seed == NULL) {
101+
goto err;
113102
}
114103

115-
// attempt to generate the key from the provided seed
116-
if (!key->pqdsa->method->pqdsa_keygen_internal(public_key,
117-
private_key,
118-
CBS_data(in))) {
119-
OPENSSL_free(public_key);
120-
OPENSSL_free(private_key);
121-
OPENSSL_free(seed);
104+
if (!key->pqdsa->method->pqdsa_keygen_internal(public_key, private_key,
105+
CBS_data(in))) {
122106
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
123-
return 0;
107+
goto err;
124108
}
125109

126-
// copy the seed data
127110
if (!CBS_copy_bytes(in, seed, key->pqdsa->keygen_seed_len)) {
128-
OPENSSL_free(public_key);
129-
OPENSSL_free(private_key);
130-
OPENSSL_free(seed);
131111
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
132-
return 0;
112+
goto err;
133113
}
134114

135-
// set the public key, private key, and seed
115+
// Success: transfer ownership to key.
116+
OPENSSL_free(key->public_key);
117+
OPENSSL_free(key->private_key);
118+
OPENSSL_free(key->seed);
136119
key->public_key = public_key;
137120
key->private_key = private_key;
138121
key->seed = seed;
139-
return 1;
122+
public_key = NULL;
123+
private_key = NULL;
124+
seed = NULL;
125+
ret = 1;
126+
127+
err:
128+
OPENSSL_free(public_key);
129+
OPENSSL_free(private_key);
130+
OPENSSL_free(seed);
131+
return ret;
140132
}
141133

142134
int PQDSA_KEY_set_raw_private_key(PQDSA_KEY *key, CBS *in) {
@@ -146,29 +138,31 @@ int PQDSA_KEY_set_raw_private_key(PQDSA_KEY *key, CBS *in) {
146138
return 0;
147139
}
148140

149-
key->private_key = OPENSSL_memdup(CBS_data(in), key->pqdsa->private_key_len);
150-
if (key->private_key == NULL) {
151-
return 0;
152-
}
153-
154-
// Create buffers to store public key based on size
155-
size_t pk_len = key->pqdsa->public_key_len;
156-
uint8_t *public_key = OPENSSL_malloc(pk_len);
157-
158-
if (public_key == NULL) {
159-
return 0;
141+
int ret = 0;
142+
uint8_t *private_key = OPENSSL_memdup(CBS_data(in), key->pqdsa->private_key_len);
143+
uint8_t *public_key = OPENSSL_malloc(key->pqdsa->public_key_len);
144+
if (private_key == NULL || public_key == NULL) {
145+
goto err;
160146
}
161147

162-
// Construct the public key from the private key
163-
if (!key->pqdsa->method->pqdsa_pack_pk_from_sk(public_key, key->private_key)) {
164-
OPENSSL_free(public_key);
148+
if (!key->pqdsa->method->pqdsa_pack_pk_from_sk(public_key, private_key)) {
165149
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
166-
return 0;
150+
goto err;
167151
}
168152

153+
// Success: transfer ownership to key.
154+
OPENSSL_free(key->public_key);
155+
OPENSSL_free(key->private_key);
169156
key->public_key = public_key;
157+
key->private_key = private_key;
158+
public_key = NULL;
159+
private_key = NULL;
160+
ret = 1;
170161

171-
return 1;
162+
err:
163+
OPENSSL_free(public_key);
164+
OPENSSL_free(private_key);
165+
return ret;
172166
}
173167

174168
/*
@@ -183,88 +177,83 @@ int PQDSA_KEY_set_raw_private_key(PQDSA_KEY *key, CBS *in) {
183177
* 4. If consistent, stores the seed, expanded private key, and derived public key
184178
* in the PQDSA_KEY structure.
185179
*/
186-
int PQDSA_KEY_set_raw_keypair_from_both(PQDSA_KEY *key, CBS *seed, CBS *expanded_key) {
187-
// Check if the parsed lengths correspond with the expected lengths.
180+
int PQDSA_KEY_set_raw_keypair_from_both(PQDSA_KEY *key, CBS *seed,
181+
CBS *expanded_key) {
182+
// Check if the parsed length corresponds with the expected length.
188183
if (CBS_len(seed) != key->pqdsa->keygen_seed_len ||
189184
CBS_len(expanded_key) != key->pqdsa->private_key_len) {
190185
OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_BUFFER_SIZE);
191186
return 0;
192187
}
193188

194-
// first allocate buffers to store keypair from seed
195-
uint8_t *seed_public_key = OPENSSL_malloc(key->pqdsa->public_key_len);
196-
if (seed_public_key == NULL) {
197-
return 0;
198-
}
199-
200-
uint8_t *seed_private_key = OPENSSL_malloc(key->pqdsa->private_key_len);
201-
if (seed_private_key == NULL) {
202-
OPENSSL_free(seed_public_key);
203-
return 0;
189+
int ret = 0;
190+
uint8_t *seed_public_key = NULL;
191+
uint8_t *seed_private_key = NULL;
192+
uint8_t *expanded_public_key = NULL;
193+
uint8_t *new_private_key = NULL;
194+
uint8_t *new_seed = NULL;
195+
196+
// Allocate temp buffers for seed-derived keypair.
197+
seed_public_key = OPENSSL_malloc(key->pqdsa->public_key_len);
198+
seed_private_key = OPENSSL_malloc(key->pqdsa->private_key_len);
199+
if (seed_public_key == NULL || seed_private_key == NULL) {
200+
goto err;
204201
}
205202

206-
// generate the key from the provided seed
203+
// Generate keypair from seed.
207204
if (!key->pqdsa->method->pqdsa_keygen_internal(seed_public_key,
208205
seed_private_key,
209206
CBS_data(seed))) {
210-
OPENSSL_free(seed_public_key);
211-
OPENSSL_free(seed_private_key);
212207
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
213-
return 0;
208+
goto err;
214209
}
215210

216-
// allocate buffers to store derived public key from the provided expanded private
217-
uint8_t *expanded_public_key = OPENSSL_malloc(key->pqdsa->public_key_len);
211+
// Derive public key from the expanded private key.
212+
expanded_public_key = OPENSSL_malloc(key->pqdsa->public_key_len);
218213
if (expanded_public_key == NULL) {
219-
OPENSSL_free(seed_public_key);
220-
OPENSSL_free(seed_private_key);
221-
return 0;
214+
goto err;
222215
}
223216

224-
// construct the public key from the expanded private key
225217
if (!key->pqdsa->method->pqdsa_pack_pk_from_sk(expanded_public_key,
226218
CBS_data(expanded_key))) {
227-
OPENSSL_free(seed_public_key);
228-
OPENSSL_free(seed_private_key);
229-
OPENSSL_free(expanded_public_key);
230219
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
231-
return 0;
220+
goto err;
232221
}
233222

234-
// compare public keys for consistency
223+
// Compare public keys for consistency.
235224
if (CRYPTO_memcmp(seed_public_key, expanded_public_key,
236225
key->pqdsa->public_key_len) != 0) {
237-
OPENSSL_free(seed_public_key);
238-
OPENSSL_free(seed_private_key);
239-
OPENSSL_free(expanded_public_key);
240226
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
241-
return 0;
227+
goto err;
228+
}
229+
230+
// Allocate final copies of private key and seed.
231+
new_private_key = OPENSSL_memdup(CBS_data(expanded_key),
232+
key->pqdsa->private_key_len);
233+
new_seed = OPENSSL_memdup(CBS_data(seed), key->pqdsa->keygen_seed_len);
234+
if (new_private_key == NULL || new_seed == NULL) {
235+
goto err;
242236
}
243237

244-
// copy expanded private key and public key
238+
// Success: transfer ownership to key.
239+
OPENSSL_free(key->public_key);
240+
OPENSSL_free(key->private_key);
241+
OPENSSL_free(key->seed);
245242
key->public_key = expanded_public_key;
243+
key->private_key = new_private_key;
244+
key->seed = new_seed;
245+
expanded_public_key = NULL;
246+
new_private_key = NULL;
247+
new_seed = NULL;
248+
ret = 1;
249+
250+
err:
246251
OPENSSL_free(seed_public_key);
247252
OPENSSL_free(seed_private_key);
248-
249-
key->private_key = OPENSSL_memdup(CBS_data(expanded_key),
250-
key->pqdsa->private_key_len);
251-
if (key->private_key == NULL) {
252-
OPENSSL_free(key->public_key);
253-
key->public_key = NULL;
254-
return 0;
255-
}
256-
257-
// copy seed into key struct
258-
key->seed = OPENSSL_memdup(CBS_data(seed), key->pqdsa->keygen_seed_len);
259-
if (key->seed == NULL) {
260-
OPENSSL_free(key->private_key);
261-
key->private_key = NULL;
262-
OPENSSL_free(key->public_key);
263-
key->public_key = NULL;
264-
return 0;
265-
}
266-
267-
return 1;
253+
OPENSSL_free(expanded_public_key);
254+
OPENSSL_free(new_private_key);
255+
OPENSSL_free(new_seed);
256+
return ret;
268257
}
269258

270259
DEFINE_LOCAL_DATA(PQDSA_METHOD, sig_ml_dsa_44_method) {

0 commit comments

Comments
 (0)