Make ASN1_NULL an opaque pointer.
crypto/asn1 represents an ASN.1 NULL value as a non-null ASN1_NULL*
pointer, (ASN1_NULL*)1. It is a non-null pointer because a null pointer
represents an omitted OPTIONAL NULL. It is an opaque pointer because
there is no sense in allocating anything.
This pointer cannot be dereferenced, yet ASN1_NULL is a typedef for int.
This is confusing and probably undefined behavior. (N1548, 6.3.2.3,
clause 7 requires pointer conversions between two pointer types be
correctly aligned, even if the pointer is never dereferenced. Strangely,
clause 5 above does not impose the same requirement when converting from
integer to pointer, though it mostly punts to the implementation
definition.) Of course, all of tasn_*.c is a giant strict aliasing
violation anyway, but an opaque struct pointer is a slightly better
choice here.
(Note that, although ASN1_BOOLEAN is also a typedef for int, that
situation is different: the ASN1_BOOLEAN representation is a plain
ASN1_BOOLEAN, not ASN1_BOOLEAN*, while the ASN1_NULL representation is a
pointer. ASN1_NULL could have had the same treatment and even used a
little less memory, but changing that would break the API.)
Update-Note: Code that was assuming ASN1_NULL was an int typedef will
fail to compile. Given this was never dereferencable, it is hard to
imagine anything relying on this.
Bug: 438
Change-Id: Ia0c652eed66e76f82a3843af1fc877f06c8d5e8f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49805
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 7c37cb6..738e422 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1343,6 +1343,41 @@
}
}
+TEST(ASN1Test, Null) {
+ // An |ASN1_NULL| is an opaque, non-null pointer. It is an arbitrary signaling
+ // value and does not need to be freed. (If the pointer is null, this is an
+ // omitted OPTIONAL NULL.)
+ EXPECT_NE(nullptr, ASN1_NULL_new());
+
+ // It is safe to free either the non-null pointer or the null one.
+ ASN1_NULL_free(ASN1_NULL_new());
+ ASN1_NULL_free(nullptr);
+
+ // A NULL may be decoded.
+ static const uint8_t kNull[] = {0x05, 0x00};
+ const uint8_t *ptr = kNull;
+ EXPECT_NE(nullptr, d2i_ASN1_NULL(nullptr, &ptr, sizeof(kNull)));
+ EXPECT_EQ(ptr, kNull + sizeof(kNull));
+
+ // It may also be re-encoded.
+ uint8_t *enc = nullptr;
+ int enc_len = i2d_ASN1_NULL(ASN1_NULL_new(), &enc);
+ ASSERT_GE(enc_len, 0);
+ EXPECT_EQ(Bytes(kNull), Bytes(enc, enc_len));
+ OPENSSL_free(enc);
+ enc = nullptr;
+
+ // Although the standalone representation of NULL is a non-null pointer, the
+ // |ASN1_TYPE| representation is a null pointer.
+ ptr = kNull;
+ bssl::UniquePtr<ASN1_TYPE> null_type(
+ d2i_ASN1_TYPE(nullptr, &ptr, sizeof(kNull)));
+ ASSERT_TRUE(null_type);
+ EXPECT_EQ(ptr, kNull + sizeof(kNull));
+ EXPECT_EQ(V_ASN1_NULL, ASN1_TYPE_get(null_type.get()));
+ EXPECT_EQ(nullptr, null_type->value.ptr);
+}
+
// The ASN.1 macros do not work on Windows shared library builds, where usage of
// |OPENSSL_EXPORT| is a bit stricter.
#if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY)
@@ -1444,6 +1479,9 @@
STACK_OF(ASN1_INTEGER) *seq;
STACK_OF(ASN1_INTEGER) *seq_imp;
STACK_OF(ASN1_INTEGER) *seq_exp;
+ ASN1_NULL *null;
+ ASN1_NULL *null_imp;
+ ASN1_NULL *null_exp;
};
DECLARE_ASN1_FUNCTIONS(REQUIRED_FIELD)
@@ -1454,6 +1492,9 @@
ASN1_SEQUENCE_OF(REQUIRED_FIELD, seq, ASN1_INTEGER),
ASN1_IMP_SEQUENCE_OF(REQUIRED_FIELD, seq_imp, ASN1_INTEGER, 2),
ASN1_EXP_SEQUENCE_OF(REQUIRED_FIELD, seq_exp, ASN1_INTEGER, 3),
+ ASN1_SIMPLE(REQUIRED_FIELD, null, ASN1_NULL),
+ ASN1_IMP(REQUIRED_FIELD, null_imp, ASN1_NULL, 4),
+ ASN1_EXP(REQUIRED_FIELD, null_exp, ASN1_NULL, 5),
} ASN1_SEQUENCE_END(REQUIRED_FIELD)
IMPLEMENT_ASN1_FUNCTIONS(REQUIRED_FIELD)
@@ -1481,6 +1522,14 @@
(*obj).*field = nullptr;
EXPECT_EQ(-1, i2d_REQUIRED_FIELD(obj.get(), nullptr));
}
+
+ for (auto field : {&REQUIRED_FIELD::null, &REQUIRED_FIELD::null_imp,
+ &REQUIRED_FIELD::null_exp}) {
+ obj.reset(REQUIRED_FIELD_new());
+ ASSERT_TRUE(obj);
+ (*obj).*field = nullptr;
+ EXPECT_EQ(-1, i2d_REQUIRED_FIELD(obj.get(), nullptr));
+ }
}
#endif // !WINDOWS || !SHARED_LIBRARY
diff --git a/include/openssl/base.h b/include/openssl/base.h
index 8be10d1..01a7e07 100644
--- a/include/openssl/base.h
+++ b/include/openssl/base.h
@@ -328,8 +328,11 @@
// CRYPTO_THREADID is a dummy value.
typedef int CRYPTO_THREADID;
+// An |ASN1_NULL| is an opaque type. asn1.h represents the ASN.1 NULL value as
+// an opaque, non-NULL |ASN1_NULL*| pointer.
+typedef struct asn1_null_st ASN1_NULL;
+
typedef int ASN1_BOOLEAN;
-typedef int ASN1_NULL;
typedef struct ASN1_ITEM_st ASN1_ITEM;
typedef struct asn1_object_st ASN1_OBJECT;
typedef struct asn1_pctx_st ASN1_PCTX;