From 7528f03c8ad73216f462f86e1d64d3d780a5cd42 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 8 Jul 2022 15:43:46 -0400 Subject: [PATCH] Fix memory leak with X509V3_ADD_DELETE. It seems no one has ever exercised this mode to notice it doesn't work. Add a test for this, which catches the memory leak. See also https://github.com/openssl/openssl/issues/18677 Change-Id: I8890febc71decd553a040bdda8739f68ed616c39 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53285 Commit-Queue: David Benjamin Reviewed-by: Bob Beck Commit-Queue: Bob Beck --- crypto/x509/x509_test.cc | 160 +++++++++++++++++++++++++++++++++++++++ crypto/x509v3/v3_lib.c | 4 +- 2 files changed, 163 insertions(+), 1 deletion(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 425d74541..50121c93c 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -4388,3 +4388,163 @@ TEST(X509Test, Print) { 40:86 )"); } + +TEST(X509Test, AddExt) { + bssl::UniquePtr x509(X509_new()); + ASSERT_TRUE(x509); + + struct Extension { + int nid; + bool critical; + std::vector data; + }; + auto expect_extensions = [&](const std::vector &exts) { + ASSERT_EQ(static_cast(X509_get_ext_count(x509.get())), exts.size()); + for (size_t i = 0; i < exts.size(); i++) { + SCOPED_TRACE(i); + X509_EXTENSION *ext = X509_get_ext(x509.get(), static_cast(i)); + EXPECT_EQ(OBJ_obj2nid(X509_EXTENSION_get_object(ext)), exts[i].nid); + EXPECT_EQ(X509_EXTENSION_get_critical(ext), exts[i].critical ? 1 : 0); + ASN1_OCTET_STRING *data = X509_EXTENSION_get_data(ext); + EXPECT_EQ(Bytes(ASN1_STRING_get0_data(data), ASN1_STRING_length(data)), + Bytes(exts[i].data)); + } + }; + + // Make a few sample extensions. + + // SEQUENCE {} + std::vector basic1_der = {0x30, 0x00}; + const uint8_t *inp = basic1_der.data(); + bssl::UniquePtr basic1_obj( + d2i_BASIC_CONSTRAINTS(nullptr, &inp, basic1_der.size())); + EXPECT_EQ(inp, basic1_der.data() + basic1_der.size()); + + // SEQUENCE { BOOLEAN { TRUE } } + std::vector basic2_der = {0x30, 0x03, 0x01, 0x01, 0xff}; + inp = basic2_der.data(); + bssl::UniquePtr basic2_obj( + d2i_BASIC_CONSTRAINTS(nullptr, &inp, basic2_der.size())); + EXPECT_EQ(inp, basic2_der.data() + basic2_der.size()); + + // OCTET_STRING {} + std::vector skid1_der = {0x04, 0x00}; + inp = skid1_der.data(); + bssl::UniquePtr skid1_obj( + d2i_ASN1_OCTET_STRING(nullptr, &inp, skid1_der.size())); + EXPECT_EQ(inp, skid1_der.data() + skid1_der.size()); + + // OCTET_STRING { "a" } + std::vector skid2_der = {0x04, 0x01, 0x61}; + inp = skid2_der.data(); + bssl::UniquePtr skid2_obj( + d2i_ASN1_OCTET_STRING(nullptr, &inp, skid2_der.size())); + EXPECT_EQ(inp, skid2_der.data() + skid2_der.size()); + + // Initially, the extension list is empty. + expect_extensions({}); + + // Adding extensions works with the default settings. + EXPECT_EQ( + 1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, basic1_obj.get(), + /*crit=*/1, X509V3_ADD_DEFAULT)); + expect_extensions({{NID_basic_constraints, true, basic1_der}}); + EXPECT_EQ(1, X509_add1_ext_i2d(x509.get(), NID_subject_key_identifier, + skid1_obj.get(), + /*crit=*/0, X509V3_ADD_DEFAULT)); + expect_extensions({{NID_basic_constraints, true, basic1_der}, + {NID_subject_key_identifier, false, skid1_der}}); + + // By default, we cannot add duplicates. + EXPECT_EQ( + 0, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, basic2_obj.get(), + /*crit=*/0, X509V3_ADD_DEFAULT)); + expect_extensions({{NID_basic_constraints, true, basic1_der}, + {NID_subject_key_identifier, false, skid1_der}}); + + // |X509V3_ADD_KEEP_EXISTING| silently keeps the existing extension if already + // present. + EXPECT_EQ( + 1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, basic2_obj.get(), + /*crit=*/0, X509V3_ADD_KEEP_EXISTING)); + expect_extensions({{NID_basic_constraints, true, basic1_der}, + {NID_subject_key_identifier, false, skid1_der}}); + + // |X509V3_ADD_REPLACE| replaces it. + EXPECT_EQ( + 1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, basic2_obj.get(), + /*crit=*/0, X509V3_ADD_REPLACE)); + expect_extensions({{NID_basic_constraints, false, basic2_der}, + {NID_subject_key_identifier, false, skid1_der}}); + + // |X509V3_ADD_REPLACE_EXISTING| also replaces matches. + EXPECT_EQ(1, X509_add1_ext_i2d(x509.get(), NID_subject_key_identifier, + skid2_obj.get(), + /*crit=*/1, X509V3_ADD_REPLACE_EXISTING)); + expect_extensions({{NID_basic_constraints, false, basic2_der}, + {NID_subject_key_identifier, true, skid2_der}}); + + // |X509V3_ADD_DELETE| ignores the value and deletes the extension. + EXPECT_EQ(1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, nullptr, 0, + X509V3_ADD_DELETE)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}}); + + // Not finding an extension to delete is an error. + EXPECT_EQ(0, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, nullptr, 0, + X509V3_ADD_DELETE)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}}); + + // |X509V3_ADD_REPLACE_EXISTING| fails if it cannot find a match. + EXPECT_EQ( + 0, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, basic1_obj.get(), + /*crit=*/1, X509V3_ADD_REPLACE_EXISTING)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}}); + + // |X509V3_ADD_REPLACE| adds a new extension if not preseent. + EXPECT_EQ( + 1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, basic1_obj.get(), + /*crit=*/1, X509V3_ADD_REPLACE)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}, + {NID_basic_constraints, true, basic1_der}}); + + // Delete the extension again. + EXPECT_EQ(1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, nullptr, 0, + X509V3_ADD_DELETE)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}}); + + // |X509V3_ADD_KEEP_EXISTING| adds a new extension if not preseent. + EXPECT_EQ( + 1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, basic1_obj.get(), + /*crit=*/1, X509V3_ADD_KEEP_EXISTING)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}, + {NID_basic_constraints, true, basic1_der}}); + + // Delete the extension again. + EXPECT_EQ(1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, nullptr, 0, + X509V3_ADD_DELETE)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}}); + + // |X509V3_ADD_APPEND| adds a new extension if not present. + EXPECT_EQ( + 1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, basic1_obj.get(), + /*crit=*/1, X509V3_ADD_APPEND)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}, + {NID_basic_constraints, true, basic1_der}}); + + // |X509V3_ADD_APPEND| keeps adding duplicates (invalid) even if present. + EXPECT_EQ( + 1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, basic2_obj.get(), + /*crit=*/0, X509V3_ADD_APPEND)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}, + {NID_basic_constraints, true, basic1_der}, + {NID_basic_constraints, false, basic2_der}}); + + // |X509V3_ADD_DELETE| only deletes one extension at a time. + EXPECT_EQ(1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, nullptr, 0, + X509V3_ADD_DELETE)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}, + {NID_basic_constraints, false, basic2_der}}); + EXPECT_EQ(1, X509_add1_ext_i2d(x509.get(), NID_basic_constraints, nullptr, 0, + X509V3_ADD_DELETE)); + expect_extensions({{NID_subject_key_identifier, true, skid2_der}}); +} diff --git a/crypto/x509v3/v3_lib.c b/crypto/x509v3/v3_lib.c index 4400b8e2b..51b0f3767 100644 --- a/crypto/x509v3/v3_lib.c +++ b/crypto/x509v3/v3_lib.c @@ -316,9 +316,11 @@ int X509V3_add1_i2d(STACK_OF(X509_EXTENSION) **x, int nid, void *value, } // If delete, just delete it if (ext_op == X509V3_ADD_DELETE) { - if (!sk_X509_EXTENSION_delete(*x, extidx)) { + X509_EXTENSION *prev_ext = sk_X509_EXTENSION_delete(*x, extidx); + if (prev_ext == NULL) { return -1; } + X509_EXTENSION_free(prev_ext); return 1; } } else {