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.
--
: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. */
