Re: bug in ASN1_time_parse(3)

[Theo Buehler]

On Thu, Dec 10, 2020 at 09:51:49PM +0100, Olivier Taïbi wrote:
> I think there is a bug in ASN1_time_parse(3): if tm is not NULL then it
> is not zero'd before parsing and the year value is added instead of set.

Thanks!

While your observation is correct, your proposed fix will break parsing
GeneralizedTimes:  The GENTIME_LENGTH case falls through to the
UTCTIME_LENGTH case, and your diff will end up zeroing the centuries
that were set for GeneralizedTimes.

Probably better to do the zeroing unconditionally to be on the safe side.

Fortunately, most callers that do not initialize the tm they pass in 
use the V_ASN1_GENERALIZED_TIME mode which must take the GENTIME_LENGTH
branch of the switch (or they would error out), so it should not be a
big problem.

The only exception I found is ASN1_UTCTIME_cmp_time_t() which is
practically unused.

> diff --git a/lib/libcrypto/asn1/a_time_tm.c b/lib/libcrypto/asn1/a_time_tm.c
> index b6e22cbd27b..a841f32a856 100644
> --- a/lib/libcrypto/asn1/a_time_tm.c
> +++ b/lib/libcrypto/asn1/a_time_tm.c
> @@ -196,7 +196,7 @@ ASN1_time_parse(const char *bytes, size_t len, struct tm 
> *tm, int mode)
>                                 return (-1);
>                         type = V_ASN1_UTCTIME;
>                 }
> -               lt->tm_year += ATOI2(p);                /* yy */
> +               lt->tm_year = ATOI2(p);                 /* yy */
>                 if (type == V_ASN1_UTCTIME) {
>                         if (lt->tm_year < 50)
>                                 lt->tm_year += 100;
> 

Index: asn1/a_time_tm.c
===================================================================
RCS file: /cvs/src/lib/libcrypto/asn1/a_time_tm.c,v
retrieving revision 1.15
diff -u -p -r1.15 a_time_tm.c
--- asn1/a_time_tm.c    25 Apr 2018 11:48:21 -0000      1.15
+++ asn1/a_time_tm.c    10 Dec 2020 21:26:07 -0000
@@ -163,10 +163,9 @@ ASN1_time_parse(const char *bytes, size_
                return (-1);
 
        lt = tm;
-       if (lt == NULL) {
-               memset(&ltm, 0, sizeof(ltm));
+       if (lt == NULL)
                lt = &ltm;
-       }
+       memset(lt, 0, sizeof(*lt));
 
        /* Timezone is required and must be GMT (Zulu). */
        if (bytes[len - 1] != 'Z')