Re: usr.bin/mg: fix -Wunused-but-set-variable warnings

2022-01-19 Thread Philip Guenther
On Sun, Jan 16, 2022 at 8:10 AM Christian Weisgerber 
wrote:

> usr.bin/mg: fix -Wunused-but-set-variable warnings
>
> * strtonum() is only called to verify that a string is numerical, the
>   return value is unused.
> * inlist is no longer used after the code was refactored.
>
> OK?
>

ok guenther@


Re: usr.sbin/dvmrpctl: fix -Wunused-but-set-variable warning

2022-01-19 Thread Philip Guenther
On Sun, Jan 16, 2022 at 12:51 PM Christian Weisgerber 
wrote:

> usr.sbin/dvmrpctl: fix -Wunused-but-set-variable warning
>
> This looks like /* not yet implemented */, but the companion functions
> show_rib_detail_msg() and show_mfc_detail_msg() are equally empty.
>

ok guenther@


Re: usr.sbin/eigrpd: fix -Wunused-but-set-variable warning

2022-01-19 Thread Philip Guenther
On Sun, Jan 16, 2022 at 1:51 PM Christian Weisgerber 
wrote:

> usr.sbin/eigrpd: fix -Wunused-but-set-variable warning
>

ok guenther@


Re: usr.sbin/ospf6ctl: fix -Wunused-but-set-variable warning

2022-01-19 Thread Philip Guenther
On Mon, Jan 17, 2022 at 6:36 AM Christian Weisgerber 
wrote:

> usr.sbin/ospf6ctl: fix -Wunused-but-set-variable warning
>
> Maybe this is uncompleted code, but I think we can remove it for
> the time being.
>
> M  usr.sbin/ospf6ctl/ospf6ctl.c
>

Agreed.  ok guenther@


kubsan undefined behavior 1U shift

2022-01-19 Thread Alexander Bluhm
Hi,

Compiling the kernel with option KUBSAN finds undefined behavior.
Here are some easy fixes that shift signed values too far.

kubsan: arch/amd64/amd64/identcpu.c:882:17: shift: left shift of negative value 
-1
kubsan: kern/kern_descrip.c:159:30: shift: left shift of 1 by 31 places cannot 
be represented in type 'int'
kubsan: kern/kern_descrip.c:170:26: shift: left shift of 1 by 31 places cannot 
be represented in type 'int'
kubsan: kern/kern_descrip.c:189:28: shift: left shift of 1 by 31 places cannot 
be represented in type 'int'
kubsan: kern/kern_sched.c:265:25: shift: left shift of 1 by 31 places cannot be 
represented in type 'int'
kubsan: kern/kern_sched.c:289:27: shift: left shift of 1 by 31 places cannot be 
represented in type 'int'
kubsan: kern/subr_pool.c:964:7: shift: left shift of 1 by 31 places cannot be 
represented in type 'int'
kubsan: netinet/in_pcb.c:200:11: shift: left shift of 1 by 31 places cannot be 
represented in type 'int'
kubsan: netinet/ip_esp.c:1005:13: shift: left shift of 1 by 31 places cannot be 
represented in type 'int'
kubsan: kern/kern_descrip.c:159:30: shift: left shift of 1 by 31 places cannot 
be represented in type 'int'
kubsan: kern/kern_descrip.c:189:28: shift: left shift of 1 by 31 places cannot 
be represented in type 'int'
kubsan: net/rtsock.c:1429:31: shift: left shift of 1 by 31 places cannot be 
represented in type 'int'
kubsan: netinet/in_pcb.c:200:11: shift: left shift of 1 by 31 places cannot be 
represented in type 'int'
kubsan: netinet/in_pcb.c:207:11: shift: left shift of 1 by 31 places cannot be 
represented in type 'int'

ok?

bluhm

Index: arch/amd64/amd64/identcpu.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/amd64/identcpu.c,v
retrieving revision 1.121
diff -u -p -r1.121 identcpu.c
--- arch/amd64/amd64/identcpu.c 2 Nov 2021 23:30:15 -   1.121
+++ arch/amd64/amd64/identcpu.c 19 Jan 2022 22:48:46 -
@@ -854,7 +854,7 @@ cpu_topology(struct cpu_info *ci)
ci->ci_pkg_id = apicid >> core_bits;
 
/* Get rid of the package bits */
-   core_mask = (1 << core_bits) - 1;
+   core_mask = (1U << core_bits) - 1;
thread_id = apicid & core_mask;
 
/* Cut logical thread_id into core id, and smt id in a core */
@@ -872,14 +872,14 @@ cpu_topology(struct cpu_info *ci)
max_coreid = ((eax >> 26) & 0x3f) + 1;
/* SMT */
smt_bits = mask_width(max_apicid / max_coreid);
-   smt_mask = (1 << smt_bits) - 1;
+   smt_mask = (1U << smt_bits) - 1;
/* Core */
core_bits = log2(max_coreid);
-   core_mask = (1 << (core_bits + smt_bits)) - 1;
+   core_mask = (1U << (core_bits + smt_bits)) - 1;
core_mask ^= smt_mask;
/* Pkg */
pkg_bits = core_bits + smt_bits;
-   pkg_mask = -1 << core_bits;
+   pkg_mask = ~0U << core_bits;
 
ci->ci_smt_id = apicid & smt_mask;
ci->ci_core_id = (apicid & core_mask) >> smt_bits;
Index: kern/kern_descrip.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.204
diff -u -p -r1.204 kern_descrip.c
--- kern/kern_descrip.c 25 Oct 2021 10:24:54 -  1.204
+++ kern/kern_descrip.c 19 Jan 2022 22:48:46 -
@@ -156,7 +156,7 @@ fd_inuse(struct filedesc *fdp, int fd)
 {
u_int off = fd >> NDENTRYSHIFT;
 
-   if (fdp->fd_lomap[off] & (1 << (fd & NDENTRYMASK)))
+   if (fdp->fd_lomap[off] & (1U << (fd & NDENTRYMASK)))
return 1;
 
return 0;
@@ -167,9 +167,9 @@ fd_used(struct filedesc *fdp, int fd)
 {
u_int off = fd >> NDENTRYSHIFT;
 
-   fdp->fd_lomap[off] |= 1 << (fd & NDENTRYMASK);
+   fdp->fd_lomap[off] |= 1U << (fd & NDENTRYMASK);
if (fdp->fd_lomap[off] == ~0)
-   fdp->fd_himap[off >> NDENTRYSHIFT] |= 1 << (off & NDENTRYMASK);
+   fdp->fd_himap[off >> NDENTRYSHIFT] |= 1U << (off & NDENTRYMASK);
 
if (fd > fdp->fd_lastfile)
fdp->fd_lastfile = fd;
@@ -185,8 +185,8 @@ fd_unused(struct filedesc *fdp, int fd)
fdp->fd_freefile = fd;
 
if (fdp->fd_lomap[off] == ~0)
-   fdp->fd_himap[off >> NDENTRYSHIFT] &= ~(1 << (off & 
NDENTRYMASK));
-   fdp->fd_lomap[off] &= ~(1 << (fd & NDENTRYMASK));
+   fdp->fd_himap[off >> NDENTRYSHIFT] &= ~(1U << (off & 
NDENTRYMASK));
+   fdp->fd_lomap[off] &= ~(1U << (fd & NDENTRYMASK));
 
 #ifdef DIAGNOSTIC
if (fd > fdp->fd_lastfile)
Index: kern/kern_sched.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_sched.c,v
retrieving revision 1.73
diff -u -p -r1.73 kern_sched.c
--- kern/kern_sched.c 

rpki-client: factor filename extension parsing into a function

2022-01-19 Thread Theo Buehler
Not sure if it is that much of a win, but it saves some repetition and
makes sure we don't forget checking the file name to be longer than 4
another time (missed on review in main() and proc_parser_file()).

Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.109
diff -u -p -r1.109 extern.h
--- extern.h19 Jan 2022 15:50:31 -  1.109
+++ extern.h19 Jan 2022 16:51:05 -
@@ -286,7 +286,7 @@ void auth_insert(struct auth_tree *, s
  * There might be others we don't consider.
  */
 enum rtype {
-   RTYPE_EOF = 0,
+   RTYPE_INVALID,
RTYPE_TAL,
RTYPE_MFT,
RTYPE_ROA,
@@ -451,6 +451,8 @@ int  valid_filename(const char *);
 int valid_filehash(int, const char *, size_t);
 int valid_uri(const char *, size_t, const char *);
 int valid_origin(const char *, const char *);
+
+enum rtype  rtype_from_file_extension(const char *);
 
 /* Working with CMS. */
 unsigned char  *cms_parse_validate(X509 **, const char *,
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.178
diff -u -p -r1.178 main.c
--- main.c  19 Jan 2022 15:50:31 -  1.178
+++ main.c  19 Jan 2022 16:51:05 -
@@ -356,6 +356,7 @@ static void
 queue_add_from_mft_set(const struct mft *mft, const char *name, struct repo 
*rp)
 {
size_t   i, sz;
+   enum rtype   type;
const struct mftfile*f;
 
for (i = 0; i < mft->filesz; i++) {
@@ -371,17 +372,18 @@ queue_add_from_mft_set(const struct mft 
f = >files[i];
sz = strlen(f->file);
assert(sz > 4);
-   if (strcasecmp(f->file + sz - 4, ".crl") == 0)
+   type = rtype_from_file_extension(f->file);
+   switch (type) {
+   case RTYPE_CER:
+   case RTYPE_ROA:
+   case RTYPE_GBR:
+   queue_add_from_mft(mft->path, f, type, rp);
+   break;
+   case RTYPE_CRL:
continue;
-   else if (strcasecmp(f->file + sz - 4, ".cer") == 0)
-   queue_add_from_mft(mft->path, f, RTYPE_CER, rp);
-   else if (strcasecmp(f->file + sz - 4, ".roa") == 0)
-   queue_add_from_mft(mft->path, f, RTYPE_ROA, rp);
-   else if (strcasecmp(f->file + sz - 4, ".gbr") == 0)
-   queue_add_from_mft(mft->path, f, RTYPE_GBR, rp);
-   else
-   logx("%s: unsupported file type: %s", name,
-   f->file);
+   default:
+   logx("%s: unsupported file type: %s", name, f->file);
+   }
}
 }
 
@@ -839,15 +841,7 @@ main(int argc, char *argv[])
goto usage;
}
if (file != NULL) {
-   size_t sz;
-
-   sz = strlen(file);
-   if (strcasecmp(file + sz - 4, ".tal") != 0 &&
-   strcasecmp(file + sz - 4, ".cer") != 0 &&
-   strcasecmp(file + sz - 4, ".crl") != 0 &&
-   strcasecmp(file + sz - 4, ".mft") != 0 &&
-   strcasecmp(file + sz - 4, ".roa") != 0 &&
-   strcasecmp(file + sz - 4, ".gbr") != 0)
+   if (rtype_from_file_extension(file) == RTYPE_INVALID)
errx(1, "unsupported or invalid file: %s", file);
 
outputdir = NULL;
Index: parser.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.45
diff -u -p -r1.45 parser.c
--- parser.c19 Jan 2022 15:50:31 -  1.45
+++ parser.c19 Jan 2022 16:51:05 -
@@ -949,23 +949,10 @@ proc_parser_file(char *file, unsigned ch
struct tal *tal = NULL;
enum rtype type;
char *aia = NULL, *aki = NULL, *ski = NULL;
-   size_t sz;
unsigned long verify_flags = X509_V_FLAG_CRL_CHECK;
 
-   sz = strlen(file);
-   if (strcasecmp(file + sz - 4, ".tal") == 0)
-   type = RTYPE_TAL;
-   else if (strcasecmp(file + sz - 4, ".cer") == 0)
-   type = RTYPE_CER;
-   else if (strcasecmp(file + sz - 4, ".crl") == 0)
-   type = RTYPE_CRL;
-   else if (strcasecmp(file + sz - 4, ".mft") == 0)
-   type = RTYPE_MFT;
-   else if (strcasecmp(file + sz - 4, ".roa") == 0)
-   type = RTYPE_ROA;
-   else if (strcasecmp(file + sz - 4, ".gbr") == 0)
-   type = RTYPE_GBR;
-   else
+   type = rtype_from_file_extension(file);
+   if (type == RTYPE_INVALID)
errx(1, "%s: unsupported file type", file);
 
switch (type) {
Index: validate.c

OpenBSD Errata: January 19, 2022 (libexpat vmm)

2022-01-19 Thread Alexander Bluhm
Errata patches for expat XML library have been released for OpenBSD
6.9 and 7.0.  Errata patch for kernel vmm has been released for
OpenBSD 7.0.

Binary updates for the amd64, i386 and arm64 platform are available
via the syspatch utility.  Source code patches can be found on the
respective errata page:

  https://www.openbsd.org/errata69.html
  https://www.openbsd.org/errata70.html



Re: Implement rpki-client -f file

2022-01-19 Thread Job Snijders
OK job@



Re: Implement rpki-client -f file

2022-01-19 Thread Theo Buehler
On Wed, Jan 19, 2022 at 02:57:44PM +0100, Claudio Jeker wrote:
> > > + return;
> > > + }
> > > +
> > > + /* TA found play back the stack and add all certs */
> > > + do {
> > > + if (parser_cert_validate(nfile, cert) == NULL)
> > > + break;
> > 
> > I think this also needs a cert_free(cert).
> 
> Certs are special and never released once valid. The are added to the
> auths tree. If parser_cert_validate() fails then cert is already freed so
> that should also be fine.

Indeed. I got it now.

>  
> > > + free(nfile);
> > > + cert = stack[stacksz - 1];
> > > + nfile = filestack[stacksz - 1];
> > 
> > I'm probably missing something, but I don't see why we don't hit this
> > in the last iteration where stacksz == 0 (which would be bad).
> > 
> > > + } while (stacksz-- > 0);
> > 
> > It's probably related to my above confusion: We don't need to free cert
> > if parser_cert_validate() fails but do need to free it if we exit the
> > while loop because stacksz == 0.
> 
> Yeah, I'm also confused. I think the code currently underflows the stack
> which seems to result in a NULL cert.
>  
> Anyway I totally rewrote that bit without recursion. Have a look below.
> Only think I dislike is the fact that parse_load_certchain() is using the
> URI instead of the filename (but should be easy enough to convert between
> the two).

This is much clearer to me and I like the non-recursive version better.

ok tb

Some tiny nits for your consideration. Apart from the %i do as you
prefer :)

> Index: parser.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 parser.c
> --- parser.c  18 Jan 2022 18:19:47 -  1.44
> +++ parser.c  19 Jan 2022 13:54:06 -
> @@ -383,24 +383,13 @@ proc_parser_mft(char *file, const unsign
>  }
>  
>  /*
> - * Certificates are from manifests (has a digest and is signed with
> - * another certificate) Parse the certificate, make sure its
> - * signatures are valid (with CRLs), then validate the RPKI content.
> - * This returns a certificate (which must not be freed) or NULL on
> - * parse failure.
> + * Validate a certificate, if invalid free the resouces and return NULL.
>   */
>  static struct cert *
> -proc_parser_cert(char *file, const unsigned char *der, size_t len)
> +parser_cert_validate(char *file, struct cert *cert)

This is the only function with a parser_ prefix.
Should this be proc_parser_cert_validate()?
(parse_cert_validate would be a bad name).

[...]

> +/*
> + * Build the certificate chain by using the Authority Information Access.
> + * This requires that the TA are already validated and added to the auths
> + * tree. Once the TA is located in the chain the chain is validated in
> + * reverse order.
> + */
> +static void
> +parse_load_certchain(char *uri)
> +{
> + struct cert *stack[MAX_CERT_DEPTH];
> + char *filestack[MAX_CERT_DEPTH];
> + struct cert *cert;
> + int i, failed;
> +
> + for (i = 0; i < MAX_CERT_DEPTH; i++) {
> + cert = parse_load_cert(uri);
> + if (cert == NULL) {
> + warnx("failed to build authority chain");
> + return;
> + }
> + stack[i] = cert;
> + filestack[i] = uri;
> + if (auth_find(, cert->aki) != NULL) {

no need for braces

> + break;  /* found the TA */
> + }
> + uri = cert->aia;
> + }
> +
> + if (i >= MAX_CERT_DEPTH) {
> + warnx("authority chain exceeds max depth of %i",

use %d instead of %i

> + MAX_CERT_DEPTH);
> + for (i = 0; i < MAX_CERT_DEPTH; i++) {

no need for braces

> + cert_free(stack[i]);
> + }
> + return;
> + }



snmpd(8): clean up variable printing

2022-01-19 Thread Martijn van Duren
The new code uses smi_print_element when debugging is enabled to trace
calls. Unfortunately the current smi_print_element lacks in quite a few
departments. This diff rewrites smi_print_element to be more concise
than what we currently have, without moving into the more complex
territory that snmp(1) has.

Unknown types are printed in a similar fashion to what tcpdump(8)'s
snmp output does.

This change helps mostly with exceptions (NOSUCH{OBJECT,INSTANCE} and
ENDOFMIBVIEW) and distinguishing between different integer types.

I kept the current implementation under smi_print_element_legacy, so
that we don't change the output of trap handlers. We should probably
revisit that one at some point, but I don't think to go into that
territory right now.

OK?

martijn@

p.s. I'm not particularly thrilled about the type hinting, but it's
the cleanest that I could come up with without being too much of an
eyesore or filling the screen up even further.

Index: smi.c
===
RCS file: /cvs/src/usr.sbin/snmpd/smi.c,v
retrieving revision 1.30
diff -u -p -r1.30 smi.c
--- smi.c   21 Oct 2021 15:08:15 -  1.30
+++ smi.c   19 Jan 2022 15:21:20 -
@@ -46,6 +46,7 @@
 
 #include "snmpd.h"
 #include "mib.h"
+#include "application.h"
 
 #define MINIMUM(a, b)  (((a) < (b)) ? (a) : (b))
 
@@ -461,8 +462,9 @@ smi_debug_elements(struct ber_element *r
 }
 #endif
 
+/* Keep around so trap handle scripts don't break */
 char *
-smi_print_element(struct ber_element *root)
+smi_print_element_legacy(struct ber_element *root)
 {
char*str = NULL, *buf, *p;
size_t   len, i;
@@ -520,6 +522,139 @@ smi_print_element(struct ber_element *ro
case BER_TYPE_SET:
default:
str = strdup("");
+   break;
+   }
+
+   return (str);
+
+ fail:
+   free(str);
+   return (NULL);
+}
+
+char *
+smi_print_element(struct ber_element *root)
+{
+   char*str = NULL, *buf, *p;
+   long longv;
+   struct ber_oid   o;
+   char strbuf[BUFSIZ];
+
+   switch (root->be_class) {
+   case BER_CLASS_UNIVERSAL:
+   switch (root->be_type) {
+   case BER_TYPE_INTEGER:
+   if (ober_get_integer(root, ) == -1)
+   goto fail;
+   if (asprintf(, "%lld", v) == -1)
+   goto fail;
+   break;
+   case BER_TYPE_OBJECT:
+   if (ober_get_oid(root, ) == -1)
+   goto fail;
+   if (asprintf(, "%s", smi_oid2string(, strbuf,
+   sizeof(strbuf), 0)) == -1)
+   goto fail;
+   break;
+   case BER_TYPE_OCTETSTRING:
+   if (ober_get_string(root, ) == -1)
+   goto fail;
+   p = reallocarray(NULL, 4, root->be_len + 1);
+   if (p == NULL)
+   goto fail;
+   strvisx(p, buf, root->be_len, VIS_NL);
+   if (asprintf(, "\"%s\"", p) == -1) {
+   free(p);
+   goto fail;
+   }
+   free(p);
+   break;
+   case BER_TYPE_NULL:
+   if (asprintf(, "null") == -1)
+   goto fail;
+   default:
+   /* Should not happen in a valid SNMP packet */
+   if (asprintf(, "[U/%u]", root->be_type) == -1)
+   goto fail;
+   break;
+   }
+   break;
+   case BER_CLASS_APPLICATION:
+   switch (root->be_type) {
+   case SNMP_T_IPADDR:
+   if (ober_get_string(root, ) == -1)
+   goto fail;
+   if (asprintf(, "%s",
+   inet_ntoa(*(struct in_addr *)buf)) == -1)
+   goto fail;
+   break;
+   case SNMP_T_COUNTER32:
+   if (ober_get_integer(root, ) == -1)
+   goto fail;
+   if (asprintf(, "%lld(c32)", v) == -1)
+   goto fail;
+   break;
+   case SNMP_T_GAUGE32:
+   if (ober_get_integer(root, ) == -1)
+   goto fail;
+   if (asprintf(, "%lld(g32)", v) == -1)
+   goto fail;
+   break;
+   case SNMP_T_TIMETICKS:
+   if (ober_get_integer(root, ) == -1)
+   goto fail;
+   if 

Re: Implement rpki-client -f file

2022-01-19 Thread Claudio Jeker
On Wed, Jan 19, 2022 at 12:56:21PM +0100, Theo Buehler wrote:
> On Wed, Jan 19, 2022 at 11:06:06AM +0100, Claudio Jeker wrote:
> > The idea is that rpki-client -f file will show a human readable output for
> > file. It will also verify that file is valid (or show an error if not).
> > 
> > This implements this as a first version. Especially the output needs
> > improvement but parsing and validation works.
> > For validation rpki-client needs to follow up the chain of trust by
> > traversing the Authority Information Access and the X509v3 CRL
> > Distribution Points fields of the certificate and follow them up until a
> > TA is found. Once a TA is found the stack can be walked back validating
> > each intermediate cert and finally the file in question. While doing that
> > all CRLs referenced by any cert are loaded into rpki-client as well.
> > Now the TA are loaded at the start by loading the TAL files and then using
> > them to validate the TAs.
> > 
> > Since all of this is done in offline mode only the valid/ and ta/
> > directory are used for these lookups. This also simplifies the lookup.
> > Just chop of rsync:// from the URI and prepend valid/ to get the path of
> > the file. Also do not run the cleanup routines at the end because that
> > would remove all of the repo.
> 
> This generally looks good. I have a few nits and questions inline.

> > +   if (file != NULL) {
> > +   size_t sz;
> > +
> > +   sz = strlen(file);
> > +   if (strcasecmp(file + sz - 4, ".tal") != 0 &&
> > +   strcasecmp(file + sz - 4, ".cer") != 0 &&
> > +   strcasecmp(file + sz - 4, ".crl") != 0 &&
> > +   strcasecmp(file + sz - 4, ".mft") != 0 &&
> > +   strcasecmp(file + sz - 4, ".roa") != 0 &&
> > +   strcasecmp(file + sz - 4, ".gbr") != 0)
> > +   errx(1, "unsupported or invalid file: %s", file);
> 
> We have so many of these file extension checks now. I wonder if it would
> be worth adding a helper function that does this and returns the correct
> RTYPE_* (perhaps renaming the unused RTYPE_EOF into RTYPE_INVALID).
> This might simplify/streamline a few things.
> 
> Obviously not part of this diff. I could take a stab at this if you're
> up for it.

It may be useful since there is similar code in the parser. I wanted to
exit asap with a unknown file but maybe we want to support multiple files
in -f mode and then check makes less sense.

I want to prevent things like rpki-client -f /etc/passwd but the check is
kind of duplicated here and in parser.c.

> > +   /* Look for the TA which is hopefully in the auths tree */
> > +   if (auth_find(, cert->aki) == NULL) {
> > +   if (stacksz >= MAX_CERT_DEPTH) {
> > +   warnx("authority chain exceeds max depth of %i",
> > +   MAX_CERT_DEPTH);
> > +   goto done;
> > +   }
> > +   stack[stacksz] = cert;
> > +   filestack[stacksz] = nfile;
> > +   stacksz++;
> > +   /* recurse on next certificate */
> > +   parse_load_certchain(cert->aia);
> 
> I'm not super keen on recursion in such complicated contexts, but I
> don't readily see a way to do this iteratively. Need to ponder this a
> bit more.

It is a similar issue to X509_verify_cert(). Recursion works nicely here
but I guess it is possible to replace it with a for() loop. I would split
parse_load_certchain into two, the part that loads the cert and the bits
responsible for the chain.
 
> > +   return;
> > +   }
> > +
> > +   /* TA found play back the stack and add all certs */
> > +   do {
> > +   if (parser_cert_validate(nfile, cert) == NULL)
> > +   break;
> 
> I think this also needs a cert_free(cert).

Certs are special and never released once valid. The are added to the
auths tree. If parser_cert_validate() fails then cert is already freed so
that should also be fine.
 
> > +   free(nfile);
> > +   cert = stack[stacksz - 1];
> > +   nfile = filestack[stacksz - 1];
> 
> I'm probably missing something, but I don't see why we don't hit this
> in the last iteration where stacksz == 0 (which would be bad).
> 
> > +   } while (stacksz-- > 0);
> 
> It's probably related to my above confusion: We don't need to free cert
> if parser_cert_validate() fails but do need to free it if we exit the
> while loop because stacksz == 0.

Yeah, I'm also confused. I think the code currently underflows the stack
which seems to result in a NULL cert.
 
Anyway I totally rewrote that bit without recursion. Have a look below.
Only think I dislike is the fact that parse_load_certchain() is using the
URI instead of the filename (but should be easy enough to convert between
the two).

-- 
:wq Claudio

Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.108
diff -u -p -r1.108 extern.h
--- 

Re: Implement rpki-client -f file

2022-01-19 Thread Theo Buehler
On Wed, Jan 19, 2022 at 11:06:06AM +0100, Claudio Jeker wrote:
> The idea is that rpki-client -f file will show a human readable output for
> file. It will also verify that file is valid (or show an error if not).
> 
> This implements this as a first version. Especially the output needs
> improvement but parsing and validation works.
> For validation rpki-client needs to follow up the chain of trust by
> traversing the Authority Information Access and the X509v3 CRL
> Distribution Points fields of the certificate and follow them up until a
> TA is found. Once a TA is found the stack can be walked back validating
> each intermediate cert and finally the file in question. While doing that
> all CRLs referenced by any cert are loaded into rpki-client as well.
> Now the TA are loaded at the start by loading the TAL files and then using
> them to validate the TAs.
> 
> Since all of this is done in offline mode only the valid/ and ta/
> directory are used for these lookups. This also simplifies the lookup.
> Just chop of rsync:// from the URI and prepend valid/ to get the path of
> the file. Also do not run the cleanup routines at the end because that
> would remove all of the repo.

This generally looks good. I have a few nits and questions inline.

> 
> -- 
> :wq Claudio
> 
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.108
> diff -u -p -r1.108 extern.h
> --- extern.h  18 Jan 2022 16:36:49 -  1.108
> +++ extern.h  18 Jan 2022 16:52:29 -
> @@ -294,6 +294,7 @@ enum rtype {
>   RTYPE_CRL,
>   RTYPE_GBR,
>   RTYPE_REPO,
> + RTYPE_FILE,
>  };
>  
>  enum http_result {
> @@ -393,6 +394,7 @@ struct msgbuf;
>  
>  /* global variables */
>  extern int verbose;
> +extern int filemode;
>  extern const char *tals[];
>  extern const char *taldescs[];
>  extern unsigned int talrepocnt[];
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.176
> diff -u -p -r1.176 main.c
> --- main.c14 Jan 2022 15:00:23 -  1.176
> +++ main.c18 Jan 2022 08:50:21 -
> @@ -67,6 +67,7 @@ const char  *bird_tablename = "ROAS";
>  
>  int  verbose;
>  int  noop;
> +int  filemode;
>  int  rrdpon = 1;
>  int  repo_timeout;
>  
> @@ -385,25 +386,23 @@ queue_add_from_mft_set(const struct mft 
>  }
>  
>  /*
> - * Add a local TAL file (RFC 7730) to the queue of files to fetch.
> + * Add a local file to the queue of files to fetch.
>   */
>  static void
> -queue_add_tal(const char *file, int talid)
> +queue_add_file(const char *file, enum rtype type, int talid)
>  {
>   unsigned char   *buf;
>   char*nfile;
>   size_t   len;
>  
>   buf = load_file(file, );
> - if (buf == NULL) {
> - warn("%s", file);
> - return;
> - }
> + if (buf == NULL)
> + err(1, "%s", file);
>  
>   if ((nfile = strdup(file)) == NULL)
>   err(1, NULL);
>   /* Not in a repository, so directly add to queue. */
> - entityq_add(NULL, nfile, RTYPE_TAL, NULL, buf, len, talid);
> + entityq_add(NULL, nfile, type, NULL, buf, len, talid);
>  }
>  
>  /*
> @@ -508,11 +507,13 @@ entity_process(struct ibuf *b, struct st
>   io_read_buf(b, , sizeof(type));
>   io_read_str(b, );
>  
> + /* in filemode messages can be ignored, only the accounting matters */
> + if (filemode)
> + goto done;
> +
>   if (filepath_add(, file) == 0) {
>   warnx("%s: File already visited", file);
> - free(file);
> - entity_queue--;
> - return;
> + goto done;
>   }
>  
>   switch (type) {
> @@ -580,10 +581,13 @@ entity_process(struct ibuf *b, struct st
>   case RTYPE_GBR:
>   st->gbrs++;
>   break;
> + case RTYPE_FILE:
> + break;
>   default:
>   errx(1, "unknown entity type %d", type);
>   }
>  
> +done:
>   free(file);
>   entity_queue--;
>  }
> @@ -771,6 +775,7 @@ main(int argc, char *argv[])
>   break;
>   case 'f':
>   file = optarg;
> + filemode = 1;
>   noop = 1;
>   break;
>   case 'j':
> @@ -831,20 +836,35 @@ main(int argc, char *argv[])
>   warnx("cache directory required");
>   goto usage;
>   }
> - if (outputdir == NULL) {
> + if (file != NULL) {
> + size_t sz;
> +
> + sz = strlen(file);
> + if (strcasecmp(file + sz - 4, ".tal") != 0 &&
> + strcasecmp(file + sz - 4, ".cer") != 0 &&
> + strcasecmp(file + sz - 4, ".crl") != 0 &&
> + strcasecmp(file + sz - 4, ".mft") != 0 &&
> + strcasecmp(file + sz - 4, 

Re: usr.sbin/ospf6d: fix -Wunused-but-set-variable warnings

2022-01-19 Thread Claudio Jeker
On Mon, Jan 17, 2022 at 02:54:32PM +0100, Christian Weisgerber wrote:
> usr.sbin/ospf6d: fix -Wunused-but-set-variable warnings
> 
> merge_config() sets "rchange", but doesn't use it.  Comparing the
> code to osfpd/ospfd.c makes me think that's an omission.  Either
> way it seems odd that the two code bases differ here.
> 
> rde_summary_update() is incomplete.  We can simply #ifdef out the
> unused variables.  The lack of indentation or braces following an
> "else" is not pretty.  I don't know if we want to fix that up.
>  
> M  usr.sbin/ospf6d/ospf6d.c
> M  usr.sbin/ospf6d/rde.c
> 
> diff 436bb480188bab67f704c5f9fcbcf0478db9c100 
> a992977b148f5fd9d4e3b9af9aeccac488edfa7a
> blob - b1193eaf336b9f5aa6a3b076efe4080881829152
> blob + af22bd43781a4eaa32b5454fe8fa4fc9992f0b4b
> --- usr.sbin/ospf6d/ospf6d.c
> +++ usr.sbin/ospf6d/ospf6d.c
> @@ -738,7 +738,7 @@ merge_config(struct ospfd_conf *conf, struct ospfd_con
>   if_start(conf, iface);
>   }
>   }
> - if (a->dirty) {
> + if (a->dirty || rchange) {
>   a->dirty = 0;
>   orig_rtr_lsa(LIST_FIRST(>iface_list)->area);
>   }

This is indeed a missing bugfix but it does not really matter since area
support in ospf6d is not finished.

Fixing this is still the right thing. OK claudio@


> blob - f4a047206ec2c2e6d2550562560d5933825fb39d
> blob + 4c43bb9296fef10f6d8f9e6a63fcc76e16fe5de9
> --- usr.sbin/ospf6d/rde.c
> +++ usr.sbin/ospf6d/rde.c
> @@ -1249,8 +1249,10 @@ void
>  rde_summary_update(struct rt_node *rte, struct area *area)
>  {
>   struct vertex   *v = NULL;
> -//XXXstruct lsa  *lsa;
> +#if 0 /* XXX */
> + struct lsa  *lsa;
>   u_int16_ttype = 0;
> +#endif
>  
>   /* first check if we actually need to announce this route */
>   if (!(rte->d_type == DT_NET || rte->flags & OSPF_RTR_E))
> @@ -1271,13 +1273,13 @@ rde_summary_update(struct rt_node *rte, struct area *a
>   /* TODO inter-area network route stuff */
>   /* TODO intra-area stuff -- condense LSA ??? */
>  
> +#if 0 /* XXX a lot todo */
>   if (rte->d_type == DT_NET) {
>   type = LSA_TYPE_INTER_A_PREFIX;
>   } else if (rte->d_type == DT_RTR) {
>   type = LSA_TYPE_INTER_A_ROUTER;
>   } else
>  
> -#if 0 /* XXX a lot todo */
>   /* update lsa but only if it was changed */
>   v = lsa_find(area, type, rte->prefix.s_addr, rde_router_id());
>   lsa = orig_sum_lsa(rte, area, type, rte->invalid);
> 

This is fine with me as well. OK claudio@

-- 
:wq Claudio



Re: rpki-client x509 verification in common function

2022-01-19 Thread Claudio Jeker
On Tue, Jan 18, 2022 at 02:41:38PM +0100, Claudio Jeker wrote:
> On Tue, Jan 18, 2022 at 02:09:08PM +0100, Theo Buehler wrote:
> > On Tue, Jan 18, 2022 at 12:16:44PM +0100, Claudio Jeker wrote:
> > > How X509_verify_cert() is called in rpki-client is mostly the same in all
> > > places so move all this X509 boilerplate into valid_x509().
> > > 
> > > This simplifies the x509 validation in the parser a fair but and will also
> > > make it easier for -f to validate certs.
> > > 
> > > OK?
> > 
> > ok
> > 
> > I would suggest we merge the three if (crl != NULL) checks into one
> > (maybe in a follow-up).
> 
> Sure, I tried to keep this as mechanical as possible since this is nasty
> code that does not permit errors.
>  
> > The _roa and _gbr paths called the warnx() with the
> > X509_verify_cert_error_string() only conditionally. I guess we can
> > adjust that later if this turns out to be too noisy.
>  
> Yes, I forgot to mention that. I think this are some left-overs from the
> time where CRLs were optional. At least I see no reason why
> X509_V_ERR_UNABLE_TO_GET_CRL errors are only printed at verbose level 1 or
> higher.

I looked a bit more into this. So I added this reduced verbosity in Rev
1.17 of main.c. This was before the Elk Lakes hackathon that fixed most of
the validation code. For example in Rev 1.39 of main a few months later the
lookup of CRLs changed to use the AKI for lookups and away from using some
sort of normalized name. I think this and some of the later diffs made
this workaround obsolete.

-- 
:wq Claudio



Implement rpki-client -f file

2022-01-19 Thread Claudio Jeker
The idea is that rpki-client -f file will show a human readable output for
file. It will also verify that file is valid (or show an error if not).

This implements this as a first version. Especially the output needs
improvement but parsing and validation works.
For validation rpki-client needs to follow up the chain of trust by
traversing the Authority Information Access and the X509v3 CRL
Distribution Points fields of the certificate and follow them up until a
TA is found. Once a TA is found the stack can be walked back validating
each intermediate cert and finally the file in question. While doing that
all CRLs referenced by any cert are loaded into rpki-client as well.
Now the TA are loaded at the start by loading the TAL files and then using
them to validate the TAs.

Since all of this is done in offline mode only the valid/ and ta/
directory are used for these lookups. This also simplifies the lookup.
Just chop of rsync:// from the URI and prepend valid/ to get the path of
the file. Also do not run the cleanup routines at the end because that
would remove all of the repo.

-- 
:wq Claudio

Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.108
diff -u -p -r1.108 extern.h
--- extern.h18 Jan 2022 16:36:49 -  1.108
+++ extern.h18 Jan 2022 16:52:29 -
@@ -294,6 +294,7 @@ enum rtype {
RTYPE_CRL,
RTYPE_GBR,
RTYPE_REPO,
+   RTYPE_FILE,
 };
 
 enum http_result {
@@ -393,6 +394,7 @@ struct msgbuf;
 
 /* global variables */
 extern int verbose;
+extern int filemode;
 extern const char *tals[];
 extern const char *taldescs[];
 extern unsigned int talrepocnt[];
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.176
diff -u -p -r1.176 main.c
--- main.c  14 Jan 2022 15:00:23 -  1.176
+++ main.c  18 Jan 2022 08:50:21 -
@@ -67,6 +67,7 @@ const char*bird_tablename = "ROAS";
 
 intverbose;
 intnoop;
+intfilemode;
 intrrdpon = 1;
 intrepo_timeout;
 
@@ -385,25 +386,23 @@ queue_add_from_mft_set(const struct mft 
 }
 
 /*
- * Add a local TAL file (RFC 7730) to the queue of files to fetch.
+ * Add a local file to the queue of files to fetch.
  */
 static void
-queue_add_tal(const char *file, int talid)
+queue_add_file(const char *file, enum rtype type, int talid)
 {
unsigned char   *buf;
char*nfile;
size_t   len;
 
buf = load_file(file, );
-   if (buf == NULL) {
-   warn("%s", file);
-   return;
-   }
+   if (buf == NULL)
+   err(1, "%s", file);
 
if ((nfile = strdup(file)) == NULL)
err(1, NULL);
/* Not in a repository, so directly add to queue. */
-   entityq_add(NULL, nfile, RTYPE_TAL, NULL, buf, len, talid);
+   entityq_add(NULL, nfile, type, NULL, buf, len, talid);
 }
 
 /*
@@ -508,11 +507,13 @@ entity_process(struct ibuf *b, struct st
io_read_buf(b, , sizeof(type));
io_read_str(b, );
 
+   /* in filemode messages can be ignored, only the accounting matters */
+   if (filemode)
+   goto done;
+
if (filepath_add(, file) == 0) {
warnx("%s: File already visited", file);
-   free(file);
-   entity_queue--;
-   return;
+   goto done;
}
 
switch (type) {
@@ -580,10 +581,13 @@ entity_process(struct ibuf *b, struct st
case RTYPE_GBR:
st->gbrs++;
break;
+   case RTYPE_FILE:
+   break;
default:
errx(1, "unknown entity type %d", type);
}
 
+done:
free(file);
entity_queue--;
 }
@@ -771,6 +775,7 @@ main(int argc, char *argv[])
break;
case 'f':
file = optarg;
+   filemode = 1;
noop = 1;
break;
case 'j':
@@ -831,20 +836,35 @@ main(int argc, char *argv[])
warnx("cache directory required");
goto usage;
}
-   if (outputdir == NULL) {
+   if (file != NULL) {
+   size_t sz;
+
+   sz = strlen(file);
+   if (strcasecmp(file + sz - 4, ".tal") != 0 &&
+   strcasecmp(file + sz - 4, ".cer") != 0 &&
+   strcasecmp(file + sz - 4, ".crl") != 0 &&
+   strcasecmp(file + sz - 4, ".mft") != 0 &&
+   strcasecmp(file + sz - 4, ".roa") != 0 &&
+   strcasecmp(file + sz - 4, ".gbr") != 0)
+   errx(1, "unsupported or invalid file: %s", file);
+
+   outputdir = NULL;
+   } else if (outputdir == NULL) {
warnx("output directory required");

Re: rpki-client: plug leak in queue_add_from_tal()

2022-01-19 Thread Claudio Jeker
On Wed, Jan 19, 2022 at 09:35:34AM +0100, Theo Buehler wrote:
> This is the straightforward way to fix the leak of nfile in case the
> repo isn't found. The other option would be to defer the strdup until
> after successful lookup, but that felt clunky. 

Yes, that's OK claudio@. I agree that this is the best way of fixing this.
 
> While I'm here: Any objections to silencing this annoying compiler
> warning in main.c? The other option would be to remove "file" and add it
> back once it's actually going to be used.

No need to silence this, I have a diff that will make use of file.
I will send it out today.
 
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.176
> diff -u -p -U5 -r1.176 main.c
> --- main.c14 Jan 2022 15:00:23 -  1.176
> +++ main.c19 Jan 2022 08:22:53 -
> @@ -427,12 +427,14 @@ queue_add_from_tal(struct tal *tal)
>   if ((nfile = strdup(nfile + 1)) == NULL)
>   err(1, NULL);
>  
>   /* Look up the repository. */
>   repo = ta_lookup(tal->id, tal);
> - if (repo == NULL)
> + if (repo == NULL) {
> + free(nfile);
>   return;
> + }
>  
>   /* steal the pkey from the tal structure */
>   data = tal->pkey;
>   tal->pkey = NULL;
>   entityq_add(NULL, nfile, RTYPE_CER, repo, data, tal->pkeysz, tal->id);
> @@ -769,10 +771,11 @@ main(int argc, char *argv[])
>   case 'e':
>   rsync_prog = optarg;
>   break;
>   case 'f':
>   file = optarg;
> + (void)file;
>   noop = 1;
>   break;
>   case 'j':
>   outformats |= FORMAT_JSON;
>   break;
> 

-- 
:wq Claudio



rpki-client: plug leak in queue_add_from_tal()

2022-01-19 Thread Theo Buehler
This is the straightforward way to fix the leak of nfile in case the
repo isn't found. The other option would be to defer the strdup until
after successful lookup, but that felt clunky. 

While I'm here: Any objections to silencing this annoying compiler
warning in main.c? The other option would be to remove "file" and add it
back once it's actually going to be used.

Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.176
diff -u -p -U5 -r1.176 main.c
--- main.c  14 Jan 2022 15:00:23 -  1.176
+++ main.c  19 Jan 2022 08:22:53 -
@@ -427,12 +427,14 @@ queue_add_from_tal(struct tal *tal)
if ((nfile = strdup(nfile + 1)) == NULL)
err(1, NULL);
 
/* Look up the repository. */
repo = ta_lookup(tal->id, tal);
-   if (repo == NULL)
+   if (repo == NULL) {
+   free(nfile);
return;
+   }
 
/* steal the pkey from the tal structure */
data = tal->pkey;
tal->pkey = NULL;
entityq_add(NULL, nfile, RTYPE_CER, repo, data, tal->pkeysz, tal->id);
@@ -769,10 +771,11 @@ main(int argc, char *argv[])
case 'e':
rsync_prog = optarg;
break;
case 'f':
file = optarg;
+   (void)file;
noop = 1;
break;
case 'j':
outformats |= FORMAT_JSON;
break;