Skip to content

Commit 4207584

Browse files
Free existing responderId union arm in OCSP_RESPID setters (#3234)
### Issues: Addresses V2196133741 (F9 — OCSP_RESPID_set_by_{key,name}: type-confused free on responderId union) ### Description of changes: `OCSP_RESPID` stores either an `X509_NAME*` (byName) or an `ASN1_OCTET_STRING*` (byKey) in a union. Re-setting the responderId leaked the prior arm or freed it through the wrong destructor, depending on the transition. Both setters now call a small `ocsp_respid_clear` helper that frees the arm matching the current `respid->type` before installing the new value, so every sequence (name↔name, name↔key, key↔key, key↔name) is correct. ### Call-outs: None. ### Testing: Extends `OCSPResponseSignTestExtended.OCSPResponseSign` with four sequential `OCSP_basic_sign` calls on one `OCSP_BASICRESP`, covering all four responderId transitions. ASAN in CI will surface any leak or misuse. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
1 parent 6664239 commit 4207584

2 files changed

Lines changed: 54 additions & 2 deletions

File tree

crypto/ocsp/ocsp_server.c

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,33 @@ int OCSP_basic_add1_cert(OCSP_BASICRESP *resp, X509 *cert) {
6565
return 1;
6666
}
6767

68+
// ocsp_respid_clear frees any previously-installed content of |respid| using
69+
// the destructor matching the current |respid->type|, so callers can install
70+
// a new value without leaking or triggering a type-confused free.
71+
static void ocsp_respid_clear(OCSP_RESPID *respid) {
72+
switch (respid->type) {
73+
case V_OCSP_RESPID_NAME:
74+
X509_NAME_free(respid->value.byName);
75+
respid->value.byName = NULL;
76+
break;
77+
case V_OCSP_RESPID_KEY:
78+
ASN1_OCTET_STRING_free(respid->value.byKey);
79+
respid->value.byKey = NULL;
80+
break;
81+
}
82+
}
83+
6884
static int OCSP_RESPID_set_by_name(OCSP_RESPID *respid, X509 *cert) {
6985
GUARD_PTR(respid);
7086
GUARD_PTR(cert);
71-
if (!X509_NAME_set(&respid->value.byName, X509_get_subject_name(cert))) {
87+
88+
X509_NAME *byName = X509_NAME_dup(X509_get_subject_name(cert));
89+
if (byName == NULL) {
7290
return 0;
7391
}
74-
92+
ocsp_respid_clear(respid);
7593
respid->type = V_OCSP_RESPID_NAME;
94+
respid->value.byName = byName;
7695
return 1;
7796
}
7897

@@ -95,6 +114,7 @@ static int OCSP_RESPID_set_by_key(OCSP_RESPID *respid, X509 *cert) {
95114
ASN1_OCTET_STRING_free(byKey);
96115
return 0;
97116
}
117+
ocsp_respid_clear(respid);
98118
respid->type = V_OCSP_RESPID_KEY;
99119
respid->value.byKey = byKey;
100120

crypto/ocsp/ocsp_test.cc

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,6 +1293,38 @@ TEST(OCSPResponseSignTestExtended, OCSPResponseSign) {
12931293
pkey.get(), EVP_sha256(), additional_cert.get(),
12941294
OCSP_NOCERTS));
12951295
EXPECT_EQ((int)sk_X509_num(basic_response.get()->certs), 0);
1296+
1297+
// Regression: re-signing the same |OCSP_BASICRESP| through any sequence of
1298+
// |OCSP_RESPID_KEY| flag combinations must not leak or free the prior
1299+
// responderId union arm through the wrong destructor. ASAN should flag any
1300+
// misuse.
1301+
basic_response.reset(OCSP_BASICRESP_new());
1302+
ASSERT_TRUE(basic_response);
1303+
EXPECT_TRUE(OCSP_basic_sign(basic_response.get(), signer_cert.get(),
1304+
pkey.get(), EVP_sha256(), additional_cert.get(),
1305+
0));
1306+
EXPECT_EQ(basic_response.get()->tbsResponseData->responderId->type,
1307+
V_OCSP_RESPID_NAME);
1308+
EXPECT_TRUE(OCSP_basic_sign(basic_response.get(), signer_cert.get(),
1309+
pkey.get(), EVP_sha256(), additional_cert.get(),
1310+
OCSP_RESPID_KEY));
1311+
EXPECT_EQ(basic_response.get()->tbsResponseData->responderId->type,
1312+
V_OCSP_RESPID_KEY);
1313+
EXPECT_TRUE(OCSP_basic_sign(basic_response.get(), signer_cert.get(),
1314+
pkey.get(), EVP_sha256(), additional_cert.get(),
1315+
OCSP_RESPID_KEY));
1316+
EXPECT_EQ(basic_response.get()->tbsResponseData->responderId->type,
1317+
V_OCSP_RESPID_KEY);
1318+
EXPECT_TRUE(OCSP_basic_sign(basic_response.get(), signer_cert.get(),
1319+
pkey.get(), EVP_sha256(), additional_cert.get(),
1320+
0));
1321+
EXPECT_EQ(basic_response.get()->tbsResponseData->responderId->type,
1322+
V_OCSP_RESPID_NAME);
1323+
EXPECT_TRUE(OCSP_basic_sign(basic_response.get(), signer_cert.get(),
1324+
pkey.get(), EVP_sha256(), additional_cert.get(),
1325+
0));
1326+
EXPECT_EQ(basic_response.get()->tbsResponseData->responderId->type,
1327+
V_OCSP_RESPID_NAME);
12961328
}
12971329

12981330
static const char extended_good_http_request_hdr[] =

0 commit comments

Comments
 (0)