Re: LibreSSL: handle EXFLAG_INVALID

2021-03-03 Thread Theo Buehler
On Thu, Feb 25, 2021 at 09:34:30PM +0100, Tobias Heider wrote:
> Hi,
> 
> while testing different x509 validator corner cases i found that a bunch of
> errors are currently not handled in libcrypto.
> 
> In particular duplicate or undecodable extensions are ignored.
> The diff below sets EXFLAG_INVALID whenever X509_get_ext_d2i() returns
> an error (other than "not found") and later throws X509_V_ERR_UNSPECIFIED
> if EXFLAG_INVALID is set.

Yes, we want this in some form. Apart from the X509_vfy bit this is a
subset of the changes in

https://github.com/openssl/openssl/pull/10756

While I'd be happy to land this in a few smaller pieces, I think we want
most of those changes.  In particular, I think we want to check for
EXFLAG_INVALID after all calls to x509v3_cache_extensions().

I don't currently understand why your x509_vfy change was not done (and
why they did not add a check to check_ca()).

> 
> Index: x509/x509_purp.c
> ===
> RCS file: /mount/openbsd/cvs/src/lib/libcrypto/x509/x509_purp.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 x509_purp.c
> --- x509/x509_purp.c  13 Sep 2020 15:06:17 -  1.2
> +++ x509/x509_purp.c  25 Feb 2021 20:25:11 -
> @@ -421,7 +421,12 @@ setup_crldp(X509 *x)
>  {
>   int i;
>  
> - x->crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, NULL, NULL);
> + x->crldp = X509_get_ext_d2i(x, NID_crl_distribution_points, , NULL);
> + if (x->crldp == NULL && i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
> + return;
> + }
> +
>   for (i = 0; i < sk_DIST_POINT_num(x->crldp); i++)
>   setup_dp(x, sk_DIST_POINT_value(x->crldp, i));
>  }
> @@ -449,7 +454,7 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_flags |= EXFLAG_V1;
>  
>   /* Handle basic constraints */
> - if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, NULL, NULL))) {
> + if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, , NULL))) {
>   if (bs->ca)
>   x->ex_flags |= EXFLAG_CA;
>   if (bs->pathlen) {
> @@ -463,10 +468,12 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_pathlen = -1;
>   BASIC_CONSTRAINTS_free(bs);
>   x->ex_flags |= EXFLAG_BCONS;
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
>   /* Handle proxy certificates */
> - if ((pci = X509_get_ext_d2i(x, NID_proxyCertInfo, NULL, NULL))) {
> + if ((pci = X509_get_ext_d2i(x, NID_proxyCertInfo, , NULL))) {
>   if (x->ex_flags & EXFLAG_CA ||
>   X509_get_ext_by_NID(x, NID_subject_alt_name, -1) >= 0 ||
>   X509_get_ext_by_NID(x, NID_issuer_alt_name, -1) >= 0) {
> @@ -485,10 +492,12 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_pcpathlen = -1;
>   PROXY_CERT_INFO_EXTENSION_free(pci);
>   x->ex_flags |= EXFLAG_PROXY;
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
>   /* Handle key usage */
> - if ((usage = X509_get_ext_d2i(x, NID_key_usage, NULL, NULL))) {
> + if ((usage = X509_get_ext_d2i(x, NID_key_usage, , NULL))) {
>   if (usage->length > 0) {
>   x->ex_kusage = usage->data[0];
>   if (usage->length > 1)
> @@ -497,9 +506,12 @@ x509v3_cache_extensions(X509 *x)
>   x->ex_kusage = 0;
>   x->ex_flags |= EXFLAG_KUSAGE;
>   ASN1_BIT_STRING_free(usage);
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
> +
>   x->ex_xkusage = 0;
> - if ((extusage = X509_get_ext_d2i(x, NID_ext_key_usage, NULL, NULL))) {
> + if ((extusage = X509_get_ext_d2i(x, NID_ext_key_usage, , NULL))) {
>   x->ex_flags |= EXFLAG_XKUSAGE;
>   for (i = 0; i < sk_ASN1_OBJECT_num(extusage); i++) {
>   switch (OBJ_obj2nid(sk_ASN1_OBJECT_value(extusage, i))) 
> {
> @@ -538,19 +550,27 @@ x509v3_cache_extensions(X509 *x)
>   }
>   }
>   sk_ASN1_OBJECT_pop_free(extusage, ASN1_OBJECT_free);
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
> - if ((ns = X509_get_ext_d2i(x, NID_netscape_cert_type, NULL, NULL))) {
> + if ((ns = X509_get_ext_d2i(x, NID_netscape_cert_type, , NULL))) {
>   if (ns->length > 0)
>   x->ex_nscert = ns->data[0];
>   else
>   x->ex_nscert = 0;
>   x->ex_flags |= EXFLAG_NSCERT;
>   ASN1_BIT_STRING_free(ns);
> + } else if (i != -1) {
> + x->ex_flags |= EXFLAG_INVALID;
>   }
>  
> - x->skid = X509_get_ext_d2i(x, NID_subject_key_identifier, NULL, NULL);
> - x->akid = X509_get_ext_d2i(x, NID_authority_key_identifier, NULL, NULL);
> + x->skid = 

Re: smtpd: use libtls

2021-03-02 Thread Theo Buehler
On Sat, Feb 13, 2021 at 06:26:02PM +0100, Eric Faurot wrote:
> Hi.
> 
> The diff seems to work for the few people who tested it (thanks).
> Anyone wants to ok this?

I read through the diff several times, but I'm not familiar with smtpd
so cannot claim a thorough review. Nothing really stood out as odd or
wrong. I like the simplification a lot.

ok tb

I only have these two trivial remarks:

>  int
> -io_start_tls(struct io *io, void *tls)
> +io_accept_tls(struct io *io, struct tls *tls)
>  {
>   int mode;
>  
>   mode = io->flags & IO_RW;
> - if (mode == 0 || mode == IO_RW)
> - errx(1, "io_start_tls(): full-duplex or unset");
> + if (mode != IO_READ)
> + errx(1, "io_connect_tls: expect IO_READ mode");

should be io_accept_tls

>  
>   if (io->tls)
>   errx(1, "io_start_tls(): TLS already started");

dito

>   io->tls = tls;
> + io->state = IO_STATE_ACCEPT_TLS;
> + io_reset(io, EV_READ, io_dispatch_accept_tls);
> +
> + return (0);
> +}



Re: rpki-client, unify err() for out of memory situation

2021-03-02 Thread Theo Buehler
On Tue, Mar 02, 2021 at 02:09:37PM +0100, Claudio Jeker wrote:
> This diff just brings all err(3) calls for out of memory situations to one
> form: err(1, NULL);
> It is not very helpful to tell if malloc, strdup or asprintf failed with no
> mem. Just one common idiom.
> 
> OK?

ok.

The https diff will again add a few more of those.



Re: Teach rpki-client some https

2021-03-02 Thread Theo Buehler
On Tue, Mar 02, 2021 at 11:45:22AM +0100, Claudio Jeker wrote:
> On Mon, Mar 01, 2021 at 11:57:03AM +0100, Claudio Jeker wrote:
> > On Sun, Feb 28, 2021 at 09:09:05AM +0100, Theo Buehler wrote:
> > > On Thu, Feb 25, 2021 at 05:03:19PM +0100, Claudio Jeker wrote:
> > > > On Fri, Feb 19, 2021 at 07:10:02PM +0100, Claudio Jeker wrote:
> > > > > Some TAL files now include an https URI where the TA can be fetched 
> > > > > from.
> > > > > With this diff rpki-client will download the TA from https unless that
> > > > > fails and then fall back to rsync.
> > > > > 
> > > > > This is not yet perfect but the diff is already large enough (adding a
> > > > > full event based https client based on ftp codebase). For RRDP more a 
> > > > > lot
> > > > > more is required and I probably will refactor the main.c code then.
> > > > > 
> > > > > At the moment this adds a local mkostempat() function to implement
> > > > > mkostemp() but with openat() instead of open(). I hope in the future 
> > > > > libc
> > > > > will provide something.
> > > > > 
> > > > > Thanks in advance for all the feedback
> > > > 
> > > > Updated diff. I found some bugs in the http.c code base regarding
> > > > conncetion failures (because of unreachable IPv6 on the server) and 
> > > > fixed
> > > > redirects. Those should now be fixed.
> > > 
> > > A couple of comments inline. Some cosmetic, some minor bugs.
> > > 
> > > Apart from a return value glitch for tls_close() the libtls handling
> > > looks correct to me and while I'm not a http expert, the state machine
> > > makes sense. The string handling (chopping up and re-encoding) looks
> > > correct with appropriate bounds checking. I spent quite a bit of time
> > > going through it.
> > > 
> > > Take from my comments whatever you like/makes sense, but I'm ok with
> > > landing this and improving in tree.
> > 
> > Thanks a lot for the review. I know that the code is a bit of mish mash
> > mainly since most of it was stolen from ftp(1).
> >  
> 
> ...
> 
> 
> Here is an updated version of the diff after the last changes to
> rpki-client. There is no longer needs the mkospathat.c file since
> rpki-client now chdirs to the cache directory.
> 
> I think this is now ready to go in.

Go for it!

ok tb



Re: Teach rpki-client some https

2021-02-28 Thread Theo Buehler
On Sun, Feb 28, 2021 at 09:09:05AM +0100, Theo Buehler wrote:

> > +   if (error == EAI_SERVICE)
> > +   error = getaddrinfo(host, "443", , >res0);
> > +   if (error) {
> 
> error != NULL

Apologies, forgot to delete that.



Re: Teach rpki-client some https

2021-02-28 Thread Theo Buehler
On Thu, Feb 25, 2021 at 05:03:19PM +0100, Claudio Jeker wrote:
> On Fri, Feb 19, 2021 at 07:10:02PM +0100, Claudio Jeker wrote:
> > Some TAL files now include an https URI where the TA can be fetched from.
> > With this diff rpki-client will download the TA from https unless that
> > fails and then fall back to rsync.
> > 
> > This is not yet perfect but the diff is already large enough (adding a
> > full event based https client based on ftp codebase). For RRDP more a lot
> > more is required and I probably will refactor the main.c code then.
> > 
> > At the moment this adds a local mkostempat() function to implement
> > mkostemp() but with openat() instead of open(). I hope in the future libc
> > will provide something.
> > 
> > Thanks in advance for all the feedback
> 
> Updated diff. I found some bugs in the http.c code base regarding
> conncetion failures (because of unreachable IPv6 on the server) and fixed
> redirects. Those should now be fixed.

A couple of comments inline. Some cosmetic, some minor bugs.

Apart from a return value glitch for tls_close() the libtls handling
looks correct to me and while I'm not a http expert, the state machine
makes sense. The string handling (chopping up and re-encoding) looks
correct with appropriate bounds checking. I spent quite a bit of time
going through it.

Take from my comments whatever you like/makes sense, but I'm ok with
landing this and improving in tree.

> 
> -- 
> :wq Claudio
> 
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/Makefile,v
> retrieving revision 1.18
> diff -u -p -r1.18 Makefile
> --- Makefile  4 Feb 2021 08:10:24 -   1.18
> +++ Makefile  19 Feb 2021 14:21:08 -
> @@ -1,13 +1,14 @@
>  #$OpenBSD: Makefile,v 1.18 2021/02/04 08:10:24 claudio Exp $
>  
>  PROG=rpki-client
> -SRCS=as.c cert.c cms.c crl.c gbr.c io.c ip.c log.c main.c mft.c 
> mkdir.c \
> - output.c output-bgpd.c output-bird.c output-csv.c output-json.c \
> - parser.c roa.c rsync.c tal.c validate.c x509.c
> +SRCS=as.c cert.c cms.c crl.c gbr.c http.c io.c ip.c log.c main.c 
> mft.c \
> + mkdir.c mkostempat.c output.c output-bgpd.c output-bird.c \
> + output-csv.c output-json.c parser.c roa.c rsync.c tal.c validate.c \
> + x509.c
>  MAN= rpki-client.8
>  
> -LDADD+= -lcrypto -lutil
> -DPADD+= ${LIBCRYPTO} ${LIBUTIL}
> +LDADD+= -ltls -lssl -lcrypto -lutil
> +DPADD+= ${LIBTLS} ${LIBSSL} ${LIBCRYPTO} ${LIBUTIL}
>  
>  CFLAGS+= -Wall -I${.CURDIR}
>  CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.47
> diff -u -p -r1.47 extern.h
> --- extern.h  22 Feb 2021 09:46:05 -  1.47
> +++ extern.h  23 Feb 2021 10:36:38 -
> @@ -399,6 +399,10 @@ void  proc_parser(int) __attribute__((n
>  char *rsync_base_uri(const char *);
>  void  proc_rsync(char *, char *, int) __attribute__((noreturn));
>  
> +/* Http-specific. */
> +
> +void  proc_http(char *, int);
> +
>  /* Logging (though really used for OpenSSL errors). */
>  
>  void  cryptowarnx(const char *, ...)
> @@ -417,6 +421,7 @@ void   io_str_buffer(struct ibuf *, cons
>  void  io_simple_read(int, void *, size_t);
>  void  io_buf_read_alloc(int, void **, size_t *);
>  void  io_str_read(int, char **);
> +int   io_recvfd(int, void *, size_t);
>  
>  /* X509 helpers. */
>  
> @@ -450,6 +455,7 @@ void  logx(const char *fmt, ...)
>   __attribute__((format(printf, 1, 2)));
>  
>  int  mkpathat(int, const char *);
> +int  mkostempat(int, char *, int);
>  
>  #define  RPKI_PATH_OUT_DIR   "/var/db/rpki-client"
>  #define  RPKI_PATH_BASE_DIR  "/var/cache/rpki-client"
> Index: http.c
> ===
> RCS file: http.c
> diff -N http.c
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ http.c24 Feb 2021 20:03:25 -
> @@ -0,0 +1,1226 @@
> +/*  $OpenBSD$  */
> +/*
> + * Copyright (c) 2020 Nils Fisher 
> + * Copyright (c) 2020 Claudio Jeker 
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION 

Re: Workflow question

2021-02-27 Thread Theo Buehler
> Following the advice in the FAQ I added my user to the wobj group. I
> suppose I could "make obj" and have the objs written to /usr/obj? Is
> this a workflow the developers recommend or follow? Thanks!

Yes. More precisely, by default 'make obj' in /usr/src/usr.bin/systat
will create a symlink obj@ -> /usr/obj/usr.bin/systat where the build
artefacts will be placed.

$ ls -ld /usr/obj/usr.bin/systat
drwxrwx---  2 build  wobj  1024 Feb 26 21:47 /usr/obj/usr.bin/systat/



Re: Mesa leak in intel iris driver

2021-02-27 Thread Theo Buehler
On Sat, Feb 27, 2021 at 12:21:35AM +1100, Jonathan Gray wrote:
> Bring in a change which was backported to Mesa 20.1 but not 20.0.
> This is for inteldrm with >= gen8/broadwell hardware.
> /var/log/Xorg.0.log with 'DRI driver: iris' and 'xdriinfo' will
> show 'Screen 0: iris' if you are using the iris driver.

This seems to help a lot on my x280 where xdriinfo prints Screen 0: iris.

inteldrm0 at pci0 dev 2 function 0 "Intel UHD Graphics 620" rev 0x07
drm0 at inteldrm0
inteldrm0: msi, KABYLAKE, gen 9
inteldrm0: 1920x1080, 32bpp

I hadn't paid attention to it previously, but as observed yesterday
after a few hours of uptime, Xorg would already use way above 100M
and grow steadily.

Running with this patch overnight I'm at the below which apart from the
TIME column looks pretty much like right after startup.

  PID USERNAME PRI NICE  SIZE   RES STATE WAIT  TIMECPU COMMAND
24637 _x11   20   35M   65M sleep/1   poll  1:29  0.10% Xorg
20308 root   20 2464K 1284K idle  netio 0:00  0.00% Xorg



Re: LibreSSL legacy verifier regression

2021-02-24 Thread Theo Buehler
On Wed, Feb 24, 2021 at 09:00:05PM +0100, Theo Buehler wrote:
> On Wed, Feb 24, 2021 at 06:47:00AM +0100, Jan Klemkow wrote:
> > Hi,
> > 
> > another co-worker of mine has found an other regress in the LibreSSL
> > legacy verifier.  I took his diff and made a test for our regression
> > framework.
> > 
> > The legacy verifier seems not to check the certificate if no root CA was
> > given.  The following test creates an expired certificate and tries to
> > verify it.  In one case it found the expected error in another, it does
> > not.
> 
> Thanks for the report and the test case, that's very helpful. The diff
> at the end addresses this.
> 
> The verifier does not find the expected error because it now bails out
> earlier.  This is a consequence of a refactoring of X509_verify_cert()
> (x509_vfy.c r1.75) that was done to integrate the new verifier.
> 
> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/x509/x509_vfy.c.diff?r1=1.74=1.75
> 
> What happens is that x509_legacy_verify_build_chain() returns ok == 0 in
> your test case. The safety net at the end of x509_verify_cert_legacy()
> sets ctx->error to X509_V_ERR_UNSPECIFIED (so the unchecked call to
> X509_verify_cert() in your regress test actually indicates verification
> failure).
> 
> 
> The diff below restores the previous behavior and fixes a bug.
> 
> Prior to the the refactoring, each 'goto end' in the code that is now in
> x509_legacy_verify_build_chain() would stop validation, while in other
> cases validation would have carried on. So indicate this via the return
> value and return ok via a pointer.
> 
> The bug is that the return value check of x509_legacy_verify_build_chain()
> should have been if (ok <= 0), not if (!ok).
> 
> 
> Regarding your regress diff: I don't think we want to land it as it is.
> 
> The verifier lives in libcrypto/x509, so the regress test belongs in
> there.
> 
> We really need to come up with an extensible design that can check a
> number of such cases (and ideally includes the bulk of your openssl/x509
> regress tests). I don't want to add a directory for every bug in the
> verifier or legacy verifier. As jsing already mentioned, I expect that
> we want to commit the test certs so we don't need perl modules from
> ports to run the regress. Then we want to add generating scripts and a
> README that gives instructions on how to regenerate the certs if needed.
> 
> I would like to have one C program that runs all tests in a loop (or
> perhaps one C program per family of regressions). It would be nice if
> this C program could also be compiled against OpenSSL 1.1.1 so we can
> easily check for differences of behavior (see x509/bettertls/Makefile
> for an example that does this).  For your test program this just means:
> don't access csc->blah, but use X509_STORE_CTX_get_blah(csc) instead.
> 
> Why does the test set TRUSTED_FIRST?
> 
> The code also needs a bit of cleaning. There are a number of unchecked
> return values, for example strdup and sk_*_push, and csc is leaked
> after X509_verify_cert().
> 
> It would also be nice to run this test against the new verifier.

Missed an obvious simplification.

Index: x509/x509_vfy.c
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
retrieving revision 1.85
diff -u -p -r1.85 x509_vfy.c
--- x509/x509_vfy.c 11 Feb 2021 04:56:43 -  1.85
+++ x509/x509_vfy.c 24 Feb 2021 20:19:34 -
@@ -240,12 +240,13 @@ x509_vfy_check_id(X509_STORE_CTX *ctx) {
  * Oooh..
  */
 static int
-X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad)
+X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad, int *out_ok)
 {
X509 *x, *xtmp, *xtmp2, *chain_ss = NULL;
int bad_chain = 0;
X509_VERIFY_PARAM *param = ctx->param;
-   int depth, i, ok = 0;
+   int ok = 0, ret = 0;
+   int depth, i;
int num, j, retry, trust;
int (*cb) (int xok, X509_STORE_CTX *xctx);
STACK_OF(X509) *sktmp = NULL;
@@ -517,11 +518,15 @@ X509_verify_cert_legacy_build_chain(X509
if (!ok)
goto end;
}
+
+   ret = 1;
  end:
sk_X509_free(sktmp);
X509_free(chain_ss);
*bad = bad_chain;
-   return ok;
+   *out_ok = ok;
+
+   return ret;
 }
 
 static int
@@ -531,8 +536,7 @@ X509_verify_cert_legacy(X509_STORE_CTX *
 
ctx->error = X509_V_OK; /* Initialize to OK */
 
-   ok = X509_verify_cert_legacy_build_chain(ctx, _chain);
-   if (!ok)
+   if (!X509_verify_cert_legacy_build_chain(ctx, _chain, ))
goto end;
 
/* We have the chain complete: now we need to check its purpose */



Re: LibreSSL legacy verifier regression

2021-02-24 Thread Theo Buehler
On Wed, Feb 24, 2021 at 06:47:00AM +0100, Jan Klemkow wrote:
> Hi,
> 
> another co-worker of mine has found an other regress in the LibreSSL
> legacy verifier.  I took his diff and made a test for our regression
> framework.
> 
> The legacy verifier seems not to check the certificate if no root CA was
> given.  The following test creates an expired certificate and tries to
> verify it.  In one case it found the expected error in another, it does
> not.

Thanks for the report and the test case, that's very helpful. The diff
at the end addresses this.

The verifier does not find the expected error because it now bails out
earlier.  This is a consequence of a refactoring of X509_verify_cert()
(x509_vfy.c r1.75) that was done to integrate the new verifier.

https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/x509/x509_vfy.c.diff?r1=1.74=1.75

What happens is that x509_legacy_verify_build_chain() returns ok == 0 in
your test case. The safety net at the end of x509_verify_cert_legacy()
sets ctx->error to X509_V_ERR_UNSPECIFIED (so the unchecked call to
X509_verify_cert() in your regress test actually indicates verification
failure).


The diff below restores the previous behavior and fixes a bug.

Prior to the the refactoring, each 'goto end' in the code that is now in
x509_legacy_verify_build_chain() would stop validation, while in other
cases validation would have carried on. So indicate this via the return
value and return ok via a pointer.

The bug is that the return value check of x509_legacy_verify_build_chain()
should have been if (ok <= 0), not if (!ok).


Regarding your regress diff: I don't think we want to land it as it is.

The verifier lives in libcrypto/x509, so the regress test belongs in
there.

We really need to come up with an extensible design that can check a
number of such cases (and ideally includes the bulk of your openssl/x509
regress tests). I don't want to add a directory for every bug in the
verifier or legacy verifier. As jsing already mentioned, I expect that
we want to commit the test certs so we don't need perl modules from
ports to run the regress. Then we want to add generating scripts and a
README that gives instructions on how to regenerate the certs if needed.

I would like to have one C program that runs all tests in a loop (or
perhaps one C program per family of regressions). It would be nice if
this C program could also be compiled against OpenSSL 1.1.1 so we can
easily check for differences of behavior (see x509/bettertls/Makefile
for an example that does this).  For your test program this just means:
don't access csc->blah, but use X509_STORE_CTX_get_blah(csc) instead.

Why does the test set TRUSTED_FIRST?

The code also needs a bit of cleaning. There are a number of unchecked
return values, for example strdup and sk_*_push, and csc is leaked
after X509_verify_cert().

It would also be nice to run this test against the new verifier.


Index: x509/x509_vfy.c
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
retrieving revision 1.85
diff -u -p -r1.85 x509_vfy.c
--- x509/x509_vfy.c 11 Feb 2021 04:56:43 -  1.85
+++ x509/x509_vfy.c 24 Feb 2021 19:09:03 -
@@ -240,7 +240,7 @@ x509_vfy_check_id(X509_STORE_CTX *ctx) {
  * Oooh..
  */
 static int
-X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad)
+X509_verify_cert_legacy_build_chain(X509_STORE_CTX *ctx, int *bad, int *out_ok)
 {
X509 *x, *xtmp, *xtmp2, *chain_ss = NULL;
int bad_chain = 0;
@@ -517,11 +517,21 @@ X509_verify_cert_legacy_build_chain(X509
if (!ok)
goto end;
}
+
+   sk_X509_free(sktmp);
+   X509_free(chain_ss);
+   *bad = bad_chain;
+   *out_ok = ok;
+
+   return 1;
+
  end:
sk_X509_free(sktmp);
X509_free(chain_ss);
*bad = bad_chain;
-   return ok;
+   *out_ok = ok;
+
+   return 0;
 }
 
 static int
@@ -531,8 +541,7 @@ X509_verify_cert_legacy(X509_STORE_CTX *
 
ctx->error = X509_V_OK; /* Initialize to OK */
 
-   ok = X509_verify_cert_legacy_build_chain(ctx, _chain);
-   if (!ok)
+   if (!X509_verify_cert_legacy_build_chain(ctx, _chain, ))
goto end;
 
/* We have the chain complete: now we need to check its purpose */



Re: rpki-client extra paranoia

2021-02-19 Thread Theo Buehler
On Fri, Feb 19, 2021 at 10:54:29AM +0100, Claudio Jeker wrote:
> Better to make sure that all URI we ingest are sensitive. Similar check
> is already done in cert.c so also do it for the TAL files (even though
> these are normally controled by the user).
> 
> OK?

ok

> -- 
> :wq Claudio
> 
> Index: tal.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/tal.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 tal.c
> --- tal.c 8 Jan 2021 08:09:07 -   1.26
> +++ tal.c 19 Feb 2021 09:21:18 -
> @@ -82,6 +82,7 @@ tal_parse_buffer(const char *fn, char *b
>   char*nl, *line, *f, *file = NULL;
>   unsigned char   *der;
>   size_t   sz, dersz;
> + ssize_t  i;
>   int  rc = 0;
>   struct tal  *tal = NULL;
>   EVP_PKEY*pkey = NULL;
> @@ -101,6 +102,13 @@ tal_parse_buffer(const char *fn, char *b
>   if (*line == '\0')
>   break;
>  
> + /* make sure only US-ASCII chars are in the URL */
> + for (i = 0; i < nl - line; i++) {
> + if (isalnum(line[i]) || ispunct(line[i]))
> + continue;
> + warnx("%s: invalid URI", fn);
> + goto out;
> + }
>   /* Check that the URI is sensible */
>   if (!(strncasecmp(line, "https://;, 8) == 0 ||
>   strncasecmp(line, "rsync://", 8) == 0)) {
> 



Re: relayd check script memory explosion

2021-02-19 Thread Theo Buehler
On Mon, Feb 15, 2021 at 12:03:42PM +1000, Jonathan Matthew wrote:
> It's fairly easy to accidentally configure relayd to try to run check scripts
> faster than they finish, for example if you have a check interval of one
> second and the check script makes a tcp connection to a host that doesn't
> exist any more.
> 
> In this situation, the hce process will keep writing messages to its imsg
> buffer to the parent process asking it to run checks, which causes its memory
> usage to grow without bounds.  If the check script starts working again
> (or if you change it to just 'exit 0') the parent works its way through the
> backlog and memory usage goes back to normal, but ideally relayd would avoid
> doing this to itself.
> 
> If we don't clear the F_CHECK_SENT and F_CHECK_DONE flags in
> hce_launch_checks(), check_script() can use them to figure out if the
> last check request it sent for the host has finished yet, so it can avoid
> building up a backlog of work for the parent.  The ICMP and script check 
> implementations clear these flags as they start checks, and the TCP check
> code doesn't use them at all, so this shouldn't affect anything else.
> 
> ok?

ok tb

> 
> 
> Index: check_script.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/check_script.c,v
> retrieving revision 1.21
> diff -u -p -u -p -r1.21 check_script.c
> --- check_script.c28 May 2017 10:39:15 -  1.21
> +++ check_script.c15 Feb 2021 01:28:54 -
> @@ -38,6 +38,9 @@ check_script(struct relayd *env, struct 
>   struct ctl_scriptscr;
>   struct table*table;
>  
> + if ((host->flags & (F_CHECK_SENT|F_CHECK_DONE)) == F_CHECK_SENT)
> + return;
> +
>   if ((table = table_find(env, host->conf.tableid)) == NULL)
>   fatalx("%s: invalid table id", __func__);
>  
> @@ -52,7 +55,9 @@ check_script(struct relayd *env, struct 
>   fatalx("invalid script path");
>   memcpy(, >conf.timeout, sizeof(scr.timeout));
>  
> - proc_compose(env->sc_ps, PROC_PARENT, IMSG_SCRIPT, , sizeof(scr));
> + if (proc_compose(env->sc_ps, PROC_PARENT, IMSG_SCRIPT, ,
> + sizeof(scr)) == 0)
> + host->flags |= F_CHECK_SENT;
>  }
>  
>  void
> Index: hce.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/hce.c,v
> retrieving revision 1.79
> diff -u -p -u -p -r1.79 hce.c
> --- hce.c 6 Aug 2018 17:31:31 -   1.79
> +++ hce.c 15 Feb 2021 01:28:54 -
> @@ -139,7 +139,6 @@ hce_launch_checks(int fd, short event, v
>   TAILQ_FOREACH(host, >hosts, entry) {
>   if ((host->flags & F_CHECK_DONE) == 0)
>   host->he = HCE_INTERVAL_TIMEOUT;
> - host->flags &= ~(F_CHECK_SENT|F_CHECK_DONE);
>   if (event_initialized(>cte.ev)) {
>   event_del(>cte.ev);
>   close(host->cte.s);
> 



rpki-client: recallocarray conversions

2021-02-19 Thread Theo Buehler
As discussed a few days ago, there are a few reallocarray + memset that
can be directly handled by recallocarray.

Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.101
diff -u -p -r1.101 main.c
--- main.c  18 Feb 2021 10:10:20 -  1.101
+++ main.c  19 Feb 2021 08:56:19 -
@@ -265,12 +265,12 @@ repo_alloc(void)
 {
struct repo *rp;
 
-   rt.repos = reallocarray(rt.repos, rt.reposz + 1, sizeof(struct repo));
+   rt.repos = recallocarray(rt.repos, rt.reposz, rt.reposz + 1,
+   sizeof(struct repo));
if (rt.repos == NULL)
-   err(1, "reallocarray");
+   err(1, "recallocarray");
 
rp = [rt.reposz++];
-   memset(rp, 0, sizeof(struct repo));
rp->id = rt.reposz - 1;
 
return rp;
Index: mft.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
retrieving revision 1.26
diff -u -p -r1.26 mft.c
--- mft.c   16 Feb 2021 07:58:30 -  1.26
+++ mft.c   19 Feb 2021 07:04:25 -
@@ -193,13 +193,12 @@ mft_parse_filehash(struct parse *p, cons
 
/* Insert the filename and hash value. */
 
-   p->res->files = reallocarray(p->res->files, p->res->filesz + 1,
-   sizeof(struct mftfile));
+   p->res->files = recallocarray(p->res->files, p->res->filesz,
+   p->res->filesz + 1, sizeof(struct mftfile));
if (p->res->files == NULL)
err(1, NULL);
 
fent = >res->files[p->res->filesz++];
-   memset(fent, 0, sizeof(struct mftfile));
 
fent->file = fn;
fn = NULL;
Index: roa.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
retrieving revision 1.14
diff -u -p -r1.14 roa.c
--- roa.c   16 Feb 2021 07:58:30 -  1.14
+++ roa.c   19 Feb 2021 07:07:30 -
@@ -111,12 +111,11 @@ roa_parse_addr(const ASN1_OCTET_STRING *
/* FIXME: maximum check. */
}
 
-   p->res->ips = reallocarray(p->res->ips,
-   p->res->ipsz + 1, sizeof(struct roa_ip));
+   p->res->ips = recallocarray(p->res->ips, p->res->ipsz, p->res->ipsz + 1,
+   sizeof(struct roa_ip));
if (p->res->ips == NULL)
err(1, NULL);
res = >res->ips[p->res->ipsz++];
-   memset(res, 0, sizeof(struct roa_ip));
 
res->addr = addr;
res->afi = afi;



Re: further x509 cleanup in rpki-client

2021-02-18 Thread Theo Buehler
On Thu, Feb 18, 2021 at 02:41:39PM +0100, Claudio Jeker wrote:
> Instead of iterating over all x509 extension and look for SKI and AKI use
> X509_get_ext_d2i(). This reduces the complexity a fair bit. Also add
> additional checks (e.g. make sure the extensions are non-critical).
> More cleanup in cert.c should follow but one step at a time.

ok tb



Re: rpki-client, create repo dir in parent process

2021-02-18 Thread Theo Buehler
On Thu, Feb 18, 2021 at 11:57:52AM +0100, Claudio Jeker wrote:
> This diff moves the mkpath() call from the rsync child to the parent.
> As a result the rsync process no longer needs cpath. It will also simplify
> integration of RRDP since that will be another process.

ok tb

> 
> -- 
> :wq Claudio
> 
> ? obj
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.44
> diff -u -p -r1.44 extern.h
> --- extern.h  16 Feb 2021 08:52:00 -  1.44
> +++ extern.h  18 Feb 2021 10:51:17 -
> @@ -449,7 +449,7 @@ intoutput_json(FILE *, struct vrp_tre
>  void logx(const char *fmt, ...)
>   __attribute__((format(printf, 1, 2)));
>  
> -int  mkpath(const char *);
> +int  mkpath(int, const char *);
>  
>  #define  RPKI_PATH_OUT_DIR   "/var/db/rpki-client"
>  #define  RPKI_PATH_BASE_DIR  "/var/cache/rpki-client"
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.101
> diff -u -p -r1.101 main.c
> --- main.c18 Feb 2021 10:10:20 -  1.101
> +++ main.c18 Feb 2021 10:51:17 -
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -91,6 +92,7 @@ RB_PROTOTYPE(filepath_tree, filepath, en
>  
>  static struct filepath_tree  fpt = RB_INITIALIZER();
>  static struct msgbuf procq, rsyncq;
> +static int   cachefd;
>  
>  const char   *bird_tablename = "ROAS";
>  
> @@ -289,6 +291,15 @@ repo_fetch(struct repo *rp)
>   return;
>   }
>  
> + /*
> +  * Create destination location.
> +  * Build up the tree to this point because GPL rsync(1)
> +  * will not build the destination for us.
> +  */
> +
> + if (mkpath(cachefd, rp->local) == -1)
> + err(1, "%s", rp->local);
> +
>   logx("%s: pulling from network", rp->local);
>   if ((b = ibuf_dynamic(256, UINT_MAX)) == NULL)
>   err(1, NULL);
> @@ -684,7 +695,7 @@ add_to_del(char **del, size_t *dsz, char
>  }
>  
>  static size_t
> -repo_cleanup(const char *cachedir)
> +repo_cleanup(int dirfd)
>  {
>   size_t i, delsz = 0;
>   char *argv[2], **del = NULL;
> @@ -692,8 +703,8 @@ repo_cleanup(const char *cachedir)
>   FTSENT *e;
>  
>   /* change working directory to the cache directory */
> - if (chdir(cachedir) == -1)
> - err(1, "%s: chdir", cachedir);
> + if (fchdir(dirfd) == -1)
> + err(1, "fchdir");
>  
>   for (i = 0; i < rt.reposz; i++) {
>   if (asprintf([0], "%s", rt.repos[i].local) == -1)
> @@ -866,6 +877,9 @@ main(int argc, char *argv[])
>   goto usage;
>   }
>  
> + if ((cachefd = open(cachedir, O_RDONLY, 0)) == -1)
> + err(1, "cache directory %s", cachedir);
> +
>   if (outformats == 0)
>   outformats = FORMAT_OPENBGPD;
>  
> @@ -891,8 +905,8 @@ main(int argc, char *argv[])
>   close(fd[1]);
>  
>   /* change working directory to the cache directory */
> - if (chdir(cachedir) == -1)
> - err(1, "%s: chdir", cachedir);
> + if (fchdir(cachefd) == -1)
> + err(1, "fchdir");
>  
>   /* Only allow access to the cache directory. */
>   if (unveil(cachedir, "r") == -1)
> @@ -924,8 +938,8 @@ main(int argc, char *argv[])
>   close(fd[1]);
>  
>   /* change working directory to the cache directory */
> - if (chdir(cachedir) == -1)
> - err(1, "%s: chdir", cachedir);
> + if (fchdir(cachefd) == -1)
> + err(1, "fchdir");
>  
>   if (pledge("stdio rpath cpath proc exec unveil", NULL)
>   == -1)
> @@ -1088,7 +1102,7 @@ main(int argc, char *argv[])
>   if (outputfiles(, ))
>   rc = 1;
>  
> - stats.del_files = repo_cleanup(cachedir);
> + stats.del_files = repo_cleanup(cachefd);
>  
>   logx("Route Origin Authorizations: %zu (%zu failed parse, %zu invalid)",
>   stats.roas, stats.roas_fail, stats.roas_invalid);
> Index: mkdir.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/mkdir.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 mkdir.c
> --- mkdir.c   2 Feb 2021 18:33:11 -   1.1
> +++ mkdir.c   18 Feb 2021 10:51:17 -
> @@ -43,7 +43,7 @@
>   *   dir_mode - file mode of intermediate directories
>   */
>  int
> -mkpath(const char *dir)
> +mkpath(int dirfd, const char *dir)
>  {
>   char *path, *slash;
>   int done;
> @@ -59,7 +59,7 @@ mkpath(const char *dir)
>   done = (*slash == '\0');
>   *slash = '\0';
>  

Re: relayd and TLS client cert verification

2021-02-17 Thread Theo Buehler
Hi

On Thu, Oct 15, 2020 at 05:52:40PM +1100, Ashe Connor wrote:
> Hi there,
> 
> A year or two ago I submitted a patch for adding TLS client certificate 
> validation to relayd.  At the time it didn't make it in, and I stopped 
> pursuing it further.  
> (https://marc.info/?l=openbsd-tech=154509330608643=2)
> 
> I'd still like to see this landed, if at all possible.  I'm continuing to use 
> this feature on my own personal websites, and it works well.

This looks pretty good to me and appears to work in basic testing.  I'd
be willing to get this in provided you address the tiny nits below.

The diff in its current form applies with a little bit of fuzz, it would
be nice if you could rebase it on top of -current.


> 
> The latest diff is attached, or can be viewed online here: 
> https://github.com/openbsd/src/compare/master...kivikakk:relayd-client-verification.patch
> 
> I've added a test that confirms client failure to connect without a 
> certificate at regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl -- it's 
> a bit awkward.  Let me know if I can redo it better.

The problem with this is that it will make the relayd regress fail:

 run-args-ssl-client-verify-fail.pl 
time SUDO= KTRACE= RELAYD= perl -I/usr/src/regress/usr.sbin/relayd 
/usr/src/regress/usr.sbin/relayd/relayd.pl copy 
/usr/src/regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl
Client no 'connect attempt failed' in client.log after 30 seconds at 
/usr/src/regress/usr.sbin/relayd/relayd.pl line 84.
*** Error 255 in . (Makefile:65 'run-args-ssl-client-verify-fail.pl')
FAILED

This will need to be done in such a way that the test passes. I don't
really understand this perl contraption, so I can't really give advice
on this.

> 
> Best,
> 
> Ashe
> 
> ---
> 
> From c63bca7ba7889b43e0a9317e807499eb8ca0db55 Mon Sep 17 00:00:00 2001
> From: Asherah Connor 
> Date: Thu, 15 Oct 2020 17:23:15 +1100
> Subject: [PATCH] TLS client certificate validation
> 
> ---
>  regress/usr.sbin/relayd/Client.pm | 13 ++
>  regress/usr.sbin/relayd/Makefile  | 18 -
>  regress/usr.sbin/relayd/Relayd.pm |  3 +++
>  .../relayd/args-ssl-client-verify-fail.pl | 25 +++
>  .../usr.sbin/relayd/args-ssl-client-verify.pl | 19 ++
>  usr.sbin/relayd/config.c  | 21 
>  usr.sbin/relayd/parse.y   | 15 ++-
>  usr.sbin/relayd/relay.c   | 21 
>  usr.sbin/relayd/relayd.c  |  9 +++
>  usr.sbin/relayd/relayd.conf.5 |  4 +++
>  usr.sbin/relayd/relayd.h  | 14 +++
>  11 files changed, 155 insertions(+), 7 deletions(-)
>  create mode 100644 regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl
>  create mode 100644 regress/usr.sbin/relayd/args-ssl-client-verify.pl
> 
> diff --git a/regress/usr.sbin/relayd/Client.pm 
> b/regress/usr.sbin/relayd/Client.pm
> index b3e011de13d..ec6fa64274e 100644
> --- a/regress/usr.sbin/relayd/Client.pm
> +++ b/regress/usr.sbin/relayd/Client.pm
> @@ -57,6 +57,11 @@ sub child {
>   PeerAddr=> $self->{connectaddr},
>   PeerPort=> $self->{connectport},
>   SSL_verify_mode => SSL_VERIFY_NONE,
> + SSL_use_cert=> $self->{offertlscert} ? 1 : 0,
> + SSL_cert_file   => $self->{offertlscert} ?
> +"client.crt" : "",
> + SSL_key_file=> $self->{offertlscert} ?
> +"client.key" : "",
>   ) or die ref($self), " $iosocket socket connect failed: $!,$SSL_ERROR";
>   if ($self->{sndbuf}) {
>   setsockopt($cs, SOL_SOCKET, SO_SNDBUF,
> @@ -86,6 +91,14 @@ sub child {
>   print STDERR "ssl cipher: ",$cs->get_cipher(),"\n";
>   print STDERR "ssl peer certificate:\n",
>   $cs->dump_peer_certificate();
> +
> + if ($self->{offertlscert}) {
> + print STDERR "ssl client certificate:\n";
> + print STDERR "Subject Name: ",
> + "${\$cs->sock_certificate('subject')}\n";
> + print STDERR "Issuer  Name: ",
> + "${\$cs->sock_certificate('issuer')}\n";
> + }
>   }
>  
>   *STDIN = *STDOUT = $self->{cs} = $cs;
> diff --git a/regress/usr.sbin/relayd/Makefile 
> b/regress/usr.sbin/relayd/Makefile
> index cd01aa3fb63..f2198f43cc9 100644
> --- a/regress/usr.sbin/relayd/Makefile
> +++ b/regress/usr.sbin/relayd/Makefile
> @@ -96,7 +96,23 @@ server.req:
>  server.crt: ca.crt server.req
>   openssl x509 -CAcreateserial -CAkey ca.key -CA ca.crt -req -in 
> server.req -out server.crt
>  
> -${REGRESS_TARGETS:M*ssl*} ${REGRESS_TARGETS:M*https*}: server.crt
> +client-ca.crt:
> + openssl req -batch -new \
> + -subj 

Re: LibreSSL regressions

2021-02-16 Thread Theo Buehler
On Tue, Feb 16, 2021 at 01:16:21PM +0100, Jan Klemkow wrote:
> On Tue, Feb 16, 2021 at 04:36:59AM +1100, Joel Sing wrote:
> > On 21-02-15 14:49:46, Jan Klemkow wrote:
> > > On Sat, Feb 13, 2021 at 03:53:48PM +0100, Theo Buehler wrote:
> > > > On Sat, Feb 13, 2021 at 11:58:04AM +0100, Jan Klemkow wrote:
> > > > > A coworker of mine has made tests with LibreSSL [1] and found some
> > > > > regressions.  I took his test descriptions and created the following
> > > > > automated regression test.  In the repository he described his 
> > > > > findings
> > > > > in detail.  I kept the numbers of the files and subtests in the target
> > > > > names for now.  So, its easier to match it with his files.
> > > > > 
> > > > > I don't know how to handle the result of "test-01-ssl".  Thats why its
> > > > > just a comment.  Someone may have an idea to handle this properly.
> > > > > 
> > > > > Any comments, wishes or OK's?
> > > > > 
> > > > > [1]: https://github.com/noxxi/libressl-tests
> > > > 
> > > > First of all thanks for the effort!
> > > > 
> > > > The perl script and probably also the Makefile should have a license.
> > > > 
> > > > Please add a check that tests whether the required perl modules are
> > > > installed (p5-IO-Socket-SSL and p5-Net-SSLeay) and otherwise prints
> > > > SKIPPED and their names, so I can install them if they're not present.
> > > > I never remember their exact capitalization and hyphenation...
> > > > 
> > > > Various comments inline, and a patch for openssl(1) at the end that may
> > > > simplify some things.
> > > 
> > > This is an updated version of the test including comments and wishes
> > > from tb@ and bluhm@.
> > > 
> > > OK?
> > 
> > This currently drives openssl(1) for tests, which means that it is
> > testing openssl(1), libssl and libcrypto, when what you're really
> > wanting to test is libcrypto's verifier. While this works, the
> > problem is that a change or breakage in libssl or openssl(1) results
> > in a regress failure for libcrypto. If this is to land in its
> > current form it really should be in regress/usr.bin/openssl -
> > alternatively, it could be reworked to explicitly test libcrypto's
> > APIs and remain here.
> > 
> > Some additional comments inline.
> 
> So, the following diff should hit all needs.
> 
> OK?

Yes, it's ok.  Thanks.

I played a bit with it, and it is a bit more fragile than I would like,
but we may fix that in tree or redo some of it without using openssl(1)
later.  The ground covered is definitely worthwhile to have in regres,
so please go ahead.  (jsing has no objection).



Re: LibreSSL regressions

2021-02-15 Thread Theo Buehler
On Tue, Feb 16, 2021 at 04:36:59AM +1100, Joel Sing wrote:
> On 21-02-15 14:49:46, Jan Klemkow wrote:
> > On Sat, Feb 13, 2021 at 03:53:48PM +0100, Theo Buehler wrote:
> > > On Sat, Feb 13, 2021 at 11:58:04AM +0100, Jan Klemkow wrote:
> > > > A coworker of mine has made tests with LibreSSL [1] and found some
> > > > regressions.  I took his test descriptions and created the following
> > > > automated regression test.  In the repository he described his findings
> > > > in detail.  I kept the numbers of the files and subtests in the target
> > > > names for now.  So, its easier to match it with his files.
> > > > 
> > > > I don't know how to handle the result of "test-01-ssl".  Thats why its
> > > > just a comment.  Someone may have an idea to handle this properly.
> > > > 
> > > > Any comments, wishes or OK's?
> > > > 
> > > > [1]: https://github.com/noxxi/libressl-tests
> > > 
> > > First of all thanks for the effort!
> > > 
> > > The perl script and probably also the Makefile should have a license.
> > > 
> > > Please add a check that tests whether the required perl modules are
> > > installed (p5-IO-Socket-SSL and p5-Net-SSLeay) and otherwise prints
> > > SKIPPED and their names, so I can install them if they're not present.
> > > I never remember their exact capitalization and hyphenation...
> > > 
> > > Various comments inline, and a patch for openssl(1) at the end that may
> > > simplify some things.
> > 
> > This is an updated version of the test including comments and wishes
> > from tb@ and bluhm@.
> > 
> > OK?
> 
> This currently drives openssl(1) for tests, which means that it is
> testing openssl(1), libssl and libcrypto, when what you're really
> wanting to test is libcrypto's verifier. While this works, the
> problem is that a change or breakage in libssl or openssl(1) results
> in a regress failure for libcrypto. If this is to land in its
> current form it really should be in regress/usr.bin/openssl -
> alternatively, it could be reworked to explicitly test libcrypto's
> APIs and remain here.

Except for the auto chain thing...

I'm ok with it going into regress/usr.bin/openssl/x509/ in essentially
its present form (with jsings and my nits addressed):

mv regress/lib/libcrypto/validate regress/usr.bin/openssl/x509

then change the SUBDIR line in the regress/usr.bin/openssl/Makefile:

-SUBDIR= options
+SUBDIR= options x509

> Some additional comments inline.

I also have a few more.

> 
> > Index: regress/lib/libcrypto/validate/Makefile
> > ===
> > RCS file: regress/lib/libcrypto/validate/Makefile
> > diff -N regress/lib/libcrypto/validate/Makefile
> > --- /dev/null   1 Jan 1970 00:00:00 -
> > +++ regress/lib/libcrypto/validate/Makefile 15 Feb 2021 13:38:22 -
> > @@ -0,0 +1,133 @@
> > +# $OpenBSD$
> > +
> > +# Copyright (c) 2021 Jan Klemkow 
> > +#
> > +# Permission to use, copy, modify, and distribute this software for any
> > +# purpose with or without fee is hereby granted, provided that the above
> > +# copyright notice and this permission notice appear in all copies.
> > +#
> > +# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> > +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> > +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> > +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> > +# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> > +# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> > +# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > +
> > +# This regression test is based on manual test descriptions from:
> > +# https://github.com/noxxi/libressl-tests
> > +
> > +# The following port must be installed for the regression tests:
> > +# p5-IO-Socket-SSL perl interface to SSL sockets
> > +
> > +PERL = perl
> > +OPENSSL ?= openssl
> > +
> > +PERL_REQUIRE !=perl -Mstrict -Mwarnings -e ' \
> > +eval { require IO::Socket::SSL } or print $@; \
> > +'
> > +.if ! empty (PERL_REQUIRE)
> > +regress:
> > +   @echo "${PERL_REQUIRE}"
> > +   @echo install these perl packages for additional tests
> > +   @echo SKIPPED
> > +.endif
> > +
> > +REGRESS_TARGETS += test-unusual-wildcard-cert-no-CA-client
> > +REGRESS_TA

Re: change rpki-client repository code

2021-02-15 Thread Theo Buehler
> > >   rt.repos = reallocarray(rt.repos,
> > >   rt.reposz + 1, sizeof(struct repo));
> > 
> > This line could be unwrapped. The code could also be simplified by using
> > recallocarray() (it looks like the -portable update.sh is prepared for
> > that).
> 
> I leave that for later. There are a bunch of reallocarrays around the code
> that are used in the same way. Let's change them all.

Sure.

> > > + /* Look up in repository table. (Lookup should actually fail here) */
> > > + for (i = 0; i < rt.reposz; i++) {
> > > + if (rt.repos[i].repouri != NULL ||
> > > + strcmp(rt.repos[i].local, local))
> > 
> > While I think this is correct, it looks odd. Couldn't this check for
> > rt.repos[i].local != NULL?
> 
> No. The rt.repos[i].repouri check is there to skip repo's added by
> repo_lookup. In repo_lookup the same check is there but the other way
> around. local on the other hand can't be NULL, it will always be set.

Ugh, of course. I misread or misunderstood. Probably both.

> I think the repouri check could be removed since there is no way that have
> conflicting local repo paths. I will just remove the check.

Yes, this makes sense. It is confusing.



Re: change rpki-client repository code

2021-02-15 Thread Theo Buehler
On Fri, Feb 12, 2021 at 10:01:38AM +0100, Claudio Jeker wrote:
> On Mon, Feb 08, 2021 at 05:15:40PM +0100, Claudio Jeker wrote:
> > Split the repository code into two parts:
> > 
> > - fetch of the trust anchors (the certs referenced by TAL files)
> > - fetch of the MFT files of a repository
> > 
> > While the two things kind of look similar there are some differences.
> > 
> > - TA files are loaded via rsync or https URI (only one file needs to be
> >   loaded)
> > - MFT files need everything inside the repository to be loaded since they
> >   reference to other files (.roa, .cer, .crl). These repositories are
> >   synced once with rsync and many mft may be part of a repo. Also these
> >   repositories can be synced via rsync or RRDP
> > 
> > To simplify these diverse options it is time to split the code up.
> > Introduce a ta_lookup() along with repo_lookup(). Refactor the repo_lookup
> > code into subfunctions repo_alloc() and repo_fetch() (both are also used
> > by ta_lookup()). Use the caRepository URI to figure out the base URI.
> > Simplify rsync_uri_parse() into rsync_base_uri() which clips of excess
> > directories from the URI (else thousends of individual rsync calls would
> > be made against the RIR's CA repos).
> > 
> > The big change is that the layout of the cache directory is changed.
> > The cache will now have two base directories:
> > - ta/ (for all trust anchors)
> > - rsync/ (for all other repositories)
> > 
> 
> My plan at the moment is that rpki-client will split the cache directory
> into three parts. ta/, rsync/, and rrdp/. This is done to ensure that data
> does not get mixed up. Once this is in then my next step is to support
> https:// links in TAL files and fetch the trust anchor via https instead
> of rsync. Later RRDP will follow.

This looks good to me although I have a couple of small nits inline.

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.42
> diff -u -p -r1.42 extern.h
> --- extern.h  8 Feb 2021 09:22:53 -   1.42
> +++ extern.h  8 Feb 2021 13:44:22 -
> @@ -392,9 +392,7 @@ void   proc_parser(int) __attribute__((n
>  
>  /* Rsync-specific. */
>  
> -int   rsync_uri_parse(const char **, size_t *,
> - const char **, size_t *, const char **, size_t *,
> - enum rtype *, const char *);
> +char *rsync_base_uri(const char *);
>  void  proc_rsync(char *, char *, int) __attribute__((noreturn));
>  
>  /* Logging (though really used for OpenSSL errors). */
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 main.c
> --- main.c5 Feb 2021 12:26:52 -   1.98
> +++ main.c8 Feb 2021 13:50:20 -
> @@ -78,11 +78,12 @@
>   * An rsync repository.
>   */
>  struct   repo {
> - char*repo;  /* repository rsync URI */
> - char*local; /* local path name */
> - char*notify; /* RRDB notify URI if available */
> - size_t   id; /* identifier (array index) */
> - int  loaded; /* whether loaded or not */
> + char*repouri;   /* CA repository base URI */
> + char*local; /* local path name */
> + char*uris[2];   /* URIs to fetch from */
> + size_t   id;/* identifier (array index) */
> + int  uriidx;/* which URI is fetched */
> + int  loaded;/* whether loaded or not */
>  };
>  
>  size_t   entity_queue;
> @@ -284,33 +285,12 @@ entityq_add(struct entityq *q, char *fil
>  }
>  
>  /*
> - * Look up a repository, queueing it for discovery if not found.
> + * Allocat a new repository be extending the repotable.
>   */
> -static const struct repo *
> -repo_lookup(const char *uri)
> +static struct repo *
> +repo_alloc(void)
>  {
> - const char  *host, *mod;
> - size_t   hostsz, modsz, i;
> - char*local;
> - struct repo *rp;
> - struct ibuf *b;
> -
> - if (!rsync_uri_parse(, ,
> - , , NULL, NULL, NULL, uri))
> - errx(1, "%s: malformed", uri);
> -
> - if (asprintf(, "%.*s/%.*s", (int)hostsz, host,
> - (int)modsz, mod) == -1)
> - err(1, "asprintf");
> -
> - /* Look up in repository table. */
> -
> - for (i = 0; i < rt.reposz; i++) {
> - if (strcmp(rt.repos[i].local, local))
> - continue;
> - free(local);
> - return [i];
> - }
> + struct repo *rp;
>  
>   rt.repos = reallocarray(rt.repos,
>   rt.reposz + 1, sizeof(struct repo));

This line could be unwrapped. The code could also be simplified by using
recallocarray() (it looks 

Re: httpd(8) fix tls comparison of servers

2021-02-15 Thread Theo Buehler
On Mon, Feb 15, 2021 at 12:41:31PM +0100, Claudio Jeker wrote:
> For SNI all TLS servers need to run with the same config. The config
> parser has an extra step for this. The problem is it also compares the
> TLS config params with non-TLS servers when a server block has both
> listen * port 80 and listen * tls port 443.
> 
> The following diff fixes that and also removes the unused last argument of
> server_tls_cmp().

I don't know why the match_keypair option was added, but it's been dead
since it landed.

ok tb

> -- 
> :wq Claudio
> 
> Index: httpd.h
> ===
> RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
> retrieving revision 1.154
> diff -u -p -r1.154 httpd.h
> --- httpd.h   27 Jan 2021 07:21:52 -  1.154
> +++ httpd.h   13 Feb 2021 08:32:34 -
> @@ -622,7 +622,7 @@ intcmdline_symset(char *);
>  
>  /* server.c */
>  void  server(struct privsep *, struct privsep_proc *);
> -int   server_tls_cmp(struct server *, struct server *, int);
> +int   server_tls_cmp(struct server *, struct server *);
>  int   server_tls_load_ca(struct server *);
>  int   server_tls_load_crl(struct server *);
>  int   server_tls_load_keypair(struct server *);
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/httpd/parse.y,v
> retrieving revision 1.124
> diff -u -p -r1.124 parse.y
> --- parse.y   22 Jan 2021 13:07:17 -  1.124
> +++ parse.y   13 Feb 2021 09:02:18 -
> @@ -333,7 +333,8 @@ server: SERVER optmatch STRING{
>   free(srv);
>   YYERROR;
>   }
> - if (server_tls_cmp(s, srv, 0) != 0) {
> + if (srv->srv_conf.flags & SRVFLAG_TLS &&
> + server_tls_cmp(s, srv) != 0) {
>   yyerror("server \"%s\": tls "
>   "configuration mismatch on same "
>   "address/port",
> Index: server.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/server.c,v
> retrieving revision 1.124
> diff -u -p -r1.124 server.c
> --- server.c  2 Jan 2021 18:35:07 -   1.124
> +++ server.c  15 Feb 2021 11:38:08 -
> @@ -127,7 +127,7 @@ server_privinit(struct server *srv)
>  }
>  
>  int
> -server_tls_cmp(struct server *s1, struct server *s2, int match_keypair)
> +server_tls_cmp(struct server *s1, struct server *s2)
>  {
>   struct server_config*sc1, *sc2;
>  
> @@ -146,13 +146,6 @@ server_tls_cmp(struct server *s1, struct
>   return (-1);
>   if (strcmp(sc1->tls_ecdhe_curves, sc2->tls_ecdhe_curves) != 0)
>   return (-1);
> -
> - if (match_keypair) {
> - if (strcmp(sc1->tls_cert_file, sc2->tls_cert_file) != 0)
> - return (-1);
> - if (strcmp(sc1->tls_key_file, sc2->tls_key_file) != 0)
> - return (-1);
> - }
>  
>   return (0);
>  }
> 



Re: LibreSSL regressions

2021-02-13 Thread Theo Buehler
On Sat, Feb 13, 2021 at 11:58:04AM +0100, Jan Klemkow wrote:
> Hi,
> 
> A coworker of mine has made tests with LibreSSL [1] and found some
> regressions.  I took his test descriptions and created the following
> automated regression test.  In the repository he described his findings
> in detail.  I kept the numbers of the files and subtests in the target
> names for now.  So, its easier to match it with his files.
> 
> I don't know how to handle the result of "test-01-ssl".  Thats why its
> just a comment.  Someone may have an idea to handle this properly.
> 
> Any comments, wishes or OK's?

First of all thanks for the effort!

The perl script and probably also the Makefile should have a license.

Please add a check that tests whether the required perl modules are
installed (p5-IO-Socket-SSL and p5-Net-SSLeay) and otherwise prints
SKIPPED and their names, so I can install them if they're not present.
I never remember their exact capitalization and hyphenation...


Various comments inline, and a patch for openssl(1) at the end that may
simplify some things.

> [1]: https://github.com/noxxi/libressl-tests
> 
> Index: regress/lib/libssl/Makefile
> ===
> RCS file: /cvs/src/regress/lib/libssl/Makefile,v
> retrieving revision 1.42
> diff -u -p -r1.42 Makefile
> --- regress/lib/libssl/Makefile   14 Oct 2020 15:53:22 -  1.42
> +++ regress/lib/libssl/Makefile   12 Feb 2021 19:42:56 -
> @@ -16,6 +16,7 @@ SUBDIR += tlsext
>  SUBDIR += tlslegacy
>  SUBDIR += key_schedule
>  SUBDIR += unit
> +SUBDIR += validate

I'm not too keen on hooking up a test that fails. It breaks my workflow.
I use REGRESS_FAIL_EARLY in my environment to spot unexpected failures.

Could you add the currently failing tests to REGRESS_EXPECTED_FAILURES?

I also don't think it's a libssl regress test. It's either for
lib/libcrypto or usr.bin/openssl, probably the latter.

>  # Things that take a long time should go below here. 
>  SUBDIR += tlsfuzzer
> Index: regress/lib/libssl/validate/Makefile
> ===
> RCS file: regress/lib/libssl/validate/Makefile
> diff -N regress/lib/libssl/validate/Makefile
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ regress/lib/libssl/validate/Makefile  13 Feb 2021 10:50:30 -
> @@ -0,0 +1,104 @@
> +# Tests from: https://github.com/noxxi/libressl-tests
> +
> +PERL=perl

Please use consistent spacing.

PERL = perl

Could you add

OPENSSL ?= openssl

and use ${OPENSSL} instead of plain openssl throughout the Makefile?
This way we can compare with OpenSSL by doing 'make OPENSSL=eopenssl11'.

> +
> +REGRESS_TARGETS =test-00-01-ssl
> +REGRESS_TARGETS +=   test-00-02-ssl
> +REGRESS_TARGETS +=   test-00-03-ssl
> +REGRESS_TARGETS +=   test-00-04-ssl
> +REGRESS_TARGETS +=   test-00-05-ssl
> +REGRESS_TARGETS +=   test-00-06-ssl
> +REGRESS_TARGETS +=   test-01-ssl
> +REGRESS_TARGETS +=   test-02-ssl
> +REGRESS_ROOT_TARGETS =   ${REGRESS_TARGETS}

Please drop this line. These tests do not require root.

> +REGRESS_CLEANUP =cleanup-ssl
> +REGRESS_SETUP =  create-libressl-test-certs

I think this should be REGRESS_SETUP_ONCE. No need to regen the certs
for each and every test.

> +
> +create-libressl-test-certs: create-libressl-test-certs.pl
> + ${PERL} ${.CURDIR}/$@.pl
> +
> +cleanup-ssl:
> + pkill openssl || true
> + rm *.pem *.key
> +
> +test-00-01-ssl:
> + # unusual wildcard cert, no CA given to client
> + # cleanup
> + pkill openssl || true
> + sleep 2

pkill + sleep 2 is a bit icky. It's not the first time I wanted to have
OpenSSL's -naccept option available for testing. See patch below.

> + # start client

# start server

> + ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \
> + -key server-unusual-wildcard.pem -www & \

I'd suggest replacing '-www' with '-naccept 1' and...

> + timeout=$$(($$(date +%s) + 5)); \
> + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \
> + do test $$(date +%s) -lt $$timeout || exit 1; done
> + # start client
> + echo "data" | openssl s_client -verify_return_error -connect 
> 127.0.0.1:4433 \

... do 'echo "Q"' here.

Unless you want to use a random port (which may be more appropriate than
killing all openssl processes and assuming that 4433 is free), I would
just drop '-connect 127.0.0.1:4433'.

> + | grep "Verify return code: 21"
> +
> +test-00-02-ssl:
> + # unusual wildcard cert, CA given to client
> + # cleanup
> + pkill openssl || true
> + sleep 2
> + # start server
> + ${KTRACE} openssl s_server -cert server-unusual-wildcard.pem \
> + -key server-unusual-wildcard.pem -www & \
> + timeout=$$(($$(date +%s) + 5)); \
> + while fstat -p $$! | ! grep -q 'tcp .* \*:4433$$'; \
> + do test $$(date +%s) -lt $$timeout || exit 1; done
> + # start client
> + 

Re: ssl(8) tweaks, mostly for ecdsa

2021-02-11 Thread Theo Buehler
On Thu, Feb 11, 2021 at 08:55:55PM +, Stuart Henderson wrote:
> acme-client works with ecdsa certificates, but if there's no existing
> key, it has no way to tell whether you want ec or rsa so it can't
> actually generate a new ec key. (even if it did, acme-client's default
> secp384r1 isn't accepted by buypass).
> 
> here are a few changes for ssl(8) that i think are helpful.
> it uses the single command that generates params and a key together,
> which is the only step needed if you're using it with acme-client,
> and then generates a csr separately (as is already done for rsa).
> 
> i've included some small changes for rsa as well (not everyone wants
> such a long key as acme-client uses by default, especially if they
> are handling high connection rates).
> 
> any comments?

This makes sense to me. I like the streamlining of the ECDSA case except
for one detail: the eccert.key is no longer saved into /etc/ssl/private
(probably to avoid line wraps). It is assumed to be there in the last
command before SEE ALSO.  The intent may be obvious to a user who has
read the RSA section but it may lead to errors for those who didn't.

I don't remember the rules for SEE ALSO. Should acme-client have an Xr
there?



Re: rpki-client parse and check caRepository Subject Information Access

2021-02-05 Thread Theo Buehler
On Fri, Feb 05, 2021 at 02:45:41PM +0100, Claudio Jeker wrote:
> RPKI certificates have 3 possible Subject Information Access URI that we
> may be interested in:
> - 1.3.6.1.5.5.7.48.5 (caRepository)
> - 1.3.6.1.5.5.7.48.10 (rpkiManifest)
> - 1.3.6.1.5.5.7.48.13 (rpkiNotify)
> 
> rpkiManifest points to the .mft file inside the caRepository.
> Because of this caRepository is the base URI for all the files below
> this certificate. rpkiNotify points to an RRDP endpoint where the XML
> data also contains URI that again need to match the caRepository. If not
> something strange is going on.
> 
> Since the caRepository data is useful extract it from the cert and also
> do a simple strstr() check to ensure that rpkiManifest starts with
> caRepository.
> 
> Currently the data is not used further than that but I want to add it to
> the repository information as a next step.

ok tb



Re: rpki-client remove debug code

2021-02-05 Thread Theo Buehler
On Thu, Feb 04, 2021 at 06:56:05PM +0100, Claudio Jeker wrote:
> This bit of debug code to understand the progress of rpki-client is no
> longer helpful. Most of the time this is a stuck rsync that causes delays
> and those are now nicely handled by an internal timeout.
> I propose to remove this.

If no longer find it useful, who will? :)

ok tb



Re: rpki-client call a file a file

2021-02-04 Thread Theo Buehler
On Thu, Feb 04, 2021 at 03:09:33PM +0100, Claudio Jeker wrote:
> The uri field in the entity queue struct is never a URI but always a local
> path to the file in the repo. Rename the field so I'm less confused.
> Compiler agrees with my change.

Agreed. Some of the functions you need to touch also expect a filename
fn, which they then open one way or the other.

ok tb



Re: rpki-client, simplify main process

2021-02-04 Thread Theo Buehler
On Thu, Feb 04, 2021 at 11:37:08AM +0100, Claudio Jeker wrote:
> Instead of passing around variables all the way down to entity_write_req()
> and repo_lookup() use global variables. Especially for the repository
> handling this will become more complex with the introduction of RRDP.
> Also shuffle code around a bit so that all entity queue functions are
> together.
> 
> OK?

ok tb

One very important comment below

> @@ -114,10 +114,9 @@ filepathcmp(struct filepath *a, struct f
>  
>  RB_HEAD(filepath_tree, filepath);
>  RB_PROTOTYPE(filepath_tree, filepath, entry, filepathcmp);
> -struct filepath_tree  fpt = RB_INITIALIZER();
>  
> -static void  entityq_flush(struct msgbuf *, struct entityq *,
> - const struct repo *);
> +static struct filepath_tree  fpt = RB_INITIALIZER();
> +static struct msgbuf procq, rsyncq;

The above line has "tab space tab" between type and variable name.



Re: rpki-client factor out the parser code into own module

2021-02-03 Thread Theo Buehler
On Wed, Feb 03, 2021 at 08:08:20PM +0100, Claudio Jeker wrote:
> This is just shuffling code around and adds a few definitions to extern.h.
> The goal is to reduce the amount of code in main.c. I constantly get lost
> in all the parsing and parent functions also I want to extend the
> repository code and so this makes space for that.
> 
> Compiles and works for me :)

ok tb

I think you can garbage collect all openssl includes in main.c.
 and  also seem unused.

In parser.c I would add openssl/asn1.h (e.g. for the sk_ business) and
use x509.h instead of x509v3.h.



Re: rpki-client remove double checking of hashes

2021-01-28 Thread Theo Buehler
On Thu, Jan 28, 2021 at 04:42:00PM +0100, Claudio Jeker wrote:
> Initially rpki-client checked the file hash while parsing the file (.roa,
> .cert or .crl) but since a while rpki-client does the hash check early
> during the .mft parsing with mft_check(). After that all files in the
> fileandhash attribute are verified and so there is no need to do it again.
> 
> All in all this simplifies the code a fair bit. The only problematic case
> was the distinction between root cert and regular cert based on the
> presence of the digest. Instead use the presence of the public key (from
> the TAL). Result is the same, logic is inverse.
> 
> So this still works for me.

Makes sense, ok tb

Please add the diff below to adjust regress when you land this.

Index: test-cert.c
===
RCS file: /cvs/src/regress/usr.sbin/rpki-client/test-cert.c,v
retrieving revision 1.6
diff -u -p -r1.6 test-cert.c
--- test-cert.c 9 Dec 2020 11:22:47 -   1.6
+++ test-cert.c 28 Jan 2021 16:14:30 -
@@ -145,7 +145,7 @@ main(int argc, char *argv[])
}
} else {
for (i = 0; i < argc; i++) {
-   p = cert_parse(, argv[i], NULL);
+   p = cert_parse(, argv[i]);
if (p == NULL)
break;
if (verb)
Index: test-roa.c
===
RCS file: /cvs/src/regress/usr.sbin/rpki-client/test-roa.c,v
retrieving revision 1.7
diff -u -p -r1.7 test-roa.c
--- test-roa.c  9 Nov 2020 16:13:02 -   1.7
+++ test-roa.c  28 Jan 2021 16:14:44 -
@@ -87,7 +87,7 @@ main(int argc, char *argv[])
errx(1, "argument missing");
 
for (i = 0; i < argc; i++) {
-   if ((p = roa_parse(, argv[i], NULL)) == NULL)
+   if ((p = roa_parse(, argv[i])) == NULL)
break;
if (verb)
roa_print(p);



Re: unwind: silence "udp connect failed" errors

2021-01-24 Thread Theo Buehler
On Sun, Jan 24, 2021 at 12:44:39PM +0100, Klemens Nanni wrote:
> unwind/libunbound always tries to connect to nameservers using both
> address families, even if only one is configured on the local machine.
> 
> So on IPv6 only boxes for example syslog gets spammed with these
> 
> Jan 24 12:23:06 eru unwind[38261]: [38261:0] error: udp connect failed: Can't 
> assign requested address for 195.54.164.36 port 53
> 
>   grep -c 'unwind.*udp connect failed' /var/log/daemon
>   1278
> 
> Diff below makes unwind not log this iff connect(2) yielded
> EADDRNOTAVAIL.
> 
> This diff wouldn't make sense for libunbound upstream or say a router
> running unbound where addresses are configured statically and admins do
> want to see such connect failures.
> 
> But with unwind on roaming clients I don't see much value in logging it,
> especially not if unwind eventually answers the query successfully.
> 
> Feedback? Objections? OK?

Probably better to sync first with the corresponding unbound commit
https://cvsweb.openbsd.org/src/usr.sbin/unbound/services/outside_network.c#rev1.21
then adjust udp_connect_needs_log() as needed.

> 
> 
> Index: libunbound/services/outside_network.c
> ===
> RCS file: /cvs/src/sbin/unwind/libunbound/services/outside_network.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 outside_network.c
> --- libunbound/services/outside_network.c 11 Dec 2020 12:21:40 -  
> 1.9
> +++ libunbound/services/outside_network.c 24 Jan 2021 11:27:04 -
> @@ -1803,7 +1803,8 @@ select_ifport(struct outside_network* ou
>   if(outnet->udp_connect) {
>   /* connect() to the destination */
>   if(connect(fd, (struct sockaddr*)>addr,
> - pend->addrlen) < 0) {
> + pend->addrlen) < 0 &&
> + errno != EADDRNOTAVAIL) {
>   log_err_addr("udp connect failed",
>   strerror(errno), >addr,
>   pend->addrlen);
> 



Re: games/canfield bug

2021-01-21 Thread Theo Buehler
> We should just ignore any of the special curses keys returned by
> getch() since canfield is not prepared to deal with them.

ok tb



Re: rad(8): get rid of inet_net_{ntop,pton}(3)

2021-01-19 Thread Theo Buehler
On Mon, Jan 18, 2021 at 06:41:26PM +0100, Florian Obser wrote:
> 
> 
> This is not an api that seems to have caught on (especially the
> AF_INET6 variant), maybe we can get rid of it entirely.
> 
> (I also suspect that the AF_INET6 version is broken on FreeBSD and
> NetBSD as well as mac osx.)
> 
> OK?

ok tb

> 
> diff --git parse.y parse.y
> index 8f32f11250d..36e5af8be76 100644
> --- parse.y
> +++ parse.y
> @@ -260,17 +260,28 @@ ra_ifaceoptsl   : NO AUTO PREFIX {
>   | PREFIX STRING {
>   struct in6_addr  addr;
>   int  prefixlen;
> + char*p;
> + const char  *errstr;
>  
>   memset(, 0, sizeof(addr));
> - prefixlen = inet_net_pton(AF_INET6, $2, ,
> - sizeof(addr));
> - if (prefixlen == -1) {
> - yyerror("error parsing prefix");
> + p = strchr($2, '/');
> + if (p != NULL) {
> + *p++ = '\0';
> + prefixlen = strtonum(p, 0, 128, );
> + if (errstr != NULL) {
> + yyerror("error parsing prefix "
> + "\"%s/%s\"", $2, p);
> + free($2);
> + YYERROR;
> + }
> + } else
> + prefixlen = 64;
> + if(inet_pton(AF_INET6, $2, ) == 0) {
> + yyerror("error parsing prefix \"%s/%d\"", $2,
> + prefixlen);
>   free($2);
>   YYERROR;
>   }
> - if (prefixlen == 128 && strchr($2, '/') == NULL)
> - prefixlen = 64;
>   mask_prefix(, prefixlen);
>   ra_prefix_conf = conf_get_ra_prefix(, prefixlen);
>   } ra_prefix_block {
> diff --git printconf.c printconf.c
> index d42890da518..feabd83de94 100644
> --- printconf.c
> +++ printconf.c
> @@ -102,7 +102,7 @@ print_config(struct rad_conf *conf)
>  {
>   struct ra_iface_conf*iface;
>   struct ra_prefix_conf   *prefix;
> - char buf[INET6_ADDRSTRLEN], *bufp;
> + char buf[INET6_ADDRSTRLEN];
>  
>   print_ra_options("", >ra_options);
>   printf("\n");
> @@ -120,9 +120,9 @@ print_config(struct rad_conf *conf)
>   printf("\tno auto prefix\n");
>  
>   SIMPLEQ_FOREACH(prefix, >ra_prefix_list, entry) {
> - bufp = inet_net_ntop(AF_INET6, >prefix,
> - prefix->prefixlen, buf, sizeof(buf));
> - printf("\tprefix %s {\n", bufp);
> + printf("\tprefix %s/%d {\n", inet_ntop(AF_INET6,
> + >prefix, buf, sizeof(buf)),
> + prefix->prefixlen);
>   print_prefix_options("\t\t", prefix);
>   printf("\t}\n");
>   }
> 
> 
> -- 
> I'm not entirely sure you are real.
> 



Re: behaviour of openssl s_server and certificate chains on 6.8

2021-01-14 Thread Theo Buehler
On Thu, Jan 14, 2021 at 02:37:20PM +0100, Robert Nagy wrote:
> On 14/01/21 14:27 +0100, Robert Nagy wrote:
> > On 14/01/21 14:20 +0100, Theo Buehler wrote:
> > > On Thu, Jan 14, 2021 at 01:32:41PM +0100, Matthieu Herrb wrote:
> > > > Hi,
> > > > 
> > > > I'm trying to debug strange beahaviour changes with certificates on a
> > > > systemc after upgrading it from 6.7 to 6.8...
> > > > 
> > > > On 6.7, If I run :
> > > > 
> > > > openssl s_server -cert mycert.pem -key mykey.pem -CAfile CA.pem
> > > > 
> > > > then openssl s_client -showcerts -connect localhost:4433
> > > > 
> > > > returns the full certificate chain mycert->CA
> > > > 
> > > > With the same commands on 6.8, I don't get the CA certificate.
> > > > 
> > > > Is this a known issue, and how can I get the chain with 6.8 ?
> > > > 
> > > > (my real application is sendmail...)
> > > 
> > > In short: Yes, this is known. You can't get the chain in 6.8.
> > > 
> > > This is the reason why ajacoutot switched sendmail to link against
> > > eopenssl11 as a workaround in -stable. As your thread on ports shows,
> > > this workaround doesn't work if you add something that links against
> > > LibreSSL to the mix.
> > > 
> > > There are several layers of unexpected things/bugs involved. The two
> > > main points are:
> > > 
> > > 1. The TLSv1.3 server in 6.8 does not do auto chain since we hoped to
> > >be able to avoid it. This was addressed post release when people
> > >using OpenLDAP ran into it.
> > >https://cvsweb.openbsd.org/src/lib/libssl/tls13_server.c#rev1.62
> > > 
> > > 2. The new verifier doesn't behave as it should when auto chain is
> > >enabled. As a workaround -current switches to the legacy verifier in
> > >this situation for about a week now. The proper fix in the new
> > >verifier is under discussion.
> > >https://cvsweb.openbsd.org/src/lib/libssl/tls13_server.c#rev1.65
> > > 
> > > I don't know whether/when there will be backports of some fixes to 6.8.
> > > As sthen said in the thread on ports, right now the simplest fix is to
> > > run -current.
> > > 
> > 
> > i think this should be an errata for 6.8

Not exactly sure what you mean by "this," but I emphatically agree,
errata are long overdue.  It would be desirable to avoid an outright
switch back to the legacy verifier if that's what you mean by the below.

> I am also getting this problem with bacula using TLS < 1.3, reported to Bob
> already, and I am currently running with the legacy verifier.

FWIW this is a different issue that was also adressed in -current.
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libcrypto/x509/x509_verify.c#rev1.26



Re: behaviour of openssl s_server and certificate chains on 6.8

2021-01-14 Thread Theo Buehler
On Thu, Jan 14, 2021 at 01:32:41PM +0100, Matthieu Herrb wrote:
> Hi,
> 
> I'm trying to debug strange beahaviour changes with certificates on a
> systemc after upgrading it from 6.7 to 6.8...
> 
> On 6.7, If I run :
> 
> openssl s_server -cert mycert.pem -key mykey.pem -CAfile CA.pem
> 
> then openssl s_client -showcerts -connect localhost:4433
> 
> returns the full certificate chain mycert->CA
> 
> With the same commands on 6.8, I don't get the CA certificate.
> 
> Is this a known issue, and how can I get the chain with 6.8 ?
> 
> (my real application is sendmail...)

In short: Yes, this is known. You can't get the chain in 6.8.

This is the reason why ajacoutot switched sendmail to link against
eopenssl11 as a workaround in -stable. As your thread on ports shows,
this workaround doesn't work if you add something that links against
LibreSSL to the mix.

There are several layers of unexpected things/bugs involved. The two
main points are:

1. The TLSv1.3 server in 6.8 does not do auto chain since we hoped to
   be able to avoid it. This was addressed post release when people
   using OpenLDAP ran into it.
   https://cvsweb.openbsd.org/src/lib/libssl/tls13_server.c#rev1.62

2. The new verifier doesn't behave as it should when auto chain is
   enabled. As a workaround -current switches to the legacy verifier in
   this situation for about a week now. The proper fix in the new
   verifier is under discussion.
   https://cvsweb.openbsd.org/src/lib/libssl/tls13_server.c#rev1.65

I don't know whether/when there will be backports of some fixes to 6.8.
As sthen said in the thread on ports, right now the simplest fix is to
run -current.



Re: cmp -s bugfix

2021-01-09 Thread Theo Buehler
On Sat, Jan 09, 2021 at 08:00:42AM +0100, Otto Moerbeek wrote:
> As reported on misc@
> 
> https://marc.info/?l=openbsd-misc=161016188503894=2

ok tb

> 
>   -Otto
> 
> Index: regular.c
> ===
> RCS file: /cvs/src/usr.bin/cmp/regular.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 regular.c
> --- regular.c 6 Feb 2015 23:21:59 -   1.12
> +++ regular.c 9 Jan 2021 06:53:20 -
> @@ -51,15 +51,15 @@ c_regular(int fd1, char *file1, off_t sk
>   off_t byte, length, line;
>   int dfound;
>  
> - if (sflag && len1 != len2)
> - exit(1);
> -
>   if (skip1 > len1)
>   eofmsg(file1);
>   len1 -= skip1;
>   if (skip2 > len2)
>   eofmsg(file2);
>   len2 -= skip2;
> +
> + if (sflag && len1 != len2)
> + exit(1);
>  
>   length = MINIMUM(len1, len2);
>   if (length > SIZE_MAX) {
> 



Re: Fix -Wincompatible-pointer-types-discards-qualifiers

2021-01-08 Thread Theo Buehler
On Thu, Jan 07, 2021 at 11:30:43PM +, Adam Barth wrote:
> Thanks so much!  This is my first patch for OpenBSD, and I don't quite have
> the workflow debugged yet.

Committed, thank you!

Probably easiest and safest way is to use git format-patch and to send
the patch file as an attachment. This is the least likely to result in
mangled whitespace. Some purists don't like it that much, but I think it
will be preferred over a diff that doesn't apply...



Re: Fix -Wincompatible-pointer-types-discards-qualifiers

2021-01-07 Thread Theo Buehler
On Thu, Jan 07, 2021 at 11:16:16PM +, Adam Barth wrote:
> Previously, this code was passing string constants to functions that did
> not declare their parameters as const. After this patch, the functions now
> declare that they do not modify these arguments, making it safe to pass
> string constants.

Thanks. Unfortunately the patch was mangled by your MUA (line wrapping
and expanded tabs).

Below is a version that applies.

ok tb

Index: lib/libc/gen/fts.c
===
RCS file: /cvs/src/lib/libc/gen/fts.c,v
retrieving revision 1.59
diff -u -p -r1.59 fts.c
--- lib/libc/gen/fts.c  28 Jun 2019 13:32:41 -  1.59
+++ lib/libc/gen/fts.c  7 Jan 2021 23:23:27 -
@@ -43,7 +43,7 @@
 
 #define MAXIMUM(a, b)  (((a) > (b)) ? (a) : (b))
 
-static FTSENT  *fts_alloc(FTS *, char *, size_t);
+static FTSENT  *fts_alloc(FTS *, const char *, size_t);
 static FTSENT  *fts_build(FTS *, int);
 static void fts_lfree(FTSENT *);
 static void fts_load(FTS *, FTSENT *);
@@ -52,7 +52,7 @@ static voidfts_padjust(FTS *, FTSENT *
 static int  fts_palloc(FTS *, size_t);
 static FTSENT  *fts_sort(FTS *, FTSENT *, int);
 static u_short  fts_stat(FTS *, FTSENT *, int, int);
-static int  fts_safe_changedir(FTS *, FTSENT *, int, char *);
+static int  fts_safe_changedir(FTS *, FTSENT *, int, const char *);
 
 #defineISDOT(a)(a[0] == '.' && (!a[1] || (a[1] == '.' && 
!a[2])))
 
@@ -901,7 +901,7 @@ fts_sort(FTS *sp, FTSENT *head, int nite
 }
 
 static FTSENT *
-fts_alloc(FTS *sp, char *name, size_t namelen)
+fts_alloc(FTS *sp, const char *name, size_t namelen)
 {
FTSENT *p;
size_t len;
@@ -1020,7 +1020,7 @@ fts_maxarglen(char * const *argv)
  * Assumes p->fts_dev and p->fts_ino are filled in.
  */
 static int
-fts_safe_changedir(FTS *sp, FTSENT *p, int fd, char *path)
+fts_safe_changedir(FTS *sp, FTSENT *p, int fd, const char *path)
 {
int ret, oerrno, newfd;
struct stat sb;



Re: use getnameinfo in bgpd to print addresses

2021-01-04 Thread Theo Buehler
On Mon, Jan 04, 2021 at 08:48:55PM +0100, Otto Moerbeek wrote:
> On Mon, Jan 04, 2021 at 05:50:53PM +0100, Otto Moerbeek wrote:
> 
> > tOn Mon, Jan 04, 2021 at 01:42:48PM +0100, Theo Buehler wrote:
> > 
> > > > > + return log_sockaddr(addr2sa(addr, 0, ), len);
> > > > 
> > > > Perhaps I haven't yet had enough coffee this year, but I'm unsure
> > > > whether it is actually guaranteed that addr2sa() is called before the
> > > > second len in this line is passed to log_sockaddr().
> > > 
> > > Answering my own question: C99 and C11 6.5.2.2.12 require that all side
> > > effects must be completed before log_sockaddr() is called. As addr2sa()
> > > changes the second len as a side effect, this should be fine.
> > > 
> > > ok tb
> > > 
> > 
> > I am not convinced. Consider
> > 
> > #include 
> > 
> > char x;
> > 
> > char f(char *p)
> > {
> > *p = 'f';
> > return 'r';
> > }
> > 
> > int main()
> > {
> > x = 'm';
> > printf("%c %c %c\n", x, f(), x);
> > }
> > 
> > prints "m r f" here. The first x is not influenced by the call to
> > f(), while the second is. So the side effetc only affects one of the args.
> > 
> > -Otto
> > 
> > 
> 
> And if you compile with gcc on amd64 you'll get "f r m".
> 
> In my reading, the C standard is clear; the order of evaluation of the
> arguments is undefined. The side effects should indeed take effect
> before calling the function, but that's something different, e.g.
> before the side effetcs are complete, some of the args may already be
> on the stack,

Yes. Thanks for the example and the clarifications. I agree that the
spec is clear that the order is unspecified. I fooled myself into
thinking the identity function should somehow be special.

Anyway, this will be fixed by claudio's follow-up on tech.



Re: use getnameinfo in bgpd to print addresses

2021-01-04 Thread Theo Buehler
> > +   return log_sockaddr(addr2sa(addr, 0, ), len);
> 
> Perhaps I haven't yet had enough coffee this year, but I'm unsure
> whether it is actually guaranteed that addr2sa() is called before the
> second len in this line is passed to log_sockaddr().

Answering my own question: C99 and C11 6.5.2.2.12 require that all side
effects must be completed before log_sockaddr() is called. As addr2sa()
changes the second len as a side effect, this should be fine.

ok tb



Re: bgpd: struct mrt vs struct mrt_config confusion

2021-01-04 Thread Theo Buehler
On Mon, Jan 04, 2021 at 12:23:35PM +0100, Claudio Jeker wrote:
> On Mon, Jan 04, 2021 at 12:09:46PM +0100, Theo Buehler wrote:
> > Pointed out by llvm scan-build. mrt_config is much larger (> 10x). As
> > far as I can tell, this isn't bad. It just overallocates and copies a
> > lot of zeroes thanks to the calloc() in parse.y.
> > 
> > Perhaps it would be better to use sizeof(*xm) instead.
> 
> I think this is wrong. There is a difference between struct mrt_config and mrt
> but the code uses MRT2MC() in various places to change between the types.
> This code could use some work to clean up this mess.

Oh good, so this is intentional. Thanks.

>  
> > Regress passes with the Makefile diff at the end (is there a better
> > way?).
> 
> Regress diff is OK. This was the addition of 'bgpctl show sets' that added
> the need for getmonotime().
>  
> > Index: usr.sbin/bgpd/mrt.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
> > retrieving revision 1.103
> > diff -u -p -r1.103 mrt.c
> > --- usr.sbin/bgpd/mrt.c 9 Jan 2020 11:55:25 -   1.103
> > +++ usr.sbin/bgpd/mrt.c 4 Jan 2021 10:16:18 -
> > @@ -1031,9 +1031,9 @@ mrt_mergeconfig(struct mrt_head *xconf, 
> > LIST_FOREACH(m, nconf, entry) {
> > if ((xm = mrt_get(xconf, m)) == NULL) {
> > /* NEW */
> > -   if ((xm = malloc(sizeof(struct mrt_config))) == NULL)
> > +   if ((xm = malloc(sizeof(struct mrt))) == NULL)
> > fatal("mrt_mergeconfig");
> > -   memcpy(xm, m, sizeof(struct mrt_config));
> > +   memcpy(xm, m, sizeof(struct mrt));
> > xm->state = MRT_STATE_OPEN;
> > LIST_INSERT_HEAD(xconf, xm, entry);
> > } else {
> > Index: usr.sbin/bgpd/parse.y
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> > retrieving revision 1.411
> > diff -u -p -r1.411 parse.y
> > --- usr.sbin/bgpd/parse.y   29 Dec 2020 15:30:34 -  1.411
> > +++ usr.sbin/bgpd/parse.y   4 Jan 2021 10:17:26 -
> > @@ -3871,7 +3871,7 @@ add_mrtconfig(enum mrt_type type, char *
> > }
> > }
> >  
> > -   if ((n = calloc(1, sizeof(struct mrt_config))) == NULL)
> > +   if ((n = calloc(1, sizeof(struct mrt))) == NULL)
> > fatal("add_mrtconfig");
> >  
> > n->type = type;
> > Index: regress/usr.sbin/bgpd/unittests/Makefile
> > ===
> > RCS file: /cvs/src/regress/usr.sbin/bgpd/unittests/Makefile,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 Makefile
> > --- regress/usr.sbin/bgpd/unittests/Makefile29 Dec 2020 16:57:50 
> > -  1.6
> > +++ regress/usr.sbin/bgpd/unittests/Makefile4 Jan 2021 10:37:38 
> > -
> > @@ -14,11 +14,11 @@ CFLAGS+= -I${.CURDIR} -I${.CURDIR}/../..
> >  LDADD= -lutil
> >  DPADD+= ${LIBUTIL}
> >  
> > -SRCS_rde_sets_test=rde_sets_test.c rde_sets.c
> > +SRCS_rde_sets_test=rde_sets_test.c rde_sets.c timer.c log.c
> >  run-regress-rde_sets_test: rde_sets_test
> > ./rde_sets_test
> >  
> > -SRCS_rde_trie_test=rde_trie_test.c rde_trie.c util.c rde_sets.c
> > +SRCS_rde_trie_test=rde_trie_test.c rde_trie.c util.c rde_sets.c 
> > timer.c log.c
> >  TRIE_TESTS=1 2 3 4 5 6
> >  TRIE4_FLAGS=-o
> >  TRIE5_FLAGS=-r
> > 
> 
> -- 
> :wq Claudio



Re: use getnameinfo in bgpd to print addresses

2021-01-04 Thread Theo Buehler
On Mon, Jan 04, 2021 at 10:46:39AM +0100, Claudio Jeker wrote:
> In bgpd most prefixes and addresses are stored as struct bgpd_addr. When
> it is printed it uses inet_ntop() which is not ideal since it does not
> handle IPv6 scoped_id. Instead convert to a struct sockaddr and use
> log_sockaddr() which in turn uses getnameinfo.
> 
> Ideally the same should be done for the VPN address types but that is a
> bit more complex.
> 
> -- 
> :wq Claudio
> 
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.408
> diff -u -p -r1.408 bgpd.h
> --- bgpd.h30 Dec 2020 07:29:56 -  1.408
> +++ bgpd.h4 Jan 2021 09:34:44 -
> @@ -1371,7 +1371,7 @@ int  aid2afi(u_int8_t, u_int16_t *, u_i
>  int   afi2aid(u_int16_t, u_int8_t, u_int8_t *);
>  sa_family_t   aid2af(u_int8_t);
>  int   af2aid(sa_family_t, u_int8_t, u_int8_t *);
> -struct sockaddr  *addr2sa(struct bgpd_addr *, u_int16_t, socklen_t *);
> +struct sockaddr  *addr2sa(const struct bgpd_addr *, u_int16_t, socklen_t 
> *);
>  void  sa2addr(struct sockaddr *, struct bgpd_addr *, u_int16_t *);
>  const char *  get_baudrate(unsigned long long, char *);
>  
> Index: util.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> retrieving revision 1.55
> diff -u -p -r1.55 util.c
> --- util.c21 Oct 2020 06:53:54 -  1.55
> +++ util.c4 Jan 2021 09:33:30 -
> @@ -39,14 +39,12 @@ log_addr(const struct bgpd_addr *addr)
>  {
>   static char buf[74];
>   chartbuf[40];
> + socklen_t   len;
>  
>   switch (addr->aid) {
>   case AID_INET:
>   case AID_INET6:
> - if (inet_ntop(aid2af(addr->aid), >ba, buf,
> - sizeof(buf)) == NULL)
> - return ("?");
> - return (buf);
> + return log_sockaddr(addr2sa(addr, 0, ), len);

Perhaps I haven't yet had enough coffee this year, but I'm unsure
whether it is actually guaranteed that addr2sa() is called before the
second len in this line is passed to log_sockaddr().

>   case AID_VPN_IPv4:
>   if (inet_ntop(AF_INET, >vpn4.addr, tbuf,
>   sizeof(tbuf)) == NULL)
> @@ -838,7 +836,7 @@ af2aid(sa_family_t af, u_int8_t safi, u_
>  }
>  
>  struct sockaddr *
> -addr2sa(struct bgpd_addr *addr, u_int16_t port, socklen_t *len)
> +addr2sa(const struct bgpd_addr *addr, u_int16_t port, socklen_t *len)
>  {
>   static struct sockaddr_storage   ss;
>   struct sockaddr_in  *sa_in = (struct sockaddr_in *)
> 



bgpd: struct mrt vs struct mrt_config confusion

2021-01-04 Thread Theo Buehler
Pointed out by llvm scan-build. mrt_config is much larger (> 10x). As
far as I can tell, this isn't bad. It just overallocates and copies a
lot of zeroes thanks to the calloc() in parse.y.

Perhaps it would be better to use sizeof(*xm) instead.

Regress passes with the Makefile diff at the end (is there a better
way?).

Index: usr.sbin/bgpd/mrt.c
===
RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
retrieving revision 1.103
diff -u -p -r1.103 mrt.c
--- usr.sbin/bgpd/mrt.c 9 Jan 2020 11:55:25 -   1.103
+++ usr.sbin/bgpd/mrt.c 4 Jan 2021 10:16:18 -
@@ -1031,9 +1031,9 @@ mrt_mergeconfig(struct mrt_head *xconf, 
LIST_FOREACH(m, nconf, entry) {
if ((xm = mrt_get(xconf, m)) == NULL) {
/* NEW */
-   if ((xm = malloc(sizeof(struct mrt_config))) == NULL)
+   if ((xm = malloc(sizeof(struct mrt))) == NULL)
fatal("mrt_mergeconfig");
-   memcpy(xm, m, sizeof(struct mrt_config));
+   memcpy(xm, m, sizeof(struct mrt));
xm->state = MRT_STATE_OPEN;
LIST_INSERT_HEAD(xconf, xm, entry);
} else {
Index: usr.sbin/bgpd/parse.y
===
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
retrieving revision 1.411
diff -u -p -r1.411 parse.y
--- usr.sbin/bgpd/parse.y   29 Dec 2020 15:30:34 -  1.411
+++ usr.sbin/bgpd/parse.y   4 Jan 2021 10:17:26 -
@@ -3871,7 +3871,7 @@ add_mrtconfig(enum mrt_type type, char *
}
}
 
-   if ((n = calloc(1, sizeof(struct mrt_config))) == NULL)
+   if ((n = calloc(1, sizeof(struct mrt))) == NULL)
fatal("add_mrtconfig");
 
n->type = type;
Index: regress/usr.sbin/bgpd/unittests/Makefile
===
RCS file: /cvs/src/regress/usr.sbin/bgpd/unittests/Makefile,v
retrieving revision 1.6
diff -u -p -r1.6 Makefile
--- regress/usr.sbin/bgpd/unittests/Makefile29 Dec 2020 16:57:50 -  
1.6
+++ regress/usr.sbin/bgpd/unittests/Makefile4 Jan 2021 10:37:38 -
@@ -14,11 +14,11 @@ CFLAGS+= -I${.CURDIR} -I${.CURDIR}/../..
 LDADD= -lutil
 DPADD+= ${LIBUTIL}
 
-SRCS_rde_sets_test=rde_sets_test.c rde_sets.c
+SRCS_rde_sets_test=rde_sets_test.c rde_sets.c timer.c log.c
 run-regress-rde_sets_test: rde_sets_test
./rde_sets_test
 
-SRCS_rde_trie_test=rde_trie_test.c rde_trie.c util.c rde_sets.c
+SRCS_rde_trie_test=rde_trie_test.c rde_trie.c util.c rde_sets.c timer.c 
log.c
 TRIE_TESTS=1 2 3 4 5 6
 TRIE4_FLAGS=-o
 TRIE5_FLAGS=-r



Re: libc/regex: turn unsafe macros to inline functions

2021-01-03 Thread Theo Buehler
On Sun, Jan 03, 2021 at 04:45:30PM +, Miod Vallat wrote:
> > Is there a reason not to do
> > 
> > return (cs->ptr[(uch)c] & cs->mask) != 0;
> > 
> > This would allow us to get rid of the !! construct in regcomp.c
> 
> Why not. What about that?

Thanks. Here's the diff rebased on top of -current. This is

ok tb

Index: regcomp.c
===
RCS file: /cvs/src/lib/libc/regex/regcomp.c,v
retrieving revision 1.42
diff -u -p -r1.42 regcomp.c
--- regcomp.c   2 Jan 2021 20:42:01 -   1.42
+++ regcomp.c   3 Jan 2021 16:48:59 -
@@ -1099,7 +1099,7 @@ freezeset(struct parse *p, cset *cs)
if (cs2->hash == h && cs2 != cs) {
/* maybe */
for (i = 0; i < css; i++)
-   if (!!CHIN(cs2, i) != !!CHIN(cs, i))
+   if (CHIN(cs2, i) != CHIN(cs, i))
break;  /* no */
if (i == css)
break;  /* yes */
Index: regex2.h
===
RCS file: /cvs/src/lib/libc/regex/regex2.h,v
retrieving revision 1.11
diff -u -p -r1.11 regex2.h
--- regex2.h3 Jan 2021 10:50:02 -   1.11
+++ regex2.h3 Jan 2021 16:49:20 -
@@ -122,10 +122,10 @@ CHsub(cset *cs, char c)
cs->hash -= c;
 }
 
-static inline uch
+static inline int
 CHIN(const cset *cs, char c)
 {
-   return cs->ptr[(uch)c] & cs->mask;
+   return (cs->ptr[(uch)c] & cs->mask) != 0;
 }
 
 /*




Re: libc/regex: turn unsafe macros to inline functions

2021-01-02 Thread Theo Buehler
On Sat, Jan 02, 2021 at 08:33:51PM +, Miod Vallat wrote:
> That code was written before inline functions were supported by
> compilers; now that they are even part of the language standard, turn
> macros into inline functions so that there is no need to document in
> comments that they will evaluate their arguments multiple times.
> 

ok tb

Minor comments inline

> (one may consider switching their names to lowercase now that these are
> no longer macros.)

No opinion.

> 
> Index: regex2.h
> ===
> RCS file: /OpenBSD/src/lib/libc/regex/regex2.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 regex2.h
> --- regex2.h  31 Dec 2020 17:20:19 -  1.10
> +++ regex2.h  2 Jan 2021 15:59:51 -
> @@ -107,10 +107,24 @@ typedef struct {
>   uch mask;   /* bit within array */
>   uch hash;   /* hash code */
>  } cset;
> -/* note that CHadd and CHsub are unsafe, and CHIN doesn't yield 0/1 */
> -#define  CHadd(cs, c)((cs)->ptr[(uch)(c)] |= (cs)->mask, (cs)->hash 
> += (c))
> -#define  CHsub(cs, c)((cs)->ptr[(uch)(c)] &= ~(cs)->mask, (cs)->hash 
> -= (c))
> -#define  CHIN(cs, c) ((cs)->ptr[(uch)(c)] & (cs)->mask)
> +
> +static inline void
> +CHadd(cset *cs, char c)
> +{
> + cs->ptr[(uch)c] |= cs->mask;
> + cs->hash += c;
> +}

I would put a blank line here

> +static inline void
> +CHsub(cset *cs, char c)
> +{
> + cs->ptr[(uch)c] &= ~cs->mask;
> + cs->hash -= c;
> +}

and here.

> +static inline uch
> +CHIN(const cset *cs, char c)
> +{
> + return cs->ptr[(uch)c] & cs->mask;

Is there a reason not to do

return (cs->ptr[(uch)c] & cs->mask) != 0;

This would allow us to get rid of the !! construct in regcomp.c

> +}
>  
>  /*
>   * main compiled-expression structure
> 



Re: libc/regex: more dead code

2021-01-02 Thread Theo Buehler
On Sat, Jan 02, 2021 at 08:31:39PM +, Miod Vallat wrote:
> The removal of the categories code made these two functions unused, so
> remove them as well.

ok tb



Re: httpd: call tls_close before closing the socket

2021-01-01 Thread Theo Buehler
On Fri, Jan 01, 2021 at 11:38:32PM +0100, Claudio Jeker wrote:
> On Fri, Jan 01, 2021 at 09:06:34PM +0100, Theo Buehler wrote:
> > httpd(8) leaks resources when clients connect via TLS.  The reason for
> > this is that server_close() closes the socket underlying the TLS
> > connection before calling tls_close().
> > 
> > The currently unchecked tls_close() call fails with EBADF when trying to
> > write out the close_notify in SSL_shutdown(). That resources are leaked
> > are bugs in libssl/libtls which will need more investigation. Anyway,
> > tls_close() wants an open socket if possible and it wants error
> > checking, so move it up and do the usual dance.
> > 
> > This diff makes a simple httpd run in essentially constant memory when I
> > hammer it with many thousand TLS connections.
> 
> Is the fd passed to to tls_close() non-blocking? If so could result in a
> busy loop on the while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT).

Thanks, I was just wondering about this as well.  The socket is of
course non-blocking and I used the blocking idiom.  I do not how likely
it is that we hit an actual WANT_POLL* situation but it is conceivable.

In my few test cases, tls_close() always succeeds the first time around.

> Also doing these individual "poll" loops outside of the main event loop
> can hurt performance since no other fd can be processed during that time.
> I wondered about this in some other event based code I wrote. Especially
> if tls_close() is called because of some error cleanup code.

tls_close() is known to need some work. So perhaps we can do this simple
diff in the interim. This addresses the annoying resource leaks I'm
seeing and the proper handling with libevent (which I'm unfamiliar with)
can be done once it's figured out.

Index: server.c
===
RCS file: /cvs/src/usr.sbin/httpd/server.c,v
retrieving revision 1.122
diff -u -p -r1.122 server.c
--- server.c31 Dec 2020 14:17:12 -  1.122
+++ server.c1 Jan 2021 22:55:29 -
@@ -1333,14 +1333,16 @@ server_close(struct client *clt, const c
 
if (clt->clt_srvbev != NULL)
bufferevent_free(clt->clt_srvbev);
+
+   /* tls_close must be called before the underlying socket is closed. */
+   if (clt->clt_tls_ctx != NULL)
+   tls_close(clt->clt_tls_ctx); /* XXX - error handling */
+   tls_free(clt->clt_tls_ctx);
+
if (clt->clt_fd != -1)
close(clt->clt_fd);
if (clt->clt_s != -1)
close(clt->clt_s);
-
-   if (clt->clt_tls_ctx != NULL)
-   tls_close(clt->clt_tls_ctx);
-   tls_free(clt->clt_tls_ctx);
 
server_inflight_dec(clt, __func__);
 



httpd: call tls_close before closing the socket

2021-01-01 Thread Theo Buehler
httpd(8) leaks resources when clients connect via TLS.  The reason for
this is that server_close() closes the socket underlying the TLS
connection before calling tls_close().

The currently unchecked tls_close() call fails with EBADF when trying to
write out the close_notify in SSL_shutdown(). That resources are leaked
are bugs in libssl/libtls which will need more investigation. Anyway,
tls_close() wants an open socket if possible and it wants error
checking, so move it up and do the usual dance.

This diff makes a simple httpd run in essentially constant memory when I
hammer it with many thousand TLS connections.

Index: server.c
===
RCS file: /cvs/src/usr.sbin/httpd/server.c,v
retrieving revision 1.122
diff -u -p -r1.122 server.c
--- server.c31 Dec 2020 14:17:12 -  1.122
+++ server.c1 Jan 2021 18:03:32 -
@@ -1333,14 +1333,25 @@ server_close(struct client *clt, const c
 
if (clt->clt_srvbev != NULL)
bufferevent_free(clt->clt_srvbev);
+
+   /* tls_close must be called before the underlying socket is closed. */
+   if (clt->clt_tls_ctx != NULL) {
+   int ret;
+
+   do {
+   ret = tls_close(clt->clt_tls_ctx);
+   } while (ret == TLS_WANT_POLLIN || ret == TLS_WANT_POLLOUT);
+   if (ret == -1) {
+   log_warnx("%s: tls_close failed (%s)", __func__,
+   tls_error(clt->clt_tls_ctx));
+   }
+   tls_free(clt->clt_tls_ctx);
+   }
+
if (clt->clt_fd != -1)
close(clt->clt_fd);
if (clt->clt_s != -1)
close(clt->clt_s);
-
-   if (clt->clt_tls_ctx != NULL)
-   tls_close(clt->clt_tls_ctx);
-   tls_free(clt->clt_tls_ctx);
 
server_inflight_dec(clt, __func__);
 



httpd: another log related leak

2020-12-31 Thread Theo Buehler
msg is allocated by vasprintf, and is leaked on return of server_sendlog.
vasprintf calculates the length of the string, so we can zap a needless
call to strlen while there.

Index: server.c
===
RCS file: /cvs/src/usr.sbin/httpd/server.c,v
retrieving revision 1.121
diff -u -p -r1.121 server.c
--- server.c11 Oct 2020 03:21:44 -  1.121
+++ server.c31 Dec 2020 10:06:28 -
@@ -1251,12 +1251,14 @@ server_sendlog(struct server_config *srv
iov[0].iov_base = _conf->id;
iov[0].iov_len = sizeof(srv_conf->id);
iov[1].iov_base = msg;
-   iov[1].iov_len = strlen(msg) + 1;
+   iov[1].iov_len = ret + 1;
 
if (proc_composev(httpd_env->sc_ps, PROC_LOGGER, cmd, iov, 2) != 0) {
log_warn("%s: failed to compose imsg", __func__);
+   free(msg);
return;
}
+   free(msg);
 }
 
 void



httpd: free log_file in logger_close()

2020-12-30 Thread Theo Buehler
The access and error logs are never freed.  They are leaked on sending
USR1 to the parent, for example.

Index: logger.c
===
RCS file: /cvs/src/usr.sbin/httpd/logger.c,v
retrieving revision 1.22
diff -u -p -r1.22 logger.c
--- logger.c2 May 2019 22:32:34 -   1.22
+++ logger.c26 Dec 2020 20:36:29 -
@@ -89,6 +89,7 @@ logger_close(void)
log->log_fd = -1;
}
TAILQ_REMOVE(_files, log, log_entry);
+   free(log);
}
 }
 



Re: libc/regex: safer pointer arithmetic

2020-12-30 Thread Theo Buehler
On Tue, Dec 29, 2020 at 06:03:36AM -0700, Todd C. Miller wrote:
> On Tue, 29 Dec 2020 10:33:26 +, Miod Vallat wrote:
> 
> > regcomp.c uses the "start + count < end" idiom to check that there are
> > "count" bytes available in an array of char "start" and "end" both point
> > to.
> >
> > This is fine, unless "start + count" goes beyond the last element of the
> > array. In this case, pedantic interpretation of the C standard makes
> > the comparison of such a pointer against "end" undefined, and optimizers
> > from hell will happily remove as much code as possible because of this.
> 
> OK millert@

Thank you. I have committed this batch of four diffs.



Re: tls_config_parse_protocols.3: more prominent protocol list

2020-12-29 Thread Theo Buehler
On Wed, Dec 30, 2020 at 07:05:47AM +, Jason McIntyre wrote:
> On Wed, Dec 30, 2020 at 02:02:44AM +0100, Klemens Nanni wrote:
> > Manuals like httpd.conf(5) refer to tls_config_parse_protocols(3) the
> > list of supported protocols.
> > 
> > Sentences with inlined elements are generally harder to read, especially
> > in such pages and/or when they contain comments.
> > 
> > Convert to a proper list that looks like this when rendered:
> > 
> >  The tls_config_parse_protocols() utility function parses a protocol
> >  string and returns the corresponding value via the protocols argument.
> >  This value can then be passed to the tls_config_set_protocols() 
> > function.
> >  The protocol string is a comma or colon separated list of keywords.
> >  Valid keywords are:
> > 
> >all (all supported protocols)
> >tlsv1.0
> >tlsv1.1
> >tlsv1.2
> >tlsv1.3
> >secure (currently TLSv1.2 and TLSv1.3)
> >default (alias for "secure")
> >legacy (alias for "all")
> > 
> > This is also reordered such that aliases are mentioned *after* the
> > string they alias.
> > 
> > Another benefit is that those items are now tagged, e.g. ":tall" in
> > less(1) brings you right to the first item in the list.
> > 
> > Available ciphers for tls_config_set_ciphers() further down are already
> > in a list and use similar comments for aliases, but I've slightly
> > adjusted those for consistency as well.
> > 
> > Feedback? OK?
> > 
> 
> morning.
> 
> the trade off is the extra space taken up, i guess. i agree it reads
> better though, and am ok with it.

While I have no objection to doing something along these lines, I dislike
how "all" is singled out in this new list.  It's really not recommended.

I would just keep the order as it was (I see no issue with mentioning
aliases first) or use strict alphabetical order of all keywords. The
newly introduced order is not obvious.

> 
> jmc
> 
> > 
> > Index: ./lib/libtls/man/tls_config_set_protocols.3
> > ===
> > RCS file: /cvs/src/lib/libtls/man/tls_config_set_protocols.3,v
> > retrieving revision 1.8
> > diff -u -p -r1.8 tls_config_set_protocols.3
> > --- ./lib/libtls/man/tls_config_set_protocols.3 22 Jan 2020 06:46:34 
> > -  1.8
> > +++ ./lib/libtls/man/tls_config_set_protocols.3 30 Dec 2020 00:48:07 
> > -
> > @@ -99,9 +99,19 @@ This value can then be passed to the
> >  .Fn tls_config_set_protocols
> >  function.
> >  The protocol string is a comma or colon separated list of keywords.
> > -Valid keywords are tlsv1.0, tlsv1.1, tlsv1.2, tlsv1.3, all (all supported
> > -protocols), default (an alias for secure), legacy (an alias for all) and
> > -secure (currently TLSv1.2 and TLSv1.3).
> > +Valid keywords are:
> > +.Pp
> > +.Bl -tag -width "tlsv1.3" -offset indent -compact
> > +.It Dv all Pq all supported protocols
> > +.It Dv tlsv1.0
> > +.It Dv tlsv1.1
> > +.It Dv tlsv1.2
> > +.It Dv tlsv1.3
> > +.It Dv secure Pq currently TLSv1.2 and TLSv1.3
> > +.It Dv default Pq alias for secure
> > +.It Dv legacy Pq alias for all
> > +.El
> > +.Pp
> >  If a value has a negative prefix (in the form of a leading exclamation 
> > mark)
> >  then it is removed from the list of available protocols, rather than being
> >  added to it.
> > @@ -116,10 +126,12 @@ Lists of ciphers are specified by name, 
> >  permitted names are:
> >  .Pp
> >  .Bl -tag -width "insecure" -offset indent -compact
> > -.It Dv "secure" (or alias "default")
> > -.It Dv "compat"
> > -.It Dv "legacy"
> > -.It Dv "insecure" (or alias "all")
> > +.It Dv all Pq all supported ciphers
> > +.It Dv secure
> > +.It Dv default Pq alias for secure
> > +.It Dv compat
> > +.It Dv legacy
> > +.It Dv insecure Pq alias for all
> >  .El
> >  .Pp
> >  Alternatively, libssl cipher strings can be specified.
> > 
> 



Re: rpki-client unmarshal empty strings as NULL

2020-12-18 Thread Theo Buehler
> This is the next step. I added asserts for strings that must be set and
> removed some of complications around optional strings. Especially cert.c
> and some of the entityq code benefits from this.

Looks good and works for me.

ok tb



Re: rpki-client unmarshal empty strings as NULL

2020-12-18 Thread Theo Buehler
On Fri, Dec 18, 2020 at 05:45:01PM +0100, Claudio Jeker wrote:
> On Fri, Dec 18, 2020 at 01:46:49PM +0100, Theo Buehler wrote:
> > On Fri, Dec 18, 2020 at 11:43:40AM +0100, Claudio Jeker wrote:
> > > On Wed, Dec 02, 2020 at 05:06:28PM +0100, Claudio Jeker wrote:
> > > > rpki-client passes both empty strings and NULL strings as zero length
> > > > objects. The unmarshal code then allocates memory in any case and so a
> > > > NULL string is unmarshalled as empty string. This is not great, 
> > > > currently
> > > > there are no empty strings but a fair amount of NULL strings.
> > > > This diff changes the behaviour and now NULL is passed as NULL.
> > > > This should simplify passing optional strings (e.g. in the entity queue
> > > > code).
> > > 
> > > I will commit this later today. It will help with some further cleanup of
> > > the codebase.
> > 
> > I'm a bit torn about this. Some of the callers clearly do not expect
> > having to deal with NULL.
> > 
> > I see some *printf("%s", NULL) (for example in proc_rsync()) that should
> > never happen but can now happen with this change. I'm unsure that there
> > are no NULL derefs that this introduces. I'm fine with this going in if
> > you intend to address this as part of the further cleanup work you
> > envision.
> > 
> 
> In most cases the code expects a non-empty string. For example in the
> rsync.c case neither host nor mod are allowed to be NULL or "".

Yes.

> I guess adding an assert(host && mod) would be enough there.

Right.

> I actually prefer the code to blow up since as mentioned
> empty strings are almost always wrong (e.g. empty filenames or hashes).
> Indeed in all those cases a check for NULL should be added in the
> unmarshal function.

Go ahead. It certainly doesn't make things worse or (more) incorrect.

ok tb


> 
> -- 
> :wq Claudio



Re: rpki-client unmarshal empty strings as NULL

2020-12-18 Thread Theo Buehler
On Fri, Dec 18, 2020 at 11:43:40AM +0100, Claudio Jeker wrote:
> On Wed, Dec 02, 2020 at 05:06:28PM +0100, Claudio Jeker wrote:
> > rpki-client passes both empty strings and NULL strings as zero length
> > objects. The unmarshal code then allocates memory in any case and so a
> > NULL string is unmarshalled as empty string. This is not great, currently
> > there are no empty strings but a fair amount of NULL strings.
> > This diff changes the behaviour and now NULL is passed as NULL.
> > This should simplify passing optional strings (e.g. in the entity queue
> > code).
> 
> I will commit this later today. It will help with some further cleanup of
> the codebase.

I'm a bit torn about this. Some of the callers clearly do not expect
having to deal with NULL.

I see some *printf("%s", NULL) (for example in proc_rsync()) that should
never happen but can now happen with this change. I'm unsure that there
are no NULL derefs that this introduces. I'm fine with this going in if
you intend to address this as part of the further cleanup work you
envision.

> 
> -- 
> :wq Claudio
> 
> ? obj
> Index: io.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/io.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 io.c
> --- io.c  2 Dec 2020 15:31:15 -   1.10
> +++ io.c  2 Dec 2020 15:54:38 -
> @@ -153,7 +153,7 @@ io_buf_read_alloc(int fd, void **res, si
>  }
>  
>  /*
> - * Read a string (which may just be \0 and zero-length), allocating
> + * Read a string (returns NULL for zero-length strings), allocating
>   * space for it.
>   */
>  void
> @@ -162,6 +162,10 @@ io_str_read(int fd, char **res)
>   size_t   sz;
>  
>   io_simple_read(fd, , sizeof(size_t));
> + if (sz == 0) {
> + *res = NULL;
> + return;
> + }
>   if ((*res = calloc(sz + 1, 1)) == NULL)
>   err(1, NULL);
>   io_simple_read(fd, *res, sz);
> 



Re: rpki-client refactor some path building

2020-12-18 Thread Theo Buehler
On Fri, Dec 18, 2020 at 11:42:38AM +0100, Claudio Jeker wrote:
> On Thu, Dec 03, 2020 at 02:33:03PM +0100, Claudio Jeker wrote:
> > Use asprintf with %.*s to construct the path based on the mft file
> > location and the filename of the referenced file.
> > 
> > Since the * field in printf(3) is expecting an int type, typecast the
> > ptrdiff_t to an int. Add an assert check to make sure there is no
> > overflow. Also do the same overflow check in mft.c where the same idiom is
> > used.
> > 
> > Maybe some PATH_MAX checks should be placed in the mft parser.
> 
> Ping

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 main.c
> --- main.c2 Dec 2020 15:31:15 -   1.85
> +++ main.c3 Dec 2020 12:25:24 -
> @@ -451,23 +451,16 @@ static void
>  queue_add_from_mft(int fd, struct entityq *q, const char *mft,
>  const struct mftfile *file, enum rtype type, size_t *eid)
>  {
> - size_t   sz;
>   char*cp, *nfile;
>  
>   /* Construct local path from filename. */
> -
> - sz = strlen(file->file) + strlen(mft);
> - if ((nfile = calloc(sz + 1, 1)) == NULL)
> - err(1, "calloc");
> -
>   /* We know this is host/module/... */
>  
> - strlcpy(nfile, mft, sz + 1);
> - cp = strrchr(nfile, '/');
> + cp = strrchr(mft, '/');
>   assert(cp != NULL);
> - cp++;
> - *cp = '\0';
> - strlcat(nfile, file->file, sz + 1);
> + assert(cp - mft < INT_MAX);
> + if (asprintf(, "%.*s/%s", (int)(cp - mft), mft, file->file) == -1)
> + err(1, "asprintf");
>  
>   /*
>* Since we're from the same directory as the MFT file, we know
> Index: mft.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 mft.c
> --- mft.c 6 Nov 2020 04:22:18 -   1.19
> +++ mft.c 3 Dec 2020 12:37:15 -
> @@ -17,6 +17,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -457,6 +458,7 @@ mft_validfilehash(const char *fn, const 
>   /* Check hash of file now, but first build path for it */
>   cp = strrchr(fn, '/');
>   assert(cp != NULL);
> + assert(cp - fn < INT_MAX);
>   if (asprintf(, "%.*s/%s", (int)(cp - fn), fn, m->file) == -1)
>   err(1, "asprintf");
> 



Re: regress print target name

2020-12-17 Thread Theo Buehler
On Thu, Dec 17, 2020 at 12:01:25PM +0100, Alexander Bluhm wrote:
> On Wed, Dec 16, 2020 at 04:42:59PM +0100, Alexander Bluhm wrote:
> > When debugging tests, it is useful to see the target name and which
> > output belongs to it.
> 
> A small addition:
> 
> Run setup_once targets in a sepearate block with headline before
> all other targets.
> 
> ok?

ok tb

> 
> bluhm
> 
> Index: share/mk/bsd.regress.mk
> ===
> RCS file: /data/mirror/openbsd/cvs/src/share/mk/bsd.regress.mk,v
> retrieving revision 1.22
> diff -u -p -r1.22 bsd.regress.mk
> --- share/mk/bsd.regress.mk   16 Dec 2020 16:53:24 -  1.22
> +++ share/mk/bsd.regress.mk   17 Dec 2020 00:56:08 -
> @@ -75,13 +75,16 @@ ${REGRESS_TARGETS}: ${REGRESS_SETUP}
>  CLEANFILES+=${REGRESS_SETUP_ONCE:S/^/stamp-/}
>  ${REGRESS_TARGETS}: ${REGRESS_SETUP_ONCE:S/^/stamp-/}
>  ${REGRESS_SETUP_ONCE:S/^/stamp-/}: .SILENT
> + echo ' ${@:S/^stamp-//} '
>   ${MAKE} -C ${.CURDIR} ${@:S/^stamp-//}
>   date >$@
> + echo
>  .endif
>  
>  regress: .SILENT
>  .if !empty(REGRESS_SETUP_ONCE)
>   rm -f ${REGRESS_SETUP_ONCE:S/^/stamp-/}
> + ${MAKE} -C ${.CURDIR} ${REGRESS_SETUP_ONCE:S/^/stamp-/}
>  .endif
>  .for RT in ${REGRESS_TARGETS}
>   echo ' ${RT} '
> 



Re: regress print target name

2020-12-16 Thread Theo Buehler
On Wed, Dec 16, 2020 at 04:42:59PM +0100, Alexander Bluhm wrote:
> When debugging tests, it is useful to see the target name and which
> output belongs to it.  A lot of my tests have echo lines, but I
> think this is better done in the framework.  Then all tests behave
> simmilar.  I would remove the echos from the Makefiles afterwards.

Agreed. While it's not exactly perfect for libssl regress tests due to
some suboptimal target names, I think it's an improvement (provided you
remove superfluous echos, as intended).

ok tb



Re: uvm_fault: kill goto in uvm_fault()

2020-12-08 Thread Theo Buehler
On Mon, Dec 07, 2020 at 04:08:46PM -0300, Martin Pieuchot wrote:
> Diff below rewrites uvm_fault() using a loop.
> 
> I added a KERNEL_LOCK/UNLOCK() dance around the part that won't be
> unlocked soon to illustrate where this is going.
> 
> ok?

I'm also ok with this.

Total bikesheed, but I would find this slightly more readable using a
do-while loop. This lets you drop the somewhat odd initialization of
error at the top and (error == ERESTART) would actually mean a restart.

> 
> Index: uvm/uvm_fault.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 uvm_fault.c
> --- uvm/uvm_fault.c   19 Nov 2020 17:06:40 -  1.108
> +++ uvm/uvm_fault.c   7 Dec 2020 18:20:16 -
> @@ -907,7 +907,7 @@ uvm_fault(vm_map_t orig_map, vaddr_t vad
>   boolean_t shadowed;
>   struct vm_anon *anons_store[UVM_MAXRANGE], **anons;
>   struct vm_page *pages[UVM_MAXRANGE];
> - int error;
> + int error = ERESTART;
>  
>   uvmexp.faults++;/* XXX: locking? */
>   TRACEPOINT(uvm, fault, vaddr, fault_type, access_type, NULL);
> @@ -923,43 +923,32 @@ uvm_fault(vm_map_t orig_map, vaddr_t vad
>   flt.narrow = FALSE; /* normal fault */
>  
>  
> - /* "goto ReFault" means restart the page fault from ground zero. */
> -ReFault:
> - anons = anons_store;
> -
> - error = uvm_fault_check(, , , access_type);
> - switch (error) {
> - case 0:
> - break;
> - case ERESTART:
> - goto ReFault;
> - default:
> - return error;
> - }
> -
> - /* (shadowed == TRUE) if there is an anon at the faulting address */
> - shadowed = uvm_fault_upper_lookup(, , anons, pages);
> -
> - /* handle case 1: fault on an anon in our amap */
> - if (shadowed == TRUE) {
> - error = uvm_fault_upper(, , anons, fault_type,
> - access_type);
> - switch (error) {
> - case ERESTART:
> - goto ReFault;
> - default:
> - return error;
> + /*
> +  * ReFault
> +  */
> + while (error == ERESTART) {
> + anons = anons_store;
> +
> + error = uvm_fault_check(, , , access_type);
> + if (error != 0)
> + continue;
> +
> + /* True if there is an anon at the faulting address */
> + shadowed = uvm_fault_upper_lookup(, , anons, pages);
> + if (shadowed == TRUE) {
> + /* case 1: fault on an anon in our amap */
> + error = uvm_fault_upper(, , anons, fault_type,
> + access_type);
> + } else {
> + /* case 2: fault on backing object or zero fill */
> + KERNEL_LOCK();
> + error = uvm_fault_lower(, , pages, fault_type,
> + access_type);
> + KERNEL_UNLOCK();
>   }
>   }
>  
> - /* handle case 2: faulting on backing object or zero fill */
> - error = uvm_fault_lower(, , pages, fault_type, access_type);
> - switch (error) {
> - case ERESTART:
> - goto ReFault;
> - default:
> - return error;
> - }
> + return error;
>  }
>  
>  int
> 



Re: rpki-client: reject bad URLs in cert files

2020-12-02 Thread Theo Buehler
On Wed, Dec 02, 2020 at 03:17:43PM +0100, Claudio Jeker wrote:
> Be stricter in what we accept as URL. Nobody should use silly encodings
> like UTF-8 or other crap in the embedded URLs. I also consider any kind of
> space as a failure (use %20 instead if that is really needed).
> 
> This makes later handling of URLs a lot safer (e.g. rpki-client prints
> part of URLs in log messages).

[...]

> + /* make sure only US-ASCII chars are in the URL */
> + for (i = 0; i < dsz; i++) {
> + if (isalnum(d[i]) || ispunct(d[i]))
> + continue;

Should this also check isascii() to ensure that this works as intended
in the -portable version?  The ENVIRONMENT section of the isalnum() and
ispunct() manuals suggests that the characters satisfying this could be
locale-dependent on other systems.

If that makes sense, I'd probably go for "isascii() && isgraph()"
instead of "isalnum() || ispunct()" on the grounds that taking a subset
of ASCII is less likely to result in unwanted things than the union of
the somewhat fuzzy locale-dependent classes isalnum() and ispunct().



Re: ACPI diff that needs wide testing

2020-12-01 Thread Theo Buehler
On Tue, Dec 01, 2020 at 08:10:33PM +0100, Mark Kettenis wrote:
> > From: Greg Steuck 
> > Date: Mon, 30 Nov 2020 20:54:59 -0800
> > 
> > Mark Kettenis  writes:
> > 
> > > The diff below fixes the way we handle named references in AML
> > > packages.  This fixes some bugs but I'd like to make sure that this
> > > doesn't inadvertedly break things.  So tests on a wide variety of
> > > machines are welcome.
> > 
> > I see a prompt crash: https://photos.app.goo.gl/5NkwAHA6jxrXFzEV7
> 
> Can you send me the acpidump output for that machine?

gnezdo provided iasl -d output:

> > The contents of
> >cp -R /var/db/acpi /tmp && iasl -d /tmp/acpi/*
> > are here: https://blackgnezdo.github.io/hp-elitebook-840.tgz



Re: rad(8): rdomain support

2020-12-01 Thread Theo Buehler
On Mon, Nov 30, 2020 at 05:46:57PM +0100, Florian Obser wrote:
> On Sun, Nov 29, 2020 at 12:20:31PM +0100, Florian Obser wrote:
> > 
> > Let rad(8) handle all rdomains in a single daemon, similar to previous
> > work in slaacd.
> > 
> > Suggested / requested by tb who showed me previous work by reyk which
> > unfortunately predated my work in slaacd and followed a different
> > pattern.
> > 
> > There has already been a fair bit of testing and input from tb on
> > previous iterations.
> > 
> > More tests, OKs?
> > 
> 
> Updated diff after some testing by tb@
> 
> - our AF_ROUTE socket needs a RTABLE_ANY ROUTE_TABLEFILTER otherwise
>   we don't see link-local addresses arriving in rdomains != 0.
> - check if the arriving link-local address is tentative (or a
>   douplicate) and ignore it because we can't use it and sendmsg failes.
>   Monitor RTM_CHGADDRATTR messages to see when the link-local address
>   becomes valid

Thank you so much for this!

I think I have done all the testing I can for my use case and this now
looks all good. I think this covers most, if not all, code paths added
by this diff. I did as thorough a review as I can, being foreign to this
part of the tree.

ok tb

I only have one tiny comment:

> +++ frontend.c

[...]

> @@ -746,15 +743,29 @@ get_link_state(char *if_name)
>   return ls;
>  }
>  
> +int
> +get_ifrdomain(char *if_name)
> +{
> + struct ifreq ifr;
> +
> + (void) strlcpy(ifr.ifr_name, if_name, sizeof(ifr.ifr_name));

I'm not sure why you explicitly indicate that you don't want to check,
while you don't so in interface_has_linklocal_address() (both copy the
same name to an array of length IFNAMSIZ).



Re: ldapd warning

2020-11-28 Thread Theo Buehler
On Sun, Nov 29, 2020 at 08:02:45AM +0100, Emil Engler wrote:
> It can overflow! Please check for the positivity and width of size_t before!

What can overflow? ret is guaranteed to be non-negative before the cast.

As for the width (which would be about truncation, not overflow): while
the standard allows for size_t to be an unsigned integer type as small
as 16 bits, we generally assume that sizeof(size_t) >= sizeof(int).
I don't think I've ever seen a width check ensuring this in our sources.

> 
> Cheers,
> Emil
> 
> On 11/28/20 11:20 PM, Theo Buehler wrote:
> > /usr/src/usr.sbin/ldapd/util.c:46:21: warning: comparison of integers of 
> > different signs:
> >'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
> >  if (ret < 0 || ret >= size)
> > ~~~ ^  
> > 
> > This has been around for a while. I forgot that I had this patch in my
> > tree.
> > 
> > Index: util.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ldapd/util.c,v
> > retrieving revision 1.12
> > diff -u -p -r1.12 util.c
> > --- util.c  24 Oct 2019 12:39:26 -  1.12
> > +++ util.c  4 Aug 2020 07:14:33 -
> > @@ -43,7 +43,7 @@ bsnprintf(char *str, size_t size, const
> > va_start(ap, format);
> > ret = vsnprintf(str, size, format, ap);
> > va_end(ap);
> > -   if (ret < 0 || ret >= size)
> > +   if (ret < 0 || (size_t)ret >= size)
> > return 0;
> > return 1;
> > 
> 



ldapd warning

2020-11-28 Thread Theo Buehler
/usr/src/usr.sbin/ldapd/util.c:46:21: warning: comparison of integers of 
different signs:
  'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
if (ret < 0 || ret >= size)
   ~~~ ^  

This has been around for a while. I forgot that I had this patch in my
tree.

Index: util.c
===
RCS file: /cvs/src/usr.sbin/ldapd/util.c,v
retrieving revision 1.12
diff -u -p -r1.12 util.c
--- util.c  24 Oct 2019 12:39:26 -  1.12
+++ util.c  4 Aug 2020 07:14:33 -
@@ -43,7 +43,7 @@ bsnprintf(char *str, size_t size, const 
va_start(ap, format);
ret = vsnprintf(str, size, format, ap);
va_end(ap);
-   if (ret < 0 || ret >= size)
+   if (ret < 0 || (size_t)ret >= size)
return 0;
 
return 1;



Re: slaacd(8): unexpected FD

2020-11-27 Thread Theo Buehler
On Fri, Nov 27, 2020 at 09:41:21PM +0100, Florian Obser wrote:
> An interface might have disappeared or switched rdomains while we
> waited for a FD. It's not a fatal condition if it arrives late.

That makes sense.

> I can only get it to lose the race by introducing a sleep in the
> parent process, but the race is still there. Found while implementing
> rdomain support in rad(8) using the same pattern.

I did not manage to reproduce this in rdomain rad, but I didn't try very
hard. I understand the race now.

ok tb

> I'm now wondering if it would be better to listen on the route socket
> for interface arrivals / departure in the parent process instead of
> the frontend asking for a raw socket to be opened. But that's a diff
> for another time.
>
> OK?
> 
> diff --git frontend.c frontend.c
> index 4b3f575611e..7efbe50df20 100644
> --- frontend.c
> +++ frontend.c
> @@ -1164,9 +1164,14 @@ set_icmp6sock(int icmp6sock, int rdomain)
>   }
>   }
>  
> - if (icmp6sock != -1)
> - fatalx("received unneeded ICMPv6 socket for rdomain %d",
> - rdomain);
> + if (icmp6sock != -1) {
> + /*
> +  * The interface disappeared or changed rdomain while we were
> +  * waiting for the parent process to open the raw socket.
> +  */
> + close(icmp6sock);
> + return;
> + }
>  
>   LIST_FOREACH (iface, , entries) {
>   if (event_initialized(>icmp6ev->ev) &&
> 
> -- 
> I'm not entirely sure you are real.
> 



Re: slaacd(8): changing rdomain

2020-11-27 Thread Theo Buehler
On Fri, Nov 27, 2020 at 09:34:59PM +0100, Florian Obser wrote:
> 
> Handle the case of an autoconf interface changing its rdomain.
> 
> To avoide code duplication have get_icmp6ev_by_rdomain() either return
> an existing icmp6ev in the correct rdomain or allocate one.
> 
> OK?

ok tb.

> 
> diff --git frontend.c frontend.c
> index 3bf418ba31e..624ff5562f3 100644
> --- frontend.c
> +++ frontend.c
> @@ -502,7 +502,6 @@ void
>  update_iface(uint32_t if_index, char* if_name)
>  {
>   struct iface*iface;
> - struct icmp6_ev *icmp6ev;
>   struct imsg_ifinfo   imsg_ifinfo;
>   int  flags, xflags, ifrdomain;
>  
> @@ -516,32 +515,29 @@ update_iface(uint32_t if_index, char* if_name)
>   if((ifrdomain = get_ifrdomain(if_name)) == -1)
>   return;
>  
> - if ((iface = get_iface_by_id(if_index)) == NULL) {
> + iface = get_iface_by_id(if_index);
> +
> + if (iface != NULL) {
> + if (iface->rdomain != ifrdomain) {
> + if (iface->icmp6ev != NULL) {
> + iface->icmp6ev->refcnt--;
> + if (iface->icmp6ev->refcnt == 0) {
> + event_del(>icmp6ev->ev);
> + close(EVENT_FD(>icmp6ev->ev));
> + free(iface->icmp6ev);
> + }
> + iface->icmp6ev = NULL;
> + }
> + iface->rdomain = ifrdomain;
> + iface->icmp6ev = get_icmp6ev_by_rdomain(ifrdomain);
> + }
> + } else {
>   if ((iface = calloc(1, sizeof(*iface))) == NULL)
>   fatal("calloc");
>   iface->if_index = if_index;
>   iface->rdomain = ifrdomain;
> + iface->icmp6ev = get_icmp6ev_by_rdomain(ifrdomain);
>  
> - if ((icmp6ev = get_icmp6ev_by_rdomain(ifrdomain)) == NULL) {
> - if ((icmp6ev = calloc(1, sizeof(*icmp6ev))) == NULL)
> - fatal("calloc");
> - icmp6ev->rcviov[0].iov_base = (caddr_t)icmp6ev->answer;
> - icmp6ev->rcviov[0].iov_len = sizeof(icmp6ev->answer);
> - icmp6ev->rcvmhdr.msg_name = (caddr_t)>from;
> - icmp6ev->rcvmhdr.msg_namelen = sizeof(icmp6ev->from);
> - icmp6ev->rcvmhdr.msg_iov = icmp6ev->rcviov;
> - icmp6ev->rcvmhdr.msg_iovlen = 1;
> - icmp6ev->rcvmhdr.msg_controllen =
> - CMSG_SPACE(sizeof(struct in6_pktinfo)) +
> - CMSG_SPACE(sizeof(int));
> - if ((icmp6ev->rcvmhdr.msg_control = malloc(icmp6ev->
> - rcvmhdr.msg_controllen)) == NULL)
> - fatal("malloc");
> - frontend_imsg_compose_main(IMSG_OPEN_ICMP6SOCK, 0,
> - , sizeof(ifrdomain));
> - }
> - iface->icmp6ev = icmp6ev;
> - iface->icmp6ev->refcnt++;
>   LIST_INSERT_HEAD(, iface, entries);
>   }
>  
> @@ -1121,13 +1117,35 @@ struct icmp6_ev*
>  get_icmp6ev_by_rdomain(int rdomain)
>  {
>   struct iface*iface;
> + struct icmp6_ev *icmp6ev = NULL;
>  
>   LIST_FOREACH (iface, , entries) {
> - if (iface->rdomain == rdomain)
> - return (iface->icmp6ev);
> + if (iface->rdomain == rdomain) {
> + icmp6ev = iface->icmp6ev;
> + break;
> + }
>   }
>  
> - return (NULL);
> + if (icmp6ev == NULL) {
> + if ((icmp6ev = calloc(1, sizeof(*icmp6ev))) == NULL)
> + fatal("calloc");
> + icmp6ev->rcviov[0].iov_base = (caddr_t)icmp6ev->answer;
> + icmp6ev->rcviov[0].iov_len = sizeof(icmp6ev->answer);
> + icmp6ev->rcvmhdr.msg_name = (caddr_t)>from;
> + icmp6ev->rcvmhdr.msg_namelen = sizeof(icmp6ev->from);
> + icmp6ev->rcvmhdr.msg_iov = icmp6ev->rcviov;
> + icmp6ev->rcvmhdr.msg_iovlen = 1;
> + icmp6ev->rcvmhdr.msg_controllen =
> + CMSG_SPACE(sizeof(struct in6_pktinfo)) +
> + CMSG_SPACE(sizeof(int));
> + if ((icmp6ev->rcvmhdr.msg_control = malloc(icmp6ev->
> + rcvmhdr.msg_controllen)) == NULL)
> + fatal("malloc");
> + frontend_imsg_compose_main(IMSG_OPEN_ICMP6SOCK, 0,
> + , sizeof(rdomain));
> + }
> + icmp6ev->refcnt++;
> + return (icmp6ev);
>  }
>  
>  void
> -- 
> 2.29.2
> 
> 
> 
> -- 
> I'm not entirely sure you are real.
> 



Re: rad(8): check if IPv6 is enabled on an interface

2020-11-26 Thread Theo Buehler
On Thu, Nov 26, 2020 at 05:49:10PM +0100, Florian Obser wrote:
> The way rad(8) is normaly used is netstart(8) setup a bunch of
> interfaces with IPv6 prefixes and one or more are listed in rad.conf.
> Everything just works.
> 
> But rad can also be configured to work on interfaces that are
> dynamically added (tap, pair, vether...) It would then spam the log
> with:
>   sendmsg on vether1: Can't assign requested address
> as noted by tb.
> 
> It's also not sensible to use such an interface, it can't possibly
> work. So we are going to ignore interfaces without link local address.

This makes sense to me. It's much nicer to avoid EADDRNOTAVAIL in the
first place than trying to handle it. It's also a big improvement in
combination with the other diff we're testing and discussing, so I'm
very much in favor of having it. The code reads fine fwiw.

ok tb



Re: rpki-client io cleanup

2020-11-19 Thread Theo Buehler
On Thu, Nov 19, 2020 at 12:31:27PM +0100, Claudio Jeker wrote:
> The io marshall code in rpki-client is a bit strange. It mixes
> non-blocking and blocking sematics and some of the code could be more
> async. This is the first mini step. Always use the buffer io API and
> remove the functions that call io_simple_write() internally.
> 
> Next step would be to build a proper write queue and kill
> io_simple_write().
> 
> Change is fairly mechanical and works for me :)
> -- 
> :wq Claudio

ok tb

> 
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.34
> diff -u -p -r1.34 extern.h
> --- extern.h  12 Sep 2020 15:46:48 -  1.34
> +++ extern.h  19 Nov 2020 11:17:42 -
> @@ -378,10 +378,8 @@ void  io_simple_write(int, const void *
>  void  io_buf_buffer(char **, size_t *, size_t *, const void *,
>   size_t);
>  void  io_buf_read_alloc(int, void **, size_t *);
> -void  io_buf_write(int, const void *, size_t);
>  void  io_str_buffer(char **, size_t *, size_t *, const char *);
>  void  io_str_read(int, char **);
> -void  io_str_write(int, const char *);
>  
>  /* X509 helpers. */
>  
> Index: io.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/io.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 io.c
> --- io.c  12 Sep 2020 15:46:48 -  1.9
> +++ io.c  19 Nov 2020 11:17:05 -
> @@ -98,17 +98,6 @@ io_buf_buffer(char **b, size_t *bsz,
>  }
>  
>  /*
> - * Write a binary buffer of the given size, which may be zero.
> - */
> -void
> -io_buf_write(int fd, const void *p, size_t sz)
> -{
> -
> - io_simple_write(fd, , sizeof(size_t));
> - io_simple_write(fd, p, sz);
> -}
> -
> -/*
>   * Like io_str_write() but into a buffer.
>   */
>  void
> @@ -117,17 +106,6 @@ io_str_buffer(char **b, size_t *bsz, siz
>   size_t   sz = (p == NULL) ? 0 : strlen(p);
>  
>   io_buf_buffer(b, bsz, bmax, p, sz);
> -}
> -
> -/*
> - * Write a NUL-terminated string, which may be zero-length.
> - */
> -void
> -io_str_write(int fd, const char *p)
> -{
> - size_t   sz = (p == NULL) ? 0 : strlen(p);
> -
> - io_buf_write(fd, p, sz);
>  }
>  
>  /*
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 main.c
> --- main.c24 Oct 2020 08:12:00 -  1.84
> +++ main.c19 Nov 2020 11:20:01 -
> @@ -301,6 +301,8 @@ repo_lookup(int fd, const char *uri)
>   const char  *host, *mod;
>   size_t   hostsz, modsz, i;
>   struct repo *rp;
> + char*b = NULL;
> + size_t   bsz = 0, bmax = 0;
>  
>   if (!rsync_uri_parse(, ,
>   , , NULL, NULL, NULL, uri))
> @@ -337,9 +339,12 @@ repo_lookup(int fd, const char *uri)
>  
>   if (!noop) {
>   logx("%s/%s: pulling from network", rp->host, rp->module);
> - io_simple_write(fd, , sizeof(size_t));
> - io_str_write(fd, rp->host);
> - io_str_write(fd, rp->module);
> + io_simple_buffer(, , , , sizeof(size_t));
> + io_str_buffer(, , , rp->host);
> + io_str_buffer(, , , rp->module);
> +
> + io_simple_write(fd, b, bsz);
> + free(b);
>   } else {
>   rp->loaded = 1;
>   logx("%s/%s: using cache", rp->host, rp->module);
> Index: rsync.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/rsync.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 rsync.c
> --- rsync.c   12 Sep 2020 15:46:48 -  1.9
> +++ rsync.c   19 Nov 2020 11:16:17 -
> @@ -171,10 +171,10 @@ proc_child(int signal)
>  void
>  proc_rsync(char *prog, char *bind_addr, int fd)
>  {
> - size_t   id, i, idsz = 0;
> + size_t   id, i, idsz = 0, bsz = 0, bmax = 0;
>   ssize_t  ssz;
>   char*host = NULL, *mod = NULL, *uri = NULL,
> - *dst = NULL, *path, *save, *cmd;
> + *dst = NULL, *path, *save, *cmd, *b = NULL;
>   const char  *pp;
>   pid_tpid;
>   char*args[32];
> @@ -265,8 +265,13 @@ proc_rsync(char *prog, char *bind_addr, 
>   ok = 0;
>   }
>  
> - io_simple_write(fd, [i].id, sizeof(size_t));
> - io_simple_write(fd, , sizeof(ok));
> + io_simple_buffer(, , ,
> + [i].id, sizeof(size_t));
> + io_simple_buffer(, , ,
> + , 

Re: uvm_fault: Kill goto Case2

2020-11-13 Thread Theo Buehler
On Fri, Nov 13, 2020 at 12:04:23PM -0300, Martin Pieuchot wrote:
> Another simple refactoring of uvm_fault() removing a goto, ok?

ok tb



Re: uvm_fault: is there an anon?

2020-11-13 Thread Theo Buehler
On Wed, Nov 04, 2020 at 11:04:12AM -0300, Martin Pieuchot wrote:
> Diff below introduces a helper that looks for existing mapping.  The
> value returned by this lookup function determines if there's an anon
> at the faulting address which tells us if we're dealign with a fault
> of type 1 or 2.
> 
> This small refactoring is part of the current work to separate the code
> handling faults of type 1 and 2.  The end goal being to move the type 1
> faults handling out of the KERNEL_LOCK().
> 
> The function name is taken from NetBSD to not introduce more difference
> than there's already.
> 
> ok?

ok tb.

I've been running the three diffs for two days and this went through two
'make release'



Re: uvm_fault: split out handling of case 1

2020-11-12 Thread Theo Buehler
On Fri, Nov 06, 2020 at 10:12:27AM -0300, Martin Pieuchot wrote:
> Diff below moves the logic dealing with faults of case 1A & 1B to its
> own function.
> 
> With this, the logic in uvm_fault() now only deals with case 2 and the
> various if/else/goto dances can be simplified.
> 
> As for the previous refactoring diffs, the name is taken from NetBSD
> but the implementation is left mostly untouched to ease reviews. 
> 
> Upcoming locking will assert that the given amap and anon are sharing
> the same lock and that it is held in this function.
> 
> ok?

It's really an entirely straightforward refactoring. The split seems
natural and the result cleaner (the new comment also helps).

ok tb



Re: [PATCH] tcpdump: Fix missing argument from icmp_print call in print-skip.c

2020-11-03 Thread Theo Buehler
On Tue, Nov 03, 2020 at 04:19:34PM +0530, Neeraj Pal wrote:
> Hi all,
> 
> It seems that there is a typo, 2nd argument - length is missing from
> the function call icmp_print in print-skip.c

There is quite a bit more that is wrong with print-skip.c than just
that (try to add it to the Makefile and compile it). It was unhooked
from the build in 1996.

Shouldn't it rather be sent to the attic?



Re: libagentx add agentx_varbind_unsigned32

2020-10-27 Thread Theo Buehler
On Tue, Oct 27, 2020 at 07:09:40PM +0100, Martijn van Duren wrote:
> To prevent any future confusion around unsigned ints I'd like to add
> agentx_varbind_unsigned32 as an alias to agentx_varbind_gauge32.
> 
> According to RFC 2578 section 2:
> -- an unsigned 32-bit quantity
> -- indistinguishable from Gauge32
> 
> OK?

ok (for this including manpage)

> Still OK to ride yesterdays bump?

I think so



Re: Let relayd use libagentx

2020-10-26 Thread Theo Buehler
On Mon, Oct 26, 2020 at 05:20:09PM +0100, Martijn van Duren wrote:
> Mostly mechanical diff coping with the API-change for libagentx
> just committed and link it to libagentx instead of using its own
> copy.
> 
> This also allows for the removal of {sub,}agentx.[ch]
> subagentx_internal.h and subagentx_log.c, not included in the diff.
> 
> OK?

This reads fine and compiles once one moves agentx.h out of the way
(I assume you'll remove the obsolete files at the same time).

ok tb (two tiny nits below)

> Index: agentx_control.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/agentx_control.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 agentx_control.c
> --- agentx_control.c  25 Oct 2020 10:17:49 -  1.2
> +++ agentx_control.c  26 Oct 2020 16:19:09 -
> @@ -37,7 +37,7 @@
>  #include 
>  
>  #include "relayd.h"
> -#include "subagentx.h"
> +#include 

This should probably move up to the other system headers
(I don't understand the order used, so no suggestion where exactly)

> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/relayd/parse.y,v
> retrieving revision 1.247
> diff -u -p -r1.247 parse.y
> --- parse.y   25 Oct 2020 10:17:49 -  1.247
> +++ parse.y   26 Oct 2020 16:19:09 -
> @@ -56,7 +56,7 @@
>  
>  #include "relayd.h"
>  #include "http.h"
> -#include "subagentx.h"
> +#include "agentx.h"

This should be  and move up to the other system headers

>  
>  TAILQ_HEAD(files, file)   files = TAILQ_HEAD_INITIALIZER(files);
>  static struct file {



Re: rpki-client cleanup

2020-10-23 Thread Theo Buehler
On Fri, Oct 23, 2020 at 03:49:23PM +0200, Claudio Jeker wrote:
> This diff reduces the number of rsync_uri_parse() calls to one.
> 
> The cert parser just checks for rsync:// and a file extension of .mft.
> This is done similar to the way the notification URL is checked and is
> streight forward.
> 
> Additonally all the extra check for RTYPE_MFT / RTYPE_CERT in the main
> process are not needed. These checks have already been done by the parser
> and the main code can assume that the cert or tal was properly parsed
> (including these checks).
> 
> All in all this simplifies the code a bit further and may allow the
> removal of rsync_uri_parse() at a later stage.

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 cert.c
> --- cert.c12 Sep 2020 15:46:48 -  1.18
> +++ cert.c23 Oct 2020 13:04:29 -
> @@ -149,7 +149,8 @@ sbgp_sia_resource_notify(struct parse *p
>  
>   /* Make sure it's a https:// address. */
>   if (dsz <= 8 || strncasecmp(d, "https://;, 8)) {
> - warnx("%s: RFC8182 section 3.2: not using https schema", p->fn);
> + warnx("%s: RFC 8182 section 3.2: not using https schema",
> + p->fn);
>   return 0;
>   }
>  
> @@ -167,32 +168,28 @@ static int
>  sbgp_sia_resource_mft(struct parse *p,
>   const unsigned char *d, size_t dsz)
>  {
> - enum rtype rt;
> -
>   if (p->res->mft != NULL) {
>   warnx("%s: RFC 6487 section 4.8.8: SIA: "
>   "MFT location already specified", p->fn);
>   return 0;
>   }
> - if ((p->res->mft = strndup((const char *)d, dsz)) == NULL)
> - err(1, NULL);
>  
>   /* Make sure it's an MFT rsync address. */
> - if (!rsync_uri_parse(NULL, NULL, NULL,
> - NULL, NULL, NULL, , p->res->mft)) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> - "failed to parse rsync URI", p->fn);
> - free(p->res->mft);
> - p->res->mft = NULL;
> + if (dsz <= 8 || strncasecmp(d, "rsync://", 8)) {
> + warnx("%s: RFC 6487 section 4.8.8: not using rsync schema",
> + p->fn);
>   return 0;
>   }
> - if (rt != RTYPE_MFT) {
> - warnx("%s: RFC 6487 section 4.8.8: SIA: "
> + if (strcasecmp(d + dsz - 4, ".mft") != 0) {
> + warnx("%s: RFC 6487 section 4.8.8: SIA: "
>   "invalid rsync URI suffix", p->fn);
> - free(p->res->mft);
> - p->res->mft = NULL;
>   return 0;
>   }
> +
> +
> + if ((p->res->mft = strndup((const char *)d, dsz)) == NULL)
> + err(1, NULL);
> +
>   return 1;
>  }
>  
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.83
> diff -u -p -r1.83 main.c
> --- main.c11 Oct 2020 12:35:24 -  1.83
> +++ main.c23 Oct 2020 13:01:41 -
> @@ -202,19 +202,6 @@ filepath_exists(char *file)
>  
>  RB_GENERATE(filepath_tree, filepath, entry, filepathcmp);
>  
> -/*
> - * Resolve the media type of a resource by looking at its suffice.
> - * Returns the type of RTYPE_EOF if not found.
> - */
> -static enum rtype
> -rtype_resolve(const char *uri)
> -{
> - enum rtype   rp;
> -
> - rsync_uri_parse(NULL, NULL, NULL, NULL, NULL, NULL, , uri);
> - return rp;
> -}
> -
>  static void
>  entity_free(struct entity *ent)
>  {
> @@ -580,8 +567,6 @@ queue_add_from_tal(int proc, int rsync, 
>   errx(1, "TAL file has no rsync:// URI");
>  
>   /* Look up the repository. */
> - assert(rtype_resolve(uri) == RTYPE_CER);
> -
>   repo = repo_lookup(rsync, uri);
>   nfile = repo_filename(repo, uri);
>  
> @@ -590,29 +575,23 @@ queue_add_from_tal(int proc, int rsync, 
>  }
>  
>  /*
> - * Add a manifest (MFT) or CRL found in an X509 certificate, RFC 6487.
> + * Add a manifest (MFT) found in an X509 certificate, RFC 6487.
>   */
>  static void
>  queue_add_from_cert(int proc, int rsync, struct entityq *q,
>  const char *rsyncuri, const char *rrdpuri, size_t *eid)
>  {
>   char*nfile;
> - enum rtype   type;
>   const struct repo   *repo;
>  
>   if (rsyncuri == NULL)
>   return;
> - if ((type = rtype_resolve(rsyncuri)) == RTYPE_EOF)
> - errx(1, "%s: unknown file type", rsyncuri);
> - if (type != RTYPE_MFT)
> - errx(1, "%s: invalid file type", rsyncuri);
>  
>   /* Look up the repository. */
> -
>   repo = repo_lookup(rsync, rsyncuri);
>   nfile = repo_filename(repo, rsyncuri);
>  
> - entityq_add(proc, q, nfile, type, repo, NULL, NULL, 0, NULL, eid);
> + entityq_add(proc, q, nfile, RTYPE_MFT, repo, NULL, NULL, 0, NULL, 

Re: Correctly set SSL error if x509_verify fails

2020-10-23 Thread Theo Buehler
On Thu, Oct 22, 2020 at 08:44:29PM -0700, Jeremy Evans wrote:
> I was trying to diagnose a certificate validation failure in Ruby's
> openssl extension tests with LibreSSL 3.2.2, and it was made more
> difficult because the verification error type was dropped, resulting
> in a SSL_R_CERTIFICATE_VERIFY_FAILED error where
> SSL_get_verify_result returned X509_V_OK.

Could you perhaps isolate a test case or explain the reproducer in a bit
more detail? I tried running ruby 2.7's regress from ports but that
always resulted in a SIGABRT (usually while running test_ftp.rb with or
without your diff).

With my diff below I once got past this abort and saw this:

OpenSSL::TestSSL#test_verify_result
[/usr/ports/pobj/ruby-2.7.2/ruby-2.7.2/test/openssl/utils.rb:279]:
exceptions on 1 threads:
<19> expected but was
<20>.

Did you see <0> here instead?

> I think this patch will fix it.  Compile tested only.  OKs, or is there
> a better way to fix it?

This will probably also address the issue with the haproxy test reported
on the libressl list:

https://marc.info/?l=libressl=160339671313132=2
https://github.com/haproxy/haproxy/issues/916

Regarding your diff, I think setting the error on the store context
should not be the responsibility of x509_verify()'s caller. After all,
x509_verify() will likely end up being public API.

The below diff should have the same effect as yours.

> Thanks,
> Jeremy
> 
> Index: x509_vfy.c
> ===
> RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
> retrieving revision 1.81
> diff -u -p -r1.81 x509_vfy.c
> --- x509_vfy.c26 Sep 2020 02:06:28 -  1.81
> +++ x509_vfy.c23 Oct 2020 03:34:10 -
> @@ -680,6 +680,9 @@ X509_verify_cert(X509_STORE_CTX *ctx)
>   if ((vctx = x509_verify_ctx_new_from_xsc(ctx, roots)) != NULL) {
>   ctx->error = X509_V_OK; /* Initialize to OK */
>   chain_count = x509_verify(vctx, NULL, NULL);
> + if (vctx->error) {
> + ctx->error = vctx->error;
> + }
>   }
>  
>   sk_X509_pop_free(roots, X509_free);
> 


Index: x509/x509_verify.c
===
RCS file: /var/cvs/src/lib/libcrypto/x509/x509_verify.c,v
retrieving revision 1.13
diff -u -p -r1.13 x509_verify.c
--- x509/x509_verify.c  26 Sep 2020 15:44:06 -  1.13
+++ x509/x509_verify.c  23 Oct 2020 05:04:05 -
@@ -853,13 +853,13 @@ x509_verify(struct x509_verify_ctx *ctx,
 
if (ctx->roots == NULL || ctx->max_depth == 0) {
ctx->error = X509_V_ERR_INVALID_CALL;
-   return 0;
+   goto err;
}
 
if (ctx->xsc != NULL) {
if (leaf != NULL || name != NULL) {
ctx->error = X509_V_ERR_INVALID_CALL;
-   return 0;
+   goto err;
}
leaf = ctx->xsc->cert;
 
@@ -872,34 +872,34 @@ x509_verify(struct x509_verify_ctx *ctx,
 */
if ((ctx->xsc->chain = sk_X509_new_null()) == NULL) {
ctx->error = X509_V_ERR_OUT_OF_MEM;
-   return 0;
+   goto err;
}
if (!X509_up_ref(leaf)) {
ctx->error = X509_V_ERR_OUT_OF_MEM;
-   return 0;
+   goto err;
}
if (!sk_X509_push(ctx->xsc->chain, leaf)) {
X509_free(leaf);
ctx->error = X509_V_ERR_OUT_OF_MEM;
-   return 0;
+   goto err;
}
ctx->xsc->error_depth = 0;
ctx->xsc->current_cert = leaf;
}
 
if (!x509_verify_cert_valid(ctx, leaf, NULL))
-   return 0;
+   goto err;
 
if (!x509_verify_cert_hostname(ctx, leaf, name))
-   return 0;
+   goto err;
 
if ((current_chain = x509_verify_chain_new()) == NULL) {
ctx->error = X509_V_ERR_OUT_OF_MEM;
-   return 0;
+   goto err;
}
if (!x509_verify_chain_append(current_chain, leaf, >error)) {
x509_verify_chain_free(current_chain);
-   return 0;
+   goto err;
}
if (x509_verify_ctx_cert_is_root(ctx, leaf))
x509_verify_ctx_add_chain(ctx, current_chain);
@@ -925,4 +925,9 @@ x509_verify(struct x509_verify_ctx *ctx,
return ctx->xsc->verify_cb(ctx->chains_count, ctx->xsc);
}
return (ctx->chains_count);
+
+ err:
+   if (ctx->xsc != NULL)
+   ctx->xsc->error = ctx->error;
+   return 0;
 }



Re: sysupgrade ug_err not found

2020-10-22 Thread Theo Buehler
On Thu, Oct 22, 2020 at 07:08:08AM +, Mikolaj Kucharski wrote:
> Hi,
> 
> Diff fixes reference to one of the functions. There is no ug_err(), but
> I guess it should be just err(). Discovered via small typo in an URL.

Committed, thanks



Re: random canary bytes for malloc

2020-10-04 Thread Theo Buehler
On Sun, Oct 04, 2020 at 09:51:14AM +0200, Otto Moerbeek wrote:
> On Tue, Sep 29, 2020 at 08:17:54AM +0200, Otto Moerbeek wrote:
> 
> > Hi,
> > 
> > until now, canary bytes (used by the C olption) were the same as the
> > bytes used to junk (0xfd).  This means that certain overwrites are not
> > detected, like setting the high bit. 
> > 
> > This makes the byte value used to write canaries random. I do not want
> > to complicate the code to handle all combinatuon of F and C, so 0xfd
> > is still acepted as a canary byte.
> > 
> > Please test with all your favourite combinations of malloc flags.
> 
> Any takers apart from tb@ who tested this earlier?
> 
>   -Otto

This works fine for me (mostly tested with CFJ since you first sent me
your diff) and also catches the BN_rand() issue that led to this the
expected ~50% of times.

I would have put braces around the do {} while loop in the second hunk,
but it's your code :)

ok tb



Re: [httpd] Fix issues related to tls/ssl

2020-10-02 Thread Theo Buehler
On Fri, Oct 02, 2020 at 02:04:15AM +1000, Joshua Sing wrote:
> Hello there, I was adding a site to httpd when I experienced a bug that
> would cause httpd to crash whenever someone accessed the site while the TLS
> cert of the first server entry was missing.
> 
> Steps to reproduce: (httpd.conf provided at bottom of email)
>  - Have a server entry at the top of the httpd.conf file with the TLS
> certificate missing.
>  - Second server entry below the first one with a valid TLS certificate and
> key.
>  - Visit the server on HTTPS.
> 
> The crash occurs because the server is listening on port 443 without setting
> up the TLS context first.
> 
> The cleanest fix is to move the handling of missing certificates to the
> configuration parser.
> 
> See attachment for the diff to fix this bug (this also reverts r1.117 of
> server.c which is no longer neccessary).

Nice. This makes sense and looks like a superior approach for solving
the acme issue from r1.117.

The diff is against 6.7-stable, so the parse.y part has some offset, but
it applies and works as intended. Please make sure that you send diffs
against -current.

ok tb

(the s/setup/accept tweak is unrelated and should probably be committed
separately).



Re: ksh "clear-screen" for vi mode

2020-09-19 Thread Theo Buehler
On Sat, Sep 19, 2020 at 03:50:52PM -0600, Todd C. Miller wrote:
> The vi and emacs edit code are completely separate.  Try the following
> diff.  I had to rename a few things to avoid clashing with ncurses.h.

This works and appears to match bash's behavior in that it only works
in normal mode. I would slightly prefer to also add the command to the
nonstandard vi commands in the switch around line 650 to have it
available from insert mode as well. This would match zsh's behavior.

Either way, ok tb



Re: ftp: allow specifying supported protocols

2020-09-05 Thread Theo Buehler
On Sun, Sep 06, 2020 at 12:55:17AM +0200, Jeremie Courreges-Anglas wrote:
> On Sat, Sep 05 2020, Theo Buehler  wrote:
> > I found this small diff useful more than once (admittedly for debugging).
> > It allows specifying the protocols that may be used by ftp the same way
> > as nc -Tprotocols works. For example:
> >
> > $ ftp -Sprotocols=tlsv1.2:tlsv1.1 https://example.com/file
> >
> > Perhaps someone else has use for it, too?
> 
> I think it's useful indeed.  ok jca@
> 
> One nit below,
> 
> > Index: ftp.1
> > ===
> > RCS file: /var/cvs/src/usr.bin/ftp/ftp.1,v
> > retrieving revision 1.119
> > diff -u -p -r1.119 ftp.1
> > --- ftp.1   11 Feb 2020 18:41:39 -  1.119
> > +++ ftp.1   5 Sep 2020 18:11:24 -
> > @@ -261,6 +261,12 @@ Don't perform server certificate validat
> >  Require the server to present a valid OCSP stapling in the TLS handshake.
> >  .It Cm noverifytime
> >  Disable validation of certificate times and OCSP validation.
> > +.It Cm protocols Ns = Ns Ar protocol_list
> > +Specify the supported TLS protocols that will be used by
> > +.Nm
> > +(see
> > +.Xr tls_config_parse_protocols 3
> > +for details).
> >  .It Cm session Ns = Ns Ar /path/to/session
> >  Specify a file to use for TLS session data.
> >  If this file has a non-zero length, the session data will be read from 
> > this file
> > Index: main.c
> > ===
> > RCS file: /var/cvs/src/usr.bin/ftp/main.c,v
> > retrieving revision 1.132
> > diff -u -p -r1.132 main.c
> > --- main.c  1 Sep 2020 12:33:48 -   1.132
> > +++ main.c  1 Sep 2020 18:13:09 -
> > @@ -209,6 +209,8 @@ char * const ssl_verify_opts[] = {
> > "noverifytime",
> >  #define SSL_SESSION8
> > "session",
> > +#define SSL_PROTOCOLS  9
> > +   "protocols",
> > NULL
> >  };
> >  
> > @@ -220,6 +222,7 @@ process_ssl_options(char *cp)
> >  {
> > const char *errstr;
> > long long depth;
> 
> (Should probably be an int.)
> 
> > +   uint32_t protocols;
> 
> Could be moved below str (smaller size on lp64, see style(9)).
> 
> > char *str;

If we care about this, shouldn't I rather move *str up below *errstr?

> >  
> > while (*cp) {
> > @@ -278,6 +281,14 @@ process_ssl_options(char *cp)
> > tls_session_fd) == -1)
> > errx(1, "failed to set session: %s",
> > tls_config_error(tls_config));
> > +   break;
> > +   case SSL_PROTOCOLS:
> > +   if (str == NULL)
> > +   errx(1, "missing protocol name");
> > +   if (tls_config_parse_protocols(, str) != 0)
> > +   errx(1, "failed to parse TLS protocols");
> > +   if (tls_config_set_protocols(tls_config, protocols) != 
> > 0)
> > +   errx(1, "failed to set TLS protocols");
> > break;
> > default:
> > errx(1, "unknown -S suboption `%s'",
> >
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE



ftp: allow specifying supported protocols

2020-09-05 Thread Theo Buehler
I found this small diff useful more than once (admittedly for debugging).
It allows specifying the protocols that may be used by ftp the same way
as nc -Tprotocols works. For example:

$ ftp -Sprotocols=tlsv1.2:tlsv1.1 https://example.com/file

Perhaps someone else has use for it, too?

Index: ftp.1
===
RCS file: /var/cvs/src/usr.bin/ftp/ftp.1,v
retrieving revision 1.119
diff -u -p -r1.119 ftp.1
--- ftp.1   11 Feb 2020 18:41:39 -  1.119
+++ ftp.1   5 Sep 2020 18:11:24 -
@@ -261,6 +261,12 @@ Don't perform server certificate validat
 Require the server to present a valid OCSP stapling in the TLS handshake.
 .It Cm noverifytime
 Disable validation of certificate times and OCSP validation.
+.It Cm protocols Ns = Ns Ar protocol_list
+Specify the supported TLS protocols that will be used by
+.Nm
+(see
+.Xr tls_config_parse_protocols 3
+for details).
 .It Cm session Ns = Ns Ar /path/to/session
 Specify a file to use for TLS session data.
 If this file has a non-zero length, the session data will be read from this 
file
Index: main.c
===
RCS file: /var/cvs/src/usr.bin/ftp/main.c,v
retrieving revision 1.132
diff -u -p -r1.132 main.c
--- main.c  1 Sep 2020 12:33:48 -   1.132
+++ main.c  1 Sep 2020 18:13:09 -
@@ -209,6 +209,8 @@ char * const ssl_verify_opts[] = {
"noverifytime",
 #define SSL_SESSION8
"session",
+#define SSL_PROTOCOLS  9
+   "protocols",
NULL
 };
 
@@ -220,6 +222,7 @@ process_ssl_options(char *cp)
 {
const char *errstr;
long long depth;
+   uint32_t protocols;
char *str;
 
while (*cp) {
@@ -278,6 +281,14 @@ process_ssl_options(char *cp)
tls_session_fd) == -1)
errx(1, "failed to set session: %s",
tls_config_error(tls_config));
+   break;
+   case SSL_PROTOCOLS:
+   if (str == NULL)
+   errx(1, "missing protocol name");
+   if (tls_config_parse_protocols(, str) != 0)
+   errx(1, "failed to parse TLS protocols");
+   if (tls_config_set_protocols(tls_config, protocols) != 
0)
+   errx(1, "failed to set TLS protocols");
break;
default:
errx(1, "unknown -S suboption `%s'",



Re: clang warning in vmctl

2020-09-05 Thread Theo Buehler
On Wed, Sep 02, 2020 at 01:49:02PM -0600, Theo de Raadt wrote:
> I had this diff in my tree for a while, but haven't gotten around
> to passing it up.
> 
> there are other warnings in the tree, found by newer clang, that
> people should review.  Some of them are just noise, in particular
> relating to missing {}.

The base system (excluding gnu/) emits the warnings below.  A handful on
loss of precision when converting from int to double in libm, md5(1)
and disklabel(8) and one missing braces/whitespace one in keynote(8).

There's the retguard-related warning in assembly files
warning: DWARF2 only supports one section per compilation unit
and the libstd++v3 warning
warning: -ffunction-sections disabled; it makes profiling impossible
which both have been there for a while.

There's loads of funny stuff in gnu. My favorite is the warning about
chsize(3) used in the rather broken gnu/usr.bin/cvs/lib/ftruncate.c
(that's not used at all and not a new warning either).


/usr/src/lib/libm/src/s_lroundl.c:51:31: warning: implicit conversion from 
'long' to 'double' changes value from 9223372036854775807 to 
9223372036854775808 [-Wimplicit-int-float-conversion]
static const type dtype_max = DTYPE_MAX + 0.5;
  ^ ~
/usr/src/lib/libm/src/s_lroundl.c:38:19: note: expanded from macro 'DTYPE_MAX'
#define DTYPE_MAX   LONG_MAX
^~~~
/usr/include/sys/limits.h:63:19: note: expanded from macro 'LONG_MAX'
# define LONG_MAX   0x7fffL 
^~~
/usr/src/lib/libm/src/s_lroundl.c:51:31: warning: implicit conversion from 
'long' to 'double' changes value from 9223372036854775807 to 
9223372036854775808 [-Wimplicit-int-float-conversion]
static const type dtype_max = DTYPE_MAX + 0.5;
  ^ ~
/usr/src/lib/libm/src/s_lroundl.c:38:19: note: expanded from macro 'DTYPE_MAX'
#define DTYPE_MAX   LONG_MAX
^~~~
/usr/include/sys/limits.h:63:19: note: expanded from macro 'LONG_MAX'
# define LONG_MAX   0x7fffL 
^~~


/usr/src/bin/md5/md5.c:780:17: warning: implicit conversion from 'time_t' (aka 
'long long') to 'double' may lose precision [-Wimplicit-int-float-conversion]
elapsed = res.tv_sec + res.tv_usec / 100.0;
  ^~ ~
/usr/src/bin/md5/md5.c:780:30: warning: implicit conversion from 'suseconds_t' 
(aka 'long') to 'double' may lose precision [-Wimplicit-int-float-conversion]
elapsed = res.tv_sec + res.tv_usec / 100.0;
   ^~~ ~

/usr/src/sbin/disklabel/editor.c:2422:12: warning: implicit conversion from 
'unsigned long long' to 'double' changes value from 18446744073709551615 to 
18446744073709551616 [-Wimplicit-int-float-conversion]
if (val > ULLONG_MAX)
~ ^~
/usr/include/sys/limits.h:74:21: note: expanded from macro 'ULLONG_MAX'
# define ULLONG_MAX 0xULL   
^
/usr/src/sbin/disklabel/editor.c:2433:44: warning: implicit conversion from 
'unsigned long long' to 'double' changes value from 18446744073709551615 to 
18446744073709551616 [-Wimplicit-int-float-conversion]
if (errno == ERANGE || *val < 0 || *val > ULLONG_MAX)
~ ^~
/usr/include/sys/limits.h:74:21: note: expanded from macro 'ULLONG_MAX'
# define ULLONG_MAX 0xULL   
^


lex.kv.c:1217:3: warning: misleading indentation; statement is not part of the 
previous 'if' [-Wmisleading-indentation]
return yy_is_jam ? 0 : yy_current_state;
^
lex.kv.c:1214:2: note: previous statement is here
if ( ! yy_is_jam )
^
1 warning generated.



Re: httpd.conf.5 TLS defaults

2020-09-04 Thread Theo Buehler
On Fri, Sep 04, 2020 at 08:48:48PM -0700, na...@airpost.net wrote:
> This is TLS v1.2 & 1.3 now. Delete it here, since the referenced man page is 
> updated.

Thanks, I'm ok with this diff. I had the diff below in my tree for a
long time (I think it was prompted by a question of tj). I did mention
the defaults since the other tls options (except client ca) do:

Index: httpd.conf.5
===
RCS file: /var/cvs/src/usr.sbin/httpd/httpd.conf.5,v
retrieving revision 1.112
diff -u -p -r1.112 httpd.conf.5
--- httpd.conf.524 Aug 2020 15:49:10 -  1.112
+++ httpd.conf.526 Aug 2020 06:41:31 -
@@ -649,12 +649,10 @@ is empty, OCSP stapling will not be used
 The default is to not use OCSP stapling.
 .It Ic protocols Ar string
 Specify the TLS protocols to enable for this server.
-If not specified, the value
-.Qq default
-will be used (secure protocols; TLSv1.2-only).
 Refer to the
 .Xr tls_config_parse_protocols 3
-function for other valid protocol string values.
+function for valid protocol string values.
+By default, TLSv1.3 and TLSv1.2 will be used.
 .It Ic ticket lifetime Ar seconds
 Enable TLS session tickets with a
 .Ar seconds



Re: Document errno for ober_read_elements

2020-09-03 Thread Theo Buehler
On Thu, Sep 03, 2020 at 08:50:39PM +0200, Martijn van Duren wrote:
> So here's my attempt at documenting errno for ober_read_elements.
> 

Two small things below, otherwise this looks correct to me.

> martijn@
> 
> Index: ober_read_elements.3
> ===
> RCS file: /cvs/src/lib/libutil/ober_read_elements.3,v
> retrieving revision 1.1
> diff -u -p -r1.1 ober_read_elements.3
> --- ober_read_elements.3  24 Oct 2019 12:39:26 -  1.1
> +++ ober_read_elements.3  3 Sep 2020 18:48:56 -
> @@ -142,9 +142,10 @@ frees any dynamically allocated storage 
>  .Fn ober_read_elements
>  returns a pointer to a fully populated list of one or more
>  .Vt ber_element
> -structures or
> -.Dv NULL
> -on a type mismatch or read error.
> +structures.
> +Otherwise \-1 is returned and the global variable
> +.Va errno
> +is set to indicate the error.
>  .Pp
>  .Fn ober_get_writebuf
>  returns the number of bytes contained within the buffer
> @@ -155,7 +156,30 @@ or \-1 on failure.
>  returns the number of bytes written.
>  Otherwise \-1 is returned and the global variable
>  .Va errno
> -is set to indicate the error.
> +is set to
> +.Er ENOMEM

.Er ENOMEM .


> +to indicate the error.

drop this (cf malloc(3)).

> +.Sh ERRORS
> +.Fn ober_read_elements
> +will fail if:
> +.Bl -tag -width Er
> +.It Bq Er ENOMEM
> +No memory was available to create the full
> +.Vt ber_element
> +structure list.
> +.It Bq Er ENOBUFS
> +.Fn ober_read_elements
> +was called before calling
> +.Fn ober_set_readbuf .
> +.It Bq Er ECANCELED
> +.Fa buf
> +does not contain enough data to complete the unpacking.
> +.It Bq Er EINVAL
> +.Fa buf
> +does not contain a valid BER data structure.
> +.It Bq Er ERANGE
> +One of the values in the structure is larger then the library can unpack.

then -> than

I am unsure wether this is correct/idiomatic English. I don't have a
better suggestion. I would wait for jmc :)

> +.El
>  .Sh SEE ALSO
>  .Xr read 2 ,
>  .Xr recv 2 ,
> 



Re: Add more errnos to ober_read_elements

2020-09-03 Thread Theo Buehler
On Thu, Sep 03, 2020 at 08:47:58PM +0200, Martijn van Duren wrote:
> Apparently I missed one...

I double checked more carefully this time, and I think you got them all
now.

ok tb



libcrypto: clean up asn1/x_info.c

2020-09-02 Thread Theo Buehler
There's this thing called calloc(3) and all variants of free(3) in
libcrypto are NULL safe.

PS: There's still many dozens of these to clean up. I don't intend to go
through these systematically anytime soon, but rather than letting this
one rot in my tree, I might as well ask for oks...

Index: asn1/x_info.c
===
RCS file: /var/cvs/src/lib/libcrypto/asn1/x_info.c,v
retrieving revision 1.17
diff -u -p -r1.17 x_info.c
--- asn1/x_info.c   29 Jan 2017 17:49:22 -  1.17
+++ asn1/x_info.c   14 Aug 2020 09:29:28 -
@@ -60,48 +60,35 @@
 
 #include 
 #include 
-#include 
 #include 
 
 X509_INFO *
 X509_INFO_new(void)
 {
-   X509_INFO *ret = NULL;
+   X509_INFO *ret;
 
-   ret = malloc(sizeof(X509_INFO));
-   if (ret == NULL) {
+   if ((ret = calloc(1, sizeof(X509_INFO))) == NULL) {
ASN1error(ERR_R_MALLOC_FAILURE);
return (NULL);
}
-
-   ret->enc_cipher.cipher = NULL;
-   ret->enc_len = 0;
-   ret->enc_data = NULL;
-
ret->references = 1;
-   ret->x509 = NULL;
-   ret->crl = NULL;
-   ret->x_pkey = NULL;
-   return (ret);
+
+   return ret;
 }
 
 void
 X509_INFO_free(X509_INFO *x)
 {
-   int i;
-
if (x == NULL)
return;
 
-   i = CRYPTO_add(>references, -1, CRYPTO_LOCK_X509_INFO);
-   if (i > 0)
+   if (CRYPTO_add(>references, -1, CRYPTO_LOCK_X509_INFO) > 0)
return;
 
X509_free(x->x509);
-   if (x->crl != NULL)
-   X509_CRL_free(x->crl);
-   if (x->x_pkey != NULL)
-   X509_PKEY_free(x->x_pkey);
+   X509_CRL_free(x->crl);
+   X509_PKEY_free(x->x_pkey);
free(x->enc_data);
+
free(x);
 }



asn1/a_bitstring.c: zeroing after recallocarray

2020-09-02 Thread Theo Buehler
The memset is not needed as recallocarray(3) does the zeroing already.
(I also think the a->data == NULL check in the if clause is redundant,
but I'm just suggesting to remove a bit that confused me)

Index: asn1/a_bitstr.c
===
RCS file: /var/cvs/src/lib/libcrypto/asn1/a_bitstr.c,v
retrieving revision 1.29
diff -u -p -U7 -r1.29 a_bitstr.c
--- asn1/a_bitstr.c 20 Oct 2018 16:07:09 -  1.29
+++ asn1/a_bitstr.c 15 Jun 2020 12:46:00 -
@@ -211,16 +211,14 @@ ASN1_BIT_STRING_set_bit(ASN1_BIT_STRING 
if ((a->length < (w + 1)) || (a->data == NULL)) {
if (!value)
return(1); /* Don't need to set */
if ((c = recallocarray(a->data, a->length, w + 1, 1)) == NULL) {
ASN1error(ERR_R_MALLOC_FAILURE);
return 0;
}
-   if (w + 1 - a->length > 0)
-   memset(c + a->length, 0, w + 1 - a->length);
a->data = c;
a->length = w + 1;
}
a->data[w] = ((a->data[w]) & iv) | v;
while ((a->length > 0) && (a->data[a->length - 1] == 0))
a->length--;
 



clang warning in yacc's output.c

2020-09-02 Thread Theo Buehler
This one is a bit less immediate than the vmctl one:

/usr/src/usr.bin/yacc/output.c:908:26: warning: comparing a pointer to a null 
character
  constant; did you mean to compare to NULL? [-Wpointer-compare]
if ((s = symnam[i]) != '\0') {
   ^~~~
   (void *)0

This time the suggestion is correct. It used to be 'if (s = symnam[i])'
and the incorrect spelling of NULL was chosen in a -Wall cleanup 19 years
ago (-r 1.6). Upstream uses a naked 0 instead of NULL, so does NetBSD.

Index: output.c
===
RCS file: /var/cvs/src/usr.bin/yacc/output.c,v
retrieving revision 1.27
diff -u -p -r1.27 output.c
--- output.c23 May 2020 21:08:38 -  1.27
+++ output.c2 Sep 2020 20:44:13 -
@@ -905,7 +905,7 @@ output_debug(void)
"\t{", symbol_prefix);
j = 80;
for (i = 0; i <= max; ++i) {
-   if ((s = symnam[i]) != '\0') {
+   if ((s = symnam[i]) != NULL) {
if (s[0] == '"') {
k = 7;
while (*++s != '"') {



clang warning in vmctl

2020-09-02 Thread Theo Buehler
clang 10 complains:

/usr/src/usr.sbin/vmctl/vmctl.c:792:35: warning: comparing a pointer to a null 
character
  constant; did you mean to compare to NULL? [-Wpointer-compare]
'/')) == NULL || ++tty == '\0')
  ^~~~
  (void *)0
The intent is probably rather this:

Index: vmctl.c
===
RCS file: /var/cvs/src/usr.sbin/vmctl/vmctl.c,v
retrieving revision 1.74
diff -u -p -r1.74 vmctl.c
--- vmctl.c 11 Mar 2020 12:47:49 -  1.74
+++ vmctl.c 2 Sep 2020 18:06:47 -
@@ -789,7 +789,7 @@ print_vm_info(struct vmop_info_result *l
tty = "-";
/* get tty - skip /dev/ path */
else if ((tty = strrchr(vmi->vir_ttyname,
-   '/')) == NULL || ++tty == '\0')
+   '/')) == NULL || *++tty == '\0')
tty = list[i].vir_ttyname;
 
(void)fmt_scaled(vir->vir_used_size, curmem);



Re: m_defrag(9) leak

2020-09-01 Thread Theo Buehler
On Mon, Aug 31, 2020 at 08:50:09AM +0200, Theo Buehler wrote:
> On Tue, Aug 25, 2020 at 03:28:03PM +0200, Claudio Jeker wrote:
> > On Tue, Aug 25, 2020 at 08:38:06PM +1000, Matt Dunwoodie wrote:
> > > On Tue, 25 Aug 2020 08:54:10 +0200
> > > Claudio Jeker  wrote:
> > > 
> > > > On Tue, Aug 25, 2020 at 08:42:47AM +0200, Martin Pieuchot wrote:
> > > > > Maxime Villard mentioned a leak due to a missing m_freem() in wg(4):
> > > > >   https://marc.info/?l=netbsd-tech-net=159827988018641=2
> > > > > 
> > > > > It seems to be that such leak is present in other uses of
> > > > > m_defrag() in the tree.  I won't take the time to go though all of
> > > > > them, but an audit would be welcome.  
> > > > 
> > > > Why does wg(4) needs m_defrag? and why is it used in this way? It
> > > > does not even check if m_defrag() is actually needed.
> > > >
> > > > m_defrag() should be used by HW drivers where DMA constraints require
> > > > the mbuf to be flattened. Software dirvers should use m_pullup /
> > > > m_pulldown etc instead.
> > > 
> > > wg(4) uses m_defrag to bring all mbuf fragments into one, so it can
> > > read handshake values, and decrypt data directly from the mbuf. In
> > > both cases it needs the full length of the packet.
> > 
> > For the handshake headers m_pullup() or m_pulldown() is normally the way
> > to go.  Doing a massive pullup to decrypt data is a poor design. You work
> > against the network stack here and this comes at a reasonably high price
> > (considering you need to allocate a new mbuf, mbuf cluster and copy all
> > data over).
> >  
> > > However you're right, it doesn't check if it needs defrag and yes
> > > m_pullup is preferable. I have a new patch for m_pullup and includes a
> > > comment on why it's done. Thoughts?
> > > 
> > > There is one other case of "m_pullup(m, m->m_pkthdr.len)" in sppp_input.
> > 
> > sppp(4) is not without its own flaws. That code was added because of
> > alignment issues. It may actually no longer be needed but I have not the
> > time right now to really think this through.
> > 
> > > (To avoid any confusion with the previous patch, there is no need to
> > > m_free(m) as m_pullup will do that for us.)
> > 
> > > diff --git if_wg.c if_wg.c
> > > index e5a1071ccf1..242bd105e72 100644
> > > --- if_wg.c
> > > +++ if_wg.c
> > > @@ -2022,7 +2022,13 @@ wg_input(void *_sc, struct mbuf *m, struct ip *ip, 
> > > struct ip6_hdr *ip6,
> > >   /* m has a IP/IPv6 header of hlen length, we don't need it anymore. */
> > >   m_adj(m, hlen);
> > >  
> > > - if (m_defrag(m, M_NOWAIT) != 0)
> > > + /*
> > > +  * Ensure mbuf is contiguous over full length of packet. This is done
> > > +  * so we can directly read the handshake values in wg_handshake, and so
> > > +  * we can decrypt a transport packet by passing a single buffer to
> > > +  * noise_remote_decrypt in wg_decap.
> > > +  */
> > > + if ((m = m_pullup(m, m->m_pkthdr.len)) == NULL)
> > >   return NULL;
> > >  
> > >   if ((m->m_pkthdr.len == sizeof(struct wg_pkt_initiation) &&
> > 
> > I'm fine with this or the m_defrag fix to go in now to fix the mbuf leak.
> 
> Could someone who understands this please commit either version of the
> diff?

I committed this diff now. Thanks



Re: m_defrag(9) leak

2020-08-31 Thread Theo Buehler
On Tue, Aug 25, 2020 at 03:28:03PM +0200, Claudio Jeker wrote:
> On Tue, Aug 25, 2020 at 08:38:06PM +1000, Matt Dunwoodie wrote:
> > On Tue, 25 Aug 2020 08:54:10 +0200
> > Claudio Jeker  wrote:
> > 
> > > On Tue, Aug 25, 2020 at 08:42:47AM +0200, Martin Pieuchot wrote:
> > > > Maxime Villard mentioned a leak due to a missing m_freem() in wg(4):
> > > > https://marc.info/?l=netbsd-tech-net=159827988018641=2
> > > > 
> > > > It seems to be that such leak is present in other uses of
> > > > m_defrag() in the tree.  I won't take the time to go though all of
> > > > them, but an audit would be welcome.  
> > > 
> > > Why does wg(4) needs m_defrag? and why is it used in this way? It
> > > does not even check if m_defrag() is actually needed.
> > >
> > > m_defrag() should be used by HW drivers where DMA constraints require
> > > the mbuf to be flattened. Software dirvers should use m_pullup /
> > > m_pulldown etc instead.
> > 
> > wg(4) uses m_defrag to bring all mbuf fragments into one, so it can
> > read handshake values, and decrypt data directly from the mbuf. In
> > both cases it needs the full length of the packet.
> 
> For the handshake headers m_pullup() or m_pulldown() is normally the way
> to go.  Doing a massive pullup to decrypt data is a poor design. You work
> against the network stack here and this comes at a reasonably high price
> (considering you need to allocate a new mbuf, mbuf cluster and copy all
> data over).
>  
> > However you're right, it doesn't check if it needs defrag and yes
> > m_pullup is preferable. I have a new patch for m_pullup and includes a
> > comment on why it's done. Thoughts?
> > 
> > There is one other case of "m_pullup(m, m->m_pkthdr.len)" in sppp_input.
> 
> sppp(4) is not without its own flaws. That code was added because of
> alignment issues. It may actually no longer be needed but I have not the
> time right now to really think this through.
> 
> > (To avoid any confusion with the previous patch, there is no need to
> > m_free(m) as m_pullup will do that for us.)
> 
> > diff --git if_wg.c if_wg.c
> > index e5a1071ccf1..242bd105e72 100644
> > --- if_wg.c
> > +++ if_wg.c
> > @@ -2022,7 +2022,13 @@ wg_input(void *_sc, struct mbuf *m, struct ip *ip, 
> > struct ip6_hdr *ip6,
> > /* m has a IP/IPv6 header of hlen length, we don't need it anymore. */
> > m_adj(m, hlen);
> >  
> > -   if (m_defrag(m, M_NOWAIT) != 0)
> > +   /*
> > +* Ensure mbuf is contiguous over full length of packet. This is done
> > +* so we can directly read the handshake values in wg_handshake, and so
> > +* we can decrypt a transport packet by passing a single buffer to
> > +* noise_remote_decrypt in wg_decap.
> > +*/
> > +   if ((m = m_pullup(m, m->m_pkthdr.len)) == NULL)
> > return NULL;
> >  
> > if ((m->m_pkthdr.len == sizeof(struct wg_pkt_initiation) &&
> 
> I'm fine with this or the m_defrag fix to go in now to fix the mbuf leak.

Could someone who understands this please commit either version of the
diff?

> 
> -- 
> :wq Claudio
> 



Re: diff: simplify ospf6d code

2020-08-21 Thread Theo Buehler
On Fri, Aug 21, 2020 at 10:08:50AM +0200, Jan Klemkow wrote:
> Hi,
> 
> The following diff simplifies a switch case statement with no functional
> change.
> 
> In case of IMSG_CTL_SHOW_DATABASE duplicated code is removed.  This part
> of the code is now the same as in ospfd.

This part looks fine.

> 
> In case of IMSG_CTL_SHOW_DB_INTRA the code was also duplicated and due
> to a missing continue the v->type variable was double check.

This doesn't look correct. v->type was checked against
LSA_TYPE_INTRA_A_PREFIX which is not the same as
LSA_TYPE_INTER_A_PREFIX

I can't tell if a continue; is missing (which looks likely) in the
IMSG_CTL_SHOW_DB_INTRA case or if the check against both INTRA and INTER
is deliberate.

> 
> OK?
> 
> bye,
> Jan
> 
> Index: rde_lsdb.c
> ===
> RCS file: /cvs/src/usr.sbin/ospf6d/rde_lsdb.c,v
> retrieving revision 1.43
> diff -u -p -r1.43 rde_lsdb.c
> --- rde_lsdb.c17 Feb 2020 08:12:22 -  1.43
> +++ rde_lsdb.c21 Aug 2020 07:52:15 -
> @@ -763,9 +763,7 @@ lsa_dump(struct lsa_tree *tree, int imsg
>   lsa_age(v);
>   switch (imsg_type) {
>   case IMSG_CTL_SHOW_DATABASE:
> - rde_imsg_compose_ospfe(IMSG_CTL_SHOW_DATABASE, 0, pid,
> - >lsa->hdr, ntohs(v->lsa->hdr.len));
> - continue;
> + break;
>   case IMSG_CTL_SHOW_DB_SELF:
>   if (v->lsa->hdr.adv_rtr == rde_router_id())
>   break;
> @@ -787,8 +785,6 @@ lsa_dump(struct lsa_tree *tree, int imsg
>   break;
>   continue;
>   case IMSG_CTL_SHOW_DB_INTRA:
> - if (v->type == LSA_TYPE_INTRA_A_PREFIX)
> - break;
>   case IMSG_CTL_SHOW_DB_SUM:
>   if (v->type == LSA_TYPE_INTER_A_PREFIX)
>   break;
> 



Re: sync libfido2 with upstream

2020-08-20 Thread Theo Buehler
On Fri, Aug 21, 2020 at 11:42:53AM +1000, Damien Miller wrote:
> On Mon, 17 Aug 2020, Damien Miller wrote:
> 
> > On Mon, 10 Aug 2020, Damien Miller wrote:
> > 
> > > Hi,
> > > 
> > > This syncs libfido2 with the current state of upstream. It includes
> > > a few new APIs that I want to use in OpenSSH to improve FIDO token
> > > support (require-PIN and fixing some corner-case bugs around multiple
> > > inserted tokens).
> > > 
> > > ok?
> > > 
> > > (major crank for ABI change)
> > 
> > So I pounced on the new API a bit too soon and before it stabilised.
> > There have been a couple more changes upstream that I need.
> > 
> > Sorry for the unneccessary churn.
> > 
> > ok?
> 
> I'd like to commit this. Ok?

ok tb



make(1): anchors in :S/old_string/new_string/

2020-08-20 Thread Theo Buehler
gnezdo noticed that :S/old_string/new_string/ modifiers such as
:S/^sth/&/ and :S/sth$/&/ with an anchor in the old_string and an &
in the new_string don't work as documented (and expected) since they
replace & with old_string including the anchors.

This is because get_spatternarg() deals with skipping the anchors in
pattern->lhs only after having replaced any '&' in new_string with
pattern->lhs.  This happens in VarGetPattern if the last argument is
non-NULL when pattern->rhs is determined, i.e., before
common_get_patternarg() is done.

I think the fix should be as simple as the diff below.  I added a small
extension of a regress test for this.  Unpatched make fails, patched
make passes.

Simple test case that illustrates the problem:

$ cat > makefile
A= foo bar barr
B= ${A:S/^b/s&/}
C= ${A:S/r$/&/}
D= ${A:S/^bar$/&/}

all:
@echo "B= $B"
@echo "C= $C"
@echo "D= $D"
EOF
$ make
B= foo s^bar s^barr
C= foo bar$ barr$
D= foo ^bar$bar barr

I'm not entirely clear on why I get that result for D, but with the
patch below, I get the expected " foo barbarian barr"

Index: usr.bin/make/varmodifiers.c
===
RCS file: /var/cvs/src/usr.bin/make/varmodifiers.c,v
retrieving revision 1.47
diff -u -p -r1.47 varmodifiers.c
--- usr.bin/make/varmodifiers.c 10 Jul 2017 07:10:29 -  1.47
+++ usr.bin/make/varmodifiers.c 20 Aug 2020 16:03:54 -
@@ -1215,21 +1215,7 @@ get_patternarg(const char **p, SymTable 
 static void *
 get_spatternarg(const char **p, SymTable *ctxt, bool err, int endc)
 {
-   VarPattern *pattern;
-
-   pattern = common_get_patternarg(p, ctxt, err, endc, true);
-   if (pattern != NULL && pattern->leftLen > 0) {
-   if (pattern->lhs[pattern->leftLen-1] == '$') {
-   pattern->leftLen--;
-   pattern->flags |= VAR_MATCH_END;
-   }
-   if (pattern->lhs[0] == '^') {
-   pattern->lhs++;
-   pattern->leftLen--;
-   pattern->flags |= VAR_MATCH_START;
-   }
-   }
-   return pattern;
+   return common_get_patternarg(p, ctxt, err, endc, true);
 }
 
 static void
@@ -1304,6 +1290,17 @@ common_get_patternarg(const char **p, Sy
>leftLen, NULL);
pattern->lbuffer = pattern->lhs;
if (pattern->lhs != NULL) {
+   if (dosubst && pattern->leftLen > 0) {
+   if (pattern->lhs[pattern->leftLen-1] == '$') {
+   pattern->leftLen--;
+   pattern->flags |= VAR_MATCH_END;
+   }
+   if (pattern->lhs[0] == '^') {
+   pattern->lhs++;
+   pattern->leftLen--;
+   pattern->flags |= VAR_MATCH_START;
+   }
+   }
pattern->rhs = VarGetPattern(ctxt, err, , delim, delim,
>rightLen, dosubst ? pattern: NULL);
if (pattern->rhs != NULL) {
Index: regress/usr.bin/make/mk21
===
RCS file: /var/cvs/src/regress/usr.bin/make/mk21,v
retrieving revision 1.2
diff -u -p -r1.2 mk21
--- regress/usr.bin/make/mk21   7 Jul 2017 16:31:37 -   1.2
+++ regress/usr.bin/make/mk21   20 Aug 2020 16:20:31 -
@@ -12,7 +12,13 @@ X?=${TRUC:C@([^:/])/.+$@\1/@}
 Y?=${TRUC:S/^${X}//:S/^://}
 Z?=${TRUC:S/^${TRUC:C@([^:/])/.+$@\1/@}//:S/^://}
 
+A?=machin truc
+B?=${A:S/^/mot: &/}
+C?=${A:S/$/&: mot/}
+
 all:
+   @echo "B= $B"
+   @echo "C= $C"
@echo "S= $S"
@echo "T= $T"
@echo "X= $X"
Index: regress/usr.bin/make/t21.out
===
RCS file: /var/cvs/src/regress/usr.bin/make/t21.out,v
retrieving revision 1.2
diff -u -p -r1.2 t21.out
--- regress/usr.bin/make/t21.out7 Jul 2017 16:31:37 -   1.2
+++ regress/usr.bin/make/t21.out20 Aug 2020 16:20:31 -
@@ -1,3 +1,5 @@
+B= mot: machin mot: truc
+C= machin: mot truc: mot
 S= sourceforge/%SUBDIR%/
 T= sourceforge/%SUBDIR%/
 X= http://heanet.dl.sourceforge.net/



Re: release.8 - .fs -> .img

2020-08-20 Thread Theo Buehler
On Wed, Aug 19, 2020 at 11:42:39PM -0700, na...@airpost.net wrote:
> Update the name of the install image.

Fixed, thanks.

> 
> Index: release.8
> ===
> RCS file: /cvs/src/share/man/man8/release.8,v
> retrieving revision 1.95
> diff -u -p -r1.95 release.8
> --- release.8 29 Apr 2020 15:02:51 -  1.95
> +++ release.8 20 Aug 2020 06:38:16 -
> @@ -258,7 +258,7 @@ This is described in
>  .Xr ports 7 .
>  .Ss 8. Create boot and installation disk images
>  The disk images
> -.No install${ Ns Va VERSION Ns }.fs
> +.No install${ Ns Va VERSION Ns }.img
>  and
>  .No install${ Ns Va VERSION Ns }.iso
>  are suitable for installs without network connectivity.
> 



Re: ldapd: adding bsd.schema

2020-08-12 Thread Theo Buehler
On Tue, Aug 11, 2020 at 10:22:51PM -0400, Aisha Tammy wrote:
> Another bump.

I think this is useful and am ok with this.

Are there any concerns? If not, I'm going to commit it tomorrow.

Index: etc/examples/ldapd.conf
===
RCS file: /cvs/src/etc/examples/ldapd.conf,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 ldapd.conf
--- etc/examples/ldapd.conf 11 Jul 2014 21:20:10 -  1.1
+++ etc/examples/ldapd.conf 18 May 2018 10:09:45 -
@@ -3,6 +3,7 @@
 schema "/etc/ldap/core.schema"
 schema "/etc/ldap/inetorgperson.schema"
 schema "/etc/ldap/nis.schema"
+schema "/etc/ldap/bsd.schema"
 
 listen on lo0
 listen on "/var/run/ldapi"
Index: usr.sbin/ldapd/Makefile
===
RCS file: /cvs/src/usr.sbin/ldapd/Makefile,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 Makefile
--- usr.sbin/ldapd/Makefile 20 Jan 2017 11:55:08 -  1.15
+++ usr.sbin/ldapd/Makefile 18 May 2018 10:09:45 -
@@ -17,7 +17,8 @@ CFLAGS+=  -Wshadow -Wpointer-arith -Wcast
 CFLAGS+=   -Wsign-compare
 CLEANFILES+=   y.tab.h parse.c
 
-SCHEMA_FILES=  core.schema \
+SCHEMA_FILES=  bsd.schema \
+   core.schema \
inetorgperson.schema \
nis.schema
 
Index: usr.sbin/ldapd/schema/bsd.schema
===
RCS file: usr.sbin/ldapd/schema/bsd.schema
diff -N usr.sbin/ldapd/schema/bsd.schema
--- /dev/null   1 Jan 1970 00:00:00 -
+++ usr.sbin/ldapd/schema/bsd.schema18 May 2018 10:09:45 -
@@ -0,0 +1,17 @@
+attributetype ( 1.3.6.1.4.1.30155.115.2 NAME 'shadowPassword'
+   DESC 'POSIX hashed password'
+   EQUALITY caseExactIA5Match
+   SYNTAX 1.3.6.1.4.1.1466.115.121.1.26 )
+
+attributetype ( 1.3.6.1.4.1.30155.115.3 NAME 'sshPublicKey'
+   DESC 'SSH public key'
+   EQUALITY caseExactIA5Match
+   SYNTAX 1.3.6.1.4.1.1466.115.121.1.26 )
+
+objectclass ( 1.3.6.1.4.1.30155.115.1 NAME 'bsdAccount'
+   SUP top
+   AUXILIARY
+   DESC 'Abstraction of an account with OpenBSD attributes'
+   MUST ( uid )
+   MAY ( shadowPassword $ shadowExpire $ modifyTimestamp $ userClass $
+   sshPublicKey ))



Re: wg: fix build without pf

2020-07-13 Thread Theo Buehler
On Sun, Jul 12, 2020 at 07:44:47PM +0200, Klemens Nanni wrote:
> Feedback? OK?

You need to have pf.h in scope to condition on NPF > 0.

panic: kernel diagnostic assertion "m->m_ptheader.pf.statekey == NULL" failed: 
file "/usr/src/sys/net/pf.c" line 7455

Index: sys/net/if_wg.c
===
RCS file: /var/cvs/src/sys/net/if_wg.c,v
retrieving revision 1.10
diff -u -p -r1.10 if_wg.c
--- sys/net/if_wg.c 12 Jul 2020 18:54:23 -  1.10
+++ sys/net/if_wg.c 13 Jul 2020 06:27:30 -
@@ -18,6 +18,7 @@
  */
 
 #include "bpfilter.h"
+#include "pf.h"
 
 #include 
 #include 



Re: ftp(1) double free

2020-07-04 Thread Theo Buehler
On Sun, Jun 21, 2020 at 02:23:54AM +0200, Jeremie Courreges-Anglas wrote:
> 
> Small bug, but still...
> 
> naddy@ reported a double free when interrupting (^C) ftp(1) at the end
> or url_get().  If that happens, the SIGINT handler longjmps and the
> cleanup path is taken a second time.  To avoid this, restore the
> previous SIGINT handler in each possible error path.  I chose to restore
> it earlier in the successful path to avoid too much duplication.
> file_get() gets the same treatment.
> 
> Note that naddy hit this because of another bug causing ftp_close()
> to stall, and this depends on the server you're talking to.  You can
> insert a sleep(10); call before "return (rval);" to help reproduce the
> crash.
> 
> ok?
> 

ok tb

> 
> Index: fetch.c
> ===
> RCS file: /d/cvs/src/usr.bin/ftp/fetch.c,v
> retrieving revision 1.195
> diff -u -p -p -u -r1.195 fetch.c
> --- fetch.c   20 Jun 2020 09:59:48 -  1.195
> +++ fetch.c   21 Jun 2020 00:12:10 -
> @@ -261,6 +261,7 @@ file_get(const char *path, const char *o
>   for (cp = buf; len > 0; len -= wlen, cp += wlen) {
>   if ((wlen = write(out, cp, len)) == -1) {
>   warn("Writing %s", savefile);
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   goto cleanup_copy;
>   }
> @@ -274,6 +275,7 @@ file_get(const char *path, const char *o
>   }
>   }
>   save_errno = errno;
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   if (hash && !progress && bytes > 0) {
>   if (bytes < mark)
> @@ -288,7 +290,6 @@ file_get(const char *path, const char *o
>   progressmeter(1, NULL);
>   if (verbose)
>   ptransfer(0);
> - (void)signal(SIGINT, oldintr);
>  
>   rval = 0;
>  
> @@ -1032,6 +1033,7 @@ noslash:
>   oldinti = signal(SIGINFO, psummary);
>   if (chunked) {
>   error = save_chunked(fin, tls, out, buf, buflen);
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   if (error == -1)
>   goto cleanup_url_get;
> @@ -1041,6 +1043,7 @@ noslash:
>   for (cp = buf; len > 0; len -= wlen, cp += wlen) {
>   if ((wlen = write(out, cp, len)) == -1) {
>   warn("Writing %s", savefile);
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   goto cleanup_url_get;
>   }
> @@ -1054,6 +1057,7 @@ noslash:
>   }
>   }
>   save_errno = errno;
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   if (hash && !progress && bytes > 0) {
>   if (bytes < mark)
> @@ -1080,7 +1084,6 @@ noslash:
>  
>   if (verbose)
>   ptransfer(0);
> - (void)signal(SIGINT, oldintr);
>  
>   rval = 0;
>   goto cleanup_url_get;
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 



random toeplitz seeds

2020-06-25 Thread Theo Buehler
This adds an stoeplitz_random_seed() function that generates a random
Toeplitz key seed with an invertible matrix T. This is necessary and
sufficient for the hash to spread out over all 65536 possible values.

While it is clear from T * (-1) == 0 that seeds with parity 0 are bad,
I don't have a neat and clean proof for the fact that a seed with
parity 1 always generates an invertible Toeplitz matrix. It's not hard
to check, but rather tedious.

I'm unsure how to hook it up. I enabled random seeds by using the
function in stoeplitz_init(), but that's just for illustration.

Index: sys/net/toeplitz.c
===
RCS file: /var/cvs/src/sys/net/toeplitz.c,v
retrieving revision 1.7
diff -u -p -r1.7 toeplitz.c
--- sys/net/toeplitz.c  19 Jun 2020 08:48:15 -  1.7
+++ sys/net/toeplitz.c  25 Jun 2020 18:43:02 -
@@ -69,9 +69,38 @@ static struct stoeplitz_cachestoeplitz_
 const struct stoeplitz_cache *const
stoeplitz_cache = _syskey_cache; 
 
+/* parity of n16: count (mod 2) of ones in the binary representation. */
+int
+parity(uint16_t n16)
+{
+   n16 = ((n16 & 0x) >> 1) ^ (n16 & 0x);
+   n16 = ((n16 & 0x) >> 2) ^ (n16 & 0x);
+   n16 = ((n16 & 0xf0f0) >> 4) ^ (n16 & 0x0f0f);
+   n16 = ((n16 & 0xff00) >> 8) ^ (n16 & 0x00ff);
+
+   return (n16);
+}
+
+/*
+ * The Toeplitz matrix obtained from a seed is invertible if and only if the
+ * parity of the seed is 1. Generate such a seed uniformly at random.
+ */
+stoeplitz_key
+stoeplitz_random_seed(void)
+{
+   stoeplitz_key seed;
+   
+   seed = arc4random() & UINT16_MAX;
+   if (parity(seed) == 0)
+   seed ^= 1;
+
+   return (seed);
+}
+
 void
 stoeplitz_init(void)
 {
+   stoeplitz_keyseed = stoeplitz_random_seed();
stoeplitz_cache_init(_syskey_cache, stoeplitz_keyseed);
 }
 



Re: improve pkg_add bandwidth usage with some mirrors

2020-06-19 Thread Theo Buehler
On Fri, Jun 19, 2020 at 11:42:44AM -, Christian Weisgerber wrote:
> On 2020-06-18, Marc Espie  wrote:
> 
> > What pkg_add does internally is a pipeline:
> >
> > ftp | signify|internal gunzip
> >
> > closing the end file handle should kill the whole chain.
> > So I need to figure out where it goes wrong, what's the
> > part that doesn't die "instantly".
> 
> That's ftp(1).  Our SSL people are sitting on a patch to libtls^H^H^Hssl.

Yes, jsing wanted to take a closer look.  I will commit the diff tonight
UTC unless I hear an objection (I have an ok beck).

Index: tls13_legacy.c
===
RCS file: /var/cvs/src/lib/libssl/tls13_legacy.c,v
retrieving revision 1.8
diff -u -p -r1.8 tls13_legacy.c
--- tls13_legacy.c  29 May 2020 17:47:30 -  1.8
+++ tls13_legacy.c  11 Jun 2020 12:19:30 -
@@ -477,6 +477,7 @@ tls13_legacy_shutdown(SSL *ssl)
struct tls13_ctx *ctx = ssl->internal->tls13;
uint8_t buf[512]; /* XXX */
ssize_t ret;
+   int want_close_notify = 1;
 
/*
 * We need to return 0 when we have sent a close-notify but have not
@@ -492,6 +493,11 @@ tls13_legacy_shutdown(SSL *ssl)
/* Send close notify. */
if (!(ssl->internal->shutdown & SSL_SENT_SHUTDOWN)) {
ssl->internal->shutdown |= SSL_SENT_SHUTDOWN;
+   /*
+* Do not try to read application data to support unilateral
+* shutdown semantics for SSL_shutdown(3).
+*/
+   want_close_notify = 0;
if ((ret = tls13_send_alert(ctx->rl, TLS13_ALERT_CLOSE_NOTIFY)) 
< 0)
return tls13_legacy_return_code(ssl, ret);
}
@@ -501,7 +507,7 @@ tls13_legacy_shutdown(SSL *ssl)
return tls13_legacy_return_code(ssl, ret);
 
/* Receive close notify. */
-   if (!ctx->close_notify_recv) {
+   if (want_close_notify && !ctx->close_notify_recv) {
/*
 * If there is still application data pending then we have no
 * option but to discard it here. The application should have



  1   2   3   4   5   6   7   8   >