On Wed, Oct 23, 2019 at 08:04:26AM +0200, Claudio Jeker wrote:
> 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.
I am a bit worried about the ASN1_STRING_cmp(until, from) line.
According to RFC 6486 4.2.1, the times are to be encoded as for
the corresponding fields in RFC 5280, which normalizes UTCTime (for
dates until 2049) to the form YYMMDDHHMMSSZ and GeneralizedTime to
YYYYMMDDHHMMSSZ (for dates from 2050 on), so this comparison is iffy.
If you think that it's not the right moment to worry about this, I'm ok
with your diff: apart from that, it looks good.
>
> > --
> > :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
>
