Remove ASN1_STRING_FLAG_MSTRING.
This flag is set when an ASN1_STRING is created from a codepath that is
aware it is an "mstring" (CHOICE of multiple string or string-like
types). With setters like X509_set_notBefore, it is very easy to
accidentally lose the flag on some field that normally has it.
The only place the flag is checked is X509_time_adj_ex. X509_time_adj_ex
usually transparently picks UTCTime vs GeneralizedTime, as in the X.509
CHOICE type. But if writing to an existing object AND if the object
lacks the flag, it will lock to whichever type the object was
previously. It is likely any caller hitting this codepath is doing so
unintentionally and has a latent bug that won't trip until 2050.
In fact, one of the ways callers might accidentally lose the
ASN1_STRING_FLAG_MSTRING flag is by using X509_time_adj_ex!
X509_time_adj_ex(NULL) does not use an mstring-aware constructor. This
CL avoids needing such a notion in the first place.
Looking through callers, the one place that wants the old behavior is a
call site within OpenSSL, to set the producedAt field in OCSP. That
field is a GeneralizedTime, rather than a UTCTime/GeneralizedTime
CHOICE. We dropped that code, but I'm making a note of it to remember
when filing upstream.
Update-Note: ASN1_STRING_FLAG_MSTRING is no longer defined and
X509_time_adj_ex now behaves more predictably. Callers that actually
wanted to lock to a specific type should call ASN1_UTCTIME_adj or
ASN1_GENERALIZEDTIME_adj instead.
Change-Id: Ib9e1c9dbd0c694e1e69f938da3992d1ffc9bd060
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48668
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/tasn_new.c b/crypto/asn1/tasn_new.c
index 540ed8f..346887b 100644
--- a/crypto/asn1/tasn_new.c
+++ b/crypto/asn1/tasn_new.c
@@ -271,7 +271,6 @@
static int ASN1_primitive_new(ASN1_VALUE **pval, const ASN1_ITEM *it)
{
ASN1_TYPE *typ;
- ASN1_STRING *str;
int utype;
if (!it)
@@ -308,10 +307,7 @@
break;
default:
- str = ASN1_STRING_type_new(utype);
- if (it->itype == ASN1_ITYPE_MSTRING && str)
- str->flags |= ASN1_STRING_FLAG_MSTRING;
- *pval = (ASN1_VALUE *)str;
+ *pval = (ASN1_VALUE *)ASN1_STRING_type_new(utype);
break;
}
if (*pval)
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 6aee451..3d55990 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -1991,17 +1991,12 @@
{
time_t t = 0;
- if (in_tm)
+ if (in_tm) {
t = *in_tm;
- else
+ } else {
time(&t);
-
- if (s && !(s->flags & ASN1_STRING_FLAG_MSTRING)) {
- if (s->type == V_ASN1_UTCTIME)
- return ASN1_UTCTIME_adj(s, t, offset_day, offset_sec);
- if (s->type == V_ASN1_GENERALIZEDTIME)
- return ASN1_GENERALIZEDTIME_adj(s, t, offset_day, offset_sec);
}
+
return ASN1_TIME_adj(s, t, offset_day, offset_sec);
}
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h
index 42f24c8..5f9c413 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -232,14 +232,6 @@
// treated as padding. This behavior is deprecated and should not be used.
#define ASN1_STRING_FLAG_BITS_LEFT 0x08
-// ASN1_STRING_FLAG_MSTRING indicates that the |ASN1_STRING| is an MSTRING type,
-// which is how this library refers to a CHOICE type of several string types.
-// For example, DirectoryString as defined in RFC5280.
-//
-// TODO(davidben): This is only used in one place within the library and is easy
-// to accidentally drop. Can it be removed?
-#define ASN1_STRING_FLAG_MSTRING 0x040
-
// ASN1_STRING_type_new returns a newly-allocated empty |ASN1_STRING| object of
// type |type|, or NULL on error.
OPENSSL_EXPORT ASN1_STRING *ASN1_STRING_type_new(int type);
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 8c5e471..751117e 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -878,13 +878,6 @@
// X509_time_adj_ex behaves like |ASN1_TIME_adj|, but adds an offset to |*t|. If
// |t| is NULL, it uses the current time instead of |*t|.
-//
-// TODO(davidben): This isn't quite right. If |s| is non-NULL and lacks the
-// |ASN1_STRING_FLAG_MSTRING| flag, it tries to preserve |s|'s current time
-// (UTCTime or GeneralizedTime), rather than implementing the RFC5280 CHOICE
-// type. Remove this behavior and then the flag. Whether |s| has the flag is
-// mostly an accident of it was constructed. Meanwhile, callers should use
-// |ASN1_TIME_adj|, which does the same thing without this quirk.
OPENSSL_EXPORT ASN1_TIME *X509_time_adj_ex(ASN1_TIME *s, int offset_day,
long offset_sec, time_t *t);
