Skip to content

Commit 8a6a595

Browse files
Fix use of uninitialized memory and consistency of code handling allocations (#133)
* Refactor logic to avoid use of uninitialized memory and improve consistency * Fix up a few more nitpicks + Potential memory leak in common gcm init ctx (never hit with provider, but could be hit from engine) + Consistency of dupctx ERR and assignment * Fix some unhandled OPENSSL_strdup failures
1 parent e3dd13a commit 8a6a595

18 files changed

+128
-125
lines changed

ScosslCommon/src/scossl_aes_aead.c

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,14 @@ SCOSSL_STATUS scossl_aes_gcm_init_ctx(SCOSSL_CIPHER_GCM_CTX *ctx, const unsigned
2121
ctx->useInvocation = 0;
2222
ctx->ivlen = SCOSSL_GCM_DEFAULT_IV_LENGTH;
2323

24-
if (iv != NULL && (ctx->iv = OPENSSL_memdup(iv, ctx->ivlen)) == NULL)
24+
if (iv != NULL)
2525
{
26-
return SCOSSL_FAILURE;
26+
OPENSSL_free(ctx->iv);
27+
28+
if ((ctx->iv = OPENSSL_memdup(iv, ctx->ivlen)) == NULL)
29+
{
30+
return SCOSSL_FAILURE;
31+
}
2732
}
2833

2934
return SCOSSL_SUCCESS;
@@ -431,7 +436,7 @@ void scossl_aes_ccm_init_ctx(SCOSSL_CIPHER_CCM_CTX *ctx,
431436
const unsigned char *iv)
432437
{
433438
ctx->ivlen = SCOSSL_CCM_MIN_IV_LENGTH;
434-
if (iv)
439+
if (iv != NULL)
435440
{
436441
memcpy(ctx->iv, iv, ctx->ivlen);
437442
}

ScosslCommon/src/scossl_mac.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,12 @@ SCOSSL_MAC_CTX *scossl_mac_dupctx(SCOSSL_MAC_CTX *ctx)
129129
}
130130
}
131131

132-
copyCtx->mdName = OPENSSL_strdup(ctx->mdName);
132+
if (ctx->mdName != NULL &&
133+
(copyCtx->mdName = OPENSSL_strdup(ctx->mdName)) == NULL)
134+
{
135+
goto cleanup;
136+
}
137+
133138
copyCtx->libctx = ctx->libctx;
134139
}
135140

ScosslCommon/src/scossl_tls1prf.c

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,23 +17,18 @@ _Use_decl_annotations_
1717
SCOSSL_TLS1_PRF_CTX *scossl_tls1prf_dupctx(SCOSSL_TLS1_PRF_CTX *ctx)
1818
{
1919
SCOSSL_TLS1_PRF_CTX *copyCtx = OPENSSL_malloc(sizeof(SCOSSL_TLS1_PRF_CTX));
20+
2021
if (copyCtx != NULL)
2122
{
22-
if (ctx->pbSecret == NULL)
23-
{
24-
copyCtx->pbSecret = NULL;
25-
}
26-
else if ((copyCtx->pbSecret = OPENSSL_memdup(ctx->pbSecret, ctx->cbSecret)) == NULL)
23+
*copyCtx = *ctx;
24+
copyCtx->pbSecret = NULL;
25+
26+
if (ctx->pbSecret != NULL &&
27+
(copyCtx->pbSecret = OPENSSL_memdup(ctx->pbSecret, ctx->cbSecret)) == NULL)
2728
{
2829
scossl_tls1prf_freectx(copyCtx);
29-
return NULL;
30+
copyCtx = NULL;
3031
}
31-
32-
copyCtx->isTlsPrf1_1 = ctx->isTlsPrf1_1;
33-
copyCtx->pHmac = ctx->pHmac;
34-
copyCtx->cbSecret = ctx->cbSecret;
35-
copyCtx->cbSeed = ctx->cbSeed;
36-
memcpy(copyCtx->seed, ctx->seed, ctx->cbSeed);
3732
}
3833

3934
return copyCtx;

SymCryptProvider/src/ciphers/p_scossl_aes_aead.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,11 @@ static SCOSSL_CIPHER_GCM_CTX *p_scossl_aes_gcm_dupctx(_In_ SCOSSL_CIPHER_GCM_CTX
6565
SCOSSL_COMMON_ALIGNED_ALLOC(copy_ctx, OPENSSL_malloc, SCOSSL_CIPHER_GCM_CTX);
6666
if (copy_ctx != NULL)
6767
{
68-
memcpy(copy_ctx, ctx, sizeof(SCOSSL_CIPHER_GCM_CTX));
68+
*copy_ctx = *ctx;
6969

7070
if (ctx->iv != NULL && (copy_ctx->iv = OPENSSL_memdup(ctx->iv, ctx->ivlen)) == NULL)
7171
{
72+
ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE);
7273
p_scossl_aes_gcm_freectx(copy_ctx);
7374
return NULL;
7475
}
@@ -552,7 +553,6 @@ static SCOSSL_STATUS p_scossl_aes_ccm_set_ctx_params(_Inout_ SCOSSL_CIPHER_CCM_C
552553
if (ctx != NULL) \
553554
{ \
554555
ctx->keylen = kbits >> 3; \
555-
ctx->ivlen = defaultIvLen; \
556556
scossl_aes_##lcmode##_init_ctx(ctx, NULL); \
557557
} \
558558
\

SymCryptProvider/src/ciphers/p_scossl_aes_xts.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ static SCOSSL_STATUS p_scossl_aes_xts_set_ctx_params(_Inout_ SCOSSL_AES_XTS_CTX
4040

4141
static SCOSSL_AES_XTS_CTX *p_scossl_aes_xts_newctx_internal(size_t keylen)
4242
{
43-
SCOSSL_COMMON_ALIGNED_ALLOC(ctx, OPENSSL_malloc, SCOSSL_AES_XTS_CTX);
43+
SCOSSL_COMMON_ALIGNED_ALLOC(ctx, OPENSSL_zalloc, SCOSSL_AES_XTS_CTX);
4444
if (ctx != NULL)
4545
{
4646
ctx->keylen = keylen;

SymCryptProvider/src/digests/p_scossl_cshake.c

Lines changed: 9 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -132,43 +132,21 @@ static SCOSSL_CSHAKE_CTX *p_scossl_cshake_dupctx(_In_ SCOSSL_CSHAKE_CTX *ctx)
132132

133133
SCOSSL_COMMON_ALIGNED_ALLOC(copyCtx, OPENSSL_zalloc, SCOSSL_CSHAKE_CTX);
134134

135-
if (ctx != NULL)
135+
if (copyCtx != NULL)
136136
{
137-
if (ctx->pbFunctionNameString != NULL)
138-
{
139-
copyCtx->pbFunctionNameString = OPENSSL_memdup(ctx->pbFunctionNameString, ctx->cbFunctionNameString);
140-
if (copyCtx->pbFunctionNameString == NULL)
141-
{
142-
ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE);
143-
goto cleanup;
144-
}
145-
}
146-
else
147-
{
148-
copyCtx->pbFunctionNameString = NULL;
149-
}
150-
copyCtx->cbFunctionNameString = ctx->cbFunctionNameString;
137+
*copyCtx = *ctx;
151138

152-
if (ctx->pbCustomizationString != NULL)
153-
{
154-
copyCtx->pbCustomizationString = OPENSSL_memdup(ctx->pbCustomizationString, ctx->cbCustomizationString);
155-
if (copyCtx->pbCustomizationString == NULL)
156-
{
157-
ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE);
158-
goto cleanup;
159-
}
160-
}
161-
else
139+
copyCtx->pbFunctionNameString = OPENSSL_memdup(ctx->pbFunctionNameString, ctx->cbFunctionNameString);
140+
copyCtx->pbCustomizationString = OPENSSL_memdup(ctx->pbCustomizationString, ctx->cbCustomizationString);
141+
142+
if ((ctx->pbFunctionNameString != NULL && copyCtx->pbFunctionNameString == NULL) ||
143+
(ctx->pbCustomizationString != NULL && copyCtx->pbCustomizationString == NULL))
162144
{
163-
copyCtx->pbCustomizationString = NULL;
145+
ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE);
146+
goto cleanup;
164147
}
165-
copyCtx->cbCustomizationString = ctx->cbCustomizationString;
166148

167149
ctx->pHash->stateCopyFunc(&ctx->state, &copyCtx->state);
168-
169-
copyCtx->pHash = ctx->pHash;
170-
copyCtx->xofState = ctx->xofState;
171-
copyCtx->xofLen = ctx->xofLen;
172150
}
173151

174152
status = SCOSSL_SUCCESS;

SymCryptProvider/src/kdf/p_scossl_hkdf.c

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,12 +62,11 @@ SCOSSL_PROV_HKDF_CTX *p_scossl_hkdf_newctx(_In_ SCOSSL_PROVCTX *provctx)
6262

6363
void p_scossl_hkdf_freectx(_Inout_ SCOSSL_PROV_HKDF_CTX *ctx)
6464
{
65-
if (ctx != NULL)
66-
{
67-
EVP_MD_free(ctx->hkdfCtx->md);
68-
scossl_hkdf_freectx(ctx->hkdfCtx);
69-
}
65+
if (ctx == NULL)
66+
return;
7067

68+
EVP_MD_free(ctx->hkdfCtx->md);
69+
scossl_hkdf_freectx(ctx->hkdfCtx);
7170
OPENSSL_free(ctx);
7271
}
7372

SymCryptProvider/src/kdf/p_scossl_pbkdf2.c

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ SCOSSL_PROV_PBKDF2_CTX *p_scossl_pbkdf2_dupctx(_In_ SCOSSL_PROV_PBKDF2_CTX *ctx)
7878
{
7979
SCOSSL_STATUS status = SCOSSL_FAILURE;
8080

81-
SCOSSL_PROV_PBKDF2_CTX *copyCtx = OPENSSL_malloc(sizeof(SCOSSL_PROV_PBKDF2_CTX));
81+
SCOSSL_PROV_PBKDF2_CTX *copyCtx = OPENSSL_zalloc(sizeof(SCOSSL_PROV_PBKDF2_CTX));
8282
if (copyCtx != NULL)
8383
{
8484
copyCtx->libctx = ctx->libctx;
@@ -98,17 +98,12 @@ SCOSSL_PROV_PBKDF2_CTX *p_scossl_pbkdf2_dupctx(_In_ SCOSSL_PROV_PBKDF2_CTX *ctx)
9898
memcpy(copyCtx->pbPassword, ctx->pbPassword, ctx->cbPassword);
9999
copyCtx->cbPassword = ctx->cbPassword;
100100
}
101-
else
102-
{
103-
copyCtx->pbPassword = NULL;
104-
copyCtx->cbPassword = 0;
105-
}
106101

107-
if ((copyCtx->pbSalt = OPENSSL_memdup(ctx->pbSalt, ctx->cbSalt)) == NULL &&
108-
ctx->pbSalt != NULL)
102+
if (ctx->pbSalt != NULL &&
103+
(copyCtx->pbSalt = OPENSSL_memdup(ctx->pbSalt, ctx->cbSalt)) == NULL)
109104
{
110105
ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE);
111-
goto cleanup;
106+
goto cleanup;
112107
}
113108
copyCtx->cbSalt = ctx->cbSalt;
114109
}

SymCryptProvider/src/kdf/p_scossl_srtpkdf.c

Lines changed: 3 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,9 @@ static SCOSSL_PROV_SRTPKDF_CTX *p_scossl_srtpkdf_dupctx(_In_ SCOSSL_PROV_SRTPKDF
9494

9595
if (copyCtx != NULL)
9696
{
97+
*copyCtx = *ctx;
98+
copyCtx->pbKey = NULL;
99+
97100
if (ctx->pbKey != NULL)
98101
{
99102
if ((copyCtx->pbKey = OPENSSL_secure_malloc(ctx->cbKey)) == NULL)
@@ -103,7 +106,6 @@ static SCOSSL_PROV_SRTPKDF_CTX *p_scossl_srtpkdf_dupctx(_In_ SCOSSL_PROV_SRTPKDF
103106
}
104107

105108
memcpy(copyCtx->pbKey, ctx->pbKey, ctx->cbKey);
106-
copyCtx->cbKey = ctx->cbKey;
107109

108110
scError = SymCryptSrtpKdfExpandKey(&copyCtx->expandedKey, copyCtx->pbKey, copyCtx->cbKey);
109111
if (scError != SYMCRYPT_NO_ERROR)
@@ -112,23 +114,6 @@ static SCOSSL_PROV_SRTPKDF_CTX *p_scossl_srtpkdf_dupctx(_In_ SCOSSL_PROV_SRTPKDF
112114
goto cleanup;
113115
}
114116
}
115-
else
116-
{
117-
copyCtx->pbKey = NULL;
118-
copyCtx->cbKey = 0;
119-
}
120-
121-
if (ctx->isSaltSet)
122-
{
123-
memcpy(copyCtx->pbSalt, ctx->pbSalt, SCOSSL_SRTP_KDF_SALT_SIZE);
124-
}
125-
126-
copyCtx->isSrtcp = ctx->isSrtcp;
127-
copyCtx->isSaltSet = ctx->isSaltSet;
128-
copyCtx->uKeyDerivationRate = ctx->uKeyDerivationRate;
129-
copyCtx->uIndex = ctx->uIndex;
130-
copyCtx->uIndexWidth = ctx->uIndexWidth;
131-
copyCtx->label = ctx->label;
132117
}
133118

134119
status = SCOSSL_SUCCESS;

SymCryptProvider/src/kdf/p_scossl_sshkdf.c

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -62,28 +62,44 @@ SCOSSL_PROV_SSHKDF_CTX *p_scossl_sshkdf_newctx(_In_ SCOSSL_PROVCTX *provctx)
6262

6363
void p_scossl_sshkdf_freectx(_Inout_ SCOSSL_PROV_SSHKDF_CTX *ctx)
6464
{
65-
if (ctx != NULL)
66-
{
67-
OPENSSL_free(ctx->mdName);
68-
scossl_sshkdf_freectx(ctx->sshkdfCtx);
69-
}
70-
65+
if (ctx == NULL)
66+
return;
67+
68+
OPENSSL_free(ctx->mdName);
69+
scossl_sshkdf_freectx(ctx->sshkdfCtx);
7170
OPENSSL_free(ctx);
7271
}
7372

7473
SCOSSL_PROV_SSHKDF_CTX *p_scossl_sshkdf_dupctx(_In_ SCOSSL_PROV_SSHKDF_CTX *ctx)
7574
{
76-
SCOSSL_PROV_SSHKDF_CTX *copyCtx = OPENSSL_malloc(sizeof(SCOSSL_PROV_SSHKDF_CTX));
75+
SCOSSL_STATUS status = SCOSSL_FAILURE;
76+
77+
SCOSSL_PROV_SSHKDF_CTX *copyCtx = OPENSSL_zalloc(sizeof(SCOSSL_PROV_SSHKDF_CTX));
7778
if (copyCtx != NULL)
7879
{
7980
if ((copyCtx->sshkdfCtx = scossl_sshkdf_dupctx(ctx->sshkdfCtx)) == NULL)
8081
{
81-
OPENSSL_free(copyCtx);
82-
return NULL;
82+
ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE);
83+
goto cleanup;
84+
}
85+
86+
if (ctx->mdName != NULL &&
87+
(copyCtx->mdName = OPENSSL_strdup(ctx->mdName)) == NULL)
88+
{
89+
ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE);
90+
goto cleanup;
8391
}
8492

8593
copyCtx->libctx = ctx->libctx;
86-
copyCtx->mdName = OPENSSL_strdup(ctx->mdName);
94+
}
95+
96+
status = SCOSSL_SUCCESS;
97+
98+
cleanup:
99+
if (status != SCOSSL_SUCCESS)
100+
{
101+
p_scossl_sshkdf_freectx(copyCtx);
102+
copyCtx = NULL;
87103
}
88104

89105
return copyCtx;
@@ -207,6 +223,12 @@ SCOSSL_STATUS p_scossl_sshkdf_set_ctx_params(_Inout_ SCOSSL_PROV_SSHKDF_CTX *ctx
207223
symcryptHashAlg = scossl_get_symcrypt_hash_algorithm(EVP_MD_type(md));
208224
EVP_MD_free(md);
209225

226+
if (mdName == NULL)
227+
{
228+
ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE);
229+
return SCOSSL_FAILURE;
230+
}
231+
210232
if (symcryptHashAlg == NULL)
211233
{
212234
OPENSSL_free(mdName);

0 commit comments

Comments
 (0)