On Wed, Oct 16, 2019 at 10:47:25PM +0200, Claudio Jeker wrote:
> On Wed, Oct 16, 2019 at 07:26:25AM -0300, Alexandre Hamada wrote:
> > Hi Tech,
> > I would like to suggest to use UTC functions on all date/time convertions,
> > to avoid some clock drift errors.
> >
> > Kind regards,
> > Alexandre Hamada
> >
> > https://patch-diff.githubusercontent.com/raw/kristapsdz/rpki-client/pull/9.patch
> >
> > From a463f8cb23375f15b74eff49a06e8934423e3dbf Mon Sep 17 00:00:00 2001
> > From: dev-gto <[email protected]>
> > Date: Wed, 16 Oct 2019 07:22:46 -0300
> > Subject: [PATCH] Avoid local time conversion
> >
> > ---
> > mft.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mft.c b/mft.c
> > index f9176b4..738f3ff 100644
> > --- a/mft.c
> > +++ b/mft.c
> > @@ -75,8 +75,8 @@ gentime2time(struct parse *p, const ASN1_GENERALIZEDTIME
> > *tp)
> > memset(&tm, 0, sizeof(struct tm));
> > if (strptime(buf, "%b %d %T %Y %Z", &tm) == NULL)
> > errx(EXIT_FAILURE, "%s: strptime", buf);
> > - if ((t = mktime(&tm)) == -1)
> > - errx(EXIT_FAILURE, "%s: mktime", buf);
> > + if ((t = timegm(&tm)) == -1)
> > + errx(EXIT_FAILURE, "%s: timegm", buf);
> > return t;
> > }
> >
>
> Hi Alexandre,
>
> How about this diff instead. This is inspired by OCSP_check_validity() and
> uses ASN1_GENERALIZEDTIME_check() and X509_cmp_time() to do the validity
> check. I think this has a way better chance to produce the expected
> results. My quick testing seems to indicate that it works but review and
> testing is very welcome.
I will commit this diff later today unless somebody speaks up.
> --
> :wq Claudio
>
> Index: mft.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 mft.c
> --- mft.c 13 Aug 2019 13:27:26 -0000 1.7
> +++ mft.c 16 Oct 2019 20:39:36 -0000
> @@ -35,49 +35,57 @@ struct parse {
> struct mft *res; /* result object */
> };
>
> -/*
> - * Convert from the ASN.1 generalised time to a time_t.
> - * Return the time.
> - * This is a stupid requirement due to using ASN1_GENERALIZEDTIME
> - * instead of the native ASN1_TIME functions for comparing time.
> - */
> -static time_t
> -gentime2time(struct parse *p, const ASN1_GENERALIZEDTIME *tp)
> +static const char *
> +gentime2str(const ASN1_GENERALIZEDTIME *time)
> {
> + static char buf[64];
> BIO *mem;
> - char *pp;
> - char buf[64];
> - long len;
> - struct tm tm;
> - time_t t;
>
> if ((mem = BIO_new(BIO_s_mem())) == NULL)
> cryptoerrx("BIO_new");
> - if (!ASN1_GENERALIZEDTIME_print(mem, tp))
> + if (!ASN1_GENERALIZEDTIME_print(mem, time))
> cryptoerrx("ASN1_GENERALIZEDTIME_print");
> + if (BIO_gets(mem, buf, sizeof(buf)) < 0)
> + cryptoerrx("BIO_gets");
>
> - /*
> - * The manpage says nothing about being NUL terminated and
> - * strptime(3) needs a string.
> - * So convert into a static buffer of decent size and NUL
> - * terminate in that way.
> - */
> -
> - len = BIO_get_mem_data(mem, &pp);
> - if (len < 0 || (size_t)len > sizeof(buf) - 1)
> - errx(EXIT_FAILURE, "BIO_get_mem_data");
> -
> - memcpy(buf, pp, len);
> - buf[len] = '\0';
> BIO_free(mem);
> + return buf;
> +}
> +
> +/*
> + * Validate and verify the time validity of the mft.
> + * Returns 1 if all is good, 0 if mft is stale, any other case -1.
> + */
> +static time_t
> +check_validity(const ASN1_GENERALIZEDTIME *from,
> + const ASN1_GENERALIZEDTIME *until, const char *fn, int force)
> +{
> + time_t now = time(NULL);
>
> - memset(&tm, 0, sizeof(struct tm));
> - if (strptime(buf, "%b %d %T %Y %Z", &tm) == NULL)
> - errx(EXIT_FAILURE, "%s: strptime", buf);
> - if ((t = mktime(&tm)) == -1)
> - errx(EXIT_FAILURE, "%s: mktime", buf);
> + if (!ASN1_GENERALIZEDTIME_check(from) ||
> + !ASN1_GENERALIZEDTIME_check(until)) {
> + warnx("%s: embedded time format invalid", fn);
> + return -1;
> + }
> + /* check that until is not before from */
> + if (ASN1_STRING_cmp(until, from) < 0) {
> + warnx("%s: bad update interval", fn);
> + return -1;
> + }
> + /* check that now is not before from */
> + if (X509_cmp_time(from, &now) > 0) {
> + warnx("%s: mft not yet valid %s", fn, gentime2str(from));
> + return -1;
> + }
> + /* check that now is not after until */
> + if (X509_cmp_time(until, &now) < 0) {
> + warnx("%s: mft expired on %s%s", fn, gentime2str(until),
> + force ? " (ignoring)" : "");
> + if (!force)
> + return 0;
> + }
>
> - return t;
> + return 1;
> }
>
> /*
> @@ -229,6 +237,7 @@ mft_parse_econtent(const unsigned char *
> {
> ASN1_SEQUENCE_ANY *seq;
> const ASN1_TYPE *t;
> + const ASN1_GENERALIZEDTIME *from, *until;
> int i, rc = -1;
> time_t this, next, now = time(NULL);
> char buf[64];
> @@ -289,7 +298,7 @@ mft_parse_econtent(const unsigned char *
> p->fn, ASN1_tag2str(t->type), t->type);
> goto out;
> }
> - this = gentime2time(p, t->value.generalizedtime);
> + from = t->value.generalizedtime;
>
> t = sk_ASN1_TYPE_value(seq, i++);
> if (t->type != V_ASN1_GENERALIZEDTIME) {
> @@ -298,26 +307,11 @@ mft_parse_econtent(const unsigned char *
> p->fn, ASN1_tag2str(t->type), t->type);
> goto out;
> }
> - next = gentime2time(p, t->value.generalizedtime);
> + until = t->value.generalizedtime;
>
> - if (this >= next) {
> - warnx("%s: bad update interval", p->fn);
> + rc = check_validity(from, until, p->fn, force);
> + if (rc != 1)
> goto out;
> - } else if (now < this) {
> - warnx("%s: before date interval (clock drift?)", p->fn);
> - goto out;
> - } else if (now >= next) {
> - ctime_r(&next, buf);
> - buf[strlen(buf) - 1] = '\0';
> - if (!force) {
> - if (verbose > 0)
> - warnx("%s: stale: expired %s", p->fn, buf);
> - rc = 0;
> - goto out;
> - }
> - if (verbose > 0)
> - warnx("%s: stale: expired %s (ignoring)", p->fn, buf);
> - }
>
> /* File list algorithm. */
>
>
--
:wq Claudio
