On Wed, Oct 23, 2019 at 08:47:25AM +0200, Theo Buehler wrote:
> 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.
Ha, I stole that from openssl I think once rpki-client can use our
libcrypto then this could can be rewritten with ASN1_time_parse() and
ASN1_time_tm_cmp(). I hope this happens before 2049 :)
> 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.
Thanks for checking.
> >
> > > --
> > > :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
> >
>
--
:wq Claudio
