Skip to content

Commit 005eb9d

Browse files
ylavicylavic
committed
Merge r1893199, r1893201, r1893203, r1893270, r1893271 from trunk:
apr_crypto: fixes for thread local storage destructor. cprng_thread_destroy() should destroy the given cprng first because this might set cprng_global to NULL. apr_crypto_prng_init() shouldn't delete cprng_thread_key on failure unless APR_CRYPTO_PRNG_PER_THREAD is used. apr_crypto: fix potential memory leaks of cprng_stream_ctx_t. This can happen in cprng_stream_ctx_make() on error paths, or at thread exit with APR_CRYPTO_PRNG_PER_THREAD like the below. Direct leak of 64 byte(s) in 8 object(s) allocated from: #0 0x7efd954c7628 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x107628) #1 0x7efd921db6ca (<unknown module>) #2 0x7efd952937a2 in apr_crypto_prng_create crypto/apr_crypto_prng.c:367 #3 0x7efd95292c1e in apr_crypto_random_thread_bytes crypto/apr_crypto_prng.c:218 #4 0x5611dbbb9440 in thread_func /home/yle/src/apache/apr/trunk.ro/test/testcrypto.c:2597 #5 0x7efd9537dd86 in dummy_worker threadproc/unix/thread.c:148 #6 0x7efd951efea6 in start_thread nptl/pthread_create.c:477 apr_crypto: s/APR_CRYPTO_CIPHER_CHACHA20_CTR/APR_CRYPTO_CIPHER_CHACHA20/g Chacha is a stream cipher, not a block cipher in counter mode. apr_crypto: cprng_stream_ctx_make() to return NULL *ctx on failure. apr_crypto_prng_create() relies on cprng_stream_ctx_make() to not set a freed/dangling cprng->ctx on failure when given NULL. apr_crypto_prng: cleanup after ourselves when apr_crypto_prng_init() fails. Submitted by: ylavic git-svn-id: https://svn.apache.org/repos/asf/apr/apr-util/branches/1.7.x@1893278 13f79535-47bb-0310-9956-ffa450edef68
1 parent cdd3d48 commit 005eb9d

File tree

4 files changed

+51
-22
lines changed

4 files changed

+51
-22
lines changed

‎crypto/apr_crypto_openssl.c

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ struct apr_crypto_digest_t {
106106

107107
struct cprng_stream_ctx_t {
108108
EVP_CIPHER_CTX *ctx;
109+
int malloced;
109110
};
110111

111112
static struct apr_crypto_block_key_digest_t key_digests[] =
@@ -1531,25 +1532,39 @@ static apr_status_t crypto_digest(
15311532
return status;
15321533
}
15331534

1535+
static void cprng_stream_ctx_free(cprng_stream_ctx_t *sctx)
1536+
{
1537+
if (sctx->ctx) {
1538+
EVP_CIPHER_CTX_free(sctx->ctx);
1539+
}
1540+
if (sctx->malloced) {
1541+
free(sctx);
1542+
}
1543+
}
1544+
15341545
static apr_status_t cprng_stream_ctx_make(cprng_stream_ctx_t **psctx,
15351546
apr_crypto_t *f, apr_crypto_cipher_e cipher, apr_pool_t *pool)
15361547
{
15371548
cprng_stream_ctx_t *sctx;
15381549
EVP_CIPHER_CTX *ctx;
15391550
const EVP_CIPHER *ecipher;
15401551

1552+
*psctx = NULL;
1553+
15411554
if (pool) {
1542-
*psctx = sctx = apr_palloc(pool, sizeof(cprng_stream_ctx_t));
1555+
sctx = apr_palloc(pool, sizeof(cprng_stream_ctx_t));
15431556
}
15441557
else {
1545-
*psctx = sctx = malloc(sizeof(cprng_stream_ctx_t));
1558+
sctx = malloc(sizeof(cprng_stream_ctx_t));
15461559
}
15471560
if (!sctx) {
15481561
return APR_ENOMEM;
15491562
}
15501563

1564+
sctx->malloced = !pool;
15511565
sctx->ctx = ctx = EVP_CIPHER_CTX_new();
15521566
if (!ctx) {
1567+
cprng_stream_ctx_free(sctx);
15531568
return APR_ENOMEM;
15541569
}
15551570

@@ -1567,6 +1582,7 @@ static apr_status_t cprng_stream_ctx_make(cprng_stream_ctx_t **psctx,
15671582
#elif defined(NID_aes_256_ctr)
15681583
ecipher = EVP_aes_256_ctr();
15691584
#else
1585+
cprng_stream_ctx_free(sctx);
15701586
return APR_ENOCIPHER;
15711587
#endif
15721588
}
@@ -1575,35 +1591,34 @@ static apr_status_t cprng_stream_ctx_make(cprng_stream_ctx_t **psctx,
15751591
ecipher = EVP_aes_256_ctr();
15761592
break;
15771593
#else
1594+
cprng_stream_ctx_free(sctx);
15781595
return APR_ENOCIPHER;
15791596
#endif
15801597
}
1581-
case APR_CRYPTO_CIPHER_CHACHA20_CTR: {
1598+
case APR_CRYPTO_CIPHER_CHACHA20: {
15821599
#if defined(NID_chacha20)
15831600
ecipher = EVP_chacha20();
15841601
break;
15851602
#else
1603+
cprng_stream_ctx_free(sctx);
15861604
return APR_ENOCIPHER;
15871605
#endif
15881606
}
15891607
default: {
1608+
cprng_stream_ctx_free(sctx);
15901609
return APR_ENOCIPHER;
15911610
}
15921611
}
15931612

15941613
if (EVP_EncryptInit_ex(ctx, ecipher, f->config->engine, NULL, NULL) <= 0) {
1595-
EVP_CIPHER_CTX_free(ctx);
1614+
cprng_stream_ctx_free(sctx);
15961615
return APR_ENOMEM;
15971616
}
15981617

1618+
*psctx = sctx;
15991619
return APR_SUCCESS;
16001620
}
16011621

1602-
static void cprng_stream_ctx_free(cprng_stream_ctx_t *sctx)
1603-
{
1604-
EVP_CIPHER_CTX_free(sctx->ctx);
1605-
}
1606-
16071622
static APR_INLINE
16081623
void cprng_stream_setkey(cprng_stream_ctx_t *sctx,
16091624
const unsigned char *key,

‎crypto/apr_crypto_prng.c

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,14 @@ static apr_threadkey_t *cprng_thread_key = NULL;
118118

119119
static void cprng_thread_destroy(void *cprng)
120120
{
121+
apr_threadkey_private_set(NULL, cprng_thread_key);
122+
if (cprng) {
123+
apr_crypto_prng_destroy(cprng);
124+
}
121125
if (!cprng_global) {
122126
apr_threadkey_private_delete(cprng_thread_key);
123127
cprng_thread_key = NULL;
124128
}
125-
apr_crypto_prng_destroy(cprng);
126129
}
127130

128131
#else /* !APR_HAS_THREADS */
@@ -141,6 +144,12 @@ APU_DECLARE(apr_status_t) apr_crypto_prng_init(apr_pool_t *pool, apr_crypto_t *c
141144
return APR_EREINIT;
142145
}
143146

147+
cprng_ring = apr_palloc(pool, sizeof(*cprng_ring));
148+
if (!cprng_ring) {
149+
return APR_ENOMEM;
150+
}
151+
APR_RING_INIT(cprng_ring, apr_crypto_prng_t, link);
152+
144153
if (flags & APR_CRYPTO_PRNG_PER_THREAD) {
145154
#if !APR_HAS_THREADS
146155
return APR_ENOTIMPL;
@@ -153,27 +162,32 @@ APU_DECLARE(apr_status_t) apr_crypto_prng_init(apr_pool_t *pool, apr_crypto_t *c
153162
#endif
154163
}
155164

156-
cprng_ring = apr_palloc(pool, sizeof(*cprng_ring));
157-
if (!cprng_ring) {
158-
return APR_ENOMEM;
159-
}
160-
APR_RING_INIT(cprng_ring, apr_crypto_prng_t, link);
161-
162165
#if APR_HAS_THREADS
163166
rv = apr_thread_mutex_create(&cprng_ring_mutex, APR_THREAD_MUTEX_DEFAULT,
164167
pool);
165168
if (rv != APR_SUCCESS) {
166-
apr_threadkey_private_delete(cprng_thread_key);
167-
cprng_thread_key = NULL;
169+
if (flags & APR_CRYPTO_PRNG_PER_THREAD) {
170+
apr_threadkey_private_delete(cprng_thread_key);
171+
cprng_thread_key = NULL;
172+
}
168173
return rv;
169174
}
170175

171176
/* Global CPRNG is locked (and obviously not per-thread) */
172177
flags = (flags | APR_CRYPTO_PRNG_LOCKED) & ~APR_CRYPTO_PRNG_PER_THREAD;
173178
#endif
174179

175-
return apr_crypto_prng_create(&cprng_global, crypto, cipher, bufsize, flags,
176-
seed, pool);
180+
rv = apr_crypto_prng_create(&cprng_global, crypto, cipher, bufsize, flags,
181+
seed, pool);
182+
if (rv != APR_SUCCESS) {
183+
if (flags & APR_CRYPTO_PRNG_PER_THREAD) {
184+
apr_threadkey_private_delete(cprng_thread_key);
185+
cprng_thread_key = NULL;
186+
}
187+
return rv;
188+
}
189+
190+
return APR_SUCCESS;
177191
}
178192

179193
APU_DECLARE(apr_status_t) apr_crypto_prng_term(void)

‎include/apr_crypto.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ typedef enum
165165
{
166166
APR_CRYPTO_CIPHER_AUTO, /** Choose the recommended cipher / autodetect the cipher */
167167
APR_CRYPTO_CIPHER_AES_256_CTR, /** AES 256 - CTR mode */
168-
APR_CRYPTO_CIPHER_CHACHA20_CTR, /** ChaCha20 - CTR mode */
168+
APR_CRYPTO_CIPHER_CHACHA20, /** ChaCha20 */
169169
} apr_crypto_cipher_e;
170170

171171
/**

‎test/testcrypto.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2514,7 +2514,7 @@ static void test_crypto_prng_aes256(abts_case *tc, void *data)
25142514

25152515
static void test_crypto_prng_chacha20(abts_case *tc, void *data)
25162516
{
2517-
return test_crypto_prng(tc, APR_CRYPTO_CIPHER_CHACHA20_CTR, test_PRNG_kat0_chacha20);
2517+
return test_crypto_prng(tc, APR_CRYPTO_CIPHER_CHACHA20, test_PRNG_kat0_chacha20);
25182518
}
25192519

25202520
#if APR_HAS_FORK

0 commit comments

Comments
 (0)