Re: OpenBSD Errata: August 12, 2022 (zlib)

2022-08-11 Thread Theo Buehler
On Thu, Aug 11, 2022 at 01:07:53PM -0700, enh wrote:
> is there a CVE or PoC for the zlib bug? it seems like OpenBSD is the
> only place where this has been fixed, and none of the various
> upstreams/forks of zlib (of which there are far too many!) seem to
> have this?

Details are here: https://marc.info/?l=oss-security=166000850502312=2

As mentioned in https://www.cve.org/CVERecord?id=CVE-2022-37434,
this overflow is only reachable if a caller previously called
inflateGetHeader() since otherwise state->head == Z_NULL.

According to codesearch.debian.org, very few things actually call this,
but it's exposed in various language bindings, so it seemed reasonable
to fix this in -stable.



rpki-client: replace unchecked X509_dup() with X509_up_ref()

2022-08-11 Thread Theo Buehler
There is no need to make a deep copy of the certificate that was
deserialized as part of the d2i_CMS_ContentInfo() call. We can as
well keep a reference to it.

While X509_dup() can actually fail, X509_up_ref() can't. It seems
cleaner to have a check.

Index: cms.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
retrieving revision 1.20
diff -u -p -r1.20 cms.c
--- cms.c   31 May 2022 18:41:43 -  1.20
+++ cms.c   10 Aug 2022 14:33:02 -
@@ -222,7 +222,11 @@ cms_parse_validate(X509 **xp, const char
"want 1 signer, have %d", fn, sk_X509_num(certs));
goto out;
}
-   *xp = X509_dup(sk_X509_value(certs, 0));
+   *xp = sk_X509_value(certs, 0);
+   if (!X509_up_ref(*xp)) {
+   *xp = NULL;
+   goto out;
+   }
 
/* Cache X509v3 extensions, see X509_check_ca(3). */
if (X509_check_purpose(*xp, -1, -1) <= 0) {



Re: rpki-client: disallow inherit in ROA EE IP Resources extension

2022-08-10 Thread Theo Buehler
On Wed, Aug 10, 2022 at 03:10:19PM +, Job Snijders wrote:
> Hi all,
> 
> An errata exists for RFC 6482, which informs us: """The EE certificate
> MUST NOT use "inherit" elements as described in [RFC3779].""" Read the
> full report here: https://www.rfc-editor.org/errata/eid3166
> 
> Although it might seem a bit 'wasteful' to d2i the IP Resources
> extension in multiple places, noodling through parameters when to check
> for inheritance and when not to check didn't improve code readability.
> I'm open to suggestions how to perform this check differently.

As I understand it, what really is missing isn't a check for inheritance
per se, but rather a check whether the prefixes in the ROA are covered
by the EE cert's IP address delegation extension (the bullet point in
RFC 6482, section 4). If we had such a check, that would be the natural
place for adding an inheritance check for the EE cert.

Below is my "overclaim" diff from a few weeks back that prepended the EE
cert to the auth chain for ROAs and RSCs so that we check their
resources against the EE cert instead of our currently incorrect checks
that permitted overclaiming. The diff was ok job and claudio told me
that it looked ok - I will need to think it through in detail once more,
however.

I believe that with something like this diff, your desired inheritance
check should be added to valid_roa() above the for() loop.

Does that make sense?

Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.84
diff -u -p -r1.84 cert.c
--- cert.c  31 May 2022 18:51:35 -  1.84
+++ cert.c  10 Aug 2022 15:40:25 -
@@ -467,23 +467,19 @@ sbgp_sia(struct parse *p, X509_EXTENSION
}
}
 
-   if (p->res->mft == NULL || p->res->repo == NULL) {
-   warnx("%s: RFC 6487 section 4.8.8: SIA: missing caRepository "
-   "or rpkiManifest", p->fn);
-   goto out;
-   }
-
-   if (strstr(p->res->mft, p->res->repo) != p->res->mft) {
-   warnx("%s: RFC 6487 section 4.8.8: SIA: "
-   "conflicting URIs for caRepository and rpkiManifest",
-   p->fn);
-   goto out;
-   }
+   if (p->res->mft != NULL && p->res->repo != NULL) {
+   if (strstr(p->res->mft, p->res->repo) != p->res->mft) {
+   warnx("%s: RFC 6487 section 4.8.8: SIA: conflicting "
+   "URIs for caRepository and rpkiManifest",
+   p->fn);
+   goto out;
+   }
 
-   if (rtype_from_file_extension(p->res->mft) != RTYPE_MFT) {
-   warnx("%s: RFC 6487 section 4.8.8: SIA: "
-   "not an MFT file", p->fn);
-   goto out;
+   if (rtype_from_file_extension(p->res->mft) != RTYPE_MFT) {
+   warnx("%s: RFC 6487 section 4.8.8: SIA: "
+   "not an MFT file", p->fn);
+   goto out;
+   }
}
 
rc = 1;
@@ -570,42 +566,20 @@ certificate_policies(struct parse *p, X5
return rc;
 }
 
-/*
- * Parse and partially validate an RPKI X509 certificate (either a trust
- * anchor or a certificate) as defined in RFC 6487.
- * Returns the parse results or NULL on failure.
- */
-struct cert *
-cert_parse_pre(const char *fn, const unsigned char *der, size_t len)
+static struct cert *
+cert_extract_x509(const char *fn, X509 *x)
 {
int  extsz;
-   int  sia_present = 0;
size_t   i;
-   X509*x = NULL;
X509_EXTENSION  *ext = NULL;
ASN1_OBJECT *obj;
struct parse p;
 
-   /* just fail for empty buffers, the warning was printed elsewhere */
-   if (der == NULL)
-   return NULL;
-
memset(, 0, sizeof(struct parse));
p.fn = fn;
if ((p.res = calloc(1, sizeof(struct cert))) == NULL)
err(1, NULL);
 
-   if ((x = d2i_X509(NULL, , len)) == NULL) {
-   cryptowarnx("%s: d2i_X509", p.fn);
-   goto out;
-   }
-
-   /* Cache X509v3 extensions, see X509_check_ca(3). */
-   if (X509_check_purpose(x, -1, -1) <= 0) {
-   cryptowarnx("%s: could not cache X509v3 extensions", p.fn);
-   goto out;
-   }
-
/* Look for X509v3 extensions. */
 
if ((extsz = X509_get_ext_count(x)) < 0)
@@ -627,7 +601,6 @@ cert_parse_pre(const char *fn, const uns
goto out;
break;
case NID_sinfo_access:
-   sia_present = 1;
if (!sbgp_sia(, ext))
goto out;
break;
@@ -647,12 +620,6 @@ cert_parse_pre(const char *fn, const uns
case NID_ext_key_usage:
break;
   

Re: rpki-client: tighten ROA parsing by forbidding AS Resources extension on the EE

2022-08-10 Thread Theo Buehler
On Wed, Aug 10, 2022 at 01:58:14PM +, Job Snijders wrote:
> Hi,
> 
> The ROA specification (RFC 6482 § 4) is a bit underspecified, but in the
> wild the RFC 3779 AS Resources extension never ever appears on ROA EE
> certificates, as it serves no purpose in the validation process. I've
> seen it happen once, in the past, which was a CA mistake.
> 
> Related reading material in the 3779 space:
> 
> The BGPSec profile (RFC 8209 § 3.1.3.4) is better in this regard: it
> explicitly forbids NID_sbgp_ipAddrBlock from being present (rpki-client
> checks this), and the upcoming ASPA RFC will also be less ambigious,
> ASPA forbids NID_sbgp_ipAddrBlock too (my WIP ASPA code checks this).
> 
> OK?

I'm fine with adding such a check. See below

> 
> Kind regards,
> 
> Job
> 
> Index: roa.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 roa.c
> --- roa.c 10 Jun 2022 10:36:43 -  1.47
> +++ roa.c 10 Aug 2022 13:49:58 -
> @@ -229,6 +229,12 @@ roa_parse(X509 **x509, const char *fn, c
>   goto out;
>   }
>  
> + if (X509_get_ext_d2i(*x509, NID_sbgp_autonomousSysNum, NULL, NULL)
> + != NULL) {

The pointer returned by X509_get_ext_d2i() needs to be freed, so this
would leak. You can check presence of an extension without allocating as
follows:

if (X509_get_ext_by_NID(*x509, NID_sbgp_autonomousSysNum, -1) != -1) {


> + warnx("%s: superfluous AS Resources extension present", fn);
> + goto out;
> + }
> +
>   at = X509_get0_notAfter(*x509);
>   if (at == NULL) {
>   warnx("%s: X509_get0_notAfter failed", fn);
> 



bgpd: inverted NULL check in krVPN6_change()

2022-08-10 Thread Theo Buehler
The below matches the VPN4 code and makes more sense given that we deref
kr6 in the else block.

Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.287
diff -u -p -U5 -r1.287 kroute.c
--- kroute.c3 Aug 2022 08:16:05 -   1.287
+++ kroute.c10 Aug 2022 13:56:28 -
@@ -619,11 +619,11 @@ krVPN6_change(struct ktable *kt, struct 
/* for blackhole and reject routes nexthop needs to be ::1 */
if (kf->flags & (F_BLACKHOLE|F_REJECT))
bcopy(, >nexthop.v6, sizeof(kf->nexthop.v6));
 
if ((kr6 = kroute6_find(kt, >prefix, kf->prefixlen,
-   kf->priority)) != NULL) {
+   kf->priority)) == NULL) {
if (kroute_insert(kt, kf) == -1)
return (-1);
} else {
kr6->mplslabel = mplslabel;
kr6->ifindex = kf->ifindex;



Re: bgpd more nexthop cleanup

2022-08-10 Thread Theo Buehler
On Wed, Aug 10, 2022 at 02:54:58PM +0200, Claudio Jeker wrote:
> This is more of what I just did in other places. Use direct assignment
> instead of memcpy(), remove double bzero() calls, switch to memset()
> and order struct kroute_nexthop in a more sensible way.

ok

> There should be no behaviour change from all this.

Agreed.



Re: bgpd fix bgpctl show network

2022-08-10 Thread Theo Buehler
On Wed, Aug 10, 2022 at 11:59:30AM +0200, Claudio Jeker wrote:
> When introducing prefix_nhvalid(p) the code in network_dump_upcall()
> was not correctly adjusted:
> 
> Before:
>   if (prefix_nexthop(p) == NULL ||
>   prefix_nexthop(p)->state != NEXTHOP_REACH)
>   kf.nexthop.aid = kf.prefix.aid;
>   else
>   kf.nexthop = prefix_nexthop(p)->true_nexthop;
> 
> Now:
>   if (prefix_nhvalid(p))
>   kf.nexthop.aid = kf.prefix.aid;
>   else
>   kf.nexthop = prefix_nexthop(p)->true_nexthop;
> 
> What it should be:
> 
>   if (prefix_nhvalid(p) &&  prefix_nexthop(p) != NULL)
>   kf.nexthop = prefix_nexthop(p)->exit_nexthop;
>   else
>   kf.nexthop.aid = kf.prefix.aid;
> 
> If the nexthop is valid we want to show it but in most cases the nexthop
> of announced networks is NULL so make sure we don't dereference NULL.
> Also I think it makes more sense to show the exit_nexthop (which is what
> is shown in show rib output by default). It is also what will be sent to
> the peers.
> 
> While there adjust the code to be a bit more logical and modern.

This all makes sense and is much nicer this way.

ok

> -- 
> :wq Claudio
> 
> Index: rde.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.560
> diff -u -p -r1.560 rde.c
> --- rde.c 28 Jul 2022 13:11:50 -  1.560
> +++ rde.c 10 Aug 2022 09:56:51 -
> @@ -4247,14 +4247,13 @@ network_dump_upcall(struct rib_entry *re
>   continue;
>   pt_getaddr(p->pt, );
>  
> - bzero(, sizeof(kf));
> - memcpy(, , sizeof(kf.prefix));
> - if (prefix_nhvalid(p))
> - kf.nexthop.aid = kf.prefix.aid;
> - else
> - memcpy(, _nexthop(p)->true_nexthop,
> - sizeof(kf.nexthop));
> + memset(, 0, sizeof(kf));
> + kf.prefix = addr;
>   kf.prefixlen = p->pt->prefixlen;
> + if (prefix_nhvalid(p) && prefix_nexthop(p) != NULL)
> + kf.nexthop = prefix_nexthop(p)->exit_nexthop;
> + else
> + kf.nexthop.aid = kf.prefix.aid;
>   if ((asp->flags & F_ANN_DYNAMIC) == 0)
>   kf.flags = F_STATIC;
>   if (imsg_compose(ibuf_se_ctl, IMSG_CTL_SHOW_NETWORK, 0,
> 



Re: fix bgpctl show network header

2022-08-10 Thread Theo Buehler
On Wed, Aug 10, 2022 at 12:17:29PM +0200, Claudio Jeker wrote:
> bgpctl show network uses the same data handler as bgpctl show fib.
> I increased the space between destination and gateway for IPv6 for the
> latter but forgot to adjust the former.
> 
> Before:
> flags: S = Static
> flags prio destination  gateway
> S0 10.2.3.0/24  0.0.0.0
> 
> After:
> flags: S = Static
> flags prio destination  gateway 
> S0 10.2.3.0/24  0.0.0.0

ok

> 
> -- 
> :wq Claudio
> 
> Index: output.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 output.c
> --- output.c  28 Jul 2022 10:40:25 -  1.25
> +++ output.c  10 Aug 2022 10:11:35 -
> @@ -46,8 +46,7 @@ show_head(struct parse_result *res)
>   break;
>   case SHOW_FIB:
>   printf("flags: B = BGP, C = Connected, S = Static\n");
> - printf("   "
> - "N = BGP Nexthop reachable via this route\n");
> + printf("   N = BGP Nexthop reachable via this route\n");
>   printf("   r = reject route, b = blackhole route\n\n");
>   printf("%-5s %-4s %-32s %-32s\n", "flags", "prio",
>   "destination", "gateway");
> @@ -83,7 +82,8 @@ show_head(struct parse_result *res)
>   break;
>   case NETWORK_SHOW:
>   printf("flags: S = Static\n");
> - printf("flags prio destination  gateway\n");
> + printf("%-5s %-4s %-32s %-32s\n", "flags", "prio",
> + "destination", "gateway");
>   break;
>   default:
>   break;
> 



ctfdump: fix a compiler warning or two

2022-08-09 Thread Theo Buehler
The revert of zlib.h r1.7 led to one or two compiler warnings in
ctfdump, depending on the architecture:

/usr/src/usr.bin/ctfdump/ctfdump.c:704:7: warning: format specifies type 
'unsigned long long' but the argument has type 'uLong' (aka 'unsigned long') 
[-Wformat]
stream.total_out, len);
^~~~

This one has an obvious fix. The format specifier for len is also wrong,
but -Wformat doesn't care.

/usr/src/usr.bin/ctfdump/ctfdump.c:702:23: warning: comparison of integers of 
different signs: 'uLong' (aka 'unsigned long') and 'off_t' (aka 'long long') 
[-Wsign-compare]
if (stream.total_out != len) {
 ^  ~~~

This one a bit less so.

z_off_t (aka 'long long') was changed to uLong (aka 'unsigned long'), so
it changed from signed to unsigned.

uLong will have to be converted to off_t due to the integer conversion
rank of 'long long' being higher than the one of long. This conversion
is possible on our ILP32 architectures. On our LP64 architectures, the
conversion of values > LONG_MAX is a priori implementation-defined.

So I think the correct fix is to cast them both to an unsigned type
wider than both of them. I chose uintmax_t since that cast is already
used in this file. The len < 0 check is only there for stylistic
reasons. As a sum of two uint32_t, len can't be < 0.

A similar fix will be needed in db_ctf_decompress() in sys/ddb/db_ctf.c
once we revert zlib.h r1.7 in the kernel.

Index: ctfdump.c
===
RCS file: /cvs/src/usr.bin/ctfdump/ctfdump.c,v
retrieving revision 1.25
diff -u -p -r1.25 ctfdump.c
--- ctfdump.c   10 Feb 2022 23:40:09 -  1.25
+++ ctfdump.c   9 Aug 2022 17:55:43 -
@@ -699,8 +699,8 @@ decompress(const char *buf, size_t size,
goto exit;
}
 
-   if (stream.total_out != len) {
-   warnx("decompression failed: %llu != %llu",
+   if (len < 0 || (uintmax_t)stream.total_out != (uintmax_t)len) {
+   warnx("decompression failed: %lu != %lld",
stream.total_out, len);
goto exit;
}



Re: random(6): undefined cast and error checking

2022-08-05 Thread Theo Buehler
On Fri, Aug 05, 2022 at 02:12:35PM -0500, luci...@bronze.ctrl-c.club wrote:
> So this is the final verison of the patch solving the following
> problems:
> 
> >The program is broken in multiple ways: return value clamping, casting
> >from double to uint32_t, wrong error checking for putchar, lack of
> >warnings when compiling.
> 
> Now the program is more pedantic and complicated, but at least it does
> what is says in the man page.
> 
> Ok, deraadt, tb?

Pretty much. Below is my suggestion, based on yours.

> +CFLAGS=-Wall -Wconversion

I dropped these again. I don't think -Wconversion is helpful here. It
doesn't understand the semantics of arc4random_buf(), so it whines for
no good reason. I can be convinced to add -Wall.

I pulled Campbell's license and code up to the top, in particular, we
can avoid prototypes. I switched a few variables from unsigned to int
since that makes more sense to me. I dropped the error check to
fprintf().

I dealt with -e differently than in your diff: reject denominators < 1
since those makes no sense. Document that. If -e is given, make sure
denom is at most 256, this way arc4random_uniform() will return a value
between 0 and 255, which exit() will not truncate. The nature of -e is
that we can't signal an error via return value, so we had better
succeed.

Other than that, I think this is good to go in. Surely the manual could
be improved...

Index: random.6
===
RCS file: /cvs/src/games/random/random.6,v
retrieving revision 1.7
diff -u -p -r1.7 random.6
--- random.631 May 2007 19:19:18 -  1.7
+++ random.65 Aug 2022 20:24:45 -
@@ -43,9 +43,10 @@
 .Nm
 reads lines from the standard input and copies them to the standard
 output with a probability of 1/denominator.
-The default value for
+The
 .Ar denominator
-is 2.
+must be at least 1,
+its default value is 2.
 .Pp
 The options are as follows:
 .Bl -tag -width Ds
@@ -55,7 +56,7 @@ If the
 option is specified,
 .Nm
 does not read or write anything, and simply exits with a random
-exit value of 0 to
+exit value of 0 to the minimum of 255 and
 .Ar denominator
 \&- 1, inclusive.
 .It Fl r
Index: random.c
===
RCS file: /cvs/src/games/random/random.c,v
retrieving revision 1.20
diff -u -p -r1.20 random.c
--- random.c7 Mar 2016 12:07:56 -   1.20
+++ random.c5 Aug 2022 20:30:53 -
@@ -33,8 +33,35 @@
  * SUCH DAMAGE.
  */
 
+/*-
+ * Copyright (c) 2014 Taylor R. Campbell
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -42,6 +69,101 @@
 __dead void usage(void);
 
 int
+clz64(uint64_t x)
+{
+   static const uint64_t mask[] = {
+   0x, 0x, 0xff00, 0xf0, 0xc, 0x2,
+   };
+   static const int zeroes[] = {
+   32, 16, 8, 4, 2, 1,
+   };
+   int clz = 0;
+   int i;
+
+   if (x == 0)
+   return 64;
+
+   for (i = 0; i < 6; i++) {
+   if ((x & mask[i]) == 0)
+   clz += zeroes[i];
+   else
+   x >>= zeroes[i];
+   }
+
+   return clz;
+}
+
+uint64_t
+random64(void)
+{
+   uint64_t r;
+
+   arc4random_buf(, sizeof(uint64_t));
+
+   return r;
+}
+
+/*
+ * random_real: Generate a stream of bits uniformly at random and
+ * interpret it as the fractional part of the binary expansion of a
+ * number in [0, 1], 0.101001010100...; then round it.
+ */
+double
+random_real(void)
+{
+   int exponent = -64;
+   uint64_t significand;
+   int shift;
+
+   /*
+

Re: remove games from PATHs provided by /etc/skel

2022-08-04 Thread Theo Buehler
On Fri, Aug 05, 2022 at 03:34:57AM +0200, Theo Buehler wrote:
> If you want games, opt into it. They are very old, full of bugs and not
> really maintained. It's very easy to get a PATH containing games via
> /etc/skel. I think this is a poor default.

Dropped a } by accident.

Index: dot.cshrc
===
RCS file: /cvs/src/etc/skel/dot.cshrc,v
retrieving revision 1.10
diff -u -p -r1.10 dot.cshrc
--- dot.cshrc   24 Jan 2020 02:09:51 -  1.10
+++ dot.cshrc   5 Aug 2022 01:37:02 -
@@ -13,7 +13,7 @@ alias ll  ls -lsA
 alias tset 'set noglob histchars=""; eval `\tset -s \!*`; unset noglob 
histchars'
 alias zsuspend
 
-set path = (~/bin /bin /sbin 
/usr/{bin,sbin,X11R6/bin,local/bin,local/sbin,games})
+set path = (~/bin /bin /sbin /usr/{bin,sbin,X11R6/bin,local/bin,local/sbin})
 
 if ($?prompt) then
# An interactive shell -- set some stuff up
Index: dot.profile
===
RCS file: /cvs/src/etc/skel/dot.profile,v
retrieving revision 1.7
diff -u -p -r1.7 dot.profile
--- dot.profile 24 Jan 2020 02:09:51 -  1.7
+++ dot.profile 5 Aug 2022 01:30:52 -
@@ -2,5 +2,5 @@
 #
 # sh/ksh initialization
 
-PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin:/usr/games
+PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin
 export PATH HOME TERM



remove games from PATHs provided by /etc/skel

2022-08-04 Thread Theo Buehler
If you want games, opt into it. They are very old, full of bugs and not
really maintained. It's very easy to get a PATH containing games via
/etc/skel. I think this is a poor default.

Index: dot.cshrc
===
RCS file: /cvs/src/etc/skel/dot.cshrc,v
retrieving revision 1.10
diff -u -p -r1.10 dot.cshrc
--- dot.cshrc   24 Jan 2020 02:09:51 -  1.10
+++ dot.cshrc   5 Aug 2022 01:31:01 -
@@ -13,7 +13,7 @@ alias ll  ls -lsA
 alias tset 'set noglob histchars=""; eval `\tset -s \!*`; unset noglob 
histchars'
 alias zsuspend
 
-set path = (~/bin /bin /sbin 
/usr/{bin,sbin,X11R6/bin,local/bin,local/sbin,games})
+set path = (~/bin /bin /sbin /usr/{bin,sbin,X11R6/bin,local/bin,local/sbin)
 
 if ($?prompt) then
# An interactive shell -- set some stuff up
Index: dot.profile
===
RCS file: /cvs/src/etc/skel/dot.profile,v
retrieving revision 1.7
diff -u -p -r1.7 dot.profile
--- dot.profile 24 Jan 2020 02:09:51 -  1.7
+++ dot.profile 5 Aug 2022 01:30:52 -
@@ -2,5 +2,5 @@
 #
 # sh/ksh initialization
 
-PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin:/usr/games
+PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin
 export PATH HOME TERM



Re: random(6): undefined cast and error checking

2022-08-04 Thread Theo Buehler
On Thu, Aug 04, 2022 at 07:11:40PM -0600, Theo de Raadt wrote:
> And anyways, this directory is not in $PATH by default, so there is no
> risk.

Unless you create a user during install in which case /etc/skel will
give you a $PATH containing /usr/games via dot.profile (I assume for
csh users via dot.cshrc).



Re: random(6): undefined cast and error checking

2022-08-04 Thread Theo Buehler
On Wed, Aug 03, 2022 at 04:21:43PM -0500, luci...@bronze.ctrl-c.club wrote:
> >On Wed, Aug 03, 2022 at 08:26:20AM -0600, Theo de Raadt wrote:
> >> luci...@bronze.ctrl-c.club wrote:
> >> 
> >> > Another way to solve this problem would be to trim the numbers with
> >> > something like this: if (denom > UINT32_MAX) denom = UINT32_MAX.
> >> 
> >> And then document that the program returns incorrect results?
> 
> t...@theobuehler.org wrote:
> 
> >See this. There's also linked BSD-licensed code that should not be hard
> >to adapt.
> >
> >https://mumble.net/~campbell/2014/04/28/uniform-random-float?
> >
> >Then pick a line if the random number drawn is < 1/denom.
> 
> So now we have two options. (1) We use the trim trick and document
> the range of values that the denominator can be in. (2) We use the
> code linked by tb@ to generate uniform doubles. (see patch below)
> 
> (2) seems to work but I need to spend some more time to understand
> the algorithm behind it.
> 
> (1) seems more robust cosidering that the man page will also contain
> information about the range of denom.

Thanks.

I have no strong opinion. I'm fine with either approach. It's such a
silly program...

Since you've already done 90% of the work for (2), we can push it over
the line. This will need adding Taylor R. Campbell's license since a
significant chunk of code is included verbatim. Add the license below
the existing one.

As an aside, random -e has been completely broken (it's non-uniform)
since forever.  To fix -e, we should clamp denom to an integer between 1
and 256, otherwise the truncation of the exit exit code to an 8-bit int
introduces bias for numbers larger than 256 (that aren't powers of 2).

Restricting denom >= 1 would seem necessary for all modes of this
program given that 1/denominator is documented to be a probability.


The patch doesn't compile due to missing clz64. Instead of using a
__builtin (which I'm unsure if it's available on all our ancient gccs),
I think we can implement this naively. Something along these lines
should be good enough (only very lightly tested):

int
clz64(uint64_t x)
{
static const uint64_t mask[] = {
0x, 0x, 0xff00, 0xf0, 0xc, 0x2,
};
static int zeroes[] = {
32, 16, 8, 4, 2, 1,
};
int clz = 0;
int i;

if (x == 0)
return 64;

for (i = 0; i < 6; i++) {
if ((x & mask[i]) == 0)
clz += zeroes[i];
else
x >>= zeroes[i];
}

return clz;
}

Some more comments inline.

> 
> Index: random.c
> ===
> RCS file: /cvs/src/games/random/random.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 random.c
> --- random.c  7 Mar 2016 12:07:56 -   1.20
> +++ random.c  3 Aug 2022 20:28:08 -
> @@ -38,6 +38,75 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 

style: keep these in alphabetical order

> +
> +uint64_t random64(void)

style: tab should be a newline:

uint64_t
random64(void)

> +{
> + uint64_t r;

style: add empty line

> + arc4random_buf(, sizeof(uint64_t));

and here

> + return r;
> +}
> +
> +/*
> + * random_real: Generate a stream of bits uniformly at random and
> + * interpret it as the fractional part of the binary expansion of a
> + * number in [0, 1], 0.101001010100...; then round it.
> + */
> +double
> +random_real(void)
> +{
> + int exponent = -64;
> + uint64_t significand;
> + unsigned shift;
> +
> + /*
> +  * Read zeros into the exponent until we hit a one; the rest
> +  * will go into the significand.
> +  */
> + while (__predict_false((significand = random64()) == 0)) {
> + exponent -= 64;
> + /*
> +  * If the exponent falls below -1074 = emin + 1 - p,
> +  * the exponent of the smallest subnormal, we are
> +  * guaranteed the result will be rounded to zero.  This
> +  * case is so unlikely it will happen in realistic
> +  * terms only if random64 is broken.
> +  */
> + if (__predict_false(exponent < -1074))
> + return 0;
> + }
> +
> + /*
> +  * There is a 1 somewhere in significand, not necessarily in
> +  * the most significant position.  If there are leading zeros,
> +  * shift them into the exponent and refill the less-significant
> +  * bits of the significand.  Can't predict one way or another
> +  * whether there are leading zeros: there's a fifty-fifty
> +  * chance, if random64 is uniformly distributed.
> +  */
> + shift = clz64(significand);
> + if (shift != 0) {
> + exponent -= shift;
> + significand <<= shift;
> + significand |= (random64() >> (64 - shift));
> + }
> +
> + /*
> +  * Set the 

Re: rpki-client unveil main process

2022-08-04 Thread Theo Buehler
On Thu, Aug 04, 2022 at 12:11:45PM +0200, Claudio Jeker wrote:
> This diff adds unveil to the main process. This is done after all files
> from the command line have been read. Both for regular and -f mode.
> Once the args have been read the process can limit the access to the
> cachedir and the output dir. In -f mode only read access to the cachdir is
> required. In regular both cachedir and outputdir need rwc rights.


> 
> -- 
> :wq Claudio
> 
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.208
> diff -u -p -r1.208 main.c
> --- main.c27 Jun 2022 10:18:27 -  1.208
> +++ main.c28 Jul 2022 16:57:16 -
> @@ -1006,8 +1006,7 @@ main(int argc, char *argv[])
>   signal(SIGALRM, suicide);
>   }
>  
> - /* TODO unveil cachedir and outputdir, no other access allowed */
> - if (pledge("stdio rpath wpath cpath fattr sendfd", NULL) == -1)
> + if (pledge("stdio rpath wpath cpath fattr sendfd unveil", NULL) == -1)
>   err(1, "pledge");
>  
>   msgbuf_init();
> @@ -1049,6 +1048,18 @@ main(int argc, char *argv[])
>   while (*argv != NULL)
>   queue_add_file(*argv++, RTYPE_FILE, 0);
>   }

This brace ends an if (filemode) block. I'm wondering if this would not
be cleaner:

if (filemode) {
while (*argv != NULL)
queue_add_file(*argv++, RTYPE_FILE, 0);

if (unveil(cachedir, "r") == -1)
err(1, "unveil cachedir");
} else {
if (unveil(outputdir, "rwc") == -1)
err(1, "unveil outputdir");
if (unveil(cachedir, "rwc") == -1)
err(1, "unveil cachedir");
}
if (unveil(NULL, NULL) == -1)
err(1, "unveil");

Either way ok

> +
> + /* from here on only cachedir and outputdir are accessed */
> + if (!filemode) {
> + if (unveil(outputdir, "rwc") == -1)
> + err(1, "unveil outputdir");
> + if (unveil(cachedir, "rwc") == -1)
> + err(1, "unveil cachedir");
> + } else
> + if (unveil(cachedir, "r") == -1)
> + err(1, "unveil cachedir");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
>  
>   /* change working directory to the cache directory */
>   if (fchdir(cachefd) == -1)
> 



Re: random(6): undefined cast and error checking

2022-08-03 Thread Theo Buehler
On Wed, Aug 03, 2022 at 08:26:20AM -0600, Theo de Raadt wrote:
> luci...@bronze.ctrl-c.club wrote:
> 
> > Another way to solve this problem would be to trim the numbers with
> > something like this: if (denom > UINT32_MAX) denom = UINT32_MAX.
> 
> And then document that the program returns incorrect results?
> 
> > >However, using drand48() will mean using a floating point modulus.
> > >We lose the uniform aspect.  I'm not the non-uniform aspects are as
> > >visible in the floating point range.  Succeeding for the full floating
> > >point range is more important than what arc4random_uniform() is trying
> > >to do.  But maybe a uniform version of the double code can grow out of
> > >using the drand48.c code?
> > 
> > Theo, I'm not sure I understand your reasoning. Are you trying to say
> > that we should see if it's possible to create a drand48_uniform? 
> 
> Yes, inside the program.
> 

See this. There's also linked BSD-licensed code that should not be hard
to adapt.

https://mumble.net/~campbell/2014/04/28/uniform-random-float?

Then pick a line if the random number drawn is < 1/denom.



Re: bgpd force fib sync in fetchtable

2022-08-02 Thread Theo Buehler
On Tue, Aug 02, 2022 at 12:34:40PM +0200, Claudio Jeker wrote:
> On startup we load the routing table in bgpd and at that moment a cleanup
> of old bgpd routes should happen. I noticed this is not the case because
> fib_sync is not set and so send_rtmsg() just returns.
> I think we need to force fib_sync in fetchtable() to make sure the cleanup
> happens correctly.

If I understand correctly, this ignores the fs flag of ktable_new() for
the fetchtable() call. In particular, 'fib-update no' is ignored in this
situation. Is that intentional?

> 
> OK?
> -- 
> :wq Claudio
> 
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.285
> diff -u -p -r1.285 kroute.c
> --- kroute.c  28 Jul 2022 14:05:13 -  1.285
> +++ kroute.c  2 Aug 2022 10:13:59 -
> @@ -2726,6 +2726,7 @@ fetchtable(struct ktable *kt)
>   char*buf = NULL, *next, *lim;
>   struct rt_msghdr*rtm;
>   struct kroute_full   kf;
> + int  fib_sync;
>  
>   mib[0] = CTL_NET;
>   mib[1] = PF_ROUTE;
> @@ -2754,6 +2755,10 @@ fetchtable(struct ktable *kt)
>   }
>   }
>  
> + /* force fib_sync on during fetch */
> + fib_sync = kt->fib_sync;
> + kt->fib_sync = 1;
> +
>   lim = buf + len;
>   for (next = buf; next < lim; next += rtm->rtm_msglen) {
>   rtm = (struct rt_msghdr *)next;
> @@ -2768,6 +2773,8 @@ fetchtable(struct ktable *kt)
>   else
>   kroute_insert(kt, );
>   }
> + kt->fib_sync = fib_sync;
> +
>   free(buf);
>   return (0);
>  }
> 



Re: bgpd more kroute refactor

2022-07-28 Thread Theo Buehler
On Thu, Jul 28, 2022 at 12:48:05PM +0200, Claudio Jeker wrote:
> Next step on the epic saga of cleaning up kroute.c
> 
> Refactor kroute_remove() so that a struct kroute_full can be passed to the
> function. It updates the struct kroute_full with the route that got removed.
> 
> I split the code into kroute[46]_remove() to make kroute_remove() less
> cluttered. The return value of those two functions is a bit overloaded but
> I hope to make this better further down the road.

That's fine for now. An obvious suggestion would be to use an out parameter for
multipath. Let kroute[46]_remove() return -1, 0 (where -1 and 0 have the same
meaning as -2 and -1 in your diff), otherwise return 1. Then you can do:

case AID_INET:
ret = kroute4_remove(kt, kf, any, );
if (ret <= 0)
return ret;
break;

Diff looks good, some comments inline.

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.283
> diff -u -p -r1.283 kroute.c
> --- kroute.c  27 Jul 2022 17:23:17 -  1.283
> +++ kroute.c  28 Jul 2022 09:20:50 -
> @@ -123,10 +123,6 @@ int  kr4_change(struct ktable *, struct k
>  int  kr6_change(struct ktable *, struct kroute_full *);
>  int  krVPN4_change(struct ktable *, struct kroute_full *);
>  int  krVPN6_change(struct ktable *, struct kroute_full *);
> -int  kr4_delete(struct ktable *, struct kroute_full *);
> -int  kr6_delete(struct ktable *, struct kroute_full *);
> -int  krVPN4_delete(struct ktable *, struct kroute_full *);
> -int  krVPN6_delete(struct ktable *, struct kroute_full *);
>  int  kr_net_match(struct ktable *, struct network_config *, uint16_t, int);
>  struct network *kr_net_find(struct ktable *, struct network *);
>  void kr_net_clear(struct ktable *);
> @@ -144,18 +140,17 @@ struct kroute   *kroute_find(struct ktable
>   uint8_t, uint8_t);
>  struct kroute*kroute_matchgw(struct kroute *, struct bgpd_addr *);
>  int   kroute_insert(struct ktable *, struct kroute_full *);
> -int   kroute_remove(struct ktable *, struct kroute *);
> +int   kroute_remove(struct ktable *, struct kroute_full *, int);
>  void  kroute_clear(struct ktable *);
>  
>  struct kroute6   *kroute6_find(struct ktable *, const struct bgpd_addr *,
>   uint8_t, uint8_t);
>  struct kroute6   *kroute6_matchgw(struct kroute6 *, struct bgpd_addr *);
> -int   kroute6_remove(struct ktable *, struct kroute6 *);
>  void  kroute6_clear(struct ktable *);
>  
>  struct knexthop  *knexthop_find(struct ktable *, struct bgpd_addr *);
>  int   knexthop_insert(struct ktable *, struct knexthop *);
> -int   knexthop_remove(struct ktable *, struct knexthop *);
> +void  knexthop_remove(struct ktable *, struct knexthop *);
>  void  knexthop_clear(struct ktable *);
>  
>  struct kif_node  *kif_find(int);
> @@ -662,18 +657,7 @@ kr_delete(u_int rtableid, struct kroute_
>   return (0);
>   kf->flags |= F_BGPD;
>   kf->priority = RTP_MINE;
> - switch (kf->prefix.aid) {
> - case AID_INET:
> - return (kr4_delete(kt, kf));
> - case AID_INET6:
> - return (kr6_delete(kt, kf));
> - case AID_VPN_IPv4:
> - return (krVPN4_delete(kt, kf));
> - case AID_VPN_IPv6:
> - return (krVPN6_delete(kt, kf));
> - }
> - log_warnx("%s: not handled AID", __func__);
> - return (-1);
> + return kroute_remove(kt, kf, 1);
>  }
>  
>  int
> @@ -689,18 +673,12 @@ kr_flush(u_int rtableid)
>  
>   RB_FOREACH_SAFE(kr, kroute_tree, >krt, next)
>   if ((kr->flags & F_BGPD_INSERTED)) {
> - if (kt->fib_sync)   /* coupled */
> - send_rtmsg(kr_state.fd, RTM_DELETE, kt,
> - kr_tofull(kr));
> - if (kroute_remove(kt, kr) == -1)
> + if (kroute_remove(kt, kr_tofull(kr), 1) == -1)
>   return (-1);
>   }
>   RB_FOREACH_SAFE(kr6, kroute6_tree, >krt6, next6)
>   if ((kr6->flags & F_BGPD_INSERTED)) {
> - if (kt->fib_sync)   /* coupled */
> - send_rtmsg(kr_state.fd, RTM_DELETE, kt,
> - kr6_tofull(kr6));
> - if (kroute6_remove(kt, kr6) == -1)
> + if (kroute_remove(kt, kr6_tofull(kr6), 1) == -1)
>   return (-1);
>   }
>  
> @@ -708,86 +686,6 @@ kr_flush(u_int rtableid)
>   return (0);
>  }
>  
> -int
> -kr4_delete(struct ktable *kt, struct kroute_full *kf)
> -{
> - struct kroute   *kr;
> -
> - if ((kr = kroute_find(kt, >prefix, kf->prefixlen,
> - 

Re: bgpd remove F_DOWN flag

2022-07-28 Thread Theo Buehler
On Thu, Jul 28, 2022 at 12:30:01PM +0200, Claudio Jeker wrote:
> When the bgpctl show fib diff is committed nothing uses F_DOWN anymore.
> Remove the flag and reshuffle some of the other flags to group them
> a bit better.

ok



Re: adjust bgpctl show fib formatting

2022-07-28 Thread Theo Buehler
On Thu, Jul 28, 2022 at 12:22:24PM +0200, Claudio Jeker wrote:
> This adjusts the output of bgpctl show fib. It removes the F_DOWN check
> since kroutes no longer track this. And it changes the flag printing code
> to reserve the space needed so that adjusting the flags does not break the
> output. Last but not least increase the size of destination and gateway to
> 32bytes so that more IPv6 addrs fit.

I like it.

ok tb

F_DOWN could now be removed from bgpd.h



Re: bgpd another try at F_KERNEL removal

2022-07-27 Thread Theo Buehler
On Wed, Jul 27, 2022 at 05:41:11PM +0200, Claudio Jeker wrote:
> My last try was not successful because kr_tofull() did not return RTP_MINE
> and so some checks for RTP_MINE instead of F_KERNEL did not work.
> 
> This diff does two things. It replaces the F_KERNEL checks with !F_BGPD
> checks. F_KERNEL and F_BGPD are mutual exclusive.
> On top of that keep kr->priority at RTP_MINE and introduce kr_priority()
> which translates the priority back to a real prio number.
> kr_priority() is mainly used for the bgpctl imsg commands.

Why do you say mainly? I can't see it used for anything else.

> In dispatch_rtmsg_addr() do the inverse and translate back to RTP_MINE
> also force the right flags and priority in kr_change and kr_delete.

This diff reads fine to me and I agree that it fixes the problem the
previous version had.

ok tb



Re: bgpd simplify kroute nexthop handling

2022-07-26 Thread Theo Buehler
On Tue, Jul 26, 2022 at 07:04:36PM +0200, Claudio Jeker wrote:
> This is what I have in mind for that.

Yep, same here.

ok

> 
> -- 
> :wq Claudio
> 
> ? obj
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.281
> diff -u -p -r1.281 kroute.c
> --- kroute.c  26 Jul 2022 16:36:33 -  1.281
> +++ kroute.c  26 Jul 2022 16:37:17 -
> @@ -2060,11 +2060,7 @@ kif_remove(struct kif_node *kif)
>   if ((kt = ktable_get(kif->k.rdomain)) != NULL)
>   knexthop_track(kt, kif->k.ifindex);
>  
> - if (RB_REMOVE(kif_tree, , kif) == NULL) {
> - log_warnx("RB_REMOVE(kif_tree, , kif)");
> - return (-1);
> - }
> -
> + RB_REMOVE(kif_tree, , kif);
>   free(kif);
>   return (0);
>  }
> @@ -2549,7 +2545,8 @@ if_announce(void *msg)
>   break;
>   case IFAN_DEPARTURE:
>   kif = kif_find(ifan->ifan_index);
> - kif_remove(kif);
> + if (kif != NULL)
> + kif_remove(kif);
>   break;
>   }
>  }



Re: bgpd simplify kroute nexthop handling

2022-07-26 Thread Theo Buehler
On Tue, Jul 26, 2022 at 05:17:23PM +0200, Claudio Jeker wrote:
> On Tue, Jul 26, 2022 at 03:51:40PM +0200, Theo Buehler wrote:
> > On Tue, Jul 26, 2022 at 03:09:37PM +0200, Claudio Jeker wrote:
> > > This is another step in the epic kroute rework.
> > > 
> > > Interfaces (kif) come with a list of kroutes attached to them which are
> > > only used to track the interface state and to fiddle with nexthop states.
> > > Now these lists are not really needed. One can just validate the nexthops
> > > without losing any relevant information.
> > > 
> > > Struct knexthop gains a ifindex member which is used to track link state
> > > changes. As a benefit gateway routes will also pick up state changes now
> > > (connected routes already did that).
> > > 
> > > With this F_DOWN becomes unused and I will remove this in another step.
> > 
> > This generally reads fine. One question:
> > 
> > > @@ -2097,31 +2048,23 @@ int
> > >  kif_remove(struct kif_node *kif)
> > >  {
> > >   struct ktable   *kt;
> > > - struct kif_kr   *kkr;
> > > - struct kif_kr6  *kkr6;
> > > +
> > > + kif->flags &= ~IFF_UP;
> > > +
> > > + /*
> > > +  * TODO, remove all kroutes using this interface,
> > > +  * the kernel does this for us but better to do it
> > > +  * here as well.
> > > +  */
> > > +
> > > + if ((kt = ktable_get(kif->k.rdomain)) != NULL)
> > > + knexthop_track(kt, kif->k.ifindex);
> > >  
> > >   if (RB_REMOVE(kif_tree, , kif) == NULL) {
> > 
> > If I'm not mistaken, the only way RB_REMOVE() can return NULL is when
> > the third argument is NULL. So this check used to act as a NULL check
> > before dereferencing kif. This diff adds two derefs earlier, which will
> > now crash.
> > 
> > Should kif_remove() in if_announce() only be called with non-NULL kif?
> 
> I have never seen the log_warnx. So I assume it can not really happen.
> Maybe remove the if check and just adjust if_announce instead.
> I'll have a look and send another diff for that.

Fine with me.

ok

>  
> > >   log_warnx("RB_REMOVE(kif_tree, , kif)");
> > >   return (-1);
> > >   }
>  
> 
> -- 
> :wq Claudio



Re: bgpd simplify kroute nexthop handling

2022-07-26 Thread Theo Buehler
On Tue, Jul 26, 2022 at 03:09:37PM +0200, Claudio Jeker wrote:
> This is another step in the epic kroute rework.
> 
> Interfaces (kif) come with a list of kroutes attached to them which are
> only used to track the interface state and to fiddle with nexthop states.
> Now these lists are not really needed. One can just validate the nexthops
> without losing any relevant information.
> 
> Struct knexthop gains a ifindex member which is used to track link state
> changes. As a benefit gateway routes will also pick up state changes now
> (connected routes already did that).
> 
> With this F_DOWN becomes unused and I will remove this in another step.

This generally reads fine. One question:

> @@ -2097,31 +2048,23 @@ int
>  kif_remove(struct kif_node *kif)
>  {
>   struct ktable   *kt;
> - struct kif_kr   *kkr;
> - struct kif_kr6  *kkr6;
> +
> + kif->flags &= ~IFF_UP;
> +
> + /*
> +  * TODO, remove all kroutes using this interface,
> +  * the kernel does this for us but better to do it
> +  * here as well.
> +  */
> +
> + if ((kt = ktable_get(kif->k.rdomain)) != NULL)
> + knexthop_track(kt, kif->k.ifindex);
>  
>   if (RB_REMOVE(kif_tree, , kif) == NULL) {

If I'm not mistaken, the only way RB_REMOVE() can return NULL is when
the third argument is NULL. So this check used to act as a NULL check
before dereferencing kif. This diff adds two derefs earlier, which will
now crash.

Should kif_remove() in if_announce() only be called with non-NULL kif?

>   log_warnx("RB_REMOVE(kif_tree, , kif)");
>   return (-1);
>   }



Re: bgpd: fix nexthop state bug in decision process

2022-07-25 Thread Theo Buehler
On Mon, Jul 25, 2022 at 12:16:50PM +0200, Claudio Jeker wrote:
> The nexthop validation or actually invalidation is buggy in bgpd since
> revision 1.90 of rde_decide.c. When I removed re->active and replaced it
> with a value that is calculated on the spot I did not realize that this
> calculation depends on the current nexthop state and not on the state used
> on the previous decision process.
> 
> The decision process used prefix_best() to get the previous best prefix.
> Now if that prefix just became invalid (because of a nexthop state change)
> prefix_best() will return NULL and oldbest and newbest will be NULL too.
> Because of this no update will be sent out because prefix_evaluate()
> thinks nothing changed.
> 
> This is also the reason why prefix_set_dmetric() sees bad dmetrics during
> nexthop flaps.
> 
> To solve this issue I decided to introduce a NEXTHOP_VALID flag in struct
> prefix (as part of the nhflags). Which is then used to validate the
> nexthop state. When a nexthop changes state nexthop_runner calls
> prefix_evaluate_nexthop() which adjust the NEXTHOP_VALID flag but this is
> done after removing the prefix from the evaluation list and before
> inserting it again. Because of this prefix_best() works correctly and
> oldbest is not NULL in the case mentioned above.
> 
> I decided to set the initial state of NEXTHOP_VALID in nexthop_link() and I
> clear it in nexthop_unlink(). The link call happens after nhflags is set
> and prefix_nhflags() masks out the NEXTHOP_VALID flag.
> 
> With this nexthops that turn invalid are properly removed from the FIB and
> Adj-RIB-Out.

Wow, that's nasty. The diff makes sense and looks correct to me.

ok tb



Re: bgpd nexthop check

2022-07-23 Thread Theo Buehler
On Sat, Jul 23, 2022 at 11:14:35AM +0200, Claudio Jeker wrote:
> Change the logic and name of bgpd_filternexthop(). This function applies
> the 'nexthop qualify via' config setting. Instead of telling if the route
> is filtered (true) or not (false) flip the logic around and rename the
> function to bgpd_oknexthop(). Also flip the internal logic around to
> simplify the logic in bgpd_oknexthop().
> 
> Adjust the kroute_match() code accordingly (which makes that code easier
> to understand).
> 
> This is just a refactor and should not change behaviour.
> -- 
> :wq Claudio
> 
> Index: bgpd.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
> retrieving revision 1.251
> diff -u -p -r1.251 bgpd.c
> --- bgpd.c22 Jul 2022 17:26:58 -  1.251
> +++ bgpd.c23 Jul 2022 08:53:58 -
> @@ -1115,23 +1115,27 @@ send_network(int type, struct network_co
>   return (0);
>  }
>  
> +/*
> + * Return true if a route can be used for nexthop resolution.
> + */
>  int
> -bgpd_filternexthop(struct kroute_full *kf)
> +bgpd_oknexthop(struct kroute_full *kf)
>  {
> - /* kernel routes are never filtered */
> - if (kf->flags & F_KERNEL && kf->prefixlen != 0)
> - return (0);
> -
> - if (cflags & BGPD_FLAG_NEXTHOP_BGP) {
> - if (kf->flags & F_BGPD)
> + if (kf->flags & F_BGPD) {
> + if (cflags & BGPD_FLAG_NEXTHOP_BGP)
> + return (1);
> + else
>   return (0);
>   }
>  
> - if (cflags & BGPD_FLAG_NEXTHOP_DEFAULT) {
> - if (kf->prefixlen == 0)
> + if (kf->prefixlen == 0) {
> + if (cflags & BGPD_FLAG_NEXTHOP_DEFAULT)
> + return (1);
> + else
>   return (0);
>   }

Your version is ok. I'd do away with the inner ifs and write this as:

if (kf->flags & F_BGPD)
return ((cflags & BGPD_FLAG_NEXTHOP_BGP) != 0);

if (kf->prefixlen == 0)
return ((cflags & BGPD_FLAG_NEXTHOP_DEFAULT) != 0);

>  
> + /* any other route is fine */
>   return (1);
>  }
>  
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.444
> diff -u -p -r1.444 bgpd.h
> --- bgpd.h22 Jul 2022 17:26:58 -  1.444
> +++ bgpd.h23 Jul 2022 08:54:09 -
> @@ -1265,7 +1265,7 @@ void send_nexthop_update(struct kroute
>  void  send_imsg_session(int, pid_t, void *, uint16_t);
>  int   send_network(int, struct network_config *,
>struct filter_set_head *);
> -int   bgpd_filternexthop(struct kroute_full *);
> +int   bgpd_oknexthop(struct kroute_full *);
>  void  set_pollfd(struct pollfd *, struct imsgbuf *);
>  int   handle_pollfd(struct pollfd *, struct imsgbuf *);
>  
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.278
> diff -u -p -r1.278 kroute.c
> --- kroute.c  22 Jul 2022 17:26:58 -  1.278
> +++ kroute.c  23 Jul 2022 08:55:27 -
> @@ -2444,7 +2444,7 @@ knexthop_send_update(struct knexthop *kn
>  }
>  
>  struct kroute *
> -kroute_match(struct ktable *kt, struct bgpd_addr *key, int matchall)
> +kroute_match(struct ktable *kt, struct bgpd_addr *key, int matchany)
>  {
>   int  i;
>   struct kroute   *kr;
> @@ -2453,8 +2453,7 @@ kroute_match(struct ktable *kt, struct b
>   for (i = 32; i >= 0; i--) {
>   applymask(, key, i);
>   if ((kr = kroute_find(kt, , i, RTP_ANY)) != NULL)
> - if (matchall ||
> - bgpd_filternexthop(kr_tofull(kr)) == 0)
> + if (matchany || bgpd_oknexthop(kr_tofull(kr)))
>   return (kr);
>   }
>  
> @@ -2462,7 +2461,7 @@ kroute_match(struct ktable *kt, struct b
>  }
>  
>  struct kroute6 *
> -kroute6_match(struct ktable *kt, struct bgpd_addr *key, int matchall)
> +kroute6_match(struct ktable *kt, struct bgpd_addr *key, int matchany)
>  {
>   int  i;
>   struct kroute6  *kr6;
> @@ -2471,8 +2470,7 @@ kroute6_match(struct ktable *kt, struct 
>   for (i = 128; i >= 0; i--) {
>   applymask(, key, i);
>   if ((kr6 = kroute6_find(kt, , i, RTP_ANY)) != NULL)
> - if (matchall ||
> - bgpd_filternexthop(kr6_tofull(kr6)) == 0)
> + if (matchany || bgpd_oknexthop(kr6_tofull(kr6)))
>   return (kr6);
>   }
>  
> 



Re: bgpd kroute F_KERNEL flag

2022-07-22 Thread Theo Buehler
On Fri, Jul 22, 2022 at 12:36:04PM +0200, Claudio Jeker wrote:
> There is no need to use F_KERNEL to tag routes from the kernel.
> All this can be done by priority (RTP_MINE vs anything else).
> The conversion is simple in most cases.
> 
> In kr_fib_delete() and kr_fib_change() check if the route is a bgpd owned
> route and in that case remove the F_BGPD_INSERTED flag. The original route
> is no longer in the FIB and so that flag should be cleared.

Makes sense and reads fine.

ok



Re: bgpd, relax setting rde evaluate all and add-path send

2022-07-21 Thread Theo Buehler
On Thu, Jul 21, 2022 at 01:00:19PM +0200, Claudio Jeker wrote:
> rde evaluate all and add-path send do not really work together.
> add-path will evaluate extra paths (if plus is used) and so it implies a
> mode of `rde evaluate all`. I added the exclusion mainly to make it clear
> that the two don't really mix.
> 
> After a request from Pier Carlo Chiodi on OpenBGPD-portable I came to the
> conclusion that it should be possible to set both on a peer (in case the
> peer has no add-path then use `rde evaluate all` as fallback).
> 
> It is enough to update manpage and remove the check in neighbor_consistent()

ok with me.

> -- 
> :wq Claudio
> 
> Index: bgpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
> retrieving revision 1.225
> diff -u -p -r1.225 bgpd.conf.5
> --- bgpd.conf.5   12 Jul 2022 17:49:33 -  1.225
> +++ bgpd.conf.5   21 Jul 2022 10:52:54 -
> @@ -893,6 +893,11 @@ and
>  are equivalent.
>  The default is
>  .Ic no .
> +If
> +.Ic add-path Ic send
> +is active then the setting of
> +.Ic rde Ic evaluate
> +is ignored.
>  .Pp
>  .It Xo
>  .Ic announce as-4byte
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.432
> diff -u -p -r1.432 parse.y
> --- parse.y   11 Jul 2022 17:08:21 -  1.432
> +++ parse.y   21 Jul 2022 10:46:23 -
> @@ -4665,14 +4665,6 @@ neighbor_consistent(struct peer *p)
>   return (-1);
>   }
>  
> - /* bail if add-path send and rde evaluate all is used together */
> - if ((p->conf.flags & PEERFLAG_EVALUATE_ALL) &&
> - (p->conf.capabilities.add_path[0] & CAPA_AP_SEND)) {
> - yyerror("neighbors with add-path send cannot use "
> - "'rde evaluate all'");
> - return (-1);
> - }
> -
>   return (0);
>  }
>  
> 



Re: bgpd, refactor kroute_insert

2022-07-21 Thread Theo Buehler
On Thu, Jul 21, 2022 at 11:10:41AM +0200, Claudio Jeker wrote:
> On Wed, Jul 20, 2022 at 12:28:25PM +0200, Claudio Jeker wrote:
> > On Wed, Jul 20, 2022 at 10:56:29AM +0200, Claudio Jeker wrote:
> > > This diff moves kroute_insert to use struct kroute_full and do the
> > > allocation for struct kroute / kroute6 inside kroute_insert. This removes
> > > a lot of similar code all over kroute.c. While doing that also convert
> > > kr_redistribute() to use struct kroute_full and kill the code duplication
> > > there as well.
> > > 
> > > A lot more to clean up but that seems like a resonable step right now.
> > 
> > This diff is not ready yet. I missed that kr/kr6 are used after
> > send_rtmsg(). Need to fix that first.
> 
> This version works now. I moved send_rtmsg() into kroute_insert() which is
> not ideal but for no it allows us to move forward.

This looks good now.  In kr_redistribute, the scope_id is not copied in
the AID_INET6 case. I assume that's not an issue since we won't try to
redistribute link-local addresses.

Other than that, the only very important remark that I have is that
kr4_change() and kroute_insert() now have two empty lines before the
last return. I'd drop at least one of those.

ok



Re: bgpd, network code cleanup

2022-07-20 Thread Theo Buehler
On Wed, Jul 20, 2022 at 12:40:18PM +0200, Claudio Jeker wrote:
> The it's just a rtlabel refcount leak diff turned into a somewhat larger
> diff. First I noticed that expand_networks() was not used for l3vpns which
> will cause problems down the line. So alter expand_networks to also handle
> l3vpn network settings.
> 
> Then I looked at kr_net_reload() and kr_net_find() and noticed that
> neither rtlabel nor priority network statements are handled properly. So
> make sure we match the network statements also against rtlabel and
> priority.
> 
> I named network_free() after filterset_free() and not free_network() which
> is used in config.c. One day I may go an make the naming consistent.

This looks much better.

ok tb

> -- 
> :wq Claudio
> 
> Index: bgpd.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
> retrieving revision 1.248
> diff -u -p -r1.248 bgpd.c
> --- bgpd.c23 Jun 2022 13:09:03 -  1.248
> +++ bgpd.c20 Jul 2022 09:50:45 -
> @@ -599,7 +599,9 @@ send_config(struct bgpd_config *conf)
>  
>   reconfpending = 3;  /* one per child */
>  
> - expand_networks(conf);
> + expand_networks(conf, >networks);
> + SIMPLEQ_FOREACH(vpn, >l3vpns, entry)
> + expand_networks(conf, >net_l);
>  
>   cflags = conf->flags;
>  
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.441
> diff -u -p -r1.441 bgpd.h
> --- bgpd.h11 Jul 2022 17:08:21 -  1.441
> +++ bgpd.h20 Jul 2022 10:02:17 -
> @@ -476,8 +476,8 @@ struct network_config {
>   struct filter_set_head   attrset;
>   char psname[SET_NAME_LEN];
>   uint64_t rd;
> - uint16_t rtlabel;
>   enum network_typetype;
> + uint16_t rtlabel;
>   uint8_t  prefixlen;
>   uint8_t  priority;
>   uint8_t  old;   /* used for reloading */
> @@ -1275,6 +1275,7 @@ int control_imsg_relay(struct imsg *);
>  /* config.c */
>  struct bgpd_config   *new_config(void);
>  void copy_config(struct bgpd_config *, struct bgpd_config *);
> +void network_free(struct network *);
>  void free_l3vpns(struct l3vpn_head *);
>  void free_config(struct bgpd_config *);
>  void free_prefixsets(struct prefixset_head *);
> @@ -1285,7 +1286,7 @@ voidfree_rtrs(struct rtr_config_head *
>  void filterlist_free(struct filter_head *);
>  int  host(const char *, struct bgpd_addr *, uint8_t *);
>  uint32_t get_bgpid(void);
> -void expand_networks(struct bgpd_config *);
> +void expand_networks(struct bgpd_config *, struct network_head *);
>  RB_PROTOTYPE(prefixset_tree, prefixset_item, entry, prefixset_cmp);
>  int  roa_cmp(struct roa *, struct roa *);
>  RB_PROTOTYPE(roa_tree, roa, entry, roa_cmp);
> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/config.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 config.c
> --- config.c  5 Jun 2022 12:43:13 -   1.102
> +++ config.c  20 Jul 2022 09:50:16 -
> @@ -86,14 +86,21 @@ copy_config(struct bgpd_config *to, stru
>  }
>  
>  void
> +network_free(struct network *n)
> +{
> + rtlabel_unref(n->net.rtlabel);
> + filterset_free(>net.attrset);
> + free(n);
> +}
> +
> +void
>  free_networks(struct network_head *networks)
>  {
>   struct network  *n;
>  
>   while ((n = TAILQ_FIRST(networks)) != NULL) {
>   TAILQ_REMOVE(networks, n, entry);
> - filterset_free(>net.attrset);
> - free(n);
> + network_free(n);
>   }
>  }
>  
> @@ -431,7 +438,8 @@ host_ip(const char *s, struct bgpd_addr 
>   sa2addr(res->ai_addr, h, NULL);
>   freeaddrinfo(res);
>   } else {/* ie. for 10/8 parsing */
> - if ((bits = inet_net_pton(AF_INET, s, >v4, sizeof(h->v4))) 
> == -1)
> + if ((bits = inet_net_pton(AF_INET, s, >v4,
> + sizeof(h->v4))) == -1)
>   return (0);
>   *len = bits;
>   h->aid = AID_INET;
> @@ -502,10 +510,9 @@ prepare_listeners(struct bgpd_config *co
>  }
>  
>  void
> -expand_networks(struct bgpd_config *c)
> +expand_networks(struct bgpd_config *c, struct network_head *nw)
>  {
>   struct network  *n, *m, *tmp;
> - struct network_head *nw = >networks;
>   struct prefixset*ps;
>   struct prefixset_item   *psi;
>  
> @@ -527,8 +534,7 @@ expand_networks(struct bgpd_config *c)
>   >net.attrset);
>   TAILQ_INSERT_TAIL(nw, m, entry);
>   }
> - filterset_free(>net.attrset);
> - 

Re: bgpd, plug rtlabel refcount leak on network statements

2022-07-20 Thread Theo Buehler
On Wed, Jul 20, 2022 at 10:35:10AM +0200, Claudio Jeker wrote:
> Found while working on kroute code. The network structs needs to release
> the rtlabel reference before being freed.

Don't expand_networks() and kr_net_delete() need this as well?

> 
> -- 
> :wq Claudio
> 
> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/config.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 config.c
> --- config.c  5 Jun 2022 12:43:13 -   1.102
> +++ config.c  19 Jul 2022 16:52:40 -
> @@ -92,6 +92,7 @@ free_networks(struct network_head *netwo
>  
>   while ((n = TAILQ_FIRST(networks)) != NULL) {
>   TAILQ_REMOVE(networks, n, entry);
> + rtlabel_unref(n->net.rtlabel);
>   filterset_free(>net.attrset);
>   free(n);
>   }
> 



Re: bgpd aspath_extract overflow check

2022-07-19 Thread Theo Buehler
On Tue, Jul 19, 2022 at 11:43:25AM +0200, Claudio Jeker wrote:
> aspath_extract() should do at least a minimal overflow check and not
> access memory after the segment. Can't use fatalx here because bgpctl
> also uses this function. Instead return 0, that is an invalid ASN.
> No code will check the return value but that is fine since all callers
> ensure that pos does not overflow.

There are a few calls of the form

as = aspath_extract(seg, seg_len - 1)

where seg_len = ptr[1], so a priori pos == -1 seems possible. Should the
check not be

if (pos < 0 || pos >= ptr[1])
return 0;

to exclude an access at ptr[-2]?

> 
> -- 
> :wq Claudio
> 
> Index: util.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 util.c
> --- util.c28 Jun 2022 05:49:05 -  1.69
> +++ util.c28 Jun 2022 08:31:10 -
> @@ -364,7 +364,7 @@ aspath_strlen(void *data, uint16_t len)
>  /*
>   * Extract the asnum out of the as segment at the specified position.
>   * Direct access is not possible because of non-aligned reads.
> - * ATTENTION: no bounds checks are done.
> + * Only works on verified 4-byte AS paths.
>   */
>  uint32_t
>  aspath_extract(const void *seg, int pos)
> @@ -372,6 +372,9 @@ aspath_extract(const void *seg, int pos)
>   const u_char*ptr = seg;
>   uint32_t as;
>  
> + /* minimal overflow check, return 0 since that is an invalid ASN */
> + if (pos >= ptr[1])
> + return (0);
>   ptr += 2 + sizeof(uint32_t) * pos;
>   memcpy(, ptr, sizeof(uint32_t));
>   return (ntohl(as));
> 



Re: bgpd name struct kroute_full vars kf

2022-07-19 Thread Theo Buehler
On Tue, Jul 19, 2022 at 12:08:40PM +0200, Claudio Jeker wrote:
> Use kf for all struct kroute_full variables in bgpd. This makes the code
> more consistent.

ok



Re: bgpd less chatter on rde exit

2022-07-18 Thread Theo Buehler
On Mon, Jul 18, 2022 at 03:46:05PM +0200, Claudio Jeker wrote:
> Noticed the other day, when the RDE dies the session engine may log the
> "Can't send message %u to RDE, ctl pipe closed" multiple times because
> the queue is still processed.
> 
> Since this error only happens after a "SE: Lost connection to RDE" error
> it does not anything to the crash log. This is why this should just be
> ignored.

ok

> 
> -- 
> :wq Claudio
> 
> Index: session.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.430
> diff -u -p -r1.430 session.c
> --- session.c 27 Jun 2022 13:26:51 -  1.430
> +++ session.c 13 Jul 2022 14:48:47 -
> @@ -3506,11 +3506,9 @@ imsg_ctl_parent(int type, uint32_t peeri
>  int
>  imsg_ctl_rde(int type, pid_t pid, void *data, uint16_t datalen)
>  {
> - if (ibuf_rde_ctl == NULL) {
> - log_warnx("Can't send message %u to RDE, ctl pipe closed",
> - type);
> + if (ibuf_rde_ctl == NULL)
>   return (0);
> - }
> +
>   /*
>* Use control socket to talk to RDE to bypass the queue of the
>* regular imsg socket.
> @@ -3521,10 +3519,8 @@ imsg_ctl_rde(int type, pid_t pid, void *
>  int
>  imsg_rde(int type, uint32_t peerid, void *data, uint16_t datalen)
>  {
> - if (ibuf_rde == NULL) {
> - log_warnx("Can't send message %u to RDE, pipe closed", type);
> + if (ibuf_rde == NULL)
>   return (0);
> - }
>  
>   return (imsg_compose(ibuf_rde, type, peerid, 0, -1, data, datalen));
>  }
> 



Re: bgpd decision process and bad dmetric

2022-07-16 Thread Theo Buehler
On Sat, Jul 16, 2022 at 12:41:07PM +0200, Claudio Jeker wrote:
> I deployed bgpd on one of more core routers and triggered the fatal
> "bad dmetric in decision process" from time to time.
> 
> I realized after a longer debugging session that one reason this happens
> is when nexthops become valid. The state change affects all prefixes at
> once but then they are reevaluated one by one (see prefix_evaluate_all()
> which is called by nexthop_runner()).
> 
> I currently have no good solution for this issue. I think the problem is
> that invalid prefixes are not sorted when added. There may be a similar
> issue when flipping a rib from no-evaluate to evaluate in the reload code.
> 
> For now neuter the fatalx and convert it to a log_debug() until I figured
> out a proper fix.

ok.

Now that the scope_id is part of struct bgpd_addr, the XXX in
pt_getaddr() can go.

> -- 
> :wq Claudio
> 
> Index: rde_decide.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
> retrieving revision 1.95
> diff -u -p -r1.95 rde_decide.c
> --- rde_decide.c  11 Jul 2022 16:46:41 -  1.95
> +++ rde_decide.c  16 Jul 2022 10:28:19 -
> @@ -331,8 +331,12 @@ prefix_set_dmetric(struct prefix *pp, st
>   PREFIX_DMETRIC_BEST : PREFIX_DMETRIC_INVALID;
>   else
>   np->dmetric = prefix_cmp(pp, np, );
> - if (np->dmetric < 0)
> - fatalx("bad dmetric in decision process");
> + if (np->dmetric < 0) {
> + struct bgpd_addr addr;
> + pt_getaddr(np->pt, );
> + log_debug("bad dmetric in decision process: %s/%u",
> + log_addr(), np->pt->prefixlen);
> + }
>   }
>  }
>  
> 



Re: Patch for 71.html

2022-07-15 Thread Theo Buehler
> Removes two extraneous ` (backtick) characters.
> Applies quoting around URLs used as the values of href attributes.
> (the / (slash) character needs to be quoted in attributes)

Committed, thanks. I don't think the / needs to be quoted, but I took
that part anyway for consistency with the rest of the page.

>From 
>https://html.spec.whatwg.org/multipage/introduction.html#intro-early-example

  The attribute value can remain unquoted if it doesn't contain ASCII
  whitespace or any of " ' ` = < or >.



Re: bgpd more IPv6 scope_id love

2022-07-14 Thread Theo Buehler
On Thu, Jul 14, 2022 at 12:23:26PM +0200, Claudio Jeker wrote:
> Noticed while syncing code. I missed some scope_id assignments and checks
> in a few places.
> 
> - VPN6 missed all of it
> - in kr_redistribute6() the copy is not really needed since link local
>   address can not be redistributed (I still added it though)
> - kroute6_compare() do actually compare the scope_id of a prefix.
>   This should make handling of link local addrs a bit more robust.
> - in kroute6_find() set the scope_id else kroute6_compare() will match
>   against stack garbage.
> - in kroute6_matchgw() we need to compare the scope_id as well
> - in kr_fib_change() compare nexthop with scope_id to know if it changed
>   and update the scope_id as well afterwards.
> 
> Works for me

This looks good and I can't find any other missed instances in kroute.c

ok tb

> @@ -3524,9 +3534,13 @@ add4:
>   if (kl->nexthop.aid == AID_INET6) {
>   if (memcmp(>nexthop,
>   >nexthop.v6,
> - sizeof(struct in6_addr)))
> + sizeof(struct in6_addr)) ||
> + kr6->nexthop_scope_id != 

trailing space after !=



Re: bgpd: add add-path send support

2022-07-11 Thread Theo Buehler
On Fri, Jul 08, 2022 at 06:43:17PM +0200, Claudio Jeker wrote:
> Add the missing bits for add-path send support.
> The config options allows for a fair amount of configuration and not all
> have been tested:
>   announce add-path send best [ plus X ]
>   announce add-path send ecmp [ plus X ] [ max Y ]
>   announce add-path send as-wide-best [ plus X ] [ max Y ]
>   announce add-path send all
> 
> Right now ECMP and as-wide-best are the same. This is because of missing
> nexthop metrics. The heavy lifting is done by up_generate_addpath()
> and is based on the same trick as 'rde evaluate all'. This is not optimal
> but it should work and the code is somewhat easy to follow.
> Also the reload behaviour is a bit special. You may want to tune the
> settings on an active session but removing the option should keep
> the last settings until session reset. Still need to think about this
> a bit more.
> 
> A few bits like the minor cleanup in rde_decide.c can be committed
> independently.

Please do.

> 
> If add-path send is not set then the system should behave as before.
> Which is the important bit of this.

The diff reads fine to me and I agree that without setting add-path send
nothing changes. I have a couple of nits inline, but I'm fine with the
diff as it is.

ok tb

> -- 
> :wq Claudio
> 
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.440
> diff -u -p -r1.440 bgpd.h
> --- bgpd.h7 Jul 2022 12:16:04 -   1.440
> +++ bgpd.h8 Jul 2022 12:32:49 -
> @@ -307,6 +307,20 @@ struct bgpd_config {
>  
>  extern int cmd_opts;
>  
> +enum addpath_mode {
> + ADDPATH_EVAL_NONE,
> + ADDPATH_EVAL_BEST,
> + ADDPATH_EVAL_ECMP,
> + ADDPATH_EVAL_AS_WIDE,
> + ADDPATH_EVAL_ALL,
> +};
> +
> +struct addpath_eval {
> + enum addpath_mode   mode;
> + int extrapaths;
> + int maxpaths;
> +};
> +
>  enum export_type {
>   EXPORT_UNSET,
>   EXPORT_NONE,
> @@ -402,6 +416,7 @@ struct peer_config {
>   struct bgpd_addr local_addr_v6;
>   struct peer_auth auth;
>   struct capabilities  capabilities;
> + struct addpath_eval  eval;
>   char group[PEER_DESCR_LEN];
>   char descr[PEER_DESCR_LEN];
>   char reason[REASON_LEN];
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.431
> diff -u -p -r1.431 parse.y
> --- parse.y   27 Jun 2022 13:26:51 -  1.431
> +++ parse.y   8 Jul 2022 12:41:23 -
> @@ -210,7 +210,7 @@ typedef struct {
>  %token   EBGP IBGP
>  %token   LOCALAS REMOTEAS DESCR LOCALADDR MULTIHOP PASSIVE MAXPREFIX 
> RESTART
>  %token   ANNOUNCE CAPABILITIES REFRESH AS4BYTE CONNECTRETRY ENHANCED 
> ADDPATH
> -%token   SEND RECV POLICY
> +%token   SEND RECV PLUS POLICY
>  %token   DEMOTE ENFORCE NEIGHBORAS ASOVERRIDE REFLECTOR DEPEND DOWN
>  %token   DUMP IN OUT SOCKET RESTRICTED
>  %token   LOG TRANSPARENT
> @@ -230,12 +230,13 @@ typedef struct {
>  %token   IPSEC ESP AH SPI IKE
>  %token   IPV4 IPV6
>  %token   QUALIFY VIA
> -%token   NE LE GE XRANGE LONGER MAXLEN
> +%token   NE LE GE XRANGE LONGER MAXLEN MAX
>  %token STRING
>  %token NUMBER
>  %type  asnumber as4number as4number_any 
> optnumber
>  %type  espah family safi restart origincode 
> nettype
>  %type  yesno inout restricted validity expires 
> enforce
> +%type  addpathopt addpathmax

Maybe addpathextra would match the 'extra' used everywhere else better than
addpathopt. Or even addextrapaths and addmaxpaths.

>  %type  string
>  %typeaddress
>  %type  prefix addrspec
> @@ -1356,6 +1357,28 @@ groupopts_l: /* empty */
>   | groupopts_l error '\n'
>   ;
>  
> +addpathopt   : /* empty */   { $$ = 0;   }
> + | PLUS NUMBER   {
> + if ($2 < 1 || $2 > USHRT_MAX) {
> + yyerror("additional paths must be between "
> + "%u and %u", 1, USHRT_MAX);
> + YYERROR;
> + }
> + $$ = $2;
> + }
> + ;
> +
> +addpathmax   : /* empty */   { $$ = 0;   }
> + | MAX NUMBER{
> + if ($2 < 1 || $2 > USHRT_MAX) {
> + yyerror("maximum additional paths must be "
> + "between %u and %u", 1, USHRT_MAX);
> + YYERROR;
> + }
> + 

Re: bgpctl adjust AID loops a bit

2022-07-08 Thread Theo Buehler
On Fri, Jul 08, 2022 at 05:04:05PM +0200, Claudio Jeker wrote:
> I noticed that some of the loops over all AID (address identifiers) start
> at 0 but they should use AID_MIN. This is not a general rule and I
> actually skipped one of the for loops because I think there 0 is actually
> better. In the cases I fixed it is known that only known AID and not
> AID_UNSPEC are allowed.
> 
> This fixes a visual glitch when using add-path.

Should the for loop on line 87 of output_json.c (after 'if (present)'
not start at AID_MIN, too?

ok



Re: bgpd: initial local path_id support for Adj-RIB-Out

2022-07-08 Thread Theo Buehler
On Fri, Jul 08, 2022 at 11:17:16AM +0200, Claudio Jeker wrote:
> This diff adds the required plumbing to support local path_ids in the
> output path. Mainly it extends prefix_adjout_update() to do the right
> thing. Since in normal mode of operation path_id_tx does not matter and
> only on prefix is in the Adj-RIB-Out the code uses prefix_adjout_lookup()
> to locate the prefix first and removes the lookup in
> prefix_adjout_update().
> 
> prefix_adjout_update() takes care if the passed in prefix switches
> path_id_tx and does the dance of unlinking the object from all RB trees
> before switching the path_id_tx.
> 
> In up_generate_default() just use 0 as the path_id_tx. There will only
> be the default route in this RIB so there is no conflict possible.

This all makes sense and reads perfectly fine.

ok tb



Re: bgpd: assign local path_id to all prefixes

2022-07-07 Thread Theo Buehler
On Thu, Jul 07, 2022 at 05:27:58PM +0200, Claudio Jeker wrote:
> This diff is assigning a local path_id to all prefixes. This path_id will
> be used for sending out add-path updates. Since the RFC specifies that the
> path_id has no meaning we assing the path_ids randomly. They just need to
> be unique per rib entry. Now this code assigne the path_id in the
> Adj-RIB-In and then inherits the number to all other ribs. Currently the
> Adj-RIB-Out is excluded since that code is not yet ready for more than one
> path. That is the next step.

ok tb



Re: bgpd: refactor update generation a bit

2022-07-06 Thread Theo Buehler
On Wed, Jul 06, 2022 at 06:56:28PM +0200, Claudio Jeker wrote:
> On Wed, Jul 06, 2022 at 06:15:45PM +0200, Theo Buehler wrote:
> > On Wed, Jul 06, 2022 at 05:07:45PM +0200, Claudio Jeker wrote:
> > > This diff changes various loops which call into up_generate_update() so
> > > that all these loops call the same function peer_generate_update() which
> > > then calls up_generate_update(). This is a step to add an alternative path
> > > to generate updates for add-path send support without altering many
> > > code-paths.
> > > 
> > > If I did not fool myself this should not alter current behaviour.
> > > rde_up_dump_upcall() gets a few more checks but all those checks should be
> > > false in this case (e.g. peer_dump checks the export_type already).
> > 
> > This looks good and I agree that the behavior is mostly preserved. I
> > noticed are two changes of behavior and I have a small question for
> > rde_up_dump_upcall():
> > 
> > > @@ -317,20 +374,10 @@ rde_up_dump_upcall(struct rib_entry *re,
> > >   struct rde_peer *peer = ptr;
> > >   struct prefix   *p;
> > >  
> > > - if (peer->state != PEER_UP)
> > > - return;
> > > - if (re->rib_id != peer->loc_rib_id)
> > > - fatalx("%s: Unexpected RIB %u != %u.", __func__, re->rib_id,
> > > - peer->loc_rib_id);
> > 
> > Failure of this check will now be silent.
> 
> This is fine, that code path was acting as an assert().
> rde_up_dump_upcall() is called by a RIB table walk for the
> peer->loc_rib_id RIB (see peer_dump()) so there should be no way that
> if (re->rib_id != peer->loc_rib_id) is true.

Indeed.

>  
> > > - if (peer->capa.mp[re->prefix->aid] == 0)
> > > - fatalx("%s: Unexpected %s prefix", __func__,
> > > - aid2str(re->prefix->aid));
> > 
> > Same here.
> 
> Similar reason. The rib walks each address family independent:
> for (i = 0; i < AID_MAX; i++) {
> if (peer->capa.mp[i])
> peer_dump(peer, i);
> }
> 
> Now this check is missing in the IMSG_REFRESH case in rde.c but other than
> that there should be no way to reach this code with a wrong aid.
> Skipping routes with non-negotiated address families should not matter
> here but it will make the code more robust.
> 

Makes sense.

>  
> > I got lost when checking that re->prefix->aid == p->pt->aid. I assume
> > that follows from the definition of the rib_entry's prefix queue?
> 
> If the prefix p is on the list of prefixes in re (part of re->prefix_h)
> then re->prefix == p->pt and so the aid check also needs to match.
> We cache the struct pt_entry * in a few places to simplify access.
> Since the code uses prefix_best(re) to get the p we know that p is part of
> the re list of prefixes.

All these comments made things a lot clearer. Thanks

> 
> > > -
> > >   /* no eligible prefix, not even for 'evaluate all' */
> > >   if ((p = prefix_best(re)) == NULL)
> > >   return;
> > > -
> > > - up_generate_updates(out_rules, peer, p, NULL);
> > > + peer_generate_update(peer, re->rib_id, p, NULL, 0);
> > >  }
> > >  
> > >  static void
> 
> Updated diff below that adds the extra aid check in the IMSG_REFRESH case. 

ok tb


> 
> -- 
> :wq Claudio
> 
> Index: rde.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.547
> diff -u -p -r1.547 rde.c
> --- rde.c 27 Jun 2022 13:26:51 -  1.547
> +++ rde.c 6 Jul 2022 16:43:22 -
> @@ -1123,12 +1123,19 @@ rde_dispatch_imsg_peer(struct rde_peer *
>   break;
>   case IMSG_REFRESH:
>   if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(rr)) {
> - log_warnx("%s: wrong imsg len", __func__);
> + log_warnx("route refresh: wrong imsg len");
>   break;
>   }
>   memcpy(, imsg.data, sizeof(rr));
>   if (rr.aid >= AID_MAX) {
> - log_warnx("%s: bad AID", __func__);
> + log_peer_warnx(>conf,
> + "route refresh: bad AID %d", rr.aid);
> + break;
> + }
> + if (peer->capa.mp[rr.aid]) {
> + log_peer_warnx(>conf,
> + "route ref

Re: bgpd: refactor update generation a bit

2022-07-06 Thread Theo Buehler
On Wed, Jul 06, 2022 at 05:07:45PM +0200, Claudio Jeker wrote:
> This diff changes various loops which call into up_generate_update() so
> that all these loops call the same function peer_generate_update() which
> then calls up_generate_update(). This is a step to add an alternative path
> to generate updates for add-path send support without altering many
> code-paths.
> 
> If I did not fool myself this should not alter current behaviour.
> rde_up_dump_upcall() gets a few more checks but all those checks should be
> false in this case (e.g. peer_dump checks the export_type already).

This looks good and I agree that the behavior is mostly preserved. I
noticed are two changes of behavior and I have a small question for
rde_up_dump_upcall():

> @@ -317,20 +374,10 @@ rde_up_dump_upcall(struct rib_entry *re,
>   struct rde_peer *peer = ptr;
>   struct prefix   *p;
>  
> - if (peer->state != PEER_UP)
> - return;
> - if (re->rib_id != peer->loc_rib_id)
> - fatalx("%s: Unexpected RIB %u != %u.", __func__, re->rib_id,
> - peer->loc_rib_id);

Failure of this check will now be silent.

> - if (peer->capa.mp[re->prefix->aid] == 0)
> - fatalx("%s: Unexpected %s prefix", __func__,
> - aid2str(re->prefix->aid));

Same here.

I got lost when checking that re->prefix->aid == p->pt->aid. I assume
that follows from the definition of the rib_entry's prefix queue?

> -
>   /* no eligible prefix, not even for 'evaluate all' */
>   if ((p = prefix_best(re)) == NULL)
>   return;
> -
> - up_generate_updates(out_rules, peer, p, NULL);
> + peer_generate_update(peer, re->rib_id, p, NULL, 0);
>  }
>  
>  static void



Re: patch: xlock: unveil issue: login.conf

2022-07-06 Thread Theo Buehler
On Wed, Jul 06, 2022 at 09:13:29AM +0200, Sebastien Marie wrote:
> Hi,
> 
> I am currently debugging some unveil issues reported when process accounting 
> in 
> enabled (see acct(2)). 
> 
> xlock is currently doing some unveil(2) violation:
> 
> $ lastcomm | grep U
> xlock-FU semarie  __ 
> 0.00 secs Wed Jul  6 07:13 (1:21:00.00)
> 
> I tracked them to be related to "login.conf" access (due to auth_userokay(3) 
> usage).
> 
> The diff belows adds all "login.conf" related files to readable files by the 
> process:
> 
> - /etc/login.conf
> - /etc/login.conf.db
> - /etc/login.conf.d/*

This makes sense and matches what is done in base.

ok tb

> 
> 
> diff 7f83513b277728e78b173796466b04c2373f0b55 
> /home/semarie/repos/openbsd/xenocara
> blob - 7fdf4f11d18bdb1bab730f008ef7ea10e0e482ca
> file + app/xlockmore/xlock/privsep.c
> --- app/xlockmore/xlock/privsep.c
> +++ app/xlockmore/xlock/privsep.c
> @@ -255,8 +255,14 @@ priv_init(gid_t gid)
>  
>   imsg_init(_ibuf, socks[0]);
>  
> + if (unveil(_PATH_LOGIN_CONF, "r") == -1)
> + err(1, "unveil %s", _PATH_LOGIN_CONF);
> + if (unveil(_PATH_LOGIN_CONF ".db", "r") == -1)
> + err(1, "unveil %s.db", _PATH_LOGIN_CONF);
> + if (unveil(_PATH_LOGIN_CONF_D, "r") == -1)
> + err(1, "unveil %s", _PATH_LOGIN_CONF_D);
>   if (unveil(_PATH_AUTHPROGDIR, "rx") == -1)
> - err(1, "unveil");
> + err(1, "unveil %s", _PATH_AUTHPROGDIR);
>   if (pledge("stdio rpath getpw proc exec", NULL) == -1)
>   err(1, "pledge");
> 
> 
> With it, I don't have unveil(2) violation anymore when running xlock(1).
>  
> Comments or OK ?
> -- 
> Sebastien Marie
> 



Re: a few fixes for cat bugs

2022-07-01 Thread Theo Buehler
On Sat, Jul 02, 2022 at 03:15:21AM +0100, Leah Rowe wrote:
> 
> Hi,
> 
> I've been playing around with a few OpenBSD userland programs. By that,
> I mean I've been studying them extensively. I'm working on creating a
> busybox-like userland for Linux with musl libc, and I need quality code
> so I decided I'd start ripping code out of OpenBSD for this purpose.
> 
> I spent today obsessing over your cat implementation. As part of my
> optimization efforts (I've been removing non-POSIX features, because I
> need the code to be as small as possible), I found several minor issues
> in cat:
> 
> 1) Unitialized variables that are assumed to be zero, judging by the
> logic in the code that uses them.

They are not used uninitialized.

> 
> 2) Memory leak; in practise, most people will run can on a few files
> and it won't take more than a few moments, then cat terminates and the
> memory is freed

Not really a leak. The static buf will not be freed before exit, but
that's fine. Your diff introduces a pretty bad bug.

> 
> 3) Unnecessary check in raw_cat on a variable being NULL, when it will
> always evaluate true because it's explicitly already initialized to null

You are removing an optimization and fixing the bug you introduced in 2.

> 
> I fixed all of these, in the attached patches which I present for your
> enjoyment. I've done these on top of your github mirror, on top of
> commit 88e033f9985d9aec739c4497f51fb9348247d4db of the main OpenBSD git
> repository (I don't know how to use CVS, I've never used it, sorry! I
> got into programming in the late 2000s right when git was made lol)
> 
> -- 
> Leah Rowe,
> Company Director
> 
> Minifree Ltd, trading as Ministry of Freedom.
> Registered in England, registration No. 9361826
> VAT Registration No. GB202190462
> Minifree Ltd, 19 Hilton Road, Canvey Island
> Essex SS8 9QA, United Kingdom
> United Kingdom

> From 63163120b6881f9d8ea2b03f722cd31e946f5704 Mon Sep 17 00:00:00 2001
> From: Leah Rowe 
> Date: Sat, 2 Jul 2022 02:27:51 +0100
> Subject: [PATCH 1/3] cat: fix uninitialized variables
> 
> ---
>  bin/cat/cat.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/bin/cat/cat.c b/bin/cat/cat.c
> index a2eff2e1b30..42960812ead 100644
> --- a/bin/cat/cat.c
> +++ b/bin/cat/cat.c
> @@ -62,6 +62,8 @@ main(int argc, char *argv[])
>   if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
>  
> + rval = bflag = eflag = nflag = sflag = tflag = vflag = 0;

These are global variables, so they are initialized to 0.

> +
>   while ((ch = getopt(argc, argv, "benstuv")) != -1) {
>   switch (ch) {
>   case 'b':
> @@ -210,7 +212,7 @@ void
>  raw_cat(int rfd, const char *filename)
>  {
>   int wfd;
> - ssize_t nr, nw, off;
> + ssize_t nr, off, nw = 0;

for (off = 0; nr; nr -= nw, off += nw) {
if ((nw = write(wfd, buf + off, nr)) == -1 || nw == 0)
err(1, "stdout");
}

nw is only used after the body of the for loop was run once, so it isn't
used uninitialized.

>   static size_t bsize;
>   static char *buf = NULL;
>   struct stat sbuf;
> -- 
> 2.25.1
> 

> From e36d05353d66cb42d33f9b5d5eb3e4dc44175e39 Mon Sep 17 00:00:00 2001
> From: Leah Rowe 
> Date: Sat, 2 Jul 2022 02:28:33 +0100
> Subject: [PATCH 2/3] cat: fix memory leak
> 
> ---
>  bin/cat/cat.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/bin/cat/cat.c b/bin/cat/cat.c
> index 42960812ead..dad337d5f68 100644
> --- a/bin/cat/cat.c
> +++ b/bin/cat/cat.c
> @@ -235,4 +235,5 @@ raw_cat(int rfd, const char *filename)
>   warn("%s", filename);
>   rval = 1;
>   }
> + free(buf);

buf is a static variable. It is allocated once, namely the first time
raw_cat is called. This diff introduces a use-after-free:

$ obj/cat cat.c cat.1 >/dev/null
cat(35527) in free(): bogus pointer (double free?) 0xa5fc8145000
Abort trap (core dumped)

>  }
> -- 
> 2.25.1
> 

> From b4e0ef740724523ed3752c6f5b82741d1acc5230 Mon Sep 17 00:00:00 2001
> From: Leah Rowe 
> Date: Sat, 2 Jul 2022 02:29:20 +0100
> Subject: [PATCH 3/3] cat: remove unnecessary check
> 
> ---
>  bin/cat/cat.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/bin/cat/cat.c b/bin/cat/cat.c
> index dad337d5f68..97edcad2101 100644
> --- a/bin/cat/cat.c
> +++ b/bin/cat/cat.c
> @@ -218,13 +218,12 @@ raw_cat(int rfd, const char *filename)
>   struct stat sbuf;
>  
>   wfd = fileno(stdout);
> - if (buf == NULL) {
> - if (fstat(wfd, ) == -1)
> - err(1, "stdout");
> - bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ);
> - if ((buf = malloc(bsize)) == NULL)
> - err(1, NULL);
> - }
> + if (fstat(wfd, ) == -1)
> + err(1, "stdout");
> + bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ);
> + if ((buf = malloc(bsize)) == NULL)
> + 

Re: one send_rtmsg is enough for bgpd

2022-06-30 Thread Theo Buehler
On Thu, Jun 30, 2022 at 04:59:50PM +0200, Claudio Jeker wrote:
> Implement send_rtmsg() using kroute_full and just use one version of this
> magical code. I use struct sockaddr_storage for all sockaddrs added to
> ensure that there is a) enough space and b) that ROUNDUP() does not cause
> the system to pass uninitialized stack memory to the kernel.
> 
> I tested IPv4 and IPv6 but not yet the MPLS version.
> kroute_full now also carries the mplslabel which could be shown in bgpctl
> show fib output (if anyone wants to add that).
> 
> I renamed some struct kroute_full pointers from *kl to *kf. I want to use
> that everywhere in the end but step by step.

ok tb

No comments for once



Re: snmpd(8): Add blocklist feature

2022-06-30 Thread Theo Buehler
On Wed, Jun 29, 2022 at 01:35:37PM +0200, Martijn van Duren wrote:
> On Tue, 2022-06-28 at 12:33 +0200, Martijn van Duren wrote:
> > On Tue, 2022-06-28 at 12:21 +0200, Martijn van Duren wrote:
> > > On Tue, 2022-06-28 at 10:21 +0200, Martijn van Duren wrote:
> > > > Back in 2020 florian@ added the filter-pf-addresses keyword.
> > > > Although useful, I always felt it was a bit too case-specific. The diff
> > > > below adds a new blocklist feature/backend, which takes hold of an
> > > > entire subtree, effectively removing it from the tree.
> > > > 
> > > > With this I've deprecated the filter-pf-address case and should
> > > > probably be removed somewhere after 7.4. The filter-routes case can't
> > > > be removed unfortunately, since its behaviour is not identical, and
> > > > instead adds filters to the routing socket, preventing updates being
> > > > pushed to snmpd(8).
> > > > 
> > > > Feedback/OK?
> > > > 
> > > > martijn@
> > > > 
> > > Also clean up after ourselves if appl_exception fails.
> > > 
> > *sigh* Missed a return.
> > 
> And also some cleanup during shutdown.

Can't spot anything wrong in this one and works fine in some basic
testing.

ok tb

> 
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/Makefile,v
> retrieving revision 1.18
> diff -u -p -r1.18 Makefile
> --- Makefile  19 Jan 2022 11:00:56 -  1.18
> +++ Makefile  29 Jun 2022 11:35:23 -
> @@ -3,6 +3,7 @@
>  PROG=snmpd
>  MAN= snmpd.8 snmpd.conf.5
>  SRCS=parse.y log.c snmpe.c application.c 
> application_legacy.c \
> + application_blocklist.c \
>   mps.c trap.c mib.c smi.c kroute.c snmpd.c timer.c \
>   pf.c proc.c usm.c traphandler.c util.c
>  
> Index: application.c
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/application.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 application.c
> --- application.c 27 Jun 2022 10:31:17 -  1.5
> +++ application.c 29 Jun 2022 11:35:23 -
> @@ -148,6 +148,7 @@ RB_PROTOTYPE_STATIC(appl_requests, appl_
>  void
>  appl_init(void)
>  {
> + appl_blocklist_init();
>   appl_legacy_init();
>  }
>  
> @@ -156,6 +157,7 @@ appl_shutdown(void)
>  {
>   struct appl_context *ctx, *tctx;
>  
> + appl_blocklist_shutdown();
>   appl_legacy_shutdown();
>  
>   TAILQ_FOREACH_SAFE(ctx, , ac_entries, tctx) {
> Index: application.h
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/application.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 application.h
> --- application.h 19 Jan 2022 10:59:35 -  1.1
> +++ application.h 29 Jun 2022 11:35:23 -
> @@ -133,3 +133,7 @@ struct ber_element *appl_exception(enum 
>  /* application_legacy.c */
>  void  appl_legacy_init(void);
>  void  appl_legacy_shutdown(void);
> +
> +/* application_blocklist.c */
> +void  appl_blocklist_init(void);
> +void  appl_blocklist_shutdown(void);
> Index: application_blocklist.c
> ===
> RCS file: application_blocklist.c
> diff -N application_blocklist.c
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ application_blocklist.c   29 Jun 2022 11:35:23 -
> @@ -0,0 +1,141 @@
> +/*   $OpenBSD: application.c,v 1.5 2022/06/27 10:31:17 martijn Exp $ */
> +
> +/*
> + * Copyright (c) 2022 Martijn van Duren 
> + *
> + * 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.
> + */
> +
> +#include 
> +#include 
> +
> +#include "application.h"
> +#include "snmpd.h"
> +
> +struct appl_varbind *appl_blocklist_response(size_t);
> +void appl_blocklist_get(struct appl_backend *, int32_t, int32_t, const char 
> *,
> +struct appl_varbind *);
> +void appl_blocklist_getnext(struct appl_backend *, int32_t, int32_t,
> +const char *, struct appl_varbind *);
> +
> +struct appl_backend_functions appl_blocklist_functions = {
> + .ab_get = appl_blocklist_get,
> + .ab_getnext = appl_blocklist_getnext,
> + .ab_getbulk = NULL,
> +};
> +
> +struct appl_backend appl_blocklist = {
> + .ab_name = "blocklist",
> + .ab_cookie = NULL,
> + .ab_retries = 0,
> +   

Re: snmpd(8): Application.c properly initialize oidbuf for appl_region

2022-06-27 Thread Theo Buehler
On Mon, Jun 27, 2022 at 12:11:42PM +0200, Martijn van Duren wrote:
> When registering a region in appl_region (through appl_register) we
> fill oidbuf with strlcat, but we don't start from a clean state and
> might have garbage prepended.
> 
> This oidbuf is only used on error-conditions, so it's unlikely to
> trigger with the current code. Diff below properly initializes it.
> 
> OK?

ok tb

The regionbuf[] has the same issue, so you should treat it the same way
after the overlap: label.

> 
> martijn@
> 
> Index: application.c
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/application.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 application.c
> --- application.c 22 Feb 2022 15:59:13 -  1.3
> +++ application.c 27 Jun 2022 10:10:37 -
> @@ -224,6 +224,7 @@ appl_region(struct appl_context *ctx, ui
>   goto overlap;
>  
>   /* Don't use smi_oid2string, because appl_register can't use it */
> + oidbuf[0] = '\0';
>   for (i = 0; i < oid->bo_n; i++) {
>   if (i != 0)
>   strlcat(oidbuf, ".", sizeof(oidbuf));
> 



Re: bgpd use more portable IPv6 magic mushrooms

2022-06-24 Thread Theo Buehler
On Fri, Jun 24, 2022 at 12:22:55PM +0200, Claudio Jeker wrote:
> It seems that IN6_IS_ADDR_MC_INTFACELOCAL() is actually spelled
> IN6_IS_ADDR_MC_NODELOCAL(). So better use that for portability.

codesearch appears to agree.

ok

> 
> -- 
> :wq Claudio
> 
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.268
> diff -u -p -r1.268 kroute.c
> --- kroute.c  23 Jun 2022 13:09:03 -  1.268
> +++ kroute.c  24 Jun 2022 10:07:42 -
> @@ -1626,13 +1626,13 @@ kr6_tofull(struct kroute6 *kr6)
>   /* only set scope_id for link-local addresses because IPv6 */
>   if (IN6_IS_ADDR_LINKLOCAL(>prefix) ||
>   IN6_IS_ADDR_MC_LINKLOCAL(>prefix) ||
> - IN6_IS_ADDR_MC_INTFACELOCAL(>prefix))
> + IN6_IS_ADDR_MC_NODELOCAL(>prefix))
>   kf.prefix.scope_id = kr6->ifindex;
>   kf.nexthop.aid = AID_INET6;
>   memcpy(, >nexthop, sizeof(struct in6_addr));
>   if (IN6_IS_ADDR_LINKLOCAL(>nexthop) ||
>   IN6_IS_ADDR_MC_LINKLOCAL(>nexthop) ||
> - IN6_IS_ADDR_MC_INTFACELOCAL(>nexthop))
> + IN6_IS_ADDR_MC_NODELOCAL(>nexthop))
>   kf.nexthop.scope_id = kr6->ifindex;
>   strlcpy(kf.label, rtlabel_id2name(kr6->labelid), sizeof(kf.label));
>   kf.flags = kr6->flags;
> Index: util.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> retrieving revision 1.67
> diff -u -p -r1.67 util.c
> --- util.c22 Jun 2022 14:49:02 -  1.67
> +++ util.c24 Jun 2022 10:08:10 -
> @@ -68,7 +68,7 @@ log_in6addr(const struct in6_addr *addr)
>   /* XXX thanks, KAME, for this ugliness... adopted from route/show.c */
>   if ((IN6_IS_ADDR_LINKLOCAL(_in6.sin6_addr) ||
>   IN6_IS_ADDR_MC_LINKLOCAL(_in6.sin6_addr) ||
> - IN6_IS_ADDR_MC_INTFACELOCAL(_in6.sin6_addr)) &&
> + IN6_IS_ADDR_MC_NODELOCAL(_in6.sin6_addr)) &&
>   sa_in6.sin6_scope_id == 0) {
>   uint16_t tmp16;
>   memcpy(, _in6.sin6_addr.s6_addr[2], sizeof(tmp16));
> @@ -922,7 +922,7 @@ sa2addr(struct sockaddr *sa, struct bgpd
>*/
>   if ((IN6_IS_ADDR_LINKLOCAL(_in6->sin6_addr) ||
>   IN6_IS_ADDR_MC_LINKLOCAL(_in6->sin6_addr) ||
> - IN6_IS_ADDR_MC_INTFACELOCAL(_in6->sin6_addr)) &&
> + IN6_IS_ADDR_MC_NODELOCAL(_in6->sin6_addr)) &&
>   sa_in6->sin6_scope_id == 0) {
>   uint16_t tmp16;
>   memcpy(, _in6->sin6_addr.s6_addr[2],
> 



Re: bgpctl fmt_timeframe cleanup

2022-06-23 Thread Theo Buehler
On Thu, Jun 23, 2022 at 12:53:37PM +0200, Claudio Jeker wrote:
> Newer gcc likes to bitch about snprintf buffers being to small for
> insane numbers. This made me look at fmt_timeframe() and I decided to
> clean it up a bit.

It also likes to throw colorful fits about uninitialized variables...

> 
> First of all the ring buffer is not needed. fmt_timeframe() is never used
> multiple time in a single printf() call. So drop that.
> Then make sure that the time_t is positive.
> Last print just the number of weeks when a session is up for more than
> 19 years.
> 
> Now gcc is still not happy but that is a false claim.

Agreed.

ok

> -- 
> :wq Claudio
> 
> Index: bgpctl.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.277
> diff -u -p -r1.277 bgpctl.c
> --- bgpctl.c  15 Jun 2022 10:10:50 -  1.277
> +++ bgpctl.c  23 Jun 2022 10:50:15 -
> @@ -575,22 +575,17 @@ fmt_auth_method(enum auth_method method)
>   }
>  }
>  
> -#define TF_BUFS  8
> -#define TF_LEN   9
> +#define TF_LEN   16
>  
>  const char *
>  fmt_timeframe(time_t t)
>  {
> - char*buf;
> - static char  tfbuf[TF_BUFS][TF_LEN];/* ring buffer */
> - static int   idx = 0;
> + static char  buf[TF_LEN];
>   unsigned int sec, min, hrs, day;
> - unsigned long long  week;
> -
> - buf = tfbuf[idx++];
> - if (idx == TF_BUFS)
> - idx = 0;
> + unsigned long long   week;
>  
> + if (t < 0)
> + t = 0;
>   week = t;
>  
>   sec = week % 60;
> @@ -602,7 +597,9 @@ fmt_timeframe(time_t t)
>   day = week % 7;
>   week /= 7;
>  
> - if (week > 0)
> + if (week >= 1000)
> + snprintf(buf, TF_LEN, "%02lluw", week);
> + else if (week > 0)
>   snprintf(buf, TF_LEN, "%02lluw%01ud%02uh", week, day, hrs);
>   else if (day > 0)
>   snprintf(buf, TF_LEN, "%01ud%02uh%02um", day, hrs, min);
> 



Re: bgpd move struct kif to kroute.c

2022-06-23 Thread Theo Buehler
On Thu, Jun 23, 2022 at 12:10:36PM +0200, Claudio Jeker wrote:
> struct kif is internal to the kroute code with the exception of the
> 'depend on' tracking message. So create an extra object for this message
> and move struct kif to kroute.c.
> 
> I renamed the IMSG just to make it clear what this is about and to make
> sure I did not miss something.

ok tb



Re: bgpd walks into IPv6 hell

2022-06-23 Thread Theo Buehler
On Thu, Jun 23, 2022 at 09:56:19AM +0200, Claudio Jeker wrote:
> IPv6 is such a lovley protocol. Add the scope_id for link local addresses
> in kr6_tofull() because IPv6 just has to be "special".
> 
> Maybe we should add a scope_id to struct kroute6 but heck it is mostly the
> interface index except for non link local addresses because there it has
> to be 0. So this works for now.

ok tb

> 
> With this link local routes show properly in bgpctl show fib output:
> flags: * = valid, B = BGP, C = Connected, S = Static
>N = BGP Nexthop reachable via this route
>r = reject route, b = blackhole route
> 
> flags prio destination  gateway
> *C  1 ::1/128  link#3
> *C  4 fe80::%re0/64link#1
> *C  1 fe80::1%lo0/128  link#3
> *C  1 fe80::1%lo1/128  link#4
> *C  4 ff01::%re0/32link#1
> *C  4 ff01::%lo0/32link#3
> *C  4 ff01::%lo1/32link#4
> *C  4 ff02::%re0/32link#1
> 
> -- 
> :wq Claudio
> 
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.265
> diff -u -p -r1.265 kroute.c
> --- kroute.c  23 Jun 2022 07:43:37 -  1.265
> +++ kroute.c  23 Jun 2022 07:44:53 -
> @@ -1602,8 +1602,17 @@ kr6_tofull(struct kroute6 *kr6)
>  
>   kf.prefix.aid = AID_INET6;
>   memcpy(, >prefix, sizeof(struct in6_addr));
> + /* only set scope_id for link-local addresses because IPv6 */
> + if (IN6_IS_ADDR_LINKLOCAL(>prefix) ||
> + IN6_IS_ADDR_MC_LINKLOCAL(>prefix) ||
> + IN6_IS_ADDR_MC_INTFACELOCAL(>prefix))
> + kf.prefix.scope_id = kr6->ifindex;
>   kf.nexthop.aid = AID_INET6;
>   memcpy(, >nexthop, sizeof(struct in6_addr));
> + if (IN6_IS_ADDR_LINKLOCAL(>nexthop) ||
> + IN6_IS_ADDR_MC_LINKLOCAL(>nexthop) ||
> + IN6_IS_ADDR_MC_INTFACELOCAL(>nexthop))
> + kf.nexthop.scope_id = kr6->ifindex;
>   strlcpy(kf.label, rtlabel_id2name(kr6->labelid), sizeof(kf.label));
>   kf.flags = kr6->flags;
>   kf.ifindex = kr6->ifindex;
> 



Re: bgpd kroute_node cleanup

2022-06-22 Thread Theo Buehler
On Wed, Jun 22, 2022 at 06:17:30PM +0200, Claudio Jeker wrote:
> Diff is huge but mostly mechanical. Remove kroute_node, kroute6_node and
> use struct kroute and kroute6 directly. Also do a similar dance for
> struct knexthop_node.

Went over it three times carefully and can't spot anything wrong.

ok



Re: bgpd move struct kroute definition to kroute.c

2022-06-22 Thread Theo Buehler
On Wed, Jun 22, 2022 at 05:00:38PM +0200, Claudio Jeker wrote:
> Both struct kroute and struct kroute6 are no longer used outside of
> kroute.c. As a first step move the definitions over to that file. 
> More will follow :)

ok tb



Re: bgpd use struct kroute_full for bgpd_filternexthop

2022-06-22 Thread Theo Buehler
On Wed, Jun 22, 2022 at 04:46:41PM +0200, Claudio Jeker wrote:
> Instead of passing either a struct kroute or struct kroute6 pointer
> use kr_tofull() and use struct kroute_full. This makes the code in
> bgpd_filternexthop() a lot cleaner.

Nice.

ok tb



Re: bgpd/bgpctl use struct kroute_full in nexthop messages

2022-06-22 Thread Theo Buehler
On Wed, Jun 22, 2022 at 04:13:43PM +0200, Claudio Jeker wrote:
> Do not leak the address family specific struct kroute into bgpctl if there
> is struct kroute_full which is address family independent.
> The result is mostly minus because the code no longer needs address family
> specific code paths.  This changes 'bgpctl show nexthop' but not its output.
> 
> OK?

This reads fine.

ok

One thing I noticed while comparing the code paths: should the KAME hack
in log_in6addr() not be synced with route/show.c similar to the recent
change in sa2addr()?

Index: util.c
===
RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
retrieving revision 1.66
diff -u -p -r1.66 util.c
--- util.c  19 Jun 2022 10:30:10 -  1.66
+++ util.c  22 Jun 2022 14:32:39 -
@@ -66,8 +66,10 @@ log_in6addr(const struct in6_addr *addr)
 
 #ifdef __KAME__
/* XXX thanks, KAME, for this ugliness... adopted from route/show.c */
-   if (IN6_IS_ADDR_LINKLOCAL(_in6.sin6_addr) ||
-   IN6_IS_ADDR_MC_LINKLOCAL(_in6.sin6_addr)) {
+   if ((IN6_IS_ADDR_LINKLOCAL(_in6.sin6_addr) ||
+   IN6_IS_ADDR_MC_LINKLOCAL(_in6.sin6_addr) ||
+   IN6_IS_ADDR_MC_INTFACELOCAL(_in6.sin6_addr)) &&
+   sa_in6.sin6_scope_id == 0) {
uint16_t tmp16;
memcpy(, _in6.sin6_addr.s6_addr[2], sizeof(tmp16));
sa_in6.sin6_scope_id = ntohs(tmp16);



Re: bgpctl use applymask()

2022-06-19 Thread Theo Buehler
On Sun, Jun 19, 2022 at 12:32:39PM +0200, Claudio Jeker wrote:
> This diff uses applymask() instead of the IPv4 and IPv6 version.
> Makes the code a tiny bit simpler.

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: parser.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
> retrieving revision 1.111
> diff -u -p -r1.111 parser.c
> --- parser.c  16 Jun 2022 15:34:07 -  1.111
> +++ parser.c  16 Jun 2022 16:51:38 -
> @@ -968,17 +968,16 @@ parse_prefix(const char *word, size_t wo
>   mask = 32;
>   if (mask > 32)
>   errx(1, "invalid netmask: too large");
> - inet4applymask(>v4, >v4, mask);
>   break;
>   case AID_INET6:
>   if (mask == -1)
>   mask = 128;
> - inet6applymask(>v6, >v6, mask);
>   break;
>   default:
>   return (0);
>   }
>  
> + applymask(, , mask);
>   *prefixlen = mask;
>   return (1);
>  }
> 



Re: [PATCH] man - apmd(8) reference

2022-06-19 Thread Theo Buehler
On Sat, Jun 18, 2022 at 11:37:00PM -0700, David Rinehart wrote:
> apm(4): apm - Alliance ProMotion video driver

This reference is about the power management apm kernel driver which
exists only on amd64, arm64, i386, loongson, and macppc, e.g.:

https://man.openbsd.org/man4/amd64/apm.4



Re: [PATCH] man - apmd(8) reference

2022-06-19 Thread Theo Buehler
On Sat, Jun 18, 2022 at 10:14:18PM -0700, David Rinehart wrote:
> apm(4) seems unrelated to apmd(8)

What makes you think so?

DESCRIPTION
 apmd monitors the advanced power management device, apm(4), acting on
 signaled events and upon user requests as sent by the apm(8) program.



Re: bgpd switch kroute_find to bgpd_addr argument

2022-06-17 Thread Theo Buehler
On Fri, Jun 17, 2022 at 11:37:29AM +0200, Claudio Jeker wrote:
> On Thu, Jun 16, 2022 at 09:01:33PM +0200, Theo Buehler wrote:
> > On Thu, Jun 16, 2022 at 07:15:51PM +0200, Claudio Jeker wrote:
> > > Not much holds us back to switch kroute_find() to use a struct bgpd_addr
> > > as prefix argument. Only the match code needs some adoption.
> > > I created applymask() for this which works on bgpd_addrs like
> > > inet4applymask works on in_addrs.
> > > 
> > > I also switched a simple case in session.c over to applymask().
> > > There is another one in bgpctl which I will send out once this is in.
> > 
> > ok
> > 
> > A nit and a question below.
> > 
> > [...]
> > 
> > > +kroute_find(struct ktable *kt, const struct bgpd_addr *prefix,
> > > +uint8_t prefixlen, uint8_t prio)
> > >  {
> > >   struct kroute_node  s;
> > >   struct kroute_node  *kn, *tmp;
> > >  
> > > - s.r.prefix.s_addr = prefix;
> > > + s.r.prefix= prefix->v4;
> > 
> > Missing space before =.
> > 
> > [...]
> > 
> > > Index: util.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> > > retrieving revision 1.64
> > > diff -u -p -r1.64 util.c
> > > --- util.c16 Jun 2022 15:33:05 -  1.64
> > > +++ util.c16 Jun 2022 16:50:47 -
> > > @@ -783,6 +783,26 @@ inet6applymask(struct in6_addr *dest, co
> > >   dest->s6_addr[i] = src->s6_addr[i] & mask.s6_addr[i];
> > >  }
> > >  
> > > +void
> > > +applymask(struct bgpd_addr *dest, const struct bgpd_addr *src, int 
> > > prefixlen)
> > > +{
> > > + struct bgpd_addr tmp;
> > > +
> > > + /* use temporary storage in case src and dest point to same struct */
> > > + tmp = *src;
> > 
> > While I'm fine with doing it this way, why is this tmp dance needed? I
> > would have thought that both inet4applymask() and inet6applymask() work
> > fine if src and dest point to the same struct. bgpctl's parse.y assumes
> > that this is the case.
> 
> While inet4applymask() and inet6applymask() work the problem comes with the
> rest of the struct bgpd_addr fields.
> 
> Now I could do a trick and copy from offsetof(rd) to sizeof(*src) -
> offsetof(rd) plus in the IPv4 case the rest of the union ba needs to
> be cleared as well. So I think while possible to do this without this
> temporary storage the code will be rather complex.
> 
> An alternative option is to just store the v4/v6 values before
> memmove(dst, src) and then using those values in the inetXapplymask calls.
> I guess that is a reasonable alternative which is a bit more optimized.
> 
> What do you think?

While I'm not against your alternative option, I think the copies are
cheap enough that we should not need to optimize them.

What confuses me is the comment and why we need temporary storage at
all.

The simple assignment is well-defined if src and dest point to the same
struct (https://port70.net/%7Ensz/c/c99/n1256.html#6.5.16.1p3), so I
would have written applymask() this way:

void
applymask(struct bgpd_addr *dest, const struct bgpd_addr *src, int prefixlen)
{
*dest = *src;
switch (src->aid) {
case AID_INET:
case AID_VPN_IPv4:
inet4applymask(>v4, >v4, prefixlen);
break;
case AID_INET6:
case AID_VPN_IPv6:
inet6applymask(>v6, >v6, prefixlen);
break;
}
}


> 
> > > + switch (src->aid) {
> > > + case AID_INET:
> > > + case AID_VPN_IPv4:
> > > + inet4applymask(, >v4, prefixlen);
> > > + break;
> > > + case AID_INET6:
> > > + case AID_VPN_IPv6:
> > > + inet6applymask(, >v6, prefixlen);
> > > + break;
> > > + }
> > > + *dest = tmp;
> > > +}
> > > +
> > >  /* address family translation functions */
> > >  const struct aid aid_vals[AID_MAX] = AID_VALS;
> > >  
> > > 
> > 
> 
> -- 
> :wq Claudio



Re: fix KAME hack in bgpd sa2addr()

2022-06-16 Thread Theo Buehler
On Thu, Jun 16, 2022 at 05:48:48PM +0200, Claudio Jeker wrote:
> Copy the version of the __KAME__ hack from route/show.c and more
> importantly copy the address after modifying the sa.

ok

> 
> This only affects IPv6 link local addresses which are not really used by
> bgpd. Still happy if a few more people can give this a spin.
> -- 
> :wq Claudio
> 
> ? obj
> Index: util.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 util.c
> --- util.c16 Jun 2022 15:33:05 -  1.64
> +++ util.c16 Jun 2022 15:46:09 -
> @@ -897,24 +897,24 @@ sa2addr(struct sockaddr *sa, struct bgpd
>   break;
>   case AF_INET6:
>   addr->aid = AID_INET6;
> - memcpy(>v6, _in6->sin6_addr, sizeof(addr->v6));
>  #ifdef __KAME__
>   /*
>* XXX thanks, KAME, for this ugliness...
>* adopted from route/show.c
>*/
> - if (IN6_IS_ADDR_LINKLOCAL(_in6->sin6_addr) ||
> - IN6_IS_ADDR_MC_LINKLOCAL(_in6->sin6_addr)) {
> + if ((IN6_IS_ADDR_LINKLOCAL(_in6->sin6_addr) ||
> + IN6_IS_ADDR_MC_LINKLOCAL(_in6->sin6_addr) ||
> + IN6_IS_ADDR_MC_INTFACELOCAL(_in6->sin6_addr)) &&
> + sa_in6->sin6_scope_id == 0) {
>   uint16_t tmp16;
>   memcpy(, _in6->sin6_addr.s6_addr[2],
>   sizeof(tmp16));
> - if (tmp16 != 0) {
> - sa_in6->sin6_scope_id = ntohs(tmp16);
> - sa_in6->sin6_addr.s6_addr[2] = 0;
> - sa_in6->sin6_addr.s6_addr[3] = 0;
> - }
> + sa_in6->sin6_scope_id = ntohs(tmp16);
> + sa_in6->sin6_addr.s6_addr[2] = 0;
> + sa_in6->sin6_addr.s6_addr[3] = 0;
>   }
>  #endif
> + memcpy(>v6, _in6->sin6_addr, sizeof(addr->v6));
>   addr->scope_id = sa_in6->sin6_scope_id; /* I hate v6 */
>   if (port)
>   *port = ntohs(sa_in6->sin6_port);
> 



Re: bgpd switch kroute_find to bgpd_addr argument

2022-06-16 Thread Theo Buehler
On Thu, Jun 16, 2022 at 07:15:51PM +0200, Claudio Jeker wrote:
> Not much holds us back to switch kroute_find() to use a struct bgpd_addr
> as prefix argument. Only the match code needs some adoption.
> I created applymask() for this which works on bgpd_addrs like
> inet4applymask works on in_addrs.
> 
> I also switched a simple case in session.c over to applymask().
> There is another one in bgpctl which I will send out once this is in.

ok

A nit and a question below.

[...]

> +kroute_find(struct ktable *kt, const struct bgpd_addr *prefix,
> +uint8_t prefixlen, uint8_t prio)
>  {
>   struct kroute_node  s;
>   struct kroute_node  *kn, *tmp;
>  
> - s.r.prefix.s_addr = prefix;
> + s.r.prefix= prefix->v4;

Missing space before =.

[...]

> Index: util.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/util.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 util.c
> --- util.c16 Jun 2022 15:33:05 -  1.64
> +++ util.c16 Jun 2022 16:50:47 -
> @@ -783,6 +783,26 @@ inet6applymask(struct in6_addr *dest, co
>   dest->s6_addr[i] = src->s6_addr[i] & mask.s6_addr[i];
>  }
>  
> +void
> +applymask(struct bgpd_addr *dest, const struct bgpd_addr *src, int prefixlen)
> +{
> + struct bgpd_addr tmp;
> +
> + /* use temporary storage in case src and dest point to same struct */
> + tmp = *src;

While I'm fine with doing it this way, why is this tmp dance needed? I
would have thought that both inet4applymask() and inet6applymask() work
fine if src and dest point to the same struct. bgpctl's parse.y assumes
that this is the case.

> + switch (src->aid) {
> + case AID_INET:
> + case AID_VPN_IPv4:
> + inet4applymask(, >v4, prefixlen);
> + break;
> + case AID_INET6:
> + case AID_VPN_IPv6:
> + inet6applymask(, >v6, prefixlen);
> + break;
> + }
> + *dest = tmp;
> +}
> +
>  /* address family translation functions */
>  const struct aid aid_vals[AID_MAX] = AID_VALS;
>  
> 



Re: bgpd name2id change

2022-06-16 Thread Theo Buehler
On Thu, Jun 16, 2022 at 02:42:31PM +0200, Claudio Jeker wrote:
> tb@ noticed that name2id conversions never check for error.
> Now I think this is fine but in that case the api should not do a half
> assed job of setting errnos.
> 
> In the end if 0 is returned from name2id but the input was not the empty
> string then an error occurred. None of the callers does this check and
> instead accept that the label is silently dropped.
> 
> Most labels come from the config only rtlabel will be picked up from the
> kernel FIB. So I don't think that dropping such lables on the floor is a
> bad thing. Also the kernel has very similar limitations.

I agree with your description and the diff.

ok

> 
> -- 
> :wq Claudio
> 
> Index: name2id.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/name2id.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 name2id.c
> --- name2id.c 6 Feb 2022 09:51:19 -   1.11
> +++ name2id.c 14 Jun 2022 17:03:47 -
> @@ -94,16 +94,18 @@ pftable_ref(uint16_t id)
>   return (_ref(_labels, id));
>  }
>  
> +/*
> + * Try to convert a name into id. If something fails 0 is returned which
> + * is the ID of the empty label.
> + */
>  uint16_t
>  _name2id(struct n2id_labels *head, const char *name)
>  {
>   struct n2id_label   *label, *p = NULL;
>   uint16_t new_id = 1;
>  
> - if (!name[0]) {
> - errno = EINVAL;
> + if (!name[0])
>   return (0);
> - }
>  
>   TAILQ_FOREACH(label, head, entry)
>   if (strcmp(name, label->name) == 0) {
> @@ -122,10 +124,8 @@ _name2id(struct n2id_labels *head, const
>   p->id == new_id; p = TAILQ_NEXT(p, entry))
>   new_id = p->id + 1;
>  
> - if (new_id > IDVAL_MAX) {
> - errno = ERANGE;
> + if (new_id > IDVAL_MAX)
>   return (0);
> - }
>  
>   if ((label = calloc(1, sizeof(struct n2id_label))) == NULL)
>   return (0);
> 



Re: bgpd: next round of kroute cleanup

2022-06-16 Thread Theo Buehler
On Thu, Jun 16, 2022 at 02:36:28PM +0200, Claudio Jeker wrote:
> This diff kills external use of prefixlen2mask() and uses inet4applymask()
> instead.  With this the IPv4 and IPv6 code is more similar.
> Also I feel the code is a bit easier to read.
>
> Also kroute{,6}_match() is changed to take a struct bgpd_addr *.
> This is another step towards removing lots of copy paste code.

I agree that it's easier to follow and cleaner after the diff.
The tree you used to isn't quite up to date - there were some offsets.

ok tb

[...]

> +const struct in_addr inet4allone = { INADDR_BROADCAST };
> +const struct in6_addrinet6allone = {{{ 0xff, 0xff, 0xff, 0xff,
> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> + 0xff, 0xff, 0xff, 0xff }}};

Any reason not to make these static?



Re: bgpd: cleanup warnings in pfkey

2022-06-16 Thread Theo Buehler
On Thu, Jun 16, 2022 at 11:34:00AM +0200, Claudio Jeker wrote:
> There is no reason to add the __func__ in this warning.
> It is just clutter in the error message with no benefit.
> 
> OK to remove?

Yes, these don't disambiguate anything.

ok



Re: bgpd prevent infinite-loop in pfkey code

2022-06-15 Thread Theo Buehler
On Wed, Jun 15, 2022 at 04:34:41PM +0200, Claudio Jeker wrote:
> Found by accident. If the pfkey message failed (sadb_msg_errno is set)
> then the pfkey code ends in an infinite loop because the erroneous message
> is not removed from the queue.
> 
> pfkey_read() PEEKS at the message and returns 0 since it is what we
> expect. pfkey_reply() realizes it is an error and returns -1 but the
> message is still in the buffer. Once we hit the poll loop the fd
> immediatly triggers since the message is still in the buffer and we call
> pfkey_read() which peeks at the message and returns 0 ...
> 
> Fix is simple, need to flush the message out in the error case.

ok

> -- 
> :wq Claudio
> 
> Index: pfkey.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/pfkey.c,v
> retrieving revision 1.63
> diff -u -p -r1.63 pfkey.c
> --- pfkey.c   15 Jun 2022 14:09:30 -  1.63
> +++ pfkey.c   15 Jun 2022 14:23:39 -
> @@ -469,6 +469,9 @@ pfkey_reply(int sd, uint32_t *spi)
>   return (0);
>   else {
>   log_warn("pfkey");
> + /* discard error message */
> + if (read(sd, , sizeof(hdr)) == -1)
> + log_warn("pfkey read");
>   return (-1);
>   }
>   }
> 



Re: don't depend on pfkeyv2.h in portable bgpd code

2022-06-15 Thread Theo Buehler
On Wed, Jun 15, 2022 at 01:43:29PM +0200, Claudio Jeker wrote:
> So pfkeyv2.h defines managed to sneak into the portable part of bgpd.
> The fix was to just dump a pfkeyv2.h version into the -portable compat
> code and call it a day. This is bad since pfkeyv2.h is highly OS
> dependent.
> 
> This diff cleans up the mess by introducing new enums for the various
> IPsec algorithms. With that the net/pfkeyv2.h include in bgpd.h can be
> removed. In session.h we just need to forward declare struct sadb_msg
> which is simple enough and no problem since the portable code only uses
> NULL as argument.
> 
> In pfkey.c the BGPD algorithm defines need to be switched to SADB ones but
> that is done with simple helper functions.

ok tb

> 
> Is anyone actively using the IPsec support in bgpd? Not many other systems
> seem to support BGP over IPsec. Also the supported algorithms are just aes
> and 3des-cbc which is probably no longer adequate.
> -- 
> :wq Claudio
> 
> ? obj
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.429
> diff -u -p -r1.429 bgpd.h
> --- bgpd.h15 Jun 2022 10:10:03 -  1.429
> +++ bgpd.h15 Jun 2022 10:56:34 -
> @@ -26,7 +26,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  
>  #include 
>  #include 
> @@ -329,6 +328,18 @@ enum auth_method {
>   AUTH_IPSEC_IKE_AH
>  };
>  
> +enum auth_alg {
> + AUTH_AALG_NONE,
> + AUTH_AALG_SHA1HMAC,
> + AUTH_AALG_MD5HMAC,
> +};
> +
> +enum auth_enc_alg {
> + AUTH_EALG_NONE,
> + AUTH_EALG_3DESCBC,
> + AUTH_EALG_AES,
> +};
> +
>  struct peer_auth {
>   charmd5key[TCP_MD5_KEY_LEN];
>   charauth_key_in[IPSEC_AUTH_KEY_LEN];
> @@ -338,13 +349,13 @@ struct peer_auth {
>   uint32_tspi_in;
>   uint32_tspi_out;
>   enum auth_methodmethod;
> + enum auth_alg   auth_alg_in;
> + enum auth_alg   auth_alg_out;
> + enum auth_enc_alg   enc_alg_in;
> + enum auth_enc_alg   enc_alg_out;
>   uint8_t md5key_len;
> - uint8_t auth_alg_in;
> - uint8_t auth_alg_out;
>   uint8_t auth_keylen_in;
>   uint8_t auth_keylen_out;
> - uint8_t enc_alg_in;
> - uint8_t enc_alg_out;
>   uint8_t enc_keylen_in;
>   uint8_t enc_keylen_out;
>  };
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.429
> diff -u -p -r1.429 parse.y
> --- parse.y   9 Jun 2022 17:33:47 -   1.429
> +++ parse.y   15 Jun 2022 10:56:35 -
> @@ -193,7 +193,7 @@ typedef struct {
>   struct filter_prefixlen prefixlen;
>   struct prefixset_item   *prefixset_item;
>   struct {
> - uint8_t enc_alg;
> + enum auth_enc_alg   enc_alg;
>   uint8_t enc_key_len;
>   charenc_key[IPSEC_ENC_KEY_LEN];
>   }   encspec;
> @@ -1609,7 +1609,7 @@ peeropts: REMOTEAS as4number{
>   curpeer->conf.auth.method = AUTH_IPSEC_IKE_AH;
>   }
>   | IPSEC espah inout SPI NUMBER STRING STRING encspec {
> - uint32_tauth_alg;
> + enum auth_alg   auth_alg;
>   uint8_t keylen;
>  
>   if (curpeer->conf.auth.method &&
> @@ -1626,10 +1626,10 @@ peeropts  : REMOTEAS as4number{
>   }
>  
>   if (!strcmp($6, "sha1")) {
> - auth_alg = SADB_AALG_SHA1HMAC;
> + auth_alg = AUTH_AALG_SHA1HMAC;
>   keylen = 20;
>   } else if (!strcmp($6, "md5")) {
> - auth_alg = SADB_AALG_MD5HMAC;
> + auth_alg = AUTH_AALG_MD5HMAC;
>   keylen = 16;
>   } else {
>   yyerror("unknown auth algorithm \"%s\"", $6);
> @@ -1860,11 +1860,11 @@ encspec   : /* nada */{
>   | STRING STRING {
>   bzero(&$$, sizeof($$));
>   if (!strcmp($1, "3des") || !strcmp($1, "3des-cbc")) {
> - $$.enc_alg = SADB_EALG_3DESCBC;
> + $$.enc_alg = AUTH_EALG_3DESCBC;
>   $$.enc_key_len = 21; /* XXX verify */
>   } else if (!strcmp($1, "aes") ||
>   !strcmp($1, "aes-128-cbc")) {
> - 

Re: bgpd more kroute cleanup

2022-06-14 Thread Theo Buehler
On Tue, Jun 14, 2022 at 06:36:13PM +0200, Claudio Jeker wrote:
> This diff does introduce a new flag for routes that have been inserted
> into the FIB. Now I actually renamed the F_BGPD_INSERTED flag to F_BGPD
> and reused F_BGPD_INSERTED as this new flag.
> Adjust and use the send_rtmsg() return value to set F_BGPD_INSERTED if the
> route message was successfully sent.
> 
> Additionally this removes F_DYNAMIC and tracking of dynamic routes (routes
> generated by the kernel because of PMTU or some other event). A routing
> protocol does not need to work with them so just filter them out like ARP
> and ND routes.
> 
> While there also just remove protect_lo() it is a function that is not
> really making anything better. There are other checks in place for
> 127.0.0.1 and ::1.

Would it not be cleaner to set or unset the F_BGPD_INSERTED flag
inside send_rt{,6}msg() depending on action instead of doing that
after inspecting the return code?

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: bgpctl/bgpctl.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.276
> diff -u -p -r1.276 bgpctl.c
> --- bgpctl/bgpctl.c   21 Mar 2022 10:16:23 -  1.276
> +++ bgpctl/bgpctl.c   14 Jun 2022 15:31:59 -
> @@ -633,14 +633,12 @@ fmt_fib_flags(uint16_t flags)
>   else
>   strlcpy(buf, "*", sizeof(buf));
>  
> - if (flags & F_BGPD_INSERTED)
> + if (flags & F_BGPD)
>   strlcat(buf, "B", sizeof(buf));
>   else if (flags & F_CONNECTED)
>   strlcat(buf, "C", sizeof(buf));
>   else if (flags & F_STATIC)
>   strlcat(buf, "S", sizeof(buf));
> - else if (flags & F_DYNAMIC)
> - strlcat(buf, "D", sizeof(buf));
>   else
>   strlcat(buf, " ", sizeof(buf));
>  
> Index: bgpctl/output.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 output.c
> --- bgpctl/output.c   6 Feb 2022 09:52:32 -   1.20
> +++ bgpctl/output.c   14 Jun 2022 15:51:49 -
> @@ -46,7 +46,7 @@ show_head(struct parse_result *res)
>   break;
>   case SHOW_FIB:
>   printf("flags: * = valid, B = BGP, C = Connected, "
> - "S = Static, D = Dynamic\n");
> + "S = Static\n");
>   printf("   "
>   "N = BGP Nexthop reachable via this route\n");
>   printf("   r = reject route, b = blackhole route\n\n");
> Index: bgpctl/output_json.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/output_json.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 output_json.c
> --- bgpctl/output_json.c  21 Mar 2022 10:16:23 -  1.14
> +++ bgpctl/output_json.c  14 Jun 2022 15:32:05 -
> @@ -357,14 +357,12 @@ json_fib(struct kroute_full *kf)
>   json_do_printf("prefix", "%s/%u", log_addr(>prefix), kf->prefixlen);
>   json_do_uint("priority", kf->priority);
>   json_do_bool("up", !(kf->flags & F_DOWN));
> - if (kf->flags & F_BGPD_INSERTED)
> + if (kf->flags & F_BGPD)
>   origin = "bgp";
>   else if (kf->flags & F_CONNECTED)
>   origin = "connected";
>   else if (kf->flags & F_STATIC)
>   origin = "static";
> - else if (kf->flags & F_DYNAMIC)
> - origin = "dynamic";
>   else
>   origin = "unknown";
>   json_do_printf("origin", "%s", origin);
> Index: bgpctl/parser.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
> retrieving revision 1.109
> diff -u -p -r1.109 parser.c
> --- bgpctl/parser.c   21 Mar 2022 10:16:23 -  1.109
> +++ bgpctl/parser.c   14 Jun 2022 15:29:05 -
> @@ -151,15 +151,15 @@ static const struct token t_show_summary
>  };
>  
>  static const struct token t_show_fib[] = {
> - { NOTOKEN,  "", NONE,NULL},
> - { FLAG, "connected",F_CONNECTED, t_show_fib},
> - { FLAG, "static",   F_STATIC,t_show_fib},
> - { FLAG, "bgp",  F_BGPD_INSERTED, t_show_fib},
> - { FLAG, "nexthop",  F_NEXTHOP,   t_show_fib},
> - { KEYWORD,  "table",NONE,t_show_fib_table},
> - { FAMILY,   "", NONE,t_show_fib},
> - { ADDRESS,  "", NONE,NULL},
> - { ENDTOKEN, "", NONE,NULL}
> + { NOTOKEN,  "", NONE,   NULL},
> + { FLAG, "connected",F_CONNECTED,t_show_fib},
> + { FLAG, "static",   F_STATIC,   t_show_fib},
> + { FLAG, "bgp",  F_BGPD, t_show_fib},
> + { FLAG, "nexthop",  F_NEXTHOP,  t_show_fib},
> +

Re: rpki-client: dedup econtent version checks

2022-06-10 Thread Theo Buehler
On Fri, Jun 10, 2022 at 11:31:42AM +0200, Theo Buehler wrote:
> This is a leftover of the conversion to ASN.1 templates. The diff
> reinstates a simplified variant of the removed cms_econtent_version(). 
> 
> None of the filetypes currently have a version other than the default,
> which means that the ->version should always be NULL. This in turn means
> that this is a bunch of mostly dead copy-pasted code.
> 
> Obviously, we will need to rethink this once we want to support a future
> version of any of these, but that will necessarily come with other
> changes. For now we're better off with one copy instead of three.

I sent an older version of the diff. This pure validation makes more
sense to me:

Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.141
diff -u -p -r1.141 extern.h
--- extern.h1 Jun 2022 10:59:21 -   1.141
+++ extern.h10 Jun 2022 09:36:47 -
@@ -508,6 +508,7 @@ int  valid_origin(const char *, const c
 int valid_x509(char *, X509_STORE_CTX *, X509 *, struct auth *,
struct crl *, int);
 int valid_rsc(const char *, struct auth *, struct rsc *);
+int valid_econtent_version(const char *, const ASN1_INTEGER *);
 
 /* Working with CMS. */
 unsigned char  *cms_parse_validate(X509 **, const char *,
Index: mft.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
retrieving revision 1.70
diff -u -p -r1.70 mft.c
--- mft.c   1 Jun 2022 10:58:34 -   1.70
+++ mft.c   10 Jun 2022 09:36:13 -
@@ -270,7 +270,6 @@ mft_parse_econtent(const unsigned char *
 {
Manifest*mft;
FileAndHash *fh;
-   long mft_version;
int  i, rc = 0;
 
if ((mft = d2i_Manifest(NULL, , dsz)) == NULL) {
@@ -279,24 +278,8 @@ mft_parse_econtent(const unsigned char *
goto out;
}
 
-   /* Validate the optional version field */
-   if (mft->version != NULL) {
-   mft_version = ASN1_INTEGER_get(mft->version);
-   if (mft_version < 0) {
-   cryptowarnx("%s: ASN1_INTEGER_get failed", p->fn);
-   goto out;
-   }
-
-   switch (mft_version) {
-   case 0:
-   warnx("%s: incorrect encoding for version 0", p->fn);
-   goto out;
-   default:
-   warnx("%s: version %ld not supported (yet)", p->fn,
-   mft_version);
-   goto out;
-   }
-   }
+   if (!valid_econtent_version(p->fn, mft->version))
+   goto out;
 
p->res->seqnum = x509_convert_seqnum(p->fn, mft->manifestNumber);
if (p->res->seqnum == NULL)
Index: roa.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
retrieving revision 1.46
diff -u -p -r1.46 roa.c
--- roa.c   31 May 2022 18:51:35 -  1.46
+++ roa.c   10 Jun 2022 09:36:22 -
@@ -103,7 +103,6 @@ static int
 roa_parse_econtent(const unsigned char *d, size_t dsz, struct parse *p)
 {
RouteOriginAttestation  *roa;
-   long roa_version;
const ROAIPAddressFamily*addrfam;
const STACK_OF(ROAIPAddress)*addrs;
int  addrsz;
@@ -120,24 +119,8 @@ roa_parse_econtent(const unsigned char *
goto out;
}
 
-   /* Validate the optional version field */
-   if (roa->version != NULL) {
-   roa_version = ASN1_INTEGER_get(roa->version);
-   if (roa_version < 0) {
-   warnx("%s: ASN1_INTEGER_get failed", p->fn);
-   goto out;
-   }
-
-   switch (roa_version) {
-   case 0:
-   warnx("%s: incorrect encoding for version 0", p->fn);
-   goto out;
-   default:
-   warnx("%s: version %ld not supported (yet)", p->fn,
-   roa_version);
-   goto out;
-   }
-   }
+   if (!valid_econtent_version(p->fn, roa->version))
+   goto out;
 
if (!as_id_parse(roa->asid, >res->asid)) {
warnx("%s: RFC 6482 section 3.2: asID: "
Index: rsc.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/rsc.c,v
retrieving revision 1.10
diff -u -p -r1.10 rsc.c
--- rsc.c   5 Jun 2022 13:31:35 -

rpki-client: dedup econtent version checks

2022-06-10 Thread Theo Buehler
This is a leftover of the conversion to ASN.1 templates. The diff
reinstates a simplified variant of the removed cms_econtent_version(). 

None of the filetypes currently have a version other than the default,
which means that the ->version should always be NULL. This in turn means
that this is a bunch of mostly dead copy-pasted code.

Obviously, we will need to rethink this once we want to support a future
version of any of these, but that will necessarily come with other
changes. For now we're better off with one copy instead of three.

Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.141
diff -u -p -r1.141 extern.h
--- extern.h1 Jun 2022 10:59:21 -   1.141
+++ extern.h10 Jun 2022 09:04:07 -
@@ -508,6 +508,8 @@ int  valid_origin(const char *, const c
 int valid_x509(char *, X509_STORE_CTX *, X509 *, struct auth *,
struct crl *, int);
 int valid_rsc(const char *, struct auth *, struct rsc *);
+int valid_econtent_version(const char *, const ASN1_INTEGER *,
+   long *);
 
 /* Working with CMS. */
 unsigned char  *cms_parse_validate(X509 **, const char *,
Index: mft.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
retrieving revision 1.70
diff -u -p -r1.70 mft.c
--- mft.c   1 Jun 2022 10:58:34 -   1.70
+++ mft.c   10 Jun 2022 09:03:38 -
@@ -279,24 +279,8 @@ mft_parse_econtent(const unsigned char *
goto out;
}
 
-   /* Validate the optional version field */
-   if (mft->version != NULL) {
-   mft_version = ASN1_INTEGER_get(mft->version);
-   if (mft_version < 0) {
-   cryptowarnx("%s: ASN1_INTEGER_get failed", p->fn);
-   goto out;
-   }
-
-   switch (mft_version) {
-   case 0:
-   warnx("%s: incorrect encoding for version 0", p->fn);
-   goto out;
-   default:
-   warnx("%s: version %ld not supported (yet)", p->fn,
-   mft_version);
-   goto out;
-   }
-   }
+   if (!valid_econtent_version(p->fn, mft->version, _version))
+   goto out;
 
p->res->seqnum = x509_convert_seqnum(p->fn, mft->manifestNumber);
if (p->res->seqnum == NULL)
Index: roa.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
retrieving revision 1.46
diff -u -p -r1.46 roa.c
--- roa.c   31 May 2022 18:51:35 -  1.46
+++ roa.c   10 Jun 2022 09:03:41 -
@@ -120,24 +120,8 @@ roa_parse_econtent(const unsigned char *
goto out;
}
 
-   /* Validate the optional version field */
-   if (roa->version != NULL) {
-   roa_version = ASN1_INTEGER_get(roa->version);
-   if (roa_version < 0) {
-   warnx("%s: ASN1_INTEGER_get failed", p->fn);
-   goto out;
-   }
-
-   switch (roa_version) {
-   case 0:
-   warnx("%s: incorrect encoding for version 0", p->fn);
-   goto out;
-   default:
-   warnx("%s: version %ld not supported (yet)", p->fn,
-   roa_version);
-   goto out;
-   }
-   }
+   if (!valid_econtent_version(p->fn, roa->version, _version))
+   goto out;
 
if (!as_id_parse(roa->asid, >res->asid)) {
warnx("%s: RFC 6482 section 3.2: asID: "
Index: rsc.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/rsc.c,v
retrieving revision 1.10
diff -u -p -r1.10 rsc.c
--- rsc.c   5 Jun 2022 13:31:35 -   1.10
+++ rsc.c   10 Jun 2022 09:03:46 -
@@ -339,24 +339,8 @@ rsc_parse_econtent(const unsigned char *
goto out;
}
 
-   /* Validate the optional version field */
-   if (rsc->version != NULL) {
-   rsc_version = ASN1_INTEGER_get(rsc->version);
-   if (rsc_version < 0) {
-   cryptowarnx("%s: RSC: ASN1_INTEGER_get failed", p->fn);
-   goto out;
-   }
-
-   switch (rsc_version) {
-   case 0:
-   warnx("%s: RSC: incorrect version encoding", p->fn);
-   goto out;
-   default:
-   warnx("%s: RSC: version %ld not supported (yet)", p->fn,
-   rsc_version);
-   goto out;
-   }
-   }
+   if (!valid_econtent_version(p->fn, rsc->version, _version))
+   goto out;
 
  

Re: bgpd fix for undefined macros

2022-06-09 Thread Theo Buehler
On Thu, Jun 09, 2022 at 07:07:12PM +0200, Claudio Jeker wrote:
> Fix a crash because of a NULL pointer dereference in parse.y

ok

> 
> Before:
> /etc/bgpd.conf:85: macro 'UNDEFINED' not defined
> Segmentation fault
> 
> After:
> /etc/bgpd.conf:85: macro 'UNDEFINED' not defined
> /etc/bgpd.conf:85: syntax error
> 
> -- 
> :wq Claudio
> 
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.428
> diff -u -p -r1.428 parse.y
> --- parse.y   2 Jun 2022 11:12:47 -   1.428
> +++ parse.y   9 Jun 2022 17:03:08 -
> @@ -3248,8 +3248,10 @@ expand_macro(void)
>   break;
>   }
>   val = symget(buf);
> - if (val == NULL)
> + if (val == NULL) {
>   yyerror("macro '%s' not defined", buf);
> + return (ERROR);
> + }
>   p = val + strlen(val) - 1;
>   lungetc(DONE_EXPAND);
>   while (p >= val) {
> 



Re: more bgpd kroute cleanup

2022-06-09 Thread Theo Buehler
On Thu, Jun 09, 2022 at 04:53:46PM +0200, Claudio Jeker wrote:
> Noticed that similar to fib_prio the rdomain id is passed around to some
> places where it really makes no sense.
> 
> First of all kr_nexthop_add() and kr_nexthop_delete() can be simplified.
> The imsg peerid is always 0. This was added in the beginning of L3VPN
> support without realizing that all bgp nexthops live in the same rdomain.
> So simplify this code path.
> 
> Then kr_shutdown() should just nuke all tables, passing one for good
> measures makes again little sense. Similar reason for kr_dispatch_msg().
> 
> Now all those function had this extra variable because of kif_remove().
> kif_remove needs to know the ktable where the connected routes live in.
> Now this is simply the rdomain id of the interface. No other table should
> have a connected route to this interface.

The reasoning seems sound and the code changes match what you describe.

> Nexthop lookups need more work but this should work well enough for now.

ok with me.



Re: bgpd: refactor kroute code a fair bit

2022-06-09 Thread Theo Buehler
> I did this change for most calloc calls in kroute.c. So now that is
> consistent.

Much better.

> I think the name2id code should not set an error for the empty string. I
> think a lot of the code depends on the mapping of "" to 0.
> The ERANGE error should be transformed to a fatalx() so that the API can't
> fail and has only expected results.
> 
> Something to tackle once this is in. At least I think most callers do not
> check for errors. So it would be better to "change" the API.

Agreed. Almost nothing checks what is returned and nothing inspects the
errno.

> > > - if ((gw = rti_info[RTAX_GATEWAY]) != NULL)
> > > - switch (gw->sa_family) {
> > > - case AF_INET:
> > > - if (kr == NULL)
> > > - fatalx("v4 gateway for !v4 dst?!");
> > > -
> > > - if (rtm->rtm_flags & RTF_CONNECTED) {
> > > - kr->r.flags |= F_CONNECTED;
> > > - break;
> > > - }
> > 
> > I can't spot where this setting of F_CONNECTED is handled after the
> > refactor. The path doing sa2addr(..., >nexthop, NULL) in the new
> > dispatch_rtmsg_addr() explicitly avoids setting that flag.
> > 
> > Perhaps this is part of the switch to using RTF_GATEWAY you mentioned...
> 
> Yes, in the new code F_CONNECTED is set when RTF_GATEWAY is unset:
> 
> kl->ifindex = rtm->rtm_index;
> if (rtm->rtm_flags & RTF_GATEWAY) {
> switch (sa->sa_family) {
> case AF_LINK:
> kl->flags |= F_CONNECTED;
> break;
> case AF_INET:
> case AF_INET6:
> sa2addr(rti_info[RTAX_GATEWAY], >nexthop, NULL);
> break;
> }
> } else {
> kl->flags |= F_CONNECTED;
> }
> 
> RTF_CONNECTED is a strange flag which was added to OpenBSD. It is not
> consistent since it is not used for P2P/loopback routes. I think depending
> on RTF_GATEWAY is a much cleaner approach. Since what we want to know is if a
> IP is directly reachable (F_CONNECTED) or reached via a gateway (RTF_GATEWAY).

Ah, now this makes sense, too. Thanks.

> This is the last version with the changes from above plus one addtional
> fix. In kr_tofull() the priority is checked for RTP_MINE and replaced with
> kr_state.fib_prio so that the `bgpctl show fib` output shows the right
> prio.

Still ok, except

> @@ -1595,7 +1595,8 @@ kr_tofull(struct kroute *kr)
>   kf.flags = kr->flags;
>   kf.ifindex = kr->ifindex;
>   kf.prefixlen = kr->prefixlen;
> - kf.priority = kr->priority;
> + kf.priority = kr->priority == RTP_MINE ?
> +  kr_state.fib_prio : kr->priority;

Secondary indent with 4 spaces instead of 5.

>  
>   return ();
>  }
> @@ -1615,7 +1616,8 @@ kr6_tofull(struct kroute6 *kr6)
>   kf.flags = kr6->flags;
>   kf.ifindex = kr6->ifindex;
>   kf.prefixlen = kr6->prefixlen;
> - kf.priority = kr6->priority;
> + kf.priority = kr6->priority == RTP_MINE ?
> +  kr_state.fib_prio : kr6->priority;

ditto.



Re: bgpd: refactor kroute code a fair bit

2022-06-09 Thread Theo Buehler
On Wed, Jun 08, 2022 at 10:47:48PM +0200, Claudio Jeker wrote:
> and here is the updated diff I forgot to include 

I think I've now done what I reasonably can do to review this. I buy
that this mostly preserves behavior and I'm convinced that it is a step
in the right direction, especially given the outline of your planned
next steps.

I'm ok with this going in once you think you've seen enough tests.

A few more things I spotted below.

> 
> -- 
> :wq Claudio
> 
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.251
> diff -u -p -r1.251 kroute.c
> --- kroute.c  7 Jun 2022 16:42:07 -   1.251
> +++ kroute.c  8 Jun 2022 16:23:51 -
> @@ -125,12 +125,11 @@ int kroute6_compare(struct kroute6_node 
>  int  knexthop_compare(struct knexthop_node *, struct knexthop_node *);
>  int  kredist_compare(struct kredist_node *, struct kredist_node *);
>  int  kif_compare(struct kif_node *, struct kif_node *);
> -void kr_fib_update_prio(u_int, uint8_t);
>  
>  struct kroute_node   *kroute_find(struct ktable *, in_addr_t, uint8_t,
>   uint8_t);
>  struct kroute_node   *kroute_matchgw(struct kroute_node *,
> - struct sockaddr_in *);
> + struct bgpd_addr *);
>  int   kroute_insert(struct ktable *, struct kroute_node *);
>  int   kroute_remove(struct ktable *, struct kroute_node *);
>  void  kroute_clear(struct ktable *);
> @@ -138,7 +137,7 @@ void   kroute_clear(struct ktable *);
>  struct kroute6_node  *kroute6_find(struct ktable *, const struct in6_addr *,
>   uint8_t, uint8_t);
>  struct kroute6_node  *kroute6_matchgw(struct kroute6_node *,
> - struct sockaddr_in6 *);
> + struct bgpd_addr *);
>  int   kroute6_insert(struct ktable *, struct kroute6_node *);
>  int   kroute6_remove(struct ktable *, struct kroute6_node *);
>  void  kroute6_clear(struct ktable *);
> @@ -188,8 +187,9 @@ int   send_rt6msg(int, int, struct ktable
>  int  dispatch_rtmsg(u_int);
>  int  fetchtable(struct ktable *);
>  int  fetchifs(int);
> -int  dispatch_rtmsg_addr(struct rt_msghdr *,
> - struct sockaddr *[RTAX_MAX], struct ktable *);
> +int  dispatch_rtmsg_addr(struct rt_msghdr *, struct kroute_full *);
> +int  kr_fib_delete(struct ktable *, struct kroute_full *, int);
> +int  kr_fib_change(struct ktable *, struct kroute_full *, int, int);
>  
>  RB_PROTOTYPE(kroute_tree, kroute_node, entry, kroute_compare)
>  RB_GENERATE(kroute_tree, kroute_node, entry, kroute_compare)
> @@ -1780,21 +1780,21 @@ kroute_find(struct ktable *kt, in_addr_t
>  }
>  
>  struct kroute_node *
> -kroute_matchgw(struct kroute_node *kr, struct sockaddr_in *sa_in)
> +kroute_matchgw(struct kroute_node *kr, struct bgpd_addr *gw)
>  {
>   in_addr_t   nexthop;
>  
> - if (sa_in == NULL) {
> + if (gw->aid != AID_INET) {
>   log_warnx("%s: no nexthop defined", __func__);
>   return (NULL);
>   }
> - nexthop = sa_in->sin_addr.s_addr;
> + nexthop = gw->v4.s_addr;
>  
> - while (kr) {
> + do {
>   if (kr->r.nexthop.s_addr == nexthop)
>   return (kr);
>   kr = kr->next;
> - }
> + } while (kr);
>  
>   return (NULL);
>  }
> @@ -1933,21 +1933,21 @@ kroute6_find(struct ktable *kt, const st
>  }
>  
>  struct kroute6_node *
> -kroute6_matchgw(struct kroute6_node *kr, struct sockaddr_in6 *sa_in6)
> +kroute6_matchgw(struct kroute6_node *kr, struct bgpd_addr *gw)
>  {
>   struct in6_addr nexthop;
>  
> - if (sa_in6 == NULL) {
> + if (gw->aid != AID_INET6) {
>   log_warnx("%s: no nexthop defined", __func__);
>   return (NULL);
>   }
> - memcpy(, _in6->sin6_addr, sizeof(nexthop));
> + nexthop = gw->v6;
>  
> - while (kr) {
> + do {
>   if (memcmp(>r.nexthop, , sizeof(nexthop)) == 0)
>   return (kr);
>   kr = kr->next;
> - }
> + } while (kr);
>  
>   return (NULL);
>  }
> @@ -3180,12 +3180,9 @@ fetchtable(struct ktable *kt)
>   int  mib[7];
>   char*buf = NULL, *next, *lim;
>   struct rt_msghdr*rtm;
> - struct sockaddr *sa, *gw, *rti_info[RTAX_MAX];
> - struct sockaddr_in  *sa_in;
> - struct sockaddr_in6 *sa_in6;
> - struct sockaddr_rtlabel *label;
> - struct kroute_node  *kr = NULL;
> - struct kroute6_node *kr6 = NULL;
> + struct kroute_full   kl;
> + struct kroute_node  *kr;
> + struct kroute6_node *kr6;
>  
>   mib[0] = CTL_NET;
>   mib[1] = PF_ROUTE;
> @@ -3219,145 

Re: bgpd: refactor kroute code a fair bit

2022-06-08 Thread Theo Buehler
On Wed, Jun 08, 2022 at 01:28:14PM +0200, Claudio Jeker wrote:
> The idea behind this diff is to use struct kroute_full in more places.
> Mainly when parsing route messages. This allows to factor out the pure
> route message parsing into dispatch_rtmsg_addr() and use it in both the
> gerneral routing socket code but also in fetchtables(). Which removes some
> duplicated code.
> 
> As a second step then this kroute_full is applied to the kroute tree. Here
> I limited my changes to be minimal for now. A follow up will look more
> into kr_fib_change() and tries to streamline that code and the bits in
> fetchtable().
> 
> Also struct kroute_full could be used in more places with the goal to have
> less mostly similar function for IPv4, IPv6 and the L3VPNs. Again this is
> for later.
> 
> This should behave the same as before but dispatch_rtmsg_addr() decides
> now in a different way if a route is connected or not (using the
> RTF_GATEWAY flag for this). With this loopback routes show now up as
> connected. It should have no noticable impact on routing.
> 
> Please give this a good test.

I spent some time with this diff. This is outside my comfort zone.  I'll
need to read this once more with a fresh head.

I like the direction and less spaghetti is good.

Some small remarks and questions inline.

> -- 
> :wq Claudio
> 
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.251
> diff -u -p -r1.251 kroute.c
> --- kroute.c  7 Jun 2022 16:42:07 -   1.251
> +++ kroute.c  7 Jun 2022 17:05:37 -
> @@ -130,7 +130,7 @@ void  kr_fib_update_prio(u_int, uint8_t);
>  struct kroute_node   *kroute_find(struct ktable *, in_addr_t, uint8_t,
>   uint8_t);
>  struct kroute_node   *kroute_matchgw(struct kroute_node *,
> - struct sockaddr_in *);
> + struct bgpd_addr *);
>  int   kroute_insert(struct ktable *, struct kroute_node *);
>  int   kroute_remove(struct ktable *, struct kroute_node *);
>  void  kroute_clear(struct ktable *);
> @@ -138,7 +138,7 @@ void   kroute_clear(struct ktable *);
>  struct kroute6_node  *kroute6_find(struct ktable *, const struct in6_addr *,
>   uint8_t, uint8_t);
>  struct kroute6_node  *kroute6_matchgw(struct kroute6_node *,
> - struct sockaddr_in6 *);
> + struct bgpd_addr *);
>  int   kroute6_insert(struct ktable *, struct kroute6_node *);
>  int   kroute6_remove(struct ktable *, struct kroute6_node *);
>  void  kroute6_clear(struct ktable *);
> @@ -188,8 +188,9 @@ int   send_rt6msg(int, int, struct ktable
>  int  dispatch_rtmsg(u_int);
>  int  fetchtable(struct ktable *);
>  int  fetchifs(int);
> -int  dispatch_rtmsg_addr(struct rt_msghdr *,
> - struct sockaddr *[RTAX_MAX], struct ktable *);
> +int  dispatch_rtmsg_addr(struct rt_msghdr *, struct kroute_full *);
> +int  kr_fib_delete(struct ktable *, struct kroute_full *, int);
> +int  kr_fib_change(struct ktable *, struct kroute_full *, int, int);
>  
>  RB_PROTOTYPE(kroute_tree, kroute_node, entry, kroute_compare)
>  RB_GENERATE(kroute_tree, kroute_node, entry, kroute_compare)
> @@ -1780,21 +1781,21 @@ kroute_find(struct ktable *kt, in_addr_t
>  }
>  
>  struct kroute_node *
> -kroute_matchgw(struct kroute_node *kr, struct sockaddr_in *sa_in)
> +kroute_matchgw(struct kroute_node *kr, struct bgpd_addr *gw)
>  {
>   in_addr_t   nexthop;
>  
> - if (sa_in == NULL) {
> + if (gw->aid != AID_INET) {
>   log_warnx("%s: no nexthop defined", __func__);
>   return (NULL);
>   }
> - nexthop = sa_in->sin_addr.s_addr;
> + nexthop = gw->v4.s_addr;
>  
> - while (kr) {
> + do {
>   if (kr->r.nexthop.s_addr == nexthop)
>   return (kr);
>   kr = kr->next;
> - }
> + } while (kr);
>  
>   return (NULL);
>  }
> @@ -1933,21 +1934,21 @@ kroute6_find(struct ktable *kt, const st
>  }
>  
>  struct kroute6_node *
> -kroute6_matchgw(struct kroute6_node *kr, struct sockaddr_in6 *sa_in6)
> +kroute6_matchgw(struct kroute6_node *kr, struct bgpd_addr *gw)
>  {
>   struct in6_addr nexthop;
>  
> - if (sa_in6 == NULL) {
> + if (gw->aid != AID_INET6) {
>   log_warnx("%s: no nexthop defined", __func__);
>   return (NULL);
>   }
> - memcpy(, _in6->sin6_addr, sizeof(nexthop));
> + nexthop = gw->v6;
>  
> - while (kr) {
> + do {
>   if (memcmp(>r.nexthop, , sizeof(nexthop)) == 0)
>   return (kr);
>   kr = kr->next;
> - }
> + } while (kr);
>  
>   return (NULL);
>  }
> @@ -3180,12 +3181,9 @@ 

Re: bgpd, fix some rtlabel leaks

2022-06-07 Thread Theo Buehler
On Tue, Jun 07, 2022 at 05:49:53PM +0200, Claudio Jeker wrote:
> The kroute code can leak rtlabel references in various conditions.
> In the end we want to drop the reference before free().
> So move the unref into kroute_remove() so it is done mostly in one place.
> On top fix a few other places where kroutes are freed.  

The places you change look correct to me, so

ok tb.

I could not figure out why some of the unchecked kroute_insert() and
kroute6_insert() don't have the same problem as those that you fix.

Around line 3800 your previous commit left a double assignment

kr6->r.labelid = 0;
kr6->r.labelid = new_labelid;

which should be fixed.



Re: bgpd, retire F_RTLABEL flag

2022-06-07 Thread Theo Buehler
On Tue, Jun 07, 2022 at 05:09:30PM +0200, Claudio Jeker wrote:
> The F_RTLABEL flag does nothing. So just remove it.
> Checking the rtlabelid != 0 is equivalent.

Doesn't twiddling the F_RTLABEL potentially change the outcome of the two

if (flags != oflags)
changed = 1;

bits in dispatch_rtmsg_addr()? If that's not a concern or if it's desired,
then this is ok.

Two minor nits below.

> -- 
> :wq Claudio
> 
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.426
> diff -u -p -r1.426 bgpd.h
> --- bgpd.h5 Jun 2022 12:43:13 -   1.426
> +++ bgpd.h7 Jun 2022 15:04:14 -
> @@ -90,7 +90,6 @@
>  #define  F_CTL_ADJ_IN0x2000  /* only set on requests */
>  #define  F_CTL_ADJ_OUT   0x4000  /* only set on requests */
>  #define  F_CTL_BEST  0x8000
> -#define  F_RTLABEL   0x1
>  #define  F_CTL_SSV   0x2 /* only used by bgpctl */
>  #define  F_CTL_INVALID   0x4 /* only set on requests */
>  #define  F_CTL_OVS_VALID 0x8
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.248
> diff -u -p -r1.248 kroute.c
> --- kroute.c  5 Jun 2022 12:43:13 -   1.248
> +++ kroute.c  7 Jun 2022 15:05:00 -
> @@ -3268,7 +3268,6 @@ fetchtable(struct ktable *kt)
>   kr->r.labelid = 0;
>   if ((label = (struct sockaddr_rtlabel *)
>   rti_info[RTAX_LABEL]) != NULL) {
> - kr->r.flags |= F_RTLABEL;
>   kr->r.labelid =
>   rtlabel_name2id(label->sr_label);
>   }
> @@ -3309,7 +3308,6 @@ fetchtable(struct ktable *kt)
>   kr6->r.labelid = 0;
>   if ((label = (struct sockaddr_rtlabel *)
>   rti_info[RTAX_LABEL]) != NULL) {
> - kr6->r.flags |= F_RTLABEL;
>   kr6->r.labelid =
>   rtlabel_name2id(label->sr_label);
>   }
> @@ -3702,15 +3700,12 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
>   rtlabel_name2id(label->sr_label);
>   if (kr->r.labelid != new_labelid) {
>   rtlabel_unref(kr->r.labelid);
> - kr->r.labelid = 0;
> - flags |= F_RTLABEL;
>   kr->r.labelid = new_labelid;
>   rtlabel_changed = 1;
>   }
>   } else if (kr->r.labelid && label == NULL) {

The 'else' corresponds to an 'if (label != NULL)' check, so the
'&& label == NULL' part could go without changing the logic.

>   rtlabel_unref(kr->r.labelid);
>   kr->r.labelid = 0;
> - flags &= ~F_RTLABEL;
>   rtlabel_changed = 1;
>   }
>  
> @@ -3760,7 +3755,6 @@ add4:
>   kr->r.priority = prio;
>  
>   if (label) {
> - kr->r.flags |= F_RTLABEL;
>   kr->r.labelid =
>   rtlabel_name2id(label->sr_label);
>   }
> @@ -3809,14 +3803,12 @@ add4:
>   if (kr6->r.labelid != new_labelid) {
>   rtlabel_unref(kr6->r.labelid);
>   kr6->r.labelid = 0;
> - flags |= F_RTLABEL;
>   kr6->r.labelid = new_labelid;
>   rtlabel_changed = 1;
>   }
>   } else if (kr6->r.labelid && label == NULL) {

ditto re label == NULL.

>   rtlabel_unref(kr6->r.labelid);
>   kr6->r.labelid = 0;
> - flags &= ~F_RTLABEL;
>   rtlabel_changed = 1;
>   }
>  
> @@ -3870,7 +3862,6 @@ add6:
>   kr6->r.priority = prio;
>  
>   if (label) {
> - kr6->r.flags |= F_RTLABEL;
>   kr6->r.labelid =
>   rtlabel_name2id(label->sr_label);
>   }
> 



Re: bgpd: cleanup fib_priority

2022-06-03 Thread Theo Buehler
On Fri, Jun 03, 2022 at 03:33:57PM +0200, Claudio Jeker wrote:
> fib_priority was slapped into the code by passing it around all the time.
> It is much smarter to use a global value in kr_state for the fib_prio and
> then tag F_BGP_INSERTED routes with RTP_MINE (0xff) which is an impossible
> RTP value. With that changing the prio requires "only" a fib decouple,
> change global state, fib couple.
> 
> It will also allow for better support of blackhole routes and makes the
> code a fair bit simpler.

Yes, this is a nicer approach. It reads fine.

ok with one tiny nit

> Index: kroute.c
> ===

[...]

> @@ -950,54 +950,30 @@ kr_fib_decouple(u_int rtableid, uint8_t 
>  
>   RB_FOREACH(kr, kroute_tree, >krt)
>   if ((kr->r.flags & F_BGPD_INSERTED))
> - send_rtmsg(kr_state.fd, RTM_DELETE, kt, >r,
> - fib_prio);
> + send_rtmsg(kr_state.fd, RTM_DELETE, kt, >r);
>   RB_FOREACH(kr6, kroute6_tree, >krt6)
>   if ((kr6->r.flags & F_BGPD_INSERTED))
> - send_rt6msg(kr_state.fd, RTM_DELETE, kt, >r,
> - fib_prio);
> + send_rt6msg(kr_state.fd, RTM_DELETE, kt, >r);
>  
>   kt->fib_sync = 0;
>  
>   log_info("kernel routing table %u (%s) decoupled", kt->rtableid,
> - kt->descr);
> -}
> + kt->descr);
> + }

Please undo this wrong indentation.



Re: libcrypto: altering tbs sigalg on X509 and X509_CRL

2022-06-02 Thread Theo Buehler
On Wed, Jun 01, 2022 at 11:00:12AM +1000, Alex Wilson wrote:
> I'm trying to sign X509 and X509_CRL objects without using X509_sign et al
> -- since the key I'm signing with isn't in the memory of the process doing
> this and you can't write an ENGINE for something that can't sign a
> pre-calculated hash and needs the whole data.
> 
> To do this I have to alter the signing algorithm, both in the outer X509 and
> the inner X509_CINF (and similar on the X509_CRL). Since X509 and X509_CRL
> became opaque, I've had to use some hacks to achieve this:
> 
> For the outer signature and algo I can use X509_get0_signature() and
> X509_CRL_get0_signature() and then cast off the const on the X509_ALGOR.
> Then I can use X509_ALGOR_set0() to set the algorithm in use.
>
> For the X509 inner algorithm I can use X509_get0_tbs_sigalg() and cast the
> const off there, too. But there is no equivalent of this for X509_CRL.

This sounds quite familiar...

> As a result I have a local patch which adds X509_get_tbs_sigalg() (returning
> a non-const pointer) and X509_CRL_get{,0}_tbs_sigalg(). It's very short, so
> I was hoping somebody could let me know if this is a terrible idea and
> whether there is any possibility of it going back in LibreSSL (or if I
> should try to take it up with OpenSSL and then port it back or something?)
>
> I have also considered adding an X509_get_signature() which returns
> non-const, as well. If any of this seems like a decent idea at all, I'm
> happy to add that and write some tests.

I would be ok with adding X509_CRL_get0_tbs_sigalg(), as this looks like
a straight up oversight. Since OpenSSL are quite liberal with adding
accessors, this should also be easy to upstream.

That one function should be good enough for your needs even if it means
some annoying casting away of const. It seems a bit of a fringe use
case, so I would rather not add more functions to the public API for
this.

As an aside, using _get_ is unfortunate and I'd rather avoid it. This
could mean get0, get1, have const or no const, depending on the phase of
Venus... I don't know OpenSSL's current stance on such cases, i.e.,
whether they added more stuff like X509_CRL_get0_tbs_sigalg_noconst().

For now I'd suggest you send a patch for X509_CRL_get0_tbs_sigalg()
with the prototype gated with #if defined(LIBRESSL_CRYPTO_INTERNAL). We
would then expose that with the next library bump, which will almost
certainly happen before 3.6 goes -stable.

If you want to add a short regress, that would be great, but it's not
strictly required for a simple accessor. I'd probably extend
regress/lib/libcrypto/x509/. You'll want static linking and you'll need
to extend CFLAGS with something like

LDADD_foo = ${CRYPTO_INT}
CFLAGS += -DLIBRESSL_CRYPTO_INTERNAL

> 
> Brief patch attached.

> commit 6f3f7990686517b788278fc26e706e2f0c7472cf
> Author: Alex Wilson 
> Date:   Wed Jun 1 10:49:08 2022 +1000
> 
> libcrypto: want to alter tbs sigalg for X509 and X509_CRL
> 
> diff lib/libcrypto/asn1/x_crl.c lib/libcrypto/asn1/x_crl.c
> --- lib/libcrypto/asn1/x_crl.c
> +++ lib/libcrypto/asn1/x_crl.c
> @@ -722,6 +722,18 @@ X509_CRL_get_lastUpdate(X509_CRL *crl)
>   return crl->crl->lastUpdate;
>  }
>  
> +const X509_ALGOR *
> +X509_CRL_get0_tbs_sigalg(const X509_CRL *crl)
> +{
> + return crl->crl->sig_alg;
> +}
> +
> +X509_ALGOR *
> +X509_CRL_get_tbs_sigalg(X509_CRL *crl)
> +{
> + return crl->crl->sig_alg;
> +}
> +
>  const ASN1_TIME *
>  X509_CRL_get0_nextUpdate(const X509_CRL *crl)
>  {
> diff lib/libcrypto/x509/x509.h lib/libcrypto/x509/x509.h
> --- lib/libcrypto/x509/x509.h
> +++ lib/libcrypto/x509/x509.h
> @@ -399,6 +399,8 @@ X509_NAME *X509_CRL_get_issuer(const X509_CRL *crl);
>  STACK_OF(X509_REVOKED) *X509_CRL_get_REVOKED(X509_CRL *crl);
>  void X509_CRL_get0_signature(const X509_CRL *crl, const ASN1_BIT_STRING 
> **psig,
>  const X509_ALGOR **palg);
> +const X509_ALGOR *X509_CRL_get0_tbs_sigalg(const X509_CRL *crl);
> +X509_ALGOR *X509_CRL_get_tbs_sigalg(X509_CRL *crl);
>  
>  int X509_REQ_get_signature_nid(const X509_REQ *req);
>  
> @@ -768,6 +770,7 @@ int ASN1_item_sign_ctx(const ASN1_ITEM *it,
>  
>  const STACK_OF(X509_EXTENSION) *X509_get0_extensions(const X509 *x);
>  const X509_ALGOR *X509_get0_tbs_sigalg(const X509 *x);
> +X509_ALGOR * X509_get_tbs_sigalg(X509 *x);
>  int  X509_set_version(X509 *x, long version);
>  long X509_get_version(const X509 *x);
>  int  X509_set_serialNumber(X509 *x, ASN1_INTEGER *serial);
> diff lib/libcrypto/x509/x509_set.c lib/libcrypto/x509/x509_set.c
> --- lib/libcrypto/x509/x509_set.c
> +++ lib/libcrypto/x509/x509_set.c
> @@ -77,6 +77,12 @@ X509_get0_tbs_sigalg(const X509 *x)
>   return x->cert_info->signature;
>  }
>  
> +X509_ALGOR *
> +X509_get_tbs_sigalg(X509 *x)
> +{
> + return x->cert_info->signature;
> +}
> +
>  int
>  X509_set_version(X509 *x, long version)
>  {



Re: bgpd cleanup RTP_ limit checks in parse.y

2022-06-02 Thread Theo Buehler
On Thu, Jun 02, 2022 at 01:07:07PM +0200, Claudio Jeker wrote:
> On Thu, Jun 02, 2022 at 12:44:49PM +0200, Theo Buehler wrote:
> > On Thu, Jun 02, 2022 at 11:38:05AM +0200, Claudio Jeker wrote:
> > > Lets use the same check for both priority checks in parse.y.
> > > Also rephrase the error messages to be less cryptic.
> > > Both checks do the same check since RTP_NONE = 0 and RTP_LOCAL = 1.
> > 
> > ok
> > 
> > > Using RTP_LOCAL as a priority is actually not possible since that one is
> > > reserved for the kernel (used by interface address entries). So maybe the
> > > check should be 'if ($2 <= RTP_LOCAL'. That would be a followup diff since
> > > that would change behaviour.
> > 
> > That would make sense to me.
> 
> See below for that.

ok

> 
> -- 
> :wq Claudio
> 
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.427
> diff -u -p -r1.427 parse.y
> --- parse.y   2 Jun 2022 11:05:15 -   1.427
> +++ parse.y   2 Jun 2022 11:06:10 -
> @@ -707,9 +707,9 @@ conf_main : AS as4number  {
>   TAILQ_INSERT_TAIL(conf->listen_addrs, la, entry);
>   }
>   | FIBPRIORITY NUMBER{
> - if ($2 < RTP_LOCAL || $2 > RTP_MAX) {
> + if ($2 <= RTP_LOCAL || $2 > RTP_MAX) {
>   yyerror("fib-priority %lld must be between "
> - "%u and %u", $2, RTP_LOCAL, RTP_MAX);
> + "%u and %u", $2, RTP_LOCAL + 1, RTP_MAX);
>   YYERROR;
>   }
>   conf->fib_priority = $2;
> @@ -1045,9 +1045,9 @@ network : NETWORK prefix filter_set {
>   }
>   | NETWORK family PRIORITY NUMBER filter_set {
>   struct network  *n;
> - if ($4 < RTP_LOCAL && $4 > RTP_MAX) {
> + if ($4 <= RTP_LOCAL && $4 > RTP_MAX) {
>   yyerror("priority %lld must be between "
> - "%u and %u", $4, RTP_LOCAL, RTP_MAX);
> + "%u and %u", $4, RTP_LOCAL + 1, RTP_MAX);
>   YYERROR;
>   }
>  



Re: bgpd cleanup RTP_ limit checks in parse.y

2022-06-02 Thread Theo Buehler
On Thu, Jun 02, 2022 at 11:38:05AM +0200, Claudio Jeker wrote:
> Lets use the same check for both priority checks in parse.y.
> Also rephrase the error messages to be less cryptic.
> Both checks do the same check since RTP_NONE = 0 and RTP_LOCAL = 1.

ok

> Using RTP_LOCAL as a priority is actually not possible since that one is
> reserved for the kernel (used by interface address entries). So maybe the
> check should be 'if ($2 <= RTP_LOCAL'. That would be a followup diff since
> that would change behaviour.

That would make sense to me.

> 
> -- 
> :wq Claudio
> 
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.426
> diff -u -p -r1.426 parse.y
> --- parse.y   2 Jun 2022 09:29:34 -   1.426
> +++ parse.y   2 Jun 2022 09:30:34 -
> @@ -707,8 +707,9 @@ conf_main : AS as4number  {
>   TAILQ_INSERT_TAIL(conf->listen_addrs, la, entry);
>   }
>   | FIBPRIORITY NUMBER{
> - if ($2 <= RTP_NONE || $2 > RTP_MAX) {
> - yyerror("invalid fib-priority");
> + if ($2 < RTP_LOCAL || $2 > RTP_MAX) {
> + yyerror("fib-priority %lld must be between "
> + "%u and %u", $2, RTP_LOCAL, RTP_MAX);
>   YYERROR;
>   }
>   conf->fib_priority = $2;
> @@ -1045,8 +1046,8 @@ network : NETWORK prefix filter_set {
>   | NETWORK family PRIORITY NUMBER filter_set {
>   struct network  *n;
>   if ($4 < RTP_LOCAL && $4 > RTP_MAX) {
> - yyerror("priority %lld > max %d or < min %d", 
> $4,
> - RTP_MAX, RTP_LOCAL);
> + yyerror("priority %lld must be between "
> + "%u and %u", $4, RTP_LOCAL, RTP_MAX);
>   YYERROR;
>   }
>  
> 



Re: bgpd, check ktable_exists return value

2022-06-02 Thread Theo Buehler
On Thu, Jun 02, 2022 at 11:05:26AM +0200, Claudio Jeker wrote:
> When setting the default routing table for bgpd make sure that
> ktable_exists() does not fail.
> Also improve the warning message in ktable_exists() a bit.

Sure, ok.

The existing checks in parse.y do 'if (ktable_exists(..) != 1)' and the
check in kroute.c uses 'if (!ktable_exists(..))'. Maybe make them all
use the same variant?

> 
> -- 
> :wq Claudio
> 
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.246
> diff -u -p -r1.246 kroute.c
> --- kroute.c  23 May 2022 13:40:12 -  1.246
> +++ kroute.c  2 Jun 2022 08:59:10 -
> @@ -440,7 +440,7 @@ ktable_exists(u_int rtableid, u_int *rdo
>   if (errno == ENOENT)
>   /* table nonexistent */
>   return (0);
> - log_warn("%s: sysctl", __func__);
> + log_warn("sysctl net.route.rtableid");
>   /* must return 0 so that the table is considered non-existent */
>   return (0);
>   }
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> retrieving revision 1.425
> diff -u -p -r1.425 parse.y
> --- parse.y   31 May 2022 09:45:33 -  1.425
> +++ parse.y   2 Jun 2022 08:53:43 -
> @@ -3496,7 +3496,9 @@ init_config(struct bgpd_config *c)
>   c->bgpid = get_bgpid();
>   c->fib_priority = RTP_BGP;
>   c->default_tableid = getrtable();
> - ktable_exists(c->default_tableid, );
> + if (!ktable_exists(c->default_tableid, ))
> + fatalx("current routing table %u does not exist",
> + c->default_tableid);
>   if (rdomid != c->default_tableid)
>   fatalx("current routing table %u is not a routing domain",
>   c->default_tableid);
> 



rpki-client: limit number of RSC checklist entries?

2022-06-01 Thread Theo Buehler
When compared to manifest FileAndHash, the RSC code doesn't limit the
size of the FileNameAndHash list. Should we do this for consistency?

The situation is of course not quite the same since we're in -f mode.
However, we do impose limits on the sizes of other resources, so it
looks like a missing check.

Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.140
diff -u -p -r1.140 extern.h
--- extern.h31 May 2022 18:41:43 -  1.140
+++ extern.h31 May 2022 20:35:41 -
@@ -700,6 +700,9 @@ int mkpathat(int, const char *);
 
 /* Maximum acceptable file size */
 #define MAX_FILE_SIZE  400
+
+/* Maximum number of FileNameAndHash entries per RSC checklist. */
+#define MAX_CHECKLIST_ENTRIES  10
 
 /* Maximum number of FileAndHash entries per manifest. */
 #define MAX_MANIFEST_ENTRIES   10
Index: mft.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
retrieving revision 1.69
diff -u -p -r1.69 mft.c
--- mft.c   31 May 2022 18:51:35 -  1.69
+++ mft.c   1 Jun 2022 06:34:39 -
@@ -323,7 +323,7 @@ mft_parse_econtent(const unsigned char *
goto out;
}
 
-   if (sk_FileAndHash_num(mft->fileList) > MAX_MANIFEST_ENTRIES) {
+   if (sk_FileAndHash_num(mft->fileList) >= MAX_MANIFEST_ENTRIES) {
warnx("%s: %d exceeds manifest entry limit (%d)", p->fn,
sk_FileAndHash_num(mft->fileList), MAX_MANIFEST_ENTRIES);
goto out;
Index: rsc.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/rsc.c,v
retrieving revision 1.7
diff -u -p -r1.7 rsc.c
--- rsc.c   31 May 2022 18:51:35 -  1.7
+++ rsc.c   1 Jun 2022 06:36:15 -
@@ -279,6 +279,12 @@ rsc_parse_checklist(struct parse *p, con
return 0;
}
 
+   if (sz >= MAX_CHECKLIST_ENTRIES) {
+   warnx("%s: %zu exceeds checklist entry limit (%d)", p->fn, sz,
+   MAX_CHECKLIST_ENTRIES);
+   return 0;
+   }
+
p->res->files = calloc(sz, sizeof(struct rscfile));
if (p->res->files == NULL)
err(1, NULL);



TLSv1.3 PSK: add support for psk_key_exchange_modes extension

2022-05-31 Thread Theo Buehler
The diff below implements sending and parsing the psk_key_exchange_modes
extension. Only PSK_DHE_KE will be supported, so clients only indicate
support for this mode and servers ignore all other modes (i.e., PSK_KE).

This is currently gated behind a use_psk_dhe_ke Boolean in the TLSv1.3
handshake struct, which isn't set client side and ignored server side.

The diff also adds boiler plate for the pre-shared key extension. Due to
the way the transcript hash for PSK binders is calculated, clients MUST
send this as the last extension, so the extension is added to the end of
tls_extensions[] which makes sure of this for both clients and servers.

Index: ssl_locl.h
===
RCS file: /cvs/src/lib/libssl/ssl_locl.h,v
retrieving revision 1.388
diff -u -p -r1.388 ssl_locl.h
--- ssl_locl.h  17 Mar 2022 17:22:16 -  1.388
+++ ssl_locl.h  31 May 2022 13:16:07 -
@@ -548,6 +548,9 @@ typedef struct ssl_handshake_tls13_st {
int use_legacy;
int hrr;
 
+   /* Client indicates psk_dhe_ke support in PskKeyExchangeMode. */
+   int use_psk_dhe_ke;
+
/* Certificate selected for use (static pointer). */
const SSL_CERT_PKEY *cpk;
 
Index: ssl_tlsext.c
===
RCS file: /cvs/src/lib/libssl/ssl_tlsext.c,v
retrieving revision 1.110
diff -u -p -r1.110 ssl_tlsext.c
--- ssl_tlsext.c5 Feb 2022 14:54:10 -   1.110
+++ ssl_tlsext.c31 May 2022 13:15:05 -
@@ -1832,6 +1832,119 @@ tlsext_cookie_client_parse(SSL *s, uint1
return 0;
 }
 
+/*
+ * Pre-Shared Key Exchange Modes - RFC 8446, 4.2.9.
+ */
+
+int
+tlsext_psk_key_exchange_modes_client_needs(SSL *s, uint16_t msg_type)
+{
+   return (s->s3->hs.tls13.use_psk_dhe_ke &&
+   s->s3->hs.our_max_tls_version >= TLS1_3_VERSION);
+}
+
+int
+tlsext_psk_key_exchange_modes_client_build(SSL *s, uint16_t msg_type, CBB *cbb)
+{
+   CBB ke_modes;
+
+   if (!CBB_add_u8_length_prefixed(cbb, _modes))
+   return 0;
+
+   /* Do not indicate support for PSK-only key establishment. */
+   if (!CBB_add_u8(_modes, TLS13_PSK_DHE_KE))
+   return 0;
+
+   if (!CBB_flush(cbb))
+   return 0;
+
+   return 1;
+}
+
+int
+tlsext_psk_key_exchange_modes_server_parse(SSL *s, uint16_t msg_type, CBS *cbs,
+int *alert)
+{
+   CBS ke_modes;
+   uint8_t ke_mode;
+
+   if (!CBS_get_u8_length_prefixed(cbs, _modes))
+   return 0;
+
+   while (CBS_len(_modes) > 0) {
+   if (!CBS_get_u8(_modes, _mode))
+   return 0;
+
+   if (ke_mode == TLS13_PSK_DHE_KE)
+   s->s3->hs.tls13.use_psk_dhe_ke = 1;
+   }
+
+   return 1;
+}
+
+/* Servers MUST NOT send this extension. */
+
+int
+tlsext_psk_key_exchange_modes_server_needs(SSL *s, uint16_t msg_type)
+{
+   return 0;
+}
+
+int
+tlsext_psk_key_exchange_modes_server_build(SSL *s, uint16_t msg_type, CBB *cbb)
+{
+   return 0;
+}
+
+int
+tlsext_psk_key_exchange_modes_client_parse(SSL *s, uint16_t msg_type, CBS *cbs,
+int *alert)
+{
+   return 0;
+}
+
+/*
+ * Pre-Shared Key Extension - RFC 8446, 4.2.11
+ */
+
+int
+tlsext_pre_shared_key_client_needs(SSL *s, uint16_t msg_type)
+{
+   return 0;
+}
+
+int
+tlsext_pre_shared_key_client_build(SSL *s, uint16_t msg_type, CBB *cbb)
+{
+   return 0;
+}
+
+int
+tlsext_pre_shared_key_client_parse(SSL *s, uint16_t msg_type, CBS *cbs,
+int *alert)
+{
+   return 0;
+}
+
+int
+tlsext_pre_shared_key_server_needs(SSL *s, uint16_t msg_type)
+{
+   return 0;
+}
+
+int
+tlsext_pre_shared_key_server_build(SSL *s, uint16_t msg_type, CBB *cbb)
+{
+   return 0;
+}
+
+int
+tlsext_pre_shared_key_server_parse(SSL *s, uint16_t msg_type, CBS *cbs,
+int *alert)
+{
+   return 0;
+}
+
 struct tls_extension_funcs {
int (*needs)(SSL *s, uint16_t msg_type);
int (*build)(SSL *s, uint16_t msg_type, CBB *cbb);
@@ -2018,8 +2131,38 @@ static const struct tls_extension tls_ex
.build = tlsext_srtp_server_build,
.parse = tlsext_srtp_server_parse,
},
-   }
+   },
 #endif /* OPENSSL_NO_SRTP */
+   {
+   .type = TLSEXT_TYPE_psk_key_exchange_modes,
+   .messages = SSL_TLSEXT_MSG_CH,
+   .client = {
+   .needs = tlsext_psk_key_exchange_modes_client_needs,
+   .build = tlsext_psk_key_exchange_modes_client_build,
+   .parse = tlsext_psk_key_exchange_modes_client_parse,
+   },
+   .server = {
+   .needs = tlsext_psk_key_exchange_modes_server_needs,
+   .build = tlsext_psk_key_exchange_modes_server_build,
+   .parse = tlsext_psk_key_exchange_modes_server_parse,
+   },
+   },
+   {
+   

TLSv1.3 PSK: reject PSK without psk_key_exchange_modes

2022-05-31 Thread Theo Buehler
RFC 8446, 4.2.9:
   A client MUST provide a "psk_key_exchange_modes" extension if it
   offers a "pre_shared_key" extension.  If clients offer
   "pre_shared_key" without a "psk_key_exchange_modes" extension,
   servers MUST abort the handshake.

The check below will make servers abort the handshake with a
missing_extension alert. Since we don't support these extensions
(i.e., we ignore them), this is currently a noop.

Index: tls13_server.c
===
RCS file: /cvs/src/lib/libssl/tls13_server.c,v
retrieving revision 1.96
diff -u -p -r1.96 tls13_server.c
--- tls13_server.c  3 Feb 2022 16:33:12 -   1.96
+++ tls13_server.c  31 May 2022 13:02:49 -
@@ -108,10 +108,15 @@ tls13_client_hello_required_extensions(s
 */
 
/*
-* If we got no pre_shared_key, then signature_algorithms and
-* supported_groups must both be present.
+* RFC 8446, 4.2.9: if we got a pre_shared_key, then we also need
+* psk_key_exchange_modes. Otherwise, section 9.2 specifies that we
+* need both signature_algorithms and supported_groups.
 */
-   if (!tlsext_extension_seen(s, TLSEXT_TYPE_pre_shared_key)) {
+   if (tlsext_extension_seen(s, TLSEXT_TYPE_pre_shared_key)) {
+   if (!tlsext_extension_seen(s,
+   TLSEXT_TYPE_psk_key_exchange_modes))
+   return 0;
+   } else {
if (!tlsext_extension_seen(s, TLSEXT_TYPE_signature_algorithms))
return 0;
if (!tlsext_extension_seen(s, TLSEXT_TYPE_supported_groups))



rpki-client: implement rsc-08.txt with templates

2022-05-31 Thread Theo Buehler
I chose to implement the constrained versions of the RFC 3779 types from
the draft because the OpenSSL RFC 3779 code has static IPAddrBlocks_it,
so we have to work around that anyway. This isn't quite minimal, but it
avoids asymmetry between ASIdentifiers and IPAddrBlocks and it's cleaner
than reusing as many of the available RFC 3779 types as possible (which
also means additional checks either when walking the structs or after).

The diff has three parts that build on top of each other. There is no
overlap outside of extern.h, so it should not make the review harder.

The mechanical cert.c diff adjusts some sbgp_addr_*() and sbgp_as_*() to
remove the struct parse argument so that we can use them from rsc.c.

The rsc.c diff is the tricky part: it switches to templates and uses the
cert.c functions. rsc_parse_aslist() and rsc_parse_iplist() are similar
to sbgp_assysnum() and sbgp_ipaddrblk(), but somewhat easier. We get
rid of the copy-paste XXXs and the last bit of low level ASN.1 fiddling. 

Remove the unused ASN1_frame() and cms_econtent_version() from cms.c.

Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.82
diff -u -p -r1.82 cert.c
--- cert.c  15 May 2022 15:00:53 -  1.82
+++ cert.c  31 May 2022 10:46:00 -
@@ -58,11 +58,12 @@ extern ASN1_OBJECT  *notify_oid;/* 1.3.6
  * Returns zero on failure (IP overlap) non-zero on success.
  */
 static int
-append_ip(struct parse *p, const struct cert_ip *ip)
+append_ip(const char *fn, struct cert_ip *ips, size_t *ipsz,
+const struct cert_ip *ip)
 {
-   if (!ip_addr_check_overlap(ip, p->fn, p->res->ips, p->res->ipsz))
+   if (!ip_addr_check_overlap(ip, fn, ips, *ipsz))
return 0;
-   p->res->ips[p->res->ipsz++] = *ip;
+   ips[(*ipsz)++] = *ip;
return 1;
 }
 
@@ -72,11 +73,12 @@ append_ip(struct parse *p, const struct 
  * as defined by RFC 3779 section 3.3.
  */
 static int
-append_as(struct parse *p, const struct cert_as *as)
+append_as(const char *fn, struct cert_as *ases, size_t *asz,
+const struct cert_as *as)
 {
-   if (!as_check_overlap(as, p->fn, p->res->as, p->res->asz))
+   if (!as_check_overlap(as, fn, ases, *asz))
return 0;
-   p->res->as[p->res->asz++] = *as;
+   ases[(*asz)++] = *as;
return 1;
 }
 
@@ -84,8 +86,9 @@ append_as(struct parse *p, const struct 
  * Parse a range of AS identifiers as in 3.2.3.8.
  * Returns zero on failure, non-zero on success.
  */
-static int
-sbgp_asrange(struct parse *p, const ASRange *range)
+int
+sbgp_as_range(const char *fn, struct cert_as *ases, size_t *asz,
+const ASRange *range)
 {
struct cert_as   as;
 
@@ -94,34 +97,35 @@ sbgp_asrange(struct parse *p, const ASRa
 
if (!as_id_parse(range->min, )) {
warnx("%s: RFC 3779 section 3.2.3.8 (via RFC 1930): "
-   "malformed AS identifier", p->fn);
+   "malformed AS identifier", fn);
return 0;
}
 
if (!as_id_parse(range->max, )) {
warnx("%s: RFC 3779 section 3.2.3.8 (via RFC 1930): "
-   "malformed AS identifier", p->fn);
+   "malformed AS identifier", fn);
return 0;
}
 
if (as.range.max == as.range.min) {
warnx("%s: RFC 3379 section 3.2.3.8: ASRange: "
-   "range is singular", p->fn);
+   "range is singular", fn);
return 0;
} else if (as.range.max < as.range.min) {
warnx("%s: RFC 3379 section 3.2.3.8: ASRange: "
-   "range is out of order", p->fn);
+   "range is out of order", fn);
return 0;
}
 
-   return append_as(p, );
+   return append_as(fn, ases, asz, );
 }
 
 /*
  * Parse an entire 3.2.3.10 integer type.
  */
-static int
-sbgp_asid(struct parse *p, const ASN1_INTEGER *i)
+int
+sbgp_as_id(const char *fn, struct cert_as *ases, size_t *asz,
+const ASN1_INTEGER *i)
 {
struct cert_as   as;
 
@@ -130,27 +134,27 @@ sbgp_asid(struct parse *p, const ASN1_IN
 
if (!as_id_parse(i, )) {
warnx("%s: RFC 3779 section 3.2.3.10 (via RFC 1930): "
-   "malformed AS identifier", p->fn);
+   "malformed AS identifier", fn);
return 0;
}
if (as.id == 0) {
warnx("%s: RFC 3779 section 3.2.3.10 (via RFC 1930): "
-   "AS identifier zero is reserved", p->fn);
+   "AS identifier zero is reserved", fn);
return 0;
}
 
-   return append_as(p, );
+   return append_as(fn, ases, asz, );
 }
 
 static int
-sbgp_asinherit(struct parse *p)
+sbgp_as_inherit(const char *fn, struct cert_as *ases, size_t *asz)
 {
struct cert_as as;
 
memset(, 0, sizeof(struct 

Re: ix(4): Add support for TCP Large Receive Offloading

2022-05-31 Thread Theo Buehler
> smc24# cd /usr/src && make includes

Do 'cd /usr/src && make obj' first.



Re: bgpd implement max-communities filter

2022-05-25 Thread Theo Buehler
On Wed, May 25, 2022 at 07:09:04PM +0200, Claudio Jeker wrote:
> This introduces max-communities, max-ext-communities and
> max-large-communities for filters which allows to limit the number of
> communities in a path.
> 
> The 3 filters can all be used together and also with other match filters
> including community X:Y.

ok

> Index: rde_community.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde_community.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 rde_community.c
> --- rde_community.c   25 May 2022 16:03:34 -  1.5
> +++ rde_community.c   25 May 2022 16:59:24 -
> @@ -283,6 +283,45 @@ struct rde_peer *peer)
>  }
>  
>  /*
> + * Count the number of communities of type type.
> + */
> +int
> +community_count(struct rde_community *comm, uint8_t type)
> +{
> + size_t l;
> + int count = 0;
> + 

previous line has an extra tab



Re: bgpd fix for non-transitive extended communities

2022-05-25 Thread Theo Buehler
On Wed, May 25, 2022 at 04:27:46PM +0200, Claudio Jeker wrote:
> On Wed, May 25, 2022 at 03:50:38PM +0200, Theo Buehler wrote:
> > On Wed, May 25, 2022 at 02:46:55PM +0200, Claudio Jeker wrote:
> > > Noticed this morning working on something else. The non-transitive
> > > extended community support is fairly messed up. In my case (set
> > > ext-community rt 1:1) it causes the community to be skipped.
> > > 
> > > First of all the check in non_transitive_community() is messed up. It uses
> > > the wrong data field, does a unneccessary ntohl and the logic seems also
> > > to be reversed.
> > > 
> > > While looking at this I decided to fix the various switch statements on
> > > type to mask out the two high bits with this extending ext community types
> > > should be a bit easier.
> > >
> > > While there be strict from where bgpd accepts non-transitive
> > > ext-communities. Now non-transitive ext communities are filtered in and 
> > > out
> > > for ebgp sessions. So non-transitive ext communities only pass on ibgp
> > > sessions.
> > > 
> > > For the rde_attr_add() the system defaults to ibgp since this is for
> > > network announcements.
> > > 
> > > There is also a trivial fix to the bgpd unittests that I excluded here.
> > > 
> > > With this ext-communities are properly sent on updates.
> > > OK?
> > 
> > Makes sense, one comment below.
> > 
> > > -- 
> > > :wq Claudio
> > > 
> > > Index: rde_community.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/bgpd/rde_community.c,v
> > > retrieving revision 1.4
> > > diff -u -p -r1.4 rde_community.c
> > > --- rde_community.c   6 Feb 2022 09:51:19 -   1.4
> > > +++ rde_community.c   25 May 2022 12:30:44 -
> > > @@ -99,7 +99,7 @@ fc2c(struct community *fc, struct rde_pe
> > >   subtype = fc->data3 & 0xff;
> > >  
> > >   c->data3 = type << 8 | subtype;
> > > - switch (type) {
> > > + switch (type & EXT_COMMUNITY_VALUE) {
> > >   case -1:
> > 
> > Should that '-1' not be changed to 'EXT_COMMUNITY_VALUE'?  Since you now
> > mask out the high two bits, it is no longer possible to reach this case.
> 
> Good point. I decided to refactor the code a but and move the -1 case into
> its own if statement above the switch. I think this is the best solution
> here. I also did tidy up some types. 

parseextcommunity(), parseextvalue() and parsesubtype() still use an int
for type and log_ext_subtype() uses a short. Not sure if you want to
adjust them as well.

The diff reads ok now.

> 
> -- 
> :wq Claudio
> 
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.423
> diff -u -p -r1.423 bgpd.h
> --- bgpd.h23 May 2022 13:40:11 -  1.423
> +++ bgpd.h25 May 2022 14:14:24 -
> @@ -989,7 +989,7 @@ struct filter_peers {
>  #define EXT_COMMUNITY_FLAG_VALID 0x01
>  
>  struct ext_comm_pairs {
> - short   type;
> + uint8_t type;
>   uint8_t subtype;
>   const char  *subname;
>  };
> Index: rde.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.545
> diff -u -p -r1.545 rde.c
> --- rde.c 6 May 2022 15:51:09 -   1.545
> +++ rde.c 25 May 2022 12:30:44 -
> @@ -1917,8 +1917,8 @@ bad_flags:
>   if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE,
>   ATTR_PARTIAL))
>   goto bad_flags;
> - if (community_ext_add(>communities, flags, p,
> - attr_len) == -1) {
> + if (community_ext_add(>communities, flags,
> + peer->conf.ebgp, p, attr_len) == -1) {
>   /*
>* mark update as bad and withdraw all routes as per
>* RFC 7606
> @@ -2062,8 +2062,8 @@ rde_attr_add(struct filterstate *state, 
>   return community_large_add(>communities, flags, p,
>   attr_len);
>   case ATTR_EXT_COMMUNITIES:
> - return community_ext_add(>communities, flags, p,
> - attr_len);
> + return community_ext_add(>communities, flags, 0,
> + p, attr_len);
>   }
>  
>   i

Re: bgpd fix for non-transitive extended communities

2022-05-25 Thread Theo Buehler
On Wed, May 25, 2022 at 02:46:55PM +0200, Claudio Jeker wrote:
> Noticed this morning working on something else. The non-transitive
> extended community support is fairly messed up. In my case (set
> ext-community rt 1:1) it causes the community to be skipped.
> 
> First of all the check in non_transitive_community() is messed up. It uses
> the wrong data field, does a unneccessary ntohl and the logic seems also
> to be reversed.
> 
> While looking at this I decided to fix the various switch statements on
> type to mask out the two high bits with this extending ext community types
> should be a bit easier.
>
> While there be strict from where bgpd accepts non-transitive
> ext-communities. Now non-transitive ext communities are filtered in and out
> for ebgp sessions. So non-transitive ext communities only pass on ibgp
> sessions.
> 
> For the rde_attr_add() the system defaults to ibgp since this is for
> network announcements.
> 
> There is also a trivial fix to the bgpd unittests that I excluded here.
> 
> With this ext-communities are properly sent on updates.
> OK?

Makes sense, one comment below.

> -- 
> :wq Claudio
> 
> Index: rde.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.545
> diff -u -p -r1.545 rde.c
> --- rde.c 6 May 2022 15:51:09 -   1.545
> +++ rde.c 25 May 2022 12:30:44 -
> @@ -1917,8 +1917,8 @@ bad_flags:
>   if (!CHECK_FLAGS(flags, ATTR_OPTIONAL|ATTR_TRANSITIVE,
>   ATTR_PARTIAL))
>   goto bad_flags;
> - if (community_ext_add(>communities, flags, p,
> - attr_len) == -1) {
> + if (community_ext_add(>communities, flags,
> + peer->conf.ebgp, p, attr_len) == -1) {
>   /*
>* mark update as bad and withdraw all routes as per
>* RFC 7606
> @@ -2062,8 +2062,8 @@ rde_attr_add(struct filterstate *state, 
>   return community_large_add(>communities, flags, p,
>   attr_len);
>   case ATTR_EXT_COMMUNITIES:
> - return community_ext_add(>communities, flags, p,
> - attr_len);
> + return community_ext_add(>communities, flags, 0,
> + p, attr_len);
>   }
>  
>   if (attr_optadd(>aspath, flags, type, p, attr_len) == -1)
> Index: rde.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.251
> diff -u -p -r1.251 rde.h
> --- rde.h 22 Mar 2022 10:53:08 -  1.251
> +++ rde.h 25 May 2022 12:30:44 -
> @@ -459,7 +459,7 @@ void  community_delete(struct rde_communi
>  
>  int  community_add(struct rde_community *, int, void *, size_t);
>  int  community_large_add(struct rde_community *, int, void *, size_t);
> -int  community_ext_add(struct rde_community *, int, void *, size_t);
> +int  community_ext_add(struct rde_community *, int, int, void *, size_t);
>  
>  int  community_write(struct rde_community *, void *, uint16_t);
>  int  community_large_write(struct rde_community *, void *, uint16_t);
> Index: rde_community.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde_community.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 rde_community.c
> --- rde_community.c   6 Feb 2022 09:51:19 -   1.4
> +++ rde_community.c   25 May 2022 12:30:44 -
> @@ -99,7 +99,7 @@ fc2c(struct community *fc, struct rde_pe
>   subtype = fc->data3 & 0xff;
>  
>   c->data3 = type << 8 | subtype;
> - switch (type) {
> + switch (type & EXT_COMMUNITY_VALUE) {
>   case -1:

Should that '-1' not be changed to 'EXT_COMMUNITY_VALUE'?  Since you now
mask out the high two bits, it is no longer possible to reach this case.

>   /* special case for 'ext-community rt *' */
>   if ((fc->flags >> 8 & 0xff) != COMMUNITY_ANY ||
> @@ -141,7 +141,6 @@ fc2c(struct community *fc, struct rde_pe
>   return 0;
>   case EXT_COMMUNITY_TRANS_OPAQUE:
>   case EXT_COMMUNITY_TRANS_EVPN:
> - case EXT_COMMUNITY_NON_TRANS_OPAQUE:
>   if ((fc->flags >> 8 & 0xff) == COMMUNITY_ANY)
>   break;
>  
> @@ -244,10 +243,9 @@ insert_community(struct rde_community *c
>  }
>  
>  static int
> -non_transitive_community(struct community *c)
> +non_transitive_ext_community(struct community *c)
>  {
> - if ((uint8_t)c->flags == COMMUNITY_TYPE_EXT &&
> - !((ntohl(c->data1) >> 24) & EXT_COMMUNITY_NON_TRANSITIVE))
> + if ((c->data3 >> 8) & EXT_COMMUNITY_NON_TRANSITIVE)
>   return 1;
>   return 0;
>  }
> @@ -404,7 +402,8 @@ community_large_add(struct rde_community
>  }
>  
>  int
> -community_ext_add(struct 

Re: rpki-client unify max request defines

2022-05-24 Thread Theo Buehler
On Tue, May 24, 2022 at 10:03:31AM +0200, Claudio Jeker wrote:
> This diff moves and renames the defines that define the maximum number of
> parallel requests for http and rsync.
> The defines are now MAX_HTTP_REQUESTS and MAX_RSYNC_REQUESTS the values
> remane the same.

ok

> Also move the memset of pollfd sets in http.c into the loop. It is not
> needed but I prefer it that way.

Also ok. I would commit this separately as it is completely unrelated.

> 
> -- 
> :wq Claudio
> 
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.137
> diff -u -p -r1.137 extern.h
> --- extern.h  11 May 2022 21:19:06 -  1.137
> +++ extern.h  24 May 2022 07:44:22 -
> @@ -702,8 +702,9 @@ int   mkpathat(int, const char *);
>  /* Maximum depth of the RPKI tree. */
>  #define MAX_CERT_DEPTH   12
>  
> -/* Maximum number of concurrent rsync processes. */
> -#define MAX_RSYNC_PROCESSES  16
> +/* Maximum number of concurrent http and rsync requests. */
> +#define MAX_HTTP_REQUESTS64
> +#define MAX_RSYNC_REQUESTS   16
>  
>  /* Maximum allowd repositories per tal */
>  #define MAX_REPO_PER_TAL 1000
> Index: http.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 http.c
> --- http.c15 May 2022 16:43:34 -  1.60
> +++ http.c24 May 2022 07:44:22 -
> @@ -71,9 +71,8 @@
>  #define HTTP_BUF_SIZE(32 * 1024)
>  #define HTTP_IDLE_TIMEOUT10
>  #define HTTP_IO_TIMEOUT  (3 * 60)
> -#define MAX_CONNECTIONS  64
>  #define MAX_CONTENTLEN   (2 * 1024 * 1024 * 1024LL)
> -#define NPFDS(MAX_CONNECTIONS + 1)
> +#define NPFDS(MAX_HTTP_REQUESTS + 1)
>  
>  enum res {
>   DONE,
> @@ -620,7 +619,7 @@ http_req_schedule(struct http_request *r
>   return 1;
>   }
>  
> - if (http_conn_count < MAX_CONNECTIONS) {
> + if (http_conn_count < MAX_HTTP_REQUESTS) {
>   http_new(req);
>   return 1;
>   }
> @@ -1793,8 +1792,6 @@ proc_http(char *bind_addr, int fd)
>   if (pledge("stdio inet dns recvfd", NULL) == -1)
>   err(1, "pledge");
>  
> - memset(, 0, sizeof(pfds));
> -
>   msgbuf_init();
>   msgq.fd = fd;
>  
> @@ -1803,6 +1800,7 @@ proc_http(char *bind_addr, int fd)
>   int timeout;
>   size_t i;
>  
> + memset(, 0, sizeof(pfds));
>   pfds[0].fd = fd;
>   pfds[0].events = POLLIN;
>   if (msgq.queued)
> Index: rsync.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/rsync.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 rsync.c
> --- rsync.c   20 Apr 2022 15:38:24 -  1.37
> +++ rsync.c   24 May 2022 07:44:22 -
> @@ -147,7 +147,7 @@ proc_rsync(char *prog, char *bind_addr, 
>   struct msgbufmsgq;
>   struct ibuf *b, *inbuf = NULL;
>   sigset_t mask, oldmask;
> - struct rsyncproc ids[MAX_RSYNC_PROCESSES] = { 0 };
> + struct rsyncproc ids[MAX_RSYNC_REQUESTS] = { 0 };
>  
>   if (pledge("stdio rpath proc exec unveil", NULL) == -1)
>   err(1, "pledge");
> @@ -211,7 +211,7 @@ proc_rsync(char *prog, char *bind_addr, 
>   int st;
>  
>   pfd.events = 0;
> - if (nprocs < MAX_RSYNC_PROCESSES)
> + if (nprocs < MAX_RSYNC_REQUESTS)
>   pfd.events |= POLLIN;
>   if (msgq.queued)
>   pfd.events |= POLLOUT;
> @@ -230,10 +230,10 @@ proc_rsync(char *prog, char *bind_addr, 
>   while ((pid = waitpid(WAIT_ANY, , WNOHANG)) > 0) {
>   int ok = 1;
>  
> - for (i = 0; i < MAX_RSYNC_PROCESSES; i++)
> + for (i = 0; i < MAX_RSYNC_REQUESTS; i++)
>   if (ids[i].pid == pid)
>   break;
> - if (i >= MAX_RSYNC_PROCESSES)
> + if (i >= MAX_RSYNC_REQUESTS)
>   errx(1, "waitpid: %d unexpected", pid);
>  
>   if (!WIFEXITED(st)) {
> @@ -278,7 +278,7 @@ proc_rsync(char *prog, char *bind_addr, 
>  
>   if (!(pfd.revents & POLLIN))
>   continue;
> - if (nprocs >= MAX_RSYNC_PROCESSES)
> + if (nprocs >= MAX_RSYNC_REQUESTS)
>   continue;
>  
>   b = io_buf_read(fd, );
> @@ -340,10 +340,10 @@ proc_rsync(char *prog, char *bind_addr, 
>  
>   /* Augment the list of running processes. */
>  
> - for (i 

rpki-client: ASN.1 bit string flag errors

2022-05-12 Thread Theo Buehler
ip_addr_parse() sticks its fingers into undocumented API surface of
libcrypto. What is true is that the unused bit count is in the lower
three bits of p->flags, provided that ASN1_STRING_FLAG_BITS_LEFT is set.
This is "documented" in asn1.h 

#define ASN1_STRING_FLAG_BITS_LEFT 0x08 /* Set if 0x07 has bits left value */

What the flags beyond 0x08 mean is mostly unspecified. They a priori
don't have anything to do with the bit string's unused bits, so the
checks and errors make no sense.

Maybe we should add a check of the form

if (p->flags & ~(ASN1_STRING_FLAG_BITS_LEFT | 0x07)) {
warnx("%s: unexpected flags set in address bit string", fn);
return 0;
}

since equivalent checks have been working so far, but I'm not sure.
This complains about an implementation detail, not on anything directly
related to the (mis)parsing of a cert, roa, or ...

Index: ip.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/ip.c,v
retrieving revision 1.22
diff -u -p -r1.22 ip.c
--- ip.c11 May 2022 18:48:35 -  1.22
+++ ip.c12 May 2022 11:20:52 -
@@ -189,17 +189,9 @@ ip_addr_parse(const ASN1_BIT_STRING *p,
/* Weird OpenSSL-ism to get unused bit count. */
 
if ((p->flags & ASN1_STRING_FLAG_BITS_LEFT))
-   unused = p->flags & ~ASN1_STRING_FLAG_BITS_LEFT;
+   unused = p->flags & 0x07;
 
-   if (unused < 0) {
-   warnx("%s: RFC 3779 section 2.2.3.8: "
-   "unused bit count must be non-negative", fn);
-   return 0;
-   } else if (unused >= 8) {
-   warnx("%s: RFC 3779 section 2.2.3.8: "
-   "unused bit count must mask an unsigned char", fn);
-   return 0;
-   } else if (p->length == 0 && unused != 0) {
+   if (p->length == 0 && unused != 0) {
warnx("%s: RFC 3779 section 2.2.3.8: "
"unused bit count must be zero if length is zero", fn);
return 0;



rpki-client: fewer reallocarrays() for IPAddrBlocks

2022-05-12 Thread Theo Buehler
This aligns sbgp_ipaddrblk() with sbgp_assysnum(), giving it a similar
treatment. We trade the reallocarray() per prefix or range with at most
two recallocarray(). I took the liberty of trimming some RFC section
numbers in warnings to avoid awkward line wrapping.

Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.80
diff -u -p -r1.80 cert.c
--- cert.c  12 May 2022 08:53:33 -  1.80
+++ cert.c  12 May 2022 09:13:15 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: cert.c,v 1.80 2022/05/12 08:53:33 tb Exp $ */
+/* $OpenBSD: cert.c,v 1.79 2022/05/12 07:45:27 claudio Exp $ */
 /*
  * Copyright (c) 2021 Job Snijders 
  * Copyright (c) 2019 Kristaps Dzonsons 
@@ -60,17 +60,9 @@ extern ASN1_OBJECT   *notify_oid;/* 1.3.6
 static int
 append_ip(struct parse *p, const struct cert_ip *ip)
 {
-   struct cert *res = p->res;
-
if (!ip_addr_check_overlap(ip, p->fn, p->res->ips, p->res->ipsz))
return 0;
-   if (res->ipsz >= MAX_IP_SIZE)
-   return 0;
-   res->ips = reallocarray(res->ips, res->ipsz + 1,
-   sizeof(struct cert_ip));
-   if (res->ips == NULL)
-   err(1, NULL);
-   res->ips[res->ipsz++] = *ip;
+   p->res->ips[p->res->ipsz++] = *ip;
return 1;
 }
 
@@ -330,84 +322,6 @@ sbgp_addr_inherit(struct parse *p, enum 
 }
 
 /*
- * Parse an IP address or range, RFC 3779 2.2.3.7.
- * We don't constrain this parse (as specified in section 2.2.3.6) to
- * having any kind of order.
- * Returns zero on failure, non-zero on success.
- */
-static int
-sbgp_addr_or_range(struct parse *p, enum afi afi, const IPAddressOrRanges 
*aors)
-{
-   const IPAddressOrRange  *aor;
-   int  i, rc = 0;
-
-   for (i = 0; i < sk_IPAddressOrRange_num(aors); i++) {
-   aor = sk_IPAddressOrRange_value(aors, i);
-   switch (aor->type) {
-   case IPAddressOrRange_addressPrefix:
-   if (!sbgp_addr(p, afi, aor->u.addressPrefix))
-   goto out;
-   break;
-   case IPAddressOrRange_addressRange:
-   if (!sbgp_addr_range(p, afi, aor->u.addressRange))
-   goto out;
-   break;
-   default:
-   warnx("%s: RFC 3779 section 2.2.3.7: IPAddressOrRange: "
-   "unknown type %d", p->fn, aor->type);
-   goto out;
-   }
-   }
-
-   rc = 1;
- out:
-   return rc;
-}
-
-/*
- * Parse a sequence of address families as in RFC 3779 sec. 2.2.3.2.
- * Ignore several stipulations of the RFC (2.2.3.3).
- * Namely, we don't require entries to be ordered in any way (type, AFI
- * or SAFI group, etc.).
- * This is because it doesn't matter for our purposes: we're going to
- * validate in the same way regardless.
- * Returns zero on failure, non-zero on success.
- */
-static int
-sbgp_ipaddrfam(struct parse *p, const IPAddressFamily *af)
-{
-   enum afi afi;
-   const IPAddressChoice   *choice;
-   int  rc = 0;
-
-   if (!ip_addr_afi_parse(p->fn, af->addressFamily, )) {
-   warnx("%s: RFC 3779 section 2.2.3.2: addressFamily: "
-   "invalid AFI", p->fn);
-   goto out;
-   }
-
-   choice = af->ipAddressChoice;
-   switch (choice->type) {
-   case IPAddressChoice_addressesOrRanges:
-   if (!sbgp_addr_or_range(p, afi, choice->u.addressesOrRanges))
-   goto out;
-   break;
-   case IPAddressChoice_inherit:
-   if (!sbgp_addr_inherit(p, afi))
-   goto out;
-   break;
-   default:
-   warnx("%s: RFC 3779 section 2.2.3.2: IPAddressChoice: "
-   "unknown type %d", p->fn, choice->type);
-   goto out;
-   }
-
-   rc = 1;
- out:
-   return rc;
-}
-
-/*
  * Parse an sbgp-ipAddrBlock X509 extension, RFC 6487 4.8.10, with
  * syntax documented in RFC 3779 starting in section 2.2.
  * Returns zero on failure, non-zero on success.
@@ -417,7 +331,11 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
 {
STACK_OF(IPAddressFamily)   *addrblk = NULL;
const IPAddressFamily   *af;
-   int  i, rc = 0;
+   const IPAddressOrRanges *aors;
+   const IPAddressOrRange  *aor;
+   enum afi afi;
+   size_t   ipsz;
+   int  i, j, rc = 0;
 
if (!X509_EXTENSION_get_critical(ext)) {
cryptowarnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
@@ -433,8 +351,58 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
 
for (i = 0; i < sk_IPAddressFamily_num(addrblk); i++) 

rpki-client: clean up ip handling in cert.c

2022-05-12 Thread Theo Buehler
Before refactoring the IP side, let's streamline the code a little.
Populate struct ip in the leaf functions instead of handing it through
several layers and copying it along the way. Pass in the afi instead of
letting struct ip carry it.

Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.79
diff -u -p -r1.79 cert.c
--- cert.c  12 May 2022 07:45:27 -  1.79
+++ cert.c  12 May 2022 08:09:07 -
@@ -257,21 +257,28 @@ sbgp_assysnum(struct parse *p, X509_EXTE
  * Returns zero on failure, non-zero on success.
  */
 static int
-sbgp_addr(struct parse *p, struct cert_ip *ip, const ASN1_BIT_STRING *bs)
+sbgp_addr(struct parse *p, enum afi afi, const ASN1_BIT_STRING *bs)
 {
-   if (!ip_addr_parse(bs, ip->afi, p->fn, >ip)) {
+   struct cert_ip  ip;
+
+   memset(, 0, sizeof(struct cert_ip));
+
+   ip.afi = afi;
+   ip.type = CERT_IP_ADDR;
+
+   if (!ip_addr_parse(bs, afi, p->fn, )) {
warnx("%s: RFC 3779 section 2.2.3.8: IPAddress: "
"invalid IP address", p->fn);
return 0;
}
 
-   if (!ip_cert_compose_ranges(ip)) {
+   if (!ip_cert_compose_ranges()) {
warnx("%s: RFC 3779 section 2.2.3.8: IPAddress: "
"IP address range reversed", p->fn);
return 0;
}
 
-   return append_ip(p, ip);
+   return append_ip(p, );
 }
 
 /*
@@ -279,28 +286,47 @@ sbgp_addr(struct parse *p, struct cert_i
  * Returns zero on failure, non-zero on success.
  */
 static int
-sbgp_addr_range(struct parse *p, struct cert_ip *ip,
-const IPAddressRange *range)
+sbgp_addr_range(struct parse *p, enum afi afi, const IPAddressRange *range)
 {
-   if (!ip_addr_parse(range->min, ip->afi, p->fn, >range.min)) {
+   struct cert_ip  ip;
+
+   memset(, 0, sizeof(struct cert_ip));
+
+   ip.afi = afi;
+   ip.type = CERT_IP_RANGE;
+
+   if (!ip_addr_parse(range->min, afi, p->fn, )) {
warnx("%s: RFC 3779 section 2.2.3.9: IPAddressRange: "
"invalid IP address", p->fn);
return 0;
}
 
-   if (!ip_addr_parse(range->max, ip->afi, p->fn, >range.max)) {
+   if (!ip_addr_parse(range->max, afi, p->fn, )) {
warnx("%s: RFC 3779 section 2.2.3.9: IPAddressRange: "
"invalid IP address", p->fn);
return 0;
}
 
-   if (!ip_cert_compose_ranges(ip)) {
+   if (!ip_cert_compose_ranges()) {
warnx("%s: RFC 3779 section 2.2.3.9: IPAddressRange: "
"IP address range reversed", p->fn);
return 0;
}
 
-   return append_ip(p, ip);
+   return append_ip(p, );
+}
+
+static int
+sbgp_addr_inherit(struct parse *p, enum afi afi)
+{
+   struct cert_ip  ip;
+
+   memset(, 0, sizeof(struct cert_ip));
+
+   ip.afi = afi;
+   ip.type = CERT_IP_INHERIT;
+
+   return append_ip(p, );
 }
 
 /*
@@ -310,25 +336,20 @@ sbgp_addr_range(struct parse *p, struct 
  * Returns zero on failure, non-zero on success.
  */
 static int
-sbgp_addr_or_range(struct parse *p, struct cert_ip *ip,
-const IPAddressOrRanges *aors)
+sbgp_addr_or_range(struct parse *p, enum afi afi, const IPAddressOrRanges 
*aors)
 {
-   struct cert_ip   nip;
const IPAddressOrRange  *aor;
int  i, rc = 0;
 
for (i = 0; i < sk_IPAddressOrRange_num(aors); i++) {
-   nip = *ip;
aor = sk_IPAddressOrRange_value(aors, i);
switch (aor->type) {
case IPAddressOrRange_addressPrefix:
-   nip.type = CERT_IP_ADDR;
-   if (!sbgp_addr(p, , aor->u.addressPrefix))
+   if (!sbgp_addr(p, afi, aor->u.addressPrefix))
goto out;
break;
case IPAddressOrRange_addressRange:
-   nip.type = CERT_IP_RANGE;
-   if (!sbgp_addr_range(p, , aor->u.addressRange))
+   if (!sbgp_addr_range(p, afi, aor->u.addressRange))
goto out;
break;
default:
@@ -355,13 +376,11 @@ sbgp_addr_or_range(struct parse *p, stru
 static int
 sbgp_ipaddrfam(struct parse *p, const IPAddressFamily *af)
 {
-   struct cert_ip   ip;
+   enum afi afi;
const IPAddressChoice   *choice;
int  rc = 0;
 
-   memset(, 0, sizeof(struct cert_ip));
-
-   if (!ip_addr_afi_parse(p->fn, af->addressFamily, )) {
+   if (!ip_addr_afi_parse(p->fn, af->addressFamily, )) {
warnx("%s: RFC 3779 section 2.2.3.2: addressFamily: "
"invalid AFI", p->fn);
goto out;
@@ -370,12 +389,11 @@ sbgp_ipaddrfam(struct parse *p, 

Re: more rpki-client refactor

2022-05-11 Thread Theo Buehler
On Wed, May 11, 2022 at 06:54:32PM +0200, Claudio Jeker wrote:
> I took the liberty and refactored the sbgp_assysnum() code a bit more.
> 
> Main goal is to replace the reallocarray() in append_as() with an upfront
> calloc() call since now the size is known. Also I decided to collaps
> sbgp_asnum() into sbgp_assysnum().
> 

Cool. As you can imagine, this quite close to some diffs I have in my
tree :)

I haven't thought about getting rid of the reallocarray() yet. I like
this approach a lot.

ok tb

> One could also inline the now very simple append_as() but I guess the
> compiler already does that for us.

I also considered removing the overlap and other checks. At this point
we know that the extension is in canonical form thanks to the early
caching (OpenSSL would need an extra X509v3_asid_is_canonical() call).

If the RFC 3779 ideas in the RSC draft materialize, everything after
the successful X509V3_EXT_d2i() call until the end should end up in a
function of the form

int
sbgp_parse_asidentifiers(const char *fn, struct cert_as **as, size_t *asz,
const ASIdentifiers *asidentifiers)

(maybe no const so that it can free asidentifiers itself).

This way we can call it from the cert and the RSC side and get rid of
all the XXX. Assuming we do the same IPAddrBlocks, of course.



rpki-client: cache X509v3 extensions early

2022-05-11 Thread Theo Buehler
Some funky libcrypto business ahead.

X509 API functions such as X509_check_ca() or X509_get_extension_flags()
cache X509v3 extensions internally if they're not already cached. They
make decisions based on (or report some of) the cached values. Although
it's unlikely, this caching may fail halfway through. The result is
fairly random in the case of X509_check_ca() (which can't report an
error itself) - in LibreSSL it would actually return 1 due to a bug I
fixed yesterday. Every use of X509_get_extension_flags() on a cert for
which we don't know that the extensions are cached already should also
check the EXFLAG_INVALID, which is annoying.

An old workaround that used to be used in libssl is to call
X509_check_purpose(x, -1, -1), which is effectively a wrapper around
x509v3_cache_extensions() that allows error checking. This way, the
reported values by the affected API functions are reliable. I suggest to
do this once we get our hands on a cert, so this issue is out of the
way.

Caching of extensions will happen sooner or later anyway, at the latest
within X509_verify_cert(). In LibreSSL this also ensures that the
RFC 3779 extensions are in canonical form before we inspect them which
I think is a good thing.

Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.77
diff -u -p -r1.77 cert.c
--- cert.c  11 May 2022 09:40:00 -  1.77
+++ cert.c  11 May 2022 13:16:19 -
@@ -597,6 +597,12 @@ cert_parse_pre(const char *fn, const uns
goto out;
}
 
+   /* Cache X509v3 extensions, see X509_check_ca(3). */
+   if (X509_check_purpose(x, -1, -1) <= 0) {
+   cryptowarnx("%s: could not cache X509v3 extensions", p.fn);
+   goto out;
+   }
+
/* Look for X509v3 extensions. */
 
if ((extsz = X509_get_ext_count(x)) < 0)
Index: cms.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
retrieving revision 1.16
diff -u -p -r1.16 cms.c
--- cms.c   28 Mar 2022 13:04:01 -  1.16
+++ cms.c   11 May 2022 13:19:14 -
@@ -224,6 +224,12 @@ cms_parse_validate(X509 **xp, const char
}
*xp = X509_dup(sk_X509_value(certs, 0));
 
+   /* Cache X509v3 extensions, see X509_check_ca(3). */
+   if (X509_check_purpose(*xp, -1, -1) <= 0) {
+   cryptowarnx("%s: could not cache X509v3 extensions", fn);
+   goto out;
+   }
+
if (CMS_SignerInfo_get0_signer_id(si, , NULL, NULL) != 1 ||
kid == NULL) {
warnx("%s: RFC 6488: could not extract SKI from SID", fn);



Re: [PATCH] reorder_kernel: also redirect stderr to new logfile

2022-05-11 Thread Theo Buehler
On Wed, May 11, 2022 at 07:05:29PM +0900, Lauri Tirkkonen wrote:
> Hi,
> 
> I had reorder_kernel failing but generating empty log files. The root cause of
> the log file being empty is that if $KERNEL_DIR.tgz exists, a new log file is
> created but stderr keeps going to the old (now deleted) one. Simple fix.

ok tb with adjusted comment

> 
> ---
>  libexec/reorder_kernel/reorder_kernel.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libexec/reorder_kernel/reorder_kernel.sh 
> b/libexec/reorder_kernel/reorder_kernel.sh
> index ded689f095e..fdb4368501e 100644
> --- a/libexec/reorder_kernel/reorder_kernel.sh
> +++ b/libexec/reorder_kernel/reorder_kernel.sh
> @@ -45,6 +45,7 @@ if [[ -f $KERNEL_DIR.tgz ]]; then
>   # The directory containing the logfile was just deleted, redirect
>   # stdout again to a new logfile.
>   exec 1>$LOGFILE
> + exec 2>&1
>   tar -C $KERNEL_DIR -xzf $KERNEL_DIR.tgz $KERNEL
>   rm -f $KERNEL_DIR.tgz
>  fi
> -- 
> 2.36.0
> 
> -- 
> Lauri Tirkkonen | lotheac @ IRCnet
> 



rpki-client: remove verify_cb()

2022-05-10 Thread Theo Buehler
With RSC and recent cert.c changes, rpki-client now has a hard
dependency on RFC 3779 support in libcrypto, so this code is no
longer useful.

Index: validate.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
retrieving revision 1.32
diff -u -p -r1.32 validate.c
--- validate.c  10 May 2022 07:41:37 -  1.32
+++ validate.c  10 May 2022 20:28:52 -
@@ -347,67 +347,6 @@ valid_origin(const char *uri, const char
 }
 
 /*
- * Callback for X509_verify_cert() to handle critical extensions in old
- * LibreSSL libraries or OpenSSL libs without RFC3779 support.
- */
-static int
-verify_cb(int ok, X509_STORE_CTX *store_ctx)
-{
-   X509*cert;
-   const STACK_OF(X509_EXTENSION)  *exts;
-   X509_EXTENSION  *ext;
-   ASN1_OBJECT *obj;
-   char*file;
-   int  depth, error, i, nid;
-
-   error = X509_STORE_CTX_get_error(store_ctx);
-   depth = X509_STORE_CTX_get_error_depth(store_ctx);
-
-   if (error != X509_V_ERR_UNHANDLED_CRITICAL_EXTENSION)
-   return ok;
-
-   if ((file = X509_STORE_CTX_get_app_data(store_ctx)) == NULL)
-   cryptoerrx("X509_STORE_CTX_get_app_data");
-
-   if ((cert = X509_STORE_CTX_get_current_cert(store_ctx)) == NULL) {
-   warnx("%s: got no current cert", file);
-   return 0;
-   }
-   if ((exts = X509_get0_extensions(cert)) == NULL) {
-   warnx("%s: got no cert extensions", file);
-   return 0;
-   }
-
-   for (i = 0; i < sk_X509_EXTENSION_num(exts); i++) {
-   ext = sk_X509_EXTENSION_value(exts, i);
-
-   /* skip over non-critical and known extensions */
-   if (!X509_EXTENSION_get_critical(ext))
-   continue;
-   if (X509_supported_extension(ext))
-   continue;
-
-   if ((obj = X509_EXTENSION_get_object(ext)) == NULL) {
-   warnx("%s: got no extension object", file);
-   return 0;
-   }
-
-   nid = OBJ_obj2nid(obj);
-   switch (nid) {
-   case NID_sbgp_ipAddrBlock:
-   case NID_sbgp_autonomousSysNum:
-   continue;
-   default:
-   warnx("%s: depth %d: unknown extension: nid %d",
-   file, depth, nid);
-   return 0;
-   }
-   }
-
-   return 1;
-}
-
-/*
  * Walk the certificate tree to the root and build a certificate
  * chain from cert->x509. All certs in the tree are validated and
  * can be loaded as trusted stack into the validator.
@@ -476,9 +415,6 @@ valid_x509(char *file, X509_STORE_CTX *s
if (!X509_VERIFY_PARAM_add0_policy(params, cp_oid))
cryptoerrx("X509_VERIFY_PARAM_add0_policy");
 
-   X509_STORE_CTX_set_verify_cb(store_ctx, verify_cb);
-   if (!X509_STORE_CTX_set_app_data(store_ctx, file))
-   cryptoerrx("X509_STORE_CTX_set_app_data");
flags = X509_V_FLAG_CRL_CHECK;
flags |= X509_V_FLAG_EXPLICIT_POLICY;
flags |= X509_V_FLAG_INHIBIT_MAP;



rpki-client: deserialize ASIdentifiers in libcrypto

2022-05-10 Thread Theo Buehler
The ASIdentifiers code is a bit strangely factored presumably due to
constraints of the low-level shoveling. I kept the coarse structure
of the code and left some house keeping for later. The changes in
sbgp_asrange() and sbgp_asid() should be easy.

The in-tree sbgp_assysnum() is tricky to follow. The main thing to
note is that the for loop at the end really only inspects asnum (and
ignores rdi). Since RDI is forbidden in RFC 6487, I added a check
and error. All the complicated gymnastics can be dropped and what
remains is a bit stricter and what it does should be rather obvious.

With this diff, ASN1_TYPE and ASN1_frame() uses are gone from cert.c.

Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.74
diff -u -p -r1.74 cert.c
--- cert.c  10 May 2022 16:43:53 -  1.74
+++ cert.c  10 May 2022 18:13:02 -
@@ -34,13 +34,6 @@
 #include "extern.h"
 
 /*
- * Type of ASIdentifier (RFC 3779, 3.2.3).
- */
-#defineASID_TYPE_ASNUM 0x00
-#define ASID_TYPE_RDI  0x01
-#define ASID_TYPE_MAX  ASID_TYPE_RDI
-
-/*
  * A parsing sequence of a file (which may just be ).
  */
 struct parse {
@@ -128,70 +121,36 @@ sbgp_addr(struct parse *p, struct cert_i
  * Returns zero on failure, non-zero on success.
  */
 static int
-sbgp_asrange(struct parse *p, const unsigned char *d, size_t dsz)
+sbgp_asrange(struct parse *p, const ASRange *range)
 {
struct cert_as   as;
-   ASN1_SEQUENCE_ANY   *seq;
-   const ASN1_TYPE *t;
-   int  rc = 0;
-
-   if ((seq = d2i_ASN1_SEQUENCE_ANY(NULL, , dsz)) == NULL) {
-   cryptowarnx("%s: RFC 3779 section 3.2.3.8: ASRange: "
-   "failed ASN.1 sequence parse", p->fn);
-   goto out;
-   }
-   if (sk_ASN1_TYPE_num(seq) != 2) {
-   warnx("%s: RFC 3779 section 3.2.3.8: ASRange: "
-   "want 2 elements, have %d", p->fn,
-   sk_ASN1_TYPE_num(seq));
-   goto out;
-   }
 
memset(, 0, sizeof(struct cert_as));
as.type = CERT_AS_RANGE;
 
-   t = sk_ASN1_TYPE_value(seq, 0);
-   if (t->type != V_ASN1_INTEGER) {
-   warnx("%s: RFC 3779 section 3.2.3.8: ASRange: "
-   "want ASN.1 integer, have %s (NID %d)",
-   p->fn, ASN1_tag2str(t->type), t->type);
-   goto out;
-   }
-   if (!as_id_parse(t->value.integer, )) {
+   if (!as_id_parse(range->min, )) {
warnx("%s: RFC 3779 section 3.2.3.8 (via RFC 1930): "
"malformed AS identifier", p->fn);
-   goto out;
+   return 0;
}
 
-   t = sk_ASN1_TYPE_value(seq, 1);
-   if (t->type != V_ASN1_INTEGER) {
-   warnx("%s: RFC 3779 section 3.2.3.8: ASRange: "
-   "want ASN.1 integer, have %s (NID %d)",
-   p->fn, ASN1_tag2str(t->type), t->type);
-   goto out;
-   }
-   if (!as_id_parse(t->value.integer, )) {
+   if (!as_id_parse(range->max, )) {
warnx("%s: RFC 3779 section 3.2.3.8 (via RFC 1930): "
"malformed AS identifier", p->fn);
-   goto out;
+   return 0;
}
 
if (as.range.max == as.range.min) {
warnx("%s: RFC 3379 section 3.2.3.8: ASRange: "
"range is singular", p->fn);
-   goto out;
+   return 0;
} else if (as.range.max < as.range.min) {
warnx("%s: RFC 3379 section 3.2.3.8: ASRange: "
"range is out of order", p->fn);
-   goto out;
+   return 0;
}
 
-   if (!append_as(p, ))
-   goto out;
-   rc = 1;
-out:
-   sk_ASN1_TYPE_pop_free(seq, ASN1_TYPE_free);
-   return rc;
+   return append_as(p, );
 }
 
 /*
@@ -224,80 +183,46 @@ sbgp_asid(struct parse *p, const ASN1_IN
  * Returns zero on failure, non-zero on success.
  */
 static int
-sbgp_asnum(struct parse *p, const unsigned char *d, size_t dsz)
+sbgp_asnum(struct parse *p, const ASIdentifierChoice *aic)
 {
struct cert_as   as;
-   ASN1_TYPE   *t, *tt;
-   ASN1_SEQUENCE_ANY   *seq = NULL;
-   int  i, rc = 0;
-   const unsigned char *sv = d;
-
-   /* We can either be a null (inherit) or sequence. */
-
-   if ((t = d2i_ASN1_TYPE(NULL, , dsz)) == NULL) {
-   cryptowarnx("%s: RFC 3779 section 3.2.3.2: ASIdentifierChoice: "
-   "failed ASN.1 type parse", p->fn);
-   goto out;
-   }
-
-   /*
-* Section 3779 3.2.3.3 is to inherit with an ASN.1 NULL type,
-* which is the easy case.
-*/
+   const ASIdOrRanges  *aors = NULL;
+   const ASIdOrRange   *aor;
+   int  i;
 
-   switch 

rpki-client: shuffle two IP address functions down a bit

2022-05-10 Thread Theo Buehler
This moves two helper functions down so that the file starts with the
code parsing ASIdentifiers, then the code dealing with IPAddrBlocks.

Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.74
diff -u -p -r1.74 cert.c
--- cert.c  10 May 2022 16:43:53 -  1.74
+++ cert.c  10 May 2022 16:44:44 -
@@ -54,34 +54,6 @@ extern ASN1_OBJECT   *manifest_oid;  /* 1.3
 extern ASN1_OBJECT *notify_oid;/* 1.3.6.1.5.5.7.48.13 (rpkiNotify) */
 
 /*
- * Append an IP address structure to our list of results.
- * This will also constrain us to having at most one inheritance
- * statement per AFI and also not have overlapping ranges (as prohibited
- * in section 2.2.3.6).
- * It does not make sure that ranges can't coalesce, that is, that any
- * two ranges abut each other.
- * This is warned against in section 2.2.3.6, but doesn't change the
- * semantics of the system.
- * Returns zero on failure (IP overlap) non-zero on success.
- */
-static int
-append_ip(struct parse *p, const struct cert_ip *ip)
-{
-   struct cert *res = p->res;
-
-   if (!ip_addr_check_overlap(ip, p->fn, p->res->ips, p->res->ipsz))
-   return 0;
-   if (res->ipsz >= MAX_IP_SIZE)
-   return 0;
-   res->ips = reallocarray(res->ips, res->ipsz + 1,
-   sizeof(struct cert_ip));
-   if (res->ips == NULL)
-   err(1, NULL);
-   res->ips[res->ipsz++] = *ip;
-   return 1;
-}
-
-/*
  * Append an AS identifier structure to our list of results.
  * Makes sure that the identifiers do not overlap or improperly inherit
  * as defined by RFC 3779 section 3.3.
@@ -102,28 +74,6 @@ append_as(struct parse *p, const struct 
 }
 
 /*
- * Construct a RFC 3779 2.2.3.8 range from its bit string.
- * Returns zero on failure, non-zero on success.
- */
-static int
-sbgp_addr(struct parse *p, struct cert_ip *ip, const ASN1_BIT_STRING *bs)
-{
-   if (!ip_addr_parse(bs, ip->afi, p->fn, >ip)) {
-   warnx("%s: RFC 3779 section 2.2.3.8: IPAddress: "
-   "invalid IP address", p->fn);
-   return 0;
-   }
-
-   if (!ip_cert_compose_ranges(ip)) {
-   warnx("%s: RFC 3779 section 2.2.3.8: IPAddress: "
-   "IP address range reversed", p->fn);
-   return 0;
-   }
-
-   return append_ip(p, ip);
-}
-
-/*
  * Parse a range of addresses as in 3.2.3.8.
  * Returns zero on failure, non-zero on success.
  */
@@ -414,6 +364,56 @@ out:
sk_ASN1_TYPE_pop_free(sseq, ASN1_TYPE_free);
free(sv);
return rc;
+}
+
+/*
+ * Append an IP address structure to our list of results.
+ * This will also constrain us to having at most one inheritance
+ * statement per AFI and also not have overlapping ranges (as prohibited
+ * in section 2.2.3.6).
+ * It does not make sure that ranges can't coalesce, that is, that any
+ * two ranges abut each other.
+ * This is warned against in section 2.2.3.6, but doesn't change the
+ * semantics of the system.
+ * Returns zero on failure (IP overlap) non-zero on success.
+ */
+static int
+append_ip(struct parse *p, const struct cert_ip *ip)
+{
+   struct cert *res = p->res;
+
+   if (!ip_addr_check_overlap(ip, p->fn, p->res->ips, p->res->ipsz))
+   return 0;
+   if (res->ipsz >= MAX_IP_SIZE)
+   return 0;
+   res->ips = reallocarray(res->ips, res->ipsz + 1,
+   sizeof(struct cert_ip));
+   if (res->ips == NULL)
+   err(1, NULL);
+   res->ips[res->ipsz++] = *ip;
+   return 1;
+}
+
+/*
+ * Construct a RFC 3779 2.2.3.8 range from its bit string.
+ * Returns zero on failure, non-zero on success.
+ */
+static int
+sbgp_addr(struct parse *p, struct cert_ip *ip, const ASN1_BIT_STRING *bs)
+{
+   if (!ip_addr_parse(bs, ip->afi, p->fn, >ip)) {
+   warnx("%s: RFC 3779 section 2.2.3.8: IPAddress: "
+   "invalid IP address", p->fn);
+   return 0;
+   }
+
+   if (!ip_cert_compose_ranges(ip)) {
+   warnx("%s: RFC 3779 section 2.2.3.8: IPAddress: "
+   "IP address range reversed", p->fn);
+   return 0;
+   }
+
+   return append_ip(p, ip);
 }
 
 /*



  1   2   3   4   5   6   7   8   9   10   >