Re: First step towards improved unlocking in the VFS layer.

[Bob Beck]

> On Jun 13, 2023, at 7:59 AM, Theo de Raadt  wrote:
> 
> Thordur I. Bjornsson  wrote:
> 
>> On Mon, Jun 12, 2023 at 9:15 PM Bob Beck  wrote:
>>> 
>>> On Mon, Jun 12, 2023 at 11:01:18AM -0600, Theo de Raadt wrote:
>>>> +   KASSERTMSG(1, "Ich Habe eine Rotweinflarsche in meinem Arsche");
>>>> That part of the diff is not OK.  If everyone did this, we would have a
>>>> mess on our hands.
>>> (or I could simply deleted it)
>> +1
>> 
>> Is it on purpose that this is completely silent ?
>> Perhaps the warning in ffs_vfsops should go "WARNING: soft updates are
>> now ignored" and the option dropped from GENERIC ?
> 
> The purpose of Bob's diff is to silently (quietly) simply disable softdep
> mount requests.
> 
> They will be downgraded to regular mounts.  But then we want to make
> sure that the back-end code isn't accidentaly called, because of some
> bug, that's where his whiny assert comes into play.
> 
> The reason to disable softdep, is that softdep has crazy callback schemes and
> context issues that are making it hard to reason about vfs locking.
> 
> If we disable softdep, we may be able to unlock nami / vfs / etc better.
> 
> When / if such locking/scheduling changes are finished, softdep can be 
> repaired.


Softdep is also a significant hindrance to progress by mere mortals in the 
areas interfacing between the filesystem and the caching mid layer.  Don’t get 
me wrong, it’s is brilliantly engineered from the point of view of it does 
absolutely everything it can within the realm of what’s permissible in current 
VFS layer pushing all the boundaries of what you can do in order to do what it 
does. 

The problem with that is it is one of the things that adds a lot of complexity, 
and makes it very hard to change things an evolve for something like, oh, 
having a log based filesystem or things like that. 

My desire to get rid to it is hopefully to make it where working in this area 
of the kernel is something more people could slowly learn how to do and make 
progress on things, without any change being an insurmountable obstacle with a 
learning curve so high people will give up in frustration because everything 
they try has subtle interactions and ends up getting backed out.

Re: First step towards improved unlocking in the VFS layer.

[Bob Beck]

On Mon, Jun 12, 2023 at 11:01:18AM -0600, Theo de Raadt wrote:
> +   KASSERTMSG(1, "Ich Habe eine Rotweinflarsche in meinem Arsche");
> 
> That part of the diff is not OK.  If everyone did this, we would have a
> mess on our hands.

Yeah, thats me nodding to my own past stupidity ;) 

changed to "softdep_mount should not have been called"

(or I could simply deleted it)

First step towards improved unlocking in the VFS layer.

[Bob Beck]

Minimal diff, further cleanup and dead code removal to follow.

---
 sys/kern/vfs_syscalls.c   |  7 +++
 sys/sys/mount.h   |  2 +-
 sys/ufs/ffs/ffs_softdep.c |  2 ++
 sys/ufs/ffs/ffs_vfsops.c  | 16 +++-
 4 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index f47f9dfcb69..51fc7b005c0 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -239,11 +239,10 @@ update:
else if (mp->mnt_flag & MNT_RDONLY)
mp->mnt_flag |= MNT_WANTRDWR;
mp->mnt_flag &=~ (MNT_NOSUID | MNT_NOEXEC | MNT_WXALLOWED | MNT_NODEV |
-   MNT_SYNCHRONOUS | MNT_ASYNC | MNT_SOFTDEP | MNT_NOATIME |
-   MNT_NOPERM | MNT_FORCE);
+   MNT_SYNCHRONOUS | MNT_ASYNC | MNT_NOATIME | MNT_NOPERM | MNT_FORCE);
mp->mnt_flag |= flags & (MNT_NOSUID | MNT_NOEXEC | MNT_WXALLOWED |
-   MNT_NODEV | MNT_SYNCHRONOUS | MNT_ASYNC | MNT_SOFTDEP |
-   MNT_NOATIME | MNT_NOPERM | MNT_FORCE);
+   MNT_NODEV | MNT_SYNCHRONOUS | MNT_ASYNC | MNT_NOATIME | MNT_NOPERM |
+   MNT_FORCE);
/*
 * Mount the filesystem.
 */
diff --git a/sys/sys/mount.h b/sys/sys/mount.h
index 40c12e97602..8f57b18dd02 100644
--- a/sys/sys/mount.h
+++ b/sys/sys/mount.h
@@ -401,7 +401,7 @@ struct mount {
 #defineMNT_STALLED 0x0010  /* filesystem stalled */ 
 #defineMNT_SWAPPABLE   0x0020  /* filesystem can be used for 
swap */
 #define MNT_WANTRDWR   0x0200  /* want upgrade to read/write */
-#define MNT_SOFTDEP 0x0400  /* soft dependencies being done */
+#define MNT_SOFTDEP 0x0400  /* soft dependencies being done - now 
ignored */
 #define MNT_DOOMED 0x0800  /* device behind filesystem is gone */
 
 #ifdef _KERNEL
diff --git a/sys/ufs/ffs/ffs_softdep.c b/sys/ufs/ffs/ffs_softdep.c
index 15017537b41..a263a1533ac 100644
--- a/sys/ufs/ffs/ffs_softdep.c
+++ b/sys/ufs/ffs/ffs_softdep.c
@@ -1224,6 +1224,8 @@ softdep_mount(struct vnode *devvp, struct mount *mp, 
struct fs *fs,
struct buf *bp;
int error, cyl;
 
+   KASSERTMSG(1, "Ich Habe eine Rotweinflarsche in meinem Arsche");
+
/*
 * When doing soft updates, the counters in the
 * superblock may have gotten out of sync, so we have
diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c
index ffe78ef140f..8350d56ba05 100644
--- a/sys/ufs/ffs/ffs_vfsops.c
+++ b/sys/ufs/ffs/ffs_vfsops.c
@@ -213,12 +213,10 @@ ffs_mount(struct mount *mp, const char *path, void *data,
int error = 0, flags;
int ronly;
 
-#ifndef FFS_SOFTUPDATES
+   /* Ask not for whom the bell tolls */
if (mp->mnt_flag & MNT_SOFTDEP) {
-   printf("WARNING: soft updates isn't compiled in\n");
mp->mnt_flag &= ~MNT_SOFTDEP;
}
-#endif
 
/*
 * Soft updates is incompatible with "async",
@@ -284,8 +282,6 @@ ffs_mount(struct mount *mp, const char *path, void *data,
if (mp->mnt_flag & MNT_FORCE)
flags |= FORCECLOSE;
error = softdep_flushfiles(mp, flags, p);
-#elif FFS_SOFTUPDATES
-   mp->mnt_flag |= MNT_SOFTDEP;
 #endif
}
/*
@@ -459,10 +455,7 @@ success:
free(fs->fs_contigdirs, M_UFSMNT, fs->fs_ncg);
}
if (!ronly) {
-   if (mp->mnt_flag & MNT_SOFTDEP)
-   fs->fs_flags |= FS_DOSOFTDEP;
-   else
-   fs->fs_flags &= ~FS_DOSOFTDEP;
+   fs->fs_flags &= ~FS_DOSOFTDEP;
}
ffs_sbupdate(ump, MNT_WAIT);
 #if 0
@@ -923,10 +916,7 @@ ffs_mountfs(struct vnode *devvp, struct mount *mp, struct 
proc *p)
}
fs->fs_fmod = 1;
fs->fs_clean = 0;
-   if (mp->mnt_flag & MNT_SOFTDEP)
-   fs->fs_flags |= FS_DOSOFTDEP;
-   else
-   fs->fs_flags &= ~FS_DOSOFTDEP;
+   fs->fs_flags &= ~FS_DOSOFTDEP;
error = ffs_sbupdate(ump, MNT_WAIT);
if (error == EROFS)
goto out;
-- 
2.40.0

Re: Add Miller-Rabin with random bases to BPSW primality check

[Bob Beck]

On Fri, Apr 28, 2023 at 10:23:15AM +0200, Theo Buehler wrote:
> The behavior of BPSW for numbers > 2^64 is not very well understood.
> While there is no known composite that passes the test, there are
> heuristics that indicate that there are likely many. Therefore it seems
> appropriate to harden the test. Having a settable number of MR rounds
> before doing a version of BPSW is also the approach taken by Go's
> primality check in math/big.
> 
> This is a first step towards incorporating some of the considerations in
> "A performant misuse-resistant API for Primality Testing" by Massimo and
> Paterson. It should be noted that it is based on this paper (and due to
> FIPS, I suppose) that OpenSSL 3 made their primality check so slow that
> the bn_prime.c regress test took over 10 minutes on an M3000 before the
> libcrypto bump (it took less than a minute with ours).
> 
> This basically adds back an equivalent of the old inadequate prime number
> test before running the strong Lucas test, with slightly cleaner code.
> While I don't think performance matters a lot here, we're then effectively
> at about twice the cost of what we had a year ago. I also think that having
> some non-determinism in the algorithm is a plus.
> 
> The implementation is straightforward. It could easily be tweaked to use
> the additional gcds in the "enhanced" MR test of FIPS 186-5, but as long
> as we are only going to throw away the additional info, this doesn't add
> all that much.
> 
> I'd like to land this and then do further work in tree. We probably want
> to crank the number of MR rounds done by default considerably. I will
> also need to undo some of the clean-up done in BN_generate_prime_ex()
> after BPSW landed.
> 
> Index: bn/bn_bpsw.c
> ===
> RCS file: /cvs/src/lib/libcrypto/bn/bn_bpsw.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 bn_bpsw.c
> --- bn/bn_bpsw.c  26 Nov 2022 16:08:51 -  1.8
> +++ bn/bn_bpsw.c  28 Apr 2023 07:56:01 -
> @@ -301,23 +301,94 @@ bn_strong_lucas_selfridge(int *is_prime,
>  }
>  
>  /*
> - * Miller-Rabin primality test for base 2.
> + * Fermat criterion in Miller-Rabin test.
> + *
> + * Check whether 1 < base < n - 1 witnesses that n is composite. For prime n:
> + *
> + *  * Fermat's little theorem: base^(n-1) = 1 (mod n).
> + *  * The only square roots of 1 (mod n) are 1 and -1.
> + *
> + * Calculate base^((n-1)/2) by writing n - 1 = k * 2^s with odd k: 
> iteratively
> + * compute (base^k)^(2^(s-1)) by successive squaring of base^k.
> + *
> + * If -1 is ever reached, base^(n-1) works out to 1 and n is a pseudoprime
> + * for base. If 1 is reached before -1, we have an unexpected square root of
> + * unity and n is composite. Otherwise base^(n-1) != 1, so n is composite.
>   */
>  
>  static int
> -bn_miller_rabin_base_2(int *is_prime, const BIGNUM *n, BN_CTX *ctx)
> +bn_fermat(int *is_prime, const BIGNUM *n, const BIGNUM *n_minus_one,
> +const BIGNUM *k, int s, BIGNUM *base, BN_CTX *ctx, BN_MONT_CTX *mctx)
>  {
> - BIGNUM *n_minus_one, *k, *x;
> - int i, s;
> + int ret = 0;
> + int i;
> +
> + /* Sanity check: ensure that 1 < base < n - 1. */
> + if (BN_cmp(base, BN_value_one()) <= 0 || BN_cmp(base, n_minus_one) >= 0)
> + goto err;
> +
> + if (!BN_mod_exp_mont_ct(base, base, k, n, ctx, mctx))
> + goto err;
> +
> + if (BN_is_one(base) || BN_cmp(base, n_minus_one) == 0) {
> + *is_prime = 1;
> + goto done;
> + }
> +
> + /* Loop invariant: base is neither 1 nor -1 (mod n). */
> + for (i = 1; i < s; i++) {
> + if (!BN_mod_sqr(base, base, n, ctx))
> + goto err;
> +
> + /* n is a pseudoprime for base. */
> + if (BN_cmp(base, n_minus_one) == 0) {
> + *is_prime = 1;
> + goto done;
> + }
> +
> + /* n is composite: there's a square root of unity != 1 or -1. */
> + if (BN_is_one(base)) {
> + *is_prime = 0;
> + goto done;
> + }
> + }
> +
> + /*
> +  * If we get here, n is definitely composite: base^(n-1) != 1.
> +  */
> +
> + *is_prime = 0;
> +
> + done:
> + ret = 1;
> +
> + err:
> + return ret;
> +}
> +
> +/*
> + * Miller-Rabin primality test for base 2 and for |rounds| of random bases.
> + * On success: is_prime == 0 implies n is composite - the converse is false.
> + */
> +
> +static int
> +bn_miller_rabin(int *is_prime, const BIGNUM *n, BN_CTX *ctx, size_t rounds)
> +{
> + BN_MONT_CTX *mctx = NULL;
> + BIGNUM *base, *k, *n_minus_one, *three;
> + size_t i;
> + int s;
>   int ret = 0;
>  
>   BN_CTX_start(ctx);
>  
> - if ((n_minus_one = BN_CTX_get(ctx)) == NULL)
> + if ((base = BN_CTX_get(ctx)) == NULL)
>   goto err;
>   if ((k = BN_CTX_get(ctx)) == NULL)
>   

Re: don't remove known vmd vm's on failure

[Bob Beck]

Tried it out here with my gimpy little test setup and your suggested repro 
case. 

Seems to be more sane to me in this case, and looks like the right thing to do, 

So ok beck@ for what that’s worth. 

> On Jan 21, 2023, at 8:08 AM, Dave Voutila  wrote:
> 
> 
> *bump*... Anyone able to test or review? Other than bikeshedding some
> function naming, this isn't a dramatic change.
> 
> Dave Voutila  writes:
> 
>> Dave Voutila  writes:
>> 
>>> It turns out not only does vmd have numerous error paths for handling
>>> when something is amiss with a guest, most of the paths don't check if
>>> it's a known vm defined in vm.conf.
>>> 
>>> As a result, vmd often removes the vm from the SLIST of vm's meaning
>>> one can't easily attempt to start it again or see it in vmctl's status
>>> output.
>>> 
>>> A simple reproduction:
>>> 
>>>  1. define a vm with memory > 4gb in vm.conf
>>>  2. run vmd in the foreground (doas vmd -d) so it's not started by rc.d
>>>  3. try to start with `vmctl start -c ${vm_name}`, you should trigger
>>> an ENOMEM and get the "Cannot allocate memory" message from vmctl.
>>>  4. try to start the same vm again...now you get EPERM!
>>>  5. the vm is no longer visible in the output from `vmctl status` :(
>>> 
>>> The problem is most of the error paths call vm_remove, which not only
>>> tears down the vm via vm_stop, but also removes it from the vm list and
>>> frees it. Only clean stops or restarts seem to perform this check
>>> currently.
>>> 
>>> Below diff refactors into checking if the vm is defined in the global
>>> config before deciding to call vm_stop or vm_remove.
>> 
>> Slight tweak... __func__->caller to actually pass the correct name to
>> vm_{stop,remove}() from vm_terminate()
>> 
>> 
>> diff refs/heads/master refs/heads/vmd-accounting
>> commit - d4e23fe7544b01187ebf3ac8ae32e955445ee666
>> commit + 46503195403bfab50cd34bd8682f35a17d54d03d
>> blob - 6bffb2519a31464836aa573dbccb7aa14ea97722
>> blob + f30dc14de1ff9d5cf121cbc08b6db183a06d0c07
>> --- usr.sbin/vmd/vmd.c
>> +++ usr.sbin/vmd/vmd.c
>> @@ -67,6 +67,8 @@ struct vmd *env;
>> int vm_claimid(const char *, int, uint32_t *);
>> void start_vm_batch(int, short, void*);
>> 
>> +static inline void vm_terminate(struct vmd_vm *, const char *);
>> +
>> struct vmd *env;
>> 
>> static struct privsep_proc procs[] = {
>> @@ -395,14 +397,14 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>> errno = vmr.vmr_result;
>> log_warn("%s: failed to forward vm result",
>>vcp->vcp_name);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> return (-1);
>> }
>> }
>> 
>> if (vmr.vmr_result) {
>> log_warnx("%s: failed to start vm", vcp->vcp_name);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> errno = vmr.vmr_result;
>> break;
>> }
>> @@ -410,7 +412,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>> /* Now configure all the interfaces */
>> if (vm_priv_ifconfig(ps, vm) == -1) {
>> log_warn("%s: failed to configure vm", vcp->vcp_name);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> break;
>> }
>> 
>> @@ -441,10 +443,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>> log_info("%s: sent vm %d successfully.",
>>vm->vm_params.vmc_params.vcp_name,
>>vm->vm_vmid);
>> - if (vm->vm_from_config)
>> - vm_stop(vm, 0, __func__);
>> - else
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> }
>> 
>> /* Send a response if a control client is waiting for it */
>> @@ -470,10 +469,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>> }
>> if (vmr.vmr_result != EAGAIN ||
>>vm->vm_params.vmc_bootdevice) {
>> - if (vm->vm_from_config)
>> - vm_stop(vm, 0, __func__);
>> - else
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> } else {
>> /* Stop VM instance but keep the tty open */
>> vm_stop(vm, 1, __func__);
>> @@ -509,7 +505,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>>imsg->hdr.peerid, -1, , sizeof(vir)) == -1) {
>> log_debug("%s: GET_INFO_VM failed for vm %d, removing",
>>__func__, vm->vm_vmid);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> return (-1);
>> }
>> break;
>> @@ -545,7 +541,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>>sizeof(vir)) == -1) {
>> log_debug("%s: GET_INFO_VM_END failed",
>>__func__);
>> - vm_remove(vm, __func__);
>> + vm_terminate(vm, __func__);
>> return (-1);
>> }
>> }
>> 
>> @@ -1943,3 +1943,14 @@ getmonotime(struct timeval *tv)
>> 
>> TIMESPEC_TO_TIMEVAL(tv, );
>> }
>> +
>> +static inline void
>> +vm_terminate(struct vmd_vm *vm, const char *caller)
>> +{
>> + if (vm->vm_from_config)
>> + vm_stop(vm, 0, caller);
>> + else {
>> + /* vm_remove calls vm_stop */
>> + vm_remove(vm, caller);
>> + }
>> +}
> 

Inconsistent isdigit(3) man page

[Bob Beck]

So isdigit(3) says in the first paragraph that

'The complete list of decimal digits is 0 and 1-9, in any locale.'

Later on it says:

'On systems supporting non-ASCII single-byte character encodings,
different c arguments may correspond to the digits, and the results of
isdigit() may depend on the LC_CTYPE locale(1).'

Argubly worring about non ASCII single byte character encodings
doesn't make sense for an OpenBSD man page, but setting that aside for
a minute it's the second part that is of concern.. "It may depend on
the LC_CTYPE locale()" - Ok, well does it or doesn't it? 

Various spec docs seem all over the place on this, so I am also
paging Dr. Posix in this email... Hi Philip! :)  Is isdigit()
safe from being screwed up by locale or not?

Should it be as below? 

Thoughts?

Index: isdigit.3
===
RCS file: /cvs/src/lib/libc/gen/isdigit.3,v
retrieving revision 1.13
diff -u -p -u -p -r1.13 isdigit.3
--- isdigit.3   11 Sep 2022 06:38:10 -  1.13
+++ isdigit.3   20 Jan 2023 16:23:08 -
@@ -59,11 +59,7 @@ non-zero if the character tests true.
 On systems supporting non-ASCII single-byte character encodings,
 different
 .Fa c
-arguments may correspond to the digits, and the results of
-.Fn isdigit
-may depend on the
-.Ev LC_CTYPE
-.Xr locale 1 .
+arguments may correspond to the digits.
 .Sh SEE ALSO
 .Xr isalnum 3 ,
 .Xr isalpha 3 ,

Re: Please test: unlock mprotect/mmap/munmap

[Bob Beck]

I have now been running it for two days, I *thought * had one hang a day ago, 
with chrome and local building churning away with me mashing on the editor.. 
but I’ve now been doing the same thing with witness on for a day and had no 
issues.  So I think whatever I might have seen is not reproducible or 
unrelated..

Ok beck@

> On Nov 8, 2022, at 10:30 AM, Martin Pieuchot  wrote:
> 
> On 08/11/22(Tue) 11:12, Mark Kettenis wrote:
>>> Date: Tue, 8 Nov 2022 10:32:14 +0100
>>> From: Christian Weisgerber 
>>> 
>>> Martin Pieuchot:
>>> 
 These 3 syscalls should now be ready to run w/o KERNEL_LOCK().  This
 will reduce contention a lot.  I'd be happy to hear from test reports
 on many architectures and possible workloads.
>>> 
>>> This survived a full amd64 package build.
>> 
>> \8/
>> 
>> I think that means it should be comitted.
> 
> I agree.  This has been tested on i386, riscv64, m88k, arm64, amd64 (of
> course) and sparc64.  I'm pretty confident.
> 

Re: more unused parts of dig

[Bob Beck]

I keep reading these as "unused parts of dlg" and wondering
why he's not remoing them himself..

ok beck@


On Sat, Jun 25, 2022 at 08:48:48PM +1000, Jonathan Gray wrote:
> Index: lib/dns/gen.c
> ===
> RCS file: /cvs/src/usr.bin/dig/lib/dns/gen.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 gen.c
> --- lib/dns/gen.c 14 Sep 2020 08:40:43 -  1.15
> +++ lib/dns/gen.c 25 Jun 2022 10:43:28 -
> @@ -83,7 +83,6 @@ static const char copyright[] =
>  #define TYPECLASSLEN 20  /* DNS mnemonic size. Must be less than 
> 100. */
>  #define TYPECLASSBUF (TYPECLASSLEN + 1)
>  #define TYPECLASSFMT "%" STR(TYPECLASSLEN) "[-0-9a-z]_%d"
> -#define ATTRIBUTESIZE 256
>  #define DIRNAMESIZE 256
>  
>  static struct cc {
> Index: lib/dns/rdata.c
> ===
> RCS file: /cvs/src/usr.bin/dig/lib/dns/rdata.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 rdata.c
> --- lib/dns/rdata.c   2 Apr 2021 06:37:40 -   1.33
> +++ lib/dns/rdata.c   25 Jun 2022 10:43:28 -
> @@ -20,7 +20,6 @@
>  
>  #include 
>  
> -#include 
>  #include 
>  #include 
>  
> Index: lib/dns/rdatalist.c
> ===
> RCS file: /cvs/src/usr.bin/dig/lib/dns/rdatalist.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 rdatalist.c
> --- lib/dns/rdatalist.c   25 Jun 2022 10:20:29 -  1.3
> +++ lib/dns/rdatalist.c   25 Jun 2022 10:43:28 -
> @@ -22,7 +22,6 @@
>  
>  #include 
>  
> -#include 
>  #include 
>  #include 
>  #include 
> Index: lib/dns/rdataset.c
> ===
> RCS file: /cvs/src/usr.bin/dig/lib/dns/rdataset.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 rdataset.c
> --- lib/dns/rdataset.c25 Jun 2022 10:20:29 -  1.11
> +++ lib/dns/rdataset.c25 Jun 2022 10:43:28 -
> @@ -159,19 +159,6 @@ dns_rdataset_makequestion(dns_rdataset_t
>   rdataset->attributes |= DNS_RDATASETATTR_QUESTION;
>  }
>  
> -void
> -dns_rdataset_clone(dns_rdataset_t *source, dns_rdataset_t *target) {
> -
> - /*
> -  * Make 'target' refer to the same rdataset as 'source'.
> -  */
> -
> - REQUIRE(source->methods != NULL);
> - REQUIRE(target->methods == NULL);
> -
> - (source->methods->clone)(source, target);
> -}
> -
>  isc_result_t
>  dns_rdataset_first(dns_rdataset_t *rdataset) {
>  
> Index: lib/dns/include/dns/rdataset.h
> ===
> RCS file: /cvs/src/usr.bin/dig/lib/dns/include/dns/rdataset.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 rdataset.h
> --- lib/dns/include/dns/rdataset.h25 Jun 2022 10:20:30 -  1.12
> +++ lib/dns/include/dns/rdataset.h25 Jun 2022 10:43:28 -
> @@ -181,20 +181,6 @@ dns_rdataset_makequestion(dns_rdataset_t
>   *\li'rdataset' is a valid, associated, question rdataset.
>   */
>  
> -void
> -dns_rdataset_clone(dns_rdataset_t *source, dns_rdataset_t *target);
> -/*%<
> - * Make 'target' refer to the same rdataset as 'source'.
> - *
> - * Requires:
> - *\li'source' is a valid, associated rdataset.
> - *
> - *\li'target' is a valid, dissociated rdataset.
> - *
> - * Ensures:
> - *\li'target' references the same rdataset as 'source'.
> - */
> -
>  isc_result_t
>  dns_rdataset_first(dns_rdataset_t *rdataset);
>  /*%<
> Index: lib/isc/lex.c
> ===
> RCS file: /cvs/src/usr.bin/dig/lib/isc/lex.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 lex.c
> --- lib/isc/lex.c 17 Jan 2022 18:19:51 -  1.14
> +++ lib/isc/lex.c 25 Jun 2022 10:43:28 -
> @@ -18,7 +18,6 @@
>  
>  /*! \file */
>  
> -#include 
>  #include 
>  
>  #include 
> @@ -238,8 +237,6 @@ typedef enum {
>   lexstate_eatline,
>   lexstate_qstring
>  } lexstate;
> -
> -#define IWSEOL (ISC_LEXOPT_INITIALWS | ISC_LEXOPT_EOL)
>  
>  static void
>  pushback(inputsource *source, int c) {
> Index: lib/isccfg/parser.c
> ===
> RCS file: /cvs/src/usr.bin/dig/lib/isccfg/parser.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 parser.c
> --- lib/isccfg/parser.c   14 Sep 2020 08:40:44 -  1.8
> +++ lib/isccfg/parser.c   25 Jun 2022 10:43:28 -
> @@ -590,49 +590,6 @@ cfg_parse_listelt(cfg_parser_t *pctx, co
>   return (result);
>  }
>  
> -int
> -cfg_obj_islist(const cfg_obj_t *obj) {
> - REQUIRE(obj != NULL);
> - return (obj->type->rep == _rep_list);
> -}
> -
> -const cfg_listelt_t *
> -cfg_list_first(const cfg_obj_t *obj) {
> - REQUIRE(obj == NULL || obj->type->rep == _rep_list);
> - if (obj == NULL)
> - return (NULL);
> - return (ISC_LIST_HEAD(obj->value.list));
> -}
> -
> -const cfg_listelt_t *
> -cfg_list_next(const cfg_listelt_t *elt) {
> - 

Re: rpki-client: cache X509v3 extensions early

[Bob Beck]

yes makes sense

ok beck@

> On May 11, 2022, at 07:53, Theo Buehler  wrote:
> 
> 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.c11 May 2022 09:40:00 -1.77
> +++ cert.c11 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.c28 Mar 2022 13:04:01 -1.16
> +++ cms.c11 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: uvm: Consider BUFPAGES_DEFICIT in swap_shortage

[Bob Beck]

On Thu, May 05, 2022 at 10:16:23AM -0600, Bob Beck wrote:
> Ugh. You???re digging in the most perilous parts of the pile. 
> 
> I will go look with you??? sigh. (This is not yet an ok for that.)
> 
> > On May 5, 2022, at 7:53 AM, Martin Pieuchot  wrote:
> > 
> > When considering the amount of free pages in the page daemon a small
> > amount is always kept for the buffer cache... except in one place.
> > 
> > The diff below gets rid of this exception.  This is important because
> > uvmpd_scan() is called conditionally using the following check:
> > 
> >  if (uvmexp.free - BUFPAGES_DEFICIT) < uvmexp.freetarg) {
> >...
> >  }
> > 
> > So in case of swap shortage we might end up freeing fewer pages than
> > wanted.

So a bit of backgroud. 

I am pretty much of the belief that this "low water mark" for pages is 
nonsense now.  I was in the midst of trying to prove that
to myself and therefore rip down some of the crazy accounting and
very arbitrary limits in the buffer cache and got distracted.

Maybe something like this to start? (buf failing that I think
your current diff is probably ok).

Index: sys/sys/mount.h
===
RCS file: /cvs/src/sys/sys/mount.h,v
retrieving revision 1.148
diff -u -p -u -p -r1.148 mount.h
--- sys/sys/mount.h 6 Apr 2021 14:17:35 -   1.148
+++ sys/sys/mount.h 5 May 2022 16:50:50 -
@@ -488,10 +488,8 @@ struct bcachestats {
 #ifdef _KERNEL
 extern struct bcachestats bcstats;
 extern long buflowpages, bufhighpages, bufbackpages;
-#define BUFPAGES_DEFICIT (((buflowpages - bcstats.numbufpages) < 0) ? 0 \
-: buflowpages - bcstats.numbufpages)
-#define BUFPAGES_INACT (((bcstats.numcleanpages - buflowpages) < 0) ? 0 \
-: bcstats.numcleanpages - buflowpages)
+#define BUFPAGES_DEFICIT 0
+#define BUFPAGES_INACT bcstats.numcleanpages
 extern int bufcachepercent;
 extern void bufadjust(int);
 struct uvm_constraint_range;


> > 
> > ok?
> > 
> > Index: uvm/uvm_pdaemon.c
> > ===
> > RCS file: /cvs/src/sys/uvm/uvm_pdaemon.c,v
> > retrieving revision 1.98
> > diff -u -p -r1.98 uvm_pdaemon.c
> > --- uvm/uvm_pdaemon.c   4 May 2022 14:58:26 -   1.98
> > +++ uvm/uvm_pdaemon.c   5 May 2022 13:40:28 -
> > @@ -923,12 +923,13 @@ uvmpd_scan(void)
> >  * detect if we're not going to be able to page anything out
> >  * until we free some swap resources from active pages.
> >  */
> > +   free = uvmexp.free - BUFPAGES_DEFICIT;
> > swap_shortage = 0;
> > -   if (uvmexp.free < uvmexp.freetarg &&
> > +   if (free < uvmexp.freetarg &&
> > uvmexp.swpginuse == uvmexp.swpages &&
> > !uvm_swapisfull() &&
> > pages_freed == 0) {
> > -   swap_shortage = uvmexp.freetarg - uvmexp.free;
> > +   swap_shortage = uvmexp.freetarg - free;
> > }
> > 
> > for (p = TAILQ_FIRST(_active);
> > 
> 

Re: acme-client: check token names

[Bob Beck]

An ok beck@ from me with my usual curmudgeonly mutterings 
about the people who made this necessary for isalnum(), walls, 
and revolutions...

> On May 5, 2022, at 7:57 AM, Florian Obser  wrote:
> 
> On 2022-05-04 13:21 +0430, Ali Farzanrad  wrote:
>> OK, I've tested following diff on my own domain and it works.
>> I did 2 modifications:
>> 
>> 1. I explicitly call setlocate with "C" to ensure C locale,
> 
> I came to the conclusion that it's best to call setlocale in first thing
> in main, that's what other code does, too and seems less surprising.
> 
>> 
>> 2. I did a string length check. According to RFC it must have 128 bit
>> of random entropy, so it should have at least 22 base64 characters,
>> but I was unsure. So I only check for empty strings.
> 
> Checking for an empty string gives us a better error message, we would
> error out with EISDIR in open(2) later, so that's good I guess.
> Trying to enforce entropy seems a bit silly though, it's there to
> protect the CA, if they mess this up it's their problem.
> 
> The following diff just moves setlocale to main and is OK florian
> 
> Or I can commit it myself is someone else OKs it.
> 
> diff --git chngproc.c chngproc.c
> index 0cbfaf27c31..f9cff65311d 100644
> --- chngproc.c
> +++ chngproc.c
> @@ -16,6 +16,7 @@
> */
> 
> #include 
> +#include 
> #include 
> #include 
> #include 
> @@ -77,6 +78,18 @@ chngproc(int netsock, const char *root)
>   goto out;
>   else if ((tok = readstr(netsock, COMM_TOK)) == NULL)
>   goto out;
> + else if (strlen(tok) < 1) {
> + warnx("token is too short");
> + goto out;
> + }
> +
> + for (i = 0; tok[i]; ++i) {
> + int ch = (unsigned char)tok[i];
> + if (!isalnum(ch) && ch != '-' && ch != '_') {
> + warnx("token is not a valid base64url");
> + goto out;
> + }
> + }
> 
>   if (asprintf(, "%s.%s", tok, th) == -1) {
>   warn("asprintf");
> diff --git main.c main.c
> index 65ea2cf3ac3..a3006ca1483 100644
> --- main.c
> +++ main.c
> @@ -20,6 +20,7 @@
> #include 
> #include 
> #include 
> +#include 
> #include 
> #include 
> #include 
> @@ -56,6 +57,9 @@ main(int argc, char *argv[])
>   struct domain_c *domain = NULL;
>   struct altname_c*ac;
> 
> + if (setlocale(LC_CTYPE, "C") == NULL)
> + errx(1, "setlocale");
> +
>   while ((c = getopt(argc, argv, "Fnrvf:")) != -1)
>   switch (c) {
>   case 'F':
> 
> 
> -- 
> I'm not entirely sure you are real.

Re: rpki-client: factor filename extension parsing into a function

[Bob Beck]

I like that.. LGTM

ok beck@


On Fri, Jan 21, 2022 at 08:37:27PM +0100, Theo Buehler wrote:
> > Lets start with that and optimize this in tree. I think we can rename the
> > function to something like rtype_from_mftfile(). In that case I would move
> > the function as well...
> 
> Like this?
> 
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.111
> diff -u -p -r1.111 extern.h
> --- extern.h  21 Jan 2022 18:49:44 -  1.111
> +++ extern.h  21 Jan 2022 19:36:09 -
> @@ -421,6 +421,8 @@ void   mft_free(struct mft *);
>  struct mft   *mft_parse(X509 **, const char *, const unsigned char *,
>   size_t);
>  struct mft   *mft_read(struct ibuf *);
> +enum rtypertype_from_file_extension(const char *);
> +enum rtypertype_from_mftfile(const char *);
>  
>  void  roa_buffer(struct ibuf *, const struct roa *);
>  void  roa_free(struct roa *);
> @@ -447,12 +449,9 @@ int   valid_ta(const char *, struct auth
>  int   valid_cert(const char *, struct auth_tree *,
>   const struct cert *);
>  int   valid_roa(const char *, struct auth_tree *, struct roa *);
> -int   valid_filename(const char *);
>  int   valid_filehash(int, const char *, size_t);
>  int   valid_uri(const char *, size_t, const char *);
>  int   valid_origin(const char *, const char *);
> -
> -enum rtypertype_from_file_extension(const char *);
>  
>  /* 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.49
> diff -u -p -r1.49 mft.c
> --- mft.c 21 Jan 2022 18:49:44 -  1.49
> +++ mft.c 21 Jan 2022 19:36:10 -
> @@ -16,6 +16,7 @@
>   */
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -121,6 +122,66 @@ check_validity(const ASN1_GENERALIZEDTIM
>  }
>  
>  /*
> + * Determine rtype corresponding to file extension. Returns RTYPE_INVALID
> + * on error or unkown extension.
> + */
> +enum rtype
> +rtype_from_file_extension(const char *fn)
> +{
> + size_t   sz;
> +
> + sz = strlen(fn);
> + if (sz < 5)
> + return RTYPE_INVALID;
> +
> + if (strcasecmp(fn + sz - 4, ".tal") == 0)
> + return RTYPE_TAL;
> + if (strcasecmp(fn + sz - 4, ".cer") == 0)
> + return RTYPE_CER;
> + if (strcasecmp(fn + sz - 4, ".crl") == 0)
> + return RTYPE_CRL;
> + if (strcasecmp(fn + sz - 4, ".mft") == 0)
> + return RTYPE_MFT;
> + if (strcasecmp(fn + sz - 4, ".roa") == 0)
> + return RTYPE_ROA;
> + if (strcasecmp(fn + sz - 4, ".gbr") == 0)
> + return RTYPE_GBR;
> +
> + return RTYPE_INVALID;
> +}
> +
> +/*
> + * Validate that a filename listed on a Manifest only contains characters
> + * permitted in draft-ietf-sidrops-6486bis section 4.2.2 and check that
> + * it's a CER, CRL, GBR or a ROA.
> + * Returns corresponding rtype or RTYPE_INVALID on error.
> + */
> +enum rtype
> +rtype_from_mftfile(const char *fn)
> +{
> + const unsigned char *c;
> + enum rtype   type;
> +
> + for (c = fn; *c != '\0'; ++c)
> + if (!isalnum(*c) && *c != '-' && *c != '_' && *c != '.')
> + return RTYPE_INVALID;
> +
> + if (strchr(fn, '.') != strrchr(fn, '.'))
> + return RTYPE_INVALID;
> +
> + type = rtype_from_file_extension(fn);
> + switch (type) {
> + case RTYPE_CER:
> + case RTYPE_CRL:
> + case RTYPE_GBR:
> + case RTYPE_ROA:
> + return type;
> + default:
> + return RTYPE_INVALID;
> + }
> +}
> +
> +/*
>   * Parse an individual "FileAndHash", RFC 6486, sec. 4.2.
>   * Return zero on failure, non-zero on success.
>   */
> @@ -161,12 +222,10 @@ mft_parse_filehash(struct parse *p, cons
>   if (fn == NULL)
>   err(1, NULL);
>  
> - if (!valid_filename(fn)) {
> + if ((type = rtype_from_mftfile(fn)) == RTYPE_INVALID) {
>   warnx("%s: invalid filename: %s", p->fn, fn);
>   goto out;
>   }
> -
> - type = rtype_from_file_extension(fn);
>  
>   /* Now hash value. */
>  
> Index: parser.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 parser.c
> --- parser.c  21 Jan 2022 18:49:44 -  1.49
> +++ parser.c  21 Jan 2022 19:36:10 -
> @@ -307,7 +307,7 @@ proc_parser_mft_check(const char *fn, st
>  
>   for (i = 0; i < p->filesz; i++) {
>   const struct mftfile *m = >files[i];
> - if (!valid_filename(m->file)) {
> + if (rtype_from_mftfile(m->file) == RTYPE_INVALID) {
>

Re: Add CT methods to standard_exts, fix timestamp printing

[Bob Beck]

ok beck@

> On Nov 23, 2021, at 21:14, Theo Buehler  wrote:
> 
> Two small diffs now that beck has linked the certificate transparency
> code to the build.
> 
> The diff for ext_dat.h links the CT methods to the standard extensions.
> This replaces the gibberish from the CT extensions which are now present
> in most certs with something readable. Try
> 
> $ openssl s_client -connect libressl.org:443 | openssl x509 -noout -text
> 
> The diff for ct_prn makes sure that the timestamp is actually printed.
> Our ASN1_GENERALIZEDTIME_set_string() does not accept fractional
> seconds, so don't feed them into it for printing.  eopenssl11 doesn't
> print the fractional sections either.
> 
> Index: x509/ext_dat.h
> ===
> RCS file: /cvs/src/lib/libcrypto/x509/ext_dat.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 ext_dat.h
> --- x509/ext_dat.h2 Sep 2021 21:27:26 -1.3
> +++ x509/ext_dat.h16 Nov 2021 16:56:19 -
> @@ -73,6 +73,7 @@ extern X509V3_EXT_METHOD v3_crl_hold, v3
> extern X509V3_EXT_METHOD v3_policy_mappings, v3_policy_constraints;
> extern X509V3_EXT_METHOD v3_name_constraints, v3_inhibit_anyp, v3_idp;
> extern const X509V3_EXT_METHOD v3_addr, v3_asid;
> +extern const X509V3_EXT_METHOD v3_ct_scts[3];
> 
> /* This table will be searched using OBJ_bsearch so it *must* kept in
>  * order of the ext_nid values.
> @@ -129,6 +130,11 @@ static const X509V3_EXT_METHOD *standard
>_idp,
>_alt[2],
>_freshest_crl,
> +#ifndef OPENSSL_NO_CT
> +_ct_scts[0],
> +_ct_scts[1],
> +_ct_scts[2],
> +#endif
> };
> 
> /* Number of standard extensions */
> Index: ct/ct_prn.c
> ===
> RCS file: /cvs/src/lib/libcrypto/ct/ct_prn.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 ct_prn.c
> --- ct/ct_prn.c20 Nov 2021 01:10:49 -1.3
> +++ ct/ct_prn.c21 Nov 2021 15:32:56 -
> @@ -71,8 +71,7 @@ timestamp_print(uint64_t timestamp, BIO 
> * Note GeneralizedTime from ASN1_GENERALIZETIME_adj is always 15
> * characters long with a final Z. Update it with fractional seconds.
> */
> -snprintf(genstr, sizeof(genstr), "%.14s.%03dZ",
> -ASN1_STRING_get0_data(gen), (unsigned int)(timestamp % 1000));
> +snprintf(genstr, sizeof(genstr), "%.14sZ", ASN1_STRING_get0_data(gen));
>if (ASN1_GENERALIZEDTIME_set_string(gen, genstr))
>ASN1_GENERALIZEDTIME_print(out, gen);
>ASN1_GENERALIZEDTIME_free(gen);
> 

Re: cert.pem sync

[Bob Beck]

ok

> On Jun 10, 2021, at 05:05, Theo Buehler  wrote:
> 
> On Thu, Jun 10, 2021 at 11:39:46AM +0100, Stuart Henderson wrote:
>> I was just reminded of the Apple cert problem with GeoTrust Global CA
>> and checked and they're using better intermediates for api.push.apple.com
>> now. OK to sync up with Mozilla's CA bundle again, including removal
>> of GeoTrust Global CA?
> 
> Thanks!
> 
>> Changes in the list first; diff below:
>> 
>> -AC Camerfirma S.A.
> 
> Ah, good.
> 
>> Staat der Nederlanden
>>   /C=NL/O=Staat der Nederlanden/CN=Staat der Nederlanden EV Root CA
>> -  /C=NL/O=Staat der Nederlanden/CN=Staat der Nederlanden Root CA - G3
> 
> I could not find any information on this removal and it's still in my
> firefox. Why is that removed in your diff?
> 

Re: NiX Spam mirroring

[Bob Beck]

Should be fixed. a bit of a pain because their new site has
an expired tls cert.


On Thu, Oct 28, 2021 at 07:30:56AM +0200, Jan Johansson wrote:
> Hello!
> 
> I write to you because I beleive that you are running the NiX Spam
> mirroring script for OpenBSD. The feed has been broken for some
> time and I hope that you can fix it.
> 
> More information is available in this thread:
> https://marc.info/?t=16328493413=1=2
> 
> Best regards,
> Jan J
> 

Re: rpki-client add back keep-alive to http requests

[Bob Beck]

ok beck@

On Thu, Sep 09, 2021 at 09:35:51AM +0200, Claudio Jeker wrote:
> While Connection: keep-alive should be the default it seems that at least
> some of the CA repositories fail to behave like that. Adding back the
> Connection header seems to fix this and delta downloads go faster again.
> 
> -- 
> :wq Claudio
> 
> Index: http.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 http.c
> --- http.c1 Sep 2021 09:39:14 -   1.38
> +++ http.c9 Sep 2021 07:26:35 -
> @@ -1026,6 +1026,7 @@ http_request(struct http_connection *con
>   conn->bufpos = 0;
>   if ((r = asprintf(>buf,
>   "GET /%s HTTP/1.1\r\n"
> + "Connection: keep-alive\r\n"
>   "Host: %s\r\n"
>   "Accept-Encoding: identity\r\n"
>   "User-Agent: " HTTP_USER_AGENT "\r\n"
> 

Re: Turn SCHED_LOCK() into a mutex

[Bob Beck]

> > This work has been started by art@ more than a decade ago and I'm
> > willing to finish it 

This is possibly one of the scariest things you can say in OpenBSD. 

I am now calling my doctor to get a giant bag of flintstones chewable
zoloft prescribed to me just so I can recover from seeing you say
this. 

(I do support your efforts doing this however.. tread carefully :) 

Re: Correctly set SSL error if x509_verify fails

[Bob Beck]

On Sun, Oct 25, 2020 at 01:43:10PM -0600, Bob Beck wrote:
> 
> 
> 
> On Fri, Oct 23, 2020 at 09:13:23AM +0200, Theo Buehler wrote:
> > On Thu, Oct 22, 2020 at 08:44:29PM -0700, Jeremy Evans wrote:
> > > I was trying to diagnose a certificate validation failure in Ruby's
> > > openssl extension tests with LibreSSL 3.2.2, and it was made more
> > > difficult because the verification error type was dropped, resulting
> > > in a SSL_R_CERTIFICATE_VERIFY_FAILED error where
> > > SSL_get_verify_result returned X509_V_OK.
> > 
> > Could you perhaps isolate a test case or explain the reproducer in a bit
> > more detail? I tried running ruby 2.7's regress from ports but that
> > always resulted in a SIGABRT (usually while running test_ftp.rb with or
> > without your diff).
> > 
> > With my diff below I once got past this abort and saw this:
> > 
> > OpenSSL::TestSSL#test_verify_result
> > [/usr/ports/pobj/ruby-2.7.2/ruby-2.7.2/test/openssl/utils.rb:279]:
> > exceptions on 1 threads:
> > <19> expected but was
> > <20>.
> > 
> > Did you see <0> here instead?
> > 
> > > I think this patch will fix it.  Compile tested only.  OKs, or is there
> > > a better way to fix it?
> > 
> > This will probably also address the issue with the haproxy test reported
> > on the libressl list:
> > 
> > https://marc.info/?l=libressl=160339671313132=2
> > https://github.com/haproxy/haproxy/issues/916
> > 
> > Regarding your diff, I think setting the error on the store context
> > should not be the responsibility of x509_verify()'s caller. After all,
> > x509_verify() will likely end up being public API.
> > 
> > The below diff should have the same effect as yours.
> 
> This looks ok to me tb@.. and appears to be the correct fix. 
> 

Actually I rescind the ok -> your diff introduces a new problem.

I added regress to catch this problem and it found an issue:
the call into the x509_vfy_check_id can set the xsc->error but
then we don't set the ctx->error and it's sitting still as V_OK
when your diff sets it. and that's bad. 

So this correctly sets the ctx->error coming back from
x509_vfy_check_id so your diff doesn't introduce another regression

you have my ok on this modified version :)  I'll commit
the change to the bettertls regress to catch it once you commit.

Index: x509/x509_verify.c
===
RCS file: /cvs/src/lib/libcrypto/x509/x509_verify.c,v
retrieving revision 1.13
diff -u -p -u -p -r1.13 x509_verify.c
--- x509/x509_verify.c  26 Sep 2020 15:44:06 -  1.13
+++ x509/x509_verify.c  25 Oct 2020 23:58:08 -
@@ -458,8 +458,12 @@ x509_verify_cert_hostname(struct x509_ve
size_t len;
 
if (name == NULL) {
-   if (ctx->xsc != NULL)
-   return x509_vfy_check_id(ctx->xsc);
+   if (ctx->xsc != NULL) {
+   int ret;
+   if ((ret = x509_vfy_check_id(ctx->xsc)) == 0)
+   ctx->error = ctx->xsc->error;
+   return ret;
+   }
return 1;
}
if ((candidate = strdup(name)) == NULL) {
@@ -853,13 +857,13 @@ x509_verify(struct x509_verify_ctx *ctx,
 
if (ctx->roots == NULL || ctx->max_depth == 0) {
ctx->error = X509_V_ERR_INVALID_CALL;
-   return 0;
+   goto err;
}
 
if (ctx->xsc != NULL) {
if (leaf != NULL || name != NULL) {
ctx->error = X509_V_ERR_INVALID_CALL;
-   return 0;
+   goto err;
}
leaf = ctx->xsc->cert;
 
@@ -872,34 +876,34 @@ x509_verify(struct x509_verify_ctx *ctx,
 */
if ((ctx->xsc->chain = sk_X509_new_null()) == NULL) {
ctx->error = X509_V_ERR_OUT_OF_MEM;
-   return 0;
+   goto err;
}
if (!X509_up_ref(leaf)) {
ctx->error = X509_V_ERR_OUT_OF_MEM;
-   return 0;
+   goto err;
}
if (!sk_X509_push(ctx->xsc->chain, leaf)) {
X509_free(leaf);
ctx->error = X509_V_ERR_OUT_OF_MEM;
-   return 0;
+   goto err;
}
ctx->xsc->error_depth = 0;
ctx->xsc->current_cert = leaf;
}
 
if (!x509_verify_cert_valid(ctx, leaf, NULL))
-   return 0;
+   goto

Re: Correctly set SSL error if x509_verify fails

[Bob Beck]

On Fri, Oct 23, 2020 at 09:13:23AM +0200, Theo Buehler wrote:
> On Thu, Oct 22, 2020 at 08:44:29PM -0700, Jeremy Evans wrote:
> > I was trying to diagnose a certificate validation failure in Ruby's
> > openssl extension tests with LibreSSL 3.2.2, and it was made more
> > difficult because the verification error type was dropped, resulting
> > in a SSL_R_CERTIFICATE_VERIFY_FAILED error where
> > SSL_get_verify_result returned X509_V_OK.
> 
> Could you perhaps isolate a test case or explain the reproducer in a bit
> more detail? I tried running ruby 2.7's regress from ports but that
> always resulted in a SIGABRT (usually while running test_ftp.rb with or
> without your diff).
> 
> With my diff below I once got past this abort and saw this:
> 
> OpenSSL::TestSSL#test_verify_result
> [/usr/ports/pobj/ruby-2.7.2/ruby-2.7.2/test/openssl/utils.rb:279]:
> exceptions on 1 threads:
> <19> expected but was
> <20>.
> 
> Did you see <0> here instead?
> 
> > I think this patch will fix it.  Compile tested only.  OKs, or is there
> > a better way to fix it?
> 
> This will probably also address the issue with the haproxy test reported
> on the libressl list:
> 
> https://marc.info/?l=libressl=160339671313132=2
> https://github.com/haproxy/haproxy/issues/916
> 
> Regarding your diff, I think setting the error on the store context
> should not be the responsibility of x509_verify()'s caller. After all,
> x509_verify() will likely end up being public API.
> 
> The below diff should have the same effect as yours.

This looks ok to me tb@.. and appears to be the correct fix. 

> 
> > Thanks,
> > Jeremy
> > 
> > Index: x509_vfy.c
> > ===
> > RCS file: /cvs/src/lib/libcrypto/x509/x509_vfy.c,v
> > retrieving revision 1.81
> > diff -u -p -r1.81 x509_vfy.c
> > --- x509_vfy.c  26 Sep 2020 02:06:28 -  1.81
> > +++ x509_vfy.c  23 Oct 2020 03:34:10 -
> > @@ -680,6 +680,9 @@ X509_verify_cert(X509_STORE_CTX *ctx)
> > if ((vctx = x509_verify_ctx_new_from_xsc(ctx, roots)) != NULL) {
> > ctx->error = X509_V_OK; /* Initialize to OK */
> > chain_count = x509_verify(vctx, NULL, NULL);
> > +   if (vctx->error) {
> > +   ctx->error = vctx->error;
> > +   }
> > }
> >  
> > sk_X509_pop_free(roots, X509_free);
> > 
> 
> 
> Index: x509/x509_verify.c
> ===
> RCS file: /var/cvs/src/lib/libcrypto/x509/x509_verify.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 x509_verify.c
> --- x509/x509_verify.c26 Sep 2020 15:44:06 -  1.13
> +++ x509/x509_verify.c23 Oct 2020 05:04:05 -
> @@ -853,13 +853,13 @@ x509_verify(struct x509_verify_ctx *ctx,
>  
>   if (ctx->roots == NULL || ctx->max_depth == 0) {
>   ctx->error = X509_V_ERR_INVALID_CALL;
> - return 0;
> + goto err;
>   }
>  
>   if (ctx->xsc != NULL) {
>   if (leaf != NULL || name != NULL) {
>   ctx->error = X509_V_ERR_INVALID_CALL;
> - return 0;
> + goto err;
>   }
>   leaf = ctx->xsc->cert;
>  
> @@ -872,34 +872,34 @@ x509_verify(struct x509_verify_ctx *ctx,
>*/
>   if ((ctx->xsc->chain = sk_X509_new_null()) == NULL) {
>   ctx->error = X509_V_ERR_OUT_OF_MEM;
> - return 0;
> + goto err;
>   }
>   if (!X509_up_ref(leaf)) {
>   ctx->error = X509_V_ERR_OUT_OF_MEM;
> - return 0;
> + goto err;
>   }
>   if (!sk_X509_push(ctx->xsc->chain, leaf)) {
>   X509_free(leaf);
>   ctx->error = X509_V_ERR_OUT_OF_MEM;
> - return 0;
> + goto err;
>   }
>   ctx->xsc->error_depth = 0;
>   ctx->xsc->current_cert = leaf;
>   }
>  
>   if (!x509_verify_cert_valid(ctx, leaf, NULL))
> - return 0;
> + goto err;
>  
>   if (!x509_verify_cert_hostname(ctx, leaf, name))
> - return 0;
> + goto err;
>  
>   if ((current_chain = x509_verify_chain_new()) == NULL) {
>   ctx->error = X509_V_ERR_OUT_OF_MEM;
> - return 0;
> + goto err;
>   }
>   if (!x509_verify_chain_append(current_chain, leaf, >error)) {
>   x509_verify_chain_free(current_chain);
> - return 0;
> + goto err;
>   }
>   if (x509_verify_ctx_cert_is_root(ctx, leaf))
>   x509_verify_ctx_add_chain(ctx, current_chain);
> @@ -925,4 +925,9 @@ x509_verify(struct x509_verify_ctx *ctx,
>   return ctx->xsc->verify_cb(ctx->chains_count, ctx->xsc);
>   }
>   return (ctx->chains_count);
> +
> + err:
> + if 

Happy 25th Birthday OpenBSD!

[Bob Beck]

Yeah, it's just a number. 

But it's been a pretty wild ride. Thanks everyone for 25 years.

-Bob

Re: [PATCH netcat] Only force fd's to -1 once

[Bob Beck]

On Sun, Sep 27, 2020 at 02:46:39PM +1000, Duncan Roe wrote:
> The motivation for this is to make debug logs less confusing.

What is this fixing and what behavior are you changing?

> 
> All changed lines have previously demonstrated the problem.
> 
> Signed-off-by: Duncan Roe 
> ---
>  usr.bin/nc/netcat.c | 27 ++-
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/usr.bin/nc/netcat.c b/usr.bin/nc/netcat.c
> index 528dbeea678..b152cfaf635 100644
> --- a/usr.bin/nc/netcat.c
> +++ b/usr.bin/nc/netcat.c
> @@ -1196,7 +1196,7 @@ readwrite(int net_fd, struct tls *tls_ctx)
>   !(pfd[POLL_NETIN].revents & POLLIN))
>   pfd[POLL_NETIN].fd = -1;
>  
> - if (pfd[POLL_NETOUT].revents & POLLHUP) {
> + if ((pfd[POLL_NETOUT].revents & POLLHUP) && pfd[POLL_NETOUT].fd 
> != -1) {
>   if (Nflag)
>   shutdown(pfd[POLL_NETOUT].fd, SHUT_WR);
>   pfd[POLL_NETOUT].fd = -1;
> @@ -1205,14 +1205,14 @@ readwrite(int net_fd, struct tls *tls_ctx)
>   if (pfd[POLL_STDOUT].revents & POLLHUP)
>   pfd[POLL_STDOUT].fd = -1;
>   /* if no net out, stop watching stdin */
> - if (pfd[POLL_NETOUT].fd == -1)
> + if (pfd[POLL_NETOUT].fd == -1 && pfd[POLL_STDIN].fd != -1)
>   pfd[POLL_STDIN].fd = -1;
>   /* if no stdout, stop watching net in */
> - if (pfd[POLL_STDOUT].fd == -1) {
> - if (pfd[POLL_NETIN].fd != -1)
> - shutdown(pfd[POLL_NETIN].fd, SHUT_RD);
> - pfd[POLL_NETIN].fd = -1;
> - }
> + if (pfd[POLL_STDOUT].fd == -1 &&
> + pfd[POLL_NETIN].fd != -1) {
> + shutdown(pfd[POLL_NETIN].fd, SHUT_RD);
> + pfd[POLL_NETIN].fd = -1;
> + }
>  
>   /* try to read from stdin */
>   if (pfd[POLL_STDIN].revents & POLLIN && stdinbufpos < BUFSIZE) {
> @@ -1299,15 +1299,16 @@ readwrite(int net_fd, struct tls *tls_ctx)
>   }
>  
>   /* stdin gone and queue empty? */
> - if (pfd[POLL_STDIN].fd == -1 && stdinbufpos == 0) {
> - if (pfd[POLL_NETOUT].fd != -1 && Nflag)
> - shutdown(pfd[POLL_NETOUT].fd, SHUT_WR);
> + if (pfd[POLL_STDIN].fd == -1 && stdinbufpos == 0 &&
> + pfd[POLL_NETOUT].fd != -1) {
> + if (Nflag)
> + shutdown(pfd[POLL_NETOUT].fd, SHUT_WR);
>   pfd[POLL_NETOUT].fd = -1;
>   }
>   /* net in gone and queue empty? */
> - if (pfd[POLL_NETIN].fd == -1 && netinbufpos == 0) {
> - pfd[POLL_STDOUT].fd = -1;
> - }
> + if (pfd[POLL_NETIN].fd == -1 && netinbufpos == 0 &&
> + pfd[POLL_STDOUT].fd != -1)
> + pfd[POLL_STDOUT].fd = -1;
>   }
>  }
>  
> -- 
> 2.17.5
> 

Re: agentx and clang static analyzer

[Bob Beck]

On Tue, Sep 15, 2020 at 11:08:04AM +0200, Martijn van Duren wrote:
> There are 3 things that actually look like valid complaints when running
> clang's static analyzer.
> 
> 1) A dead store in agentx_recv.
> 2) sizeof(ipaddress) intead of sizeof(*ipaddress). Since this is ipv4,
>this is only a problem if sizeof(pointer) is smaller then 4 bytes,
>which can't happen afaik.
> 3) dst does indeed need to be dereffed, but since dstlen and buflen are
>initialized to 0 and srclen is practically always larger then 0
>we're fine. I'm keeping the *dst here as an additional safeguard.
> 
> The rest seem like false positives to me.
> 
> OK?
> 
> martijn@
> 
> Index: agentx.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/agentx.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 agentx.c
> --- agentx.c  14 Sep 2020 11:30:25 -  1.16
> +++ agentx.c  15 Sep 2020 09:05:57 -
> @@ -135,7 +135,6 @@ agentx_recv(struct agentx *ax)
>   header.aph_packetid = agentx_pdutoh32(, u8);
>   u8 += 4;
>   header.aph_plength = agentx_pdutoh32(, u8);
> - u8 += 4;
>  
>   if (header.aph_version != 1) {
>   errno = EPROTO;

ok for this piece above


> Index: subagentx.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/subagentx.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 subagentx.c
> --- subagentx.c   14 Sep 2020 11:30:25 -  1.1
> +++ subagentx.c   15 Sep 2020 09:05:58 -
> @@ -2929,7 +2929,7 @@ getnext:
>   index->sav_idatacomplete = 1;
>   break;
>   case AGENTX_DATA_TYPE_IPADDRESS:
> - ipaddress = calloc(1, sizeof(ipaddress));
> + ipaddress = calloc(1, sizeof(*ipaddress));
>   if (ipaddress == NULL) {
>   subagentx_log_sag_warn(sag,
>   "Failed to bind ipaddress index");

Not ok for this, it's probably correct, but there are other instances of this
in this code and so you need to engage brain, not static analyzer, go fix or
don't fix them all in a separate commit. 


> @@ -3833,7 +3833,7 @@ subagentx_strcat(char **dst, const char 
>   }
>  
>   srclen = strlen(src);
> - if (dst == NULL || dstlen + srclen > buflen) {
> + if (*dst == NULL || dstlen + srclen > buflen) {
>   nbuflen = (((dstlen + srclen) / 512) + 1) * 512;
>   tmp = recallocarray(*dst, buflen, nbuflen, sizeof(*tmp));
>   if (tmp == NULL)
> 
ok for this piece above

Re: acme-client: improve account creation error message

[Bob Beck]

But what if I like json and I am already set up to be a hipster and
feed all the untrusted inputs through jq..

(ok beck@)

On Mon, Sep 14, 2020 at 03:37:25PM +0200, Florian Obser wrote:
> not helpful:
> $ doas acme-client $(hostname)
> acme-client: https://api.test4.buypass.no/acme-v02/new-acct: bad HTTP: 400
> 
> vomitting unformated json is not better:
> $ doas acme-client -v $(hostname)
> acme-client: transfer buffer: 
> [{"type":"urn:ietf:params:acme:error:malformed","detail":"Email is a required 
> contact","code":400,"message":"MALFORMED_BAD_REQUEST","details":"HTTP 400 Bad 
> Request"}] (164 bytes)
> 
> let's do this:
> $ doas obj/acme-client -v $(hostname)
> acme-client: Email is a required contact
> 
> OK?
> 
> diff --git extern.h extern.h
> index 529d3350205..364425b0500 100644
> --- extern.h
> +++ extern.h
> @@ -259,6 +259,7 @@ intjson_parse_order(struct jsmnn *, 
> struct order *);
>  int   json_parse_upd_order(struct jsmnn *, struct order *);
>  void  json_free_capaths(struct capaths *);
>  int   json_parse_capaths(struct jsmnn *, struct capaths *);
> +char *json_getstr(struct jsmnn *, const char *);
>  
>  char *json_fmt_newcert(const char *);
>  char *json_fmt_chkacc(void);
> diff --git json.c json.c
> index 61d2631359f..a6762eeb258 100644
> --- json.c
> +++ json.c
> @@ -297,7 +297,7 @@ json_getobj(struct jsmnn *n, const char *name)
>   * that it's the correct type.
>   * Returns NULL on failure.
>   */
> -static char *
> +char *
>  json_getstr(struct jsmnn *n, const char *name)
>  {
>   size_t   i;
> diff --git netproc.c netproc.c
> index 7b8152196d1..05e36897c38 100644
> --- netproc.c
> +++ netproc.c
> @@ -371,15 +371,27 @@ sreq(struct conn *c, const char *addr, int kid, const 
> char *req, char **loc)
>  static int
>  donewacc(struct conn *c, const struct capaths *p)
>  {
> + struct jsmnn*j = NULL;
>   int  rc = 0;
> - char*req;
> + char*req, *detail, *error = NULL;
>   long lc;
>  
>   if ((req = json_fmt_newacc()) == NULL)
>   warnx("json_fmt_newacc");
>   else if ((lc = sreq(c, p->newaccount, 0, req, >kid)) < 0)
>   warnx("%s: bad comm", p->newaccount);
> - else if (lc != 200 && lc != 201)
> + else if (lc == 400) {
> + if ((j = json_parse(c->buf.buf, c->buf.sz)) == NULL)
> + warnx("%s: bad JSON object", p->newaccount);
> + else {
> + detail = json_getstr(j, "detail");
> + if (detail != NULL && stravis(, detail, VIS_SAFE)
> + != -1) {
> + warnx("%s", error);
> + free(error);
> + }
> + }
> + } else if (lc != 200 && lc != 201)
>   warnx("%s: bad HTTP: %ld", p->newaccount, lc);
>   else if (c->buf.buf == NULL || c->buf.sz == 0)
>   warnx("%s: empty response", p->newaccount);
> 
> 
> -- 
> I'm not entirely sure you are real.
> 

Re: dt: add static vfs probes

[Bob Beck]

ok beck@

On Mon, Sep 14, 2020 at 12:45:55PM +0200, Jasper Lievisse Adriaanse wrote:
> Hi,
> 
> Whilst analyzing the cleaner I added tracepoints called 'cleaner' and 
> 'bufcache_take' to
> track its behaviour.
> 
> For the sake of symmetry I've added one in bufcache_release() too and moved 
> the assignment
> of 'pages' until after the KASSERT(), following the flow of bufcache_take().
> 
> Sample usage of these probes:
> 
> tracepoint:vfs:bufcache_take {
> printf("bcache_take:%d(%s) flags: 0x%x cache: %d pages: %d\n",
> tid, comm, arg0 , arg1, arg2);
> }
> 
> tracepoint:vfs:bufcache_rel{
> printf("bcache_rel:%d(%s) flags: 0x%x cache: %d pages: %d\n",
> tid, comm, arg0, arg1, arg2);
> }
> 
> tracepoint:vfs:cleaner{
> printf("cleaner:%d(%s) flags: 0x%x pushed: %d lodirtypages: %d, 
> hidirtypages: %d\n",
> tid, comm, arg0, arg1, arg2, arg3);
> }
> 
> OK to commit this?
> 
> Index: dev/dt/dt_prov_static.c
> ===
> RCS file: /cvs/src/sys/dev/dt/dt_prov_static.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 dt_prov_static.c
> --- dev/dt/dt_prov_static.c   13 Sep 2020 14:55:08 -  1.4
> +++ dev/dt/dt_prov_static.c   14 Sep 2020 10:43:43 -
> @@ -58,6 +58,13 @@ DT_STATIC_PROBE3(uvm, map_insert, "vaddr
>  DT_STATIC_PROBE3(uvm, map_remove, "vaddr_t", "vaddr_t", "vm_prot_t");
>  
>  /*
> + * VFS
> + */
> +DT_STATIC_PROBE3(vfs, bufcache_rel, "long", "int", "int64_t");
> +DT_STATIC_PROBE3(vfs, bufcache_take, "long", "int", "int64_t");
> +DT_STATIC_PROBE4(vfs, cleaner, "long", "int", "long", "long");
> +
> +/*
>   * List of all static probes
>   */
>  struct dt_probe *dtps_static[] = {
> @@ -76,6 +83,10 @@ struct dt_probe *dtps_static[] = {
>   &_DT_STATIC_P(uvm, fault),
>   &_DT_STATIC_P(uvm, map_insert),
>   &_DT_STATIC_P(uvm, map_remove),
> + /* VFS */
> + &_DT_STATIC_P(vfs, bufcache_rel),
> + &_DT_STATIC_P(vfs, bufcache_take),
> + &_DT_STATIC_P(vfs, cleaner),
>  };
>  
>  int
> Index: kern/vfs_bio.c
> ===
> RCS file: /cvs/src/sys/kern/vfs_bio.c,v
> retrieving revision 1.202
> diff -u -p -r1.202 vfs_bio.c
> --- kern/vfs_bio.c12 Sep 2020 11:57:24 -  1.202
> +++ kern/vfs_bio.c14 Sep 2020 10:43:43 -
> @@ -57,6 +57,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  /* XXX Should really be in buf.h, but for uvm_constraint_range.. */
> @@ -1209,6 +1210,9 @@ buf_daemon(void *arg)
>   }
>  
>   while ((bp = bufcache_getdirtybuf())) {
> + TRACEPOINT(vfs, cleaner, bp->b_flags, pushed,
> + lodirtypages, hidirtypages);
> +
>   if (UNCLEAN_PAGES < lodirtypages &&
>   bcstats.kvaslots_avail > 2 * RESERVE_SLOTS &&
>   pushed >= 16)
> @@ -1693,6 +1697,9 @@ bufcache_take(struct buf *bp)
>   KASSERT((bp->cache < NUM_CACHES));
>  
>   pages = atop(bp->b_bufsize);
> +
> + TRACEPOINT(vfs, bufcache_take, bp->b_flags, bp->cache, pages);
> +
>   struct bufcache *cache = [bp->cache];
>   if (!ISSET(bp->b_flags, B_DELWRI)) {
>  if (ISSET(bp->b_flags, B_COLD)) {
> @@ -1756,8 +1763,11 @@ bufcache_release(struct buf *bp)
>   int64_t pages;
>   struct bufcache *cache = [bp->cache];
>  
> - pages = atop(bp->b_bufsize);
>   KASSERT(ISSET(bp->b_flags, B_BC));
> + pages = atop(bp->b_bufsize);
> +
> + TRACEPOINT(vfs, bufcache_rel, bp->b_flags, bp->cache, pages);
> +
>   if (fliphigh) {
>   if (ISSET(bp->b_flags, B_DMA) && bp->cache > 0)
>   panic("B_DMA buffer release from cache %d",
> -- 
> jasper
> 

Re: rpki-client cleanup includes

[Bob Beck]

ok beck@

On Sat, Sep 12, 2020 at 05:42:39PM +0200, Claudio Jeker wrote:
> extern.h uses stuff from openssl/x509.h so put that include in there
> and remove all the various other openssl includes in other files that
> actually don't need x509 functions.
> 
> -- 
> :wq Claudio
> 
> Index: as.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/as.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 as.c
> --- as.c  27 Nov 2019 17:18:24 -  1.5
> +++ as.c  12 Sep 2020 15:02:20 -
> @@ -25,8 +25,6 @@
>  #include 
>  #include 
>  
> -#include 
> -
>  #include "extern.h"
>  
>  /*
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 cert.c
> --- cert.c28 Jul 2020 07:35:04 -  1.17
> +++ cert.c12 Sep 2020 15:02:20 -
> @@ -26,7 +26,6 @@
>  #include 
>  #include 
>  
> -#include 
>  #include  /* DIST_POINT */
>  
>  #include "extern.h"
> Index: crl.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 crl.c
> --- crl.c 2 Apr 2020 09:16:43 -   1.8
> +++ crl.c 12 Sep 2020 15:02:20 -
> @@ -26,8 +26,6 @@
>  #include 
>  #include 
>  
> -#include 
> -
>  #include "extern.h"
>  
>  X509_CRL *
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.33
> diff -u -p -r1.33 extern.h
> --- extern.h  12 Sep 2020 10:02:01 -  1.33
> +++ extern.h  12 Sep 2020 15:02:20 -
> @@ -20,6 +20,8 @@
>  #include 
>  #include 
>  
> +#include 
> +
>  enum cert_as_type {
>   CERT_AS_ID, /* single identifier */
>   CERT_AS_INHERIT, /* inherit from parent */
> Index: io.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/io.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 io.c
> --- io.c  29 Nov 2019 05:09:50 -  1.8
> +++ io.c  12 Sep 2020 15:02:20 -
> @@ -25,8 +25,6 @@
>  #include 
>  #include 
>  
> -#include 
> -
>  #include "extern.h"
>  
>  void
> Index: ip.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/ip.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 ip.c
> --- ip.c  16 Apr 2020 14:39:44 -  1.12
> +++ ip.c  12 Sep 2020 15:02:20 -
> @@ -25,8 +25,6 @@
>  #include 
>  #include 
>  
> -#include 
> -
>  #include "extern.h"
>  
>  #define   PREFIX_SIZE(x)  (((x) + 7) / 8)
> Index: log.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/log.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 log.c
> --- log.c 29 Nov 2019 05:14:11 -  1.5
> +++ log.c 12 Sep 2020 15:02:20 -
> @@ -21,7 +21,6 @@
>  #include 
>  
>  #include 
> -#include 
>  
>  #include "extern.h"
>  
> Index: mft.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 mft.c
> --- mft.c 30 Jun 2020 12:52:44 -  1.15
> +++ mft.c 12 Sep 2020 15:02:20 -
> @@ -24,7 +24,6 @@
>  #include 
>  #include 
>  
> -#include 
>  #include 
>  
>  #include "extern.h"
> Index: output-bgpd.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/output-bgpd.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 output-bgpd.c
> --- output-bgpd.c 28 Apr 2020 13:41:35 -  1.17
> +++ output-bgpd.c 12 Sep 2020 15:02:20 -
> @@ -16,7 +16,6 @@
>   */
>  
>  #include 
> -#include 
>  
>  #include "extern.h"
>  
> Index: output-bird.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/output-bird.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 output-bird.c
> --- output-bird.c 28 Apr 2020 15:03:39 -  1.9
> +++ output-bird.c 12 Sep 2020 15:02:20 -
> @@ -17,7 +17,6 @@
>   */
>  
>  #include 
> -#include 
>  
>  #include "extern.h"
>  
> Index: output-csv.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/output-csv.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 output-csv.c
> --- output-csv.c  28 Apr 2020 13:41:35 -  1.7
> +++ output-csv.c  12 Sep 2020 15:02:20 -
> @@ -16,7 +16,6 @@
>   */
>  
>  #include 
> -#include 
>  
>  #include "extern.h"
>  
> Index: output-json.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/output-json.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 output-json.c
> --- output-json.c 3 May 2020 20:24:02 -   1.12
> 

Re: tmpfs bug in reclaim

[Bob Beck]

In the spirit of be careful what sticks to you, 

this has ok beck@


On Mon, Jul 13, 2020 at 11:56:18AM +0200, Gerhard Roth wrote:
> tmpfs_reclaim() has to make sure that the VFS cache has no more
> locks held for the vnode. Else vclean() could panic because v_holdcnt
> is non-zero.
> 
> I know that tmpfs is disabled by default, but it would be nice
> to have this fix in the code base anyway.
> 
> Gerhard
> 
> 
> Index: sys/tmpfs/tmpfs_vnops.c
> ===
> RCS file: /cvs/src/sys/tmpfs/tmpfs_vnops.c,v
> retrieving revision 1.42
> diff -u -p -u -p -r1.42 tmpfs_vnops.c
> --- sys/tmpfs/tmpfs_vnops.c   11 Jun 2020 09:18:43 -  1.42
> +++ sys/tmpfs/tmpfs_vnops.c   13 Jul 2020 09:48:37 -
> @@ -1080,6 +1080,8 @@ tmpfs_reclaim(void *v)
>   racing = TMPFS_NODE_RECLAIMING(node);
>   rw_exit_write(>tn_nlock);
>  
> + cache_purge(vp);
> +
>   /*
>* If inode is not referenced, i.e. no links, then destroy it.
>* Note: if racing - inode is about to get a new vnode, leave it.
> 

Re: Stuck in Needbuf state, trying to understand (6.7)

[Bob Beck]

On Mon, Jun 29, 2020 at 03:56:43PM -0400, sven falempin wrote:
> On Mon, Jun 29, 2020 at 12:58 PM sven falempin 
> wrote:
>
> It works in the original problematic setup.
> 
> Will it go to base ?
> 

Yes.

revision 1.201
date: 2020/07/14 06:02:50;  author: beck;  state: Exp;  lines: +9 -3;  
commitid: G6yRUUYskLjLY0oH;
Do not convert the NOCACHE buffers that come from a vnd strategy routine
into more delayed writes if the vnd is mounted from a file on an MNT_ASYNC
filesystem. This prevents a situaiton where the cleaner can not clean
delayed writes out without making more delayed writes, and we end up
waiting for the syncer to spit things occasionaly when it runs.

noticed and reported by sven falempin  on tech,
who validated this fixes his issue.

ok krw@

Re: Stuck in Needbuf state, trying to understand (6.7)

[Bob Beck]

> Awesome, thanks!
> 
> I will test that, ASAP,
> do not hesitate to slay dragon,
> i heard the bathing in the blood pool is good for the skin
> 
> Little concern, I did the test without the MFS and ran into issues ,
> anyway i get back to you (or list ?) when i have test report with patched
> kernel

Yes, howver, you didn't tell my what options you had on the filesystem mounted
when you did the test without MFS, because it matters. If you had your 
filesystem
mounted ASYNC it would have exhibited the same behavoir.  the issue is due to 
the
async mount, which MFS does by default, not strictly to do with MFS. 


> 
> Again thanks for helping.
> 
> -- 
> --
> -
> Knowing is not enough; we must apply. Willing is not enough; we must do

Re: Stuck in Needbuf state, trying to understand (6.7)

[Bob Beck]

On Sun, Jun 28, 2020 at 12:18:06PM -0400, sven falempin wrote:
> On Sun, Jun 28, 2020 at 2:40 AM Bryan Linton  wrote:
> 
> > On 2020-06-27 19:29:31, Bob Beck  wrote:
> > >
> > > No.
> > >
> > > I know *exactly* what needbuf is but to attempt to diagnose what your
> > > problem is we need exact details. especially:
> > >
> > > 1) The configuration of your system including all the details of the
> > filesystems
> > > you have mounted, all options used, etc.
> > >
> > > 2) The script you are using to generate the problem (Not a paraphrasing
> > of what
> > > you think the script does) What filesystems it is using.
> > >
> >
> > Not the OP, but this problem sounds almost exactly like the bug I
> > reported last year.
> >
> > There is a detailed list of steps I used to reproduce the bug in
> > the following bug report.
> >
> > https://marc.info/?l=openbsd-bugs=156412299418191
> >
> > I was even able to bisect and identify the commit which first
> > caused the breakage for me.
> >
> >
> > ---8<---
> >
> > CVSROOT:/cvs
> > Module name:src
> > Changes by: b...@cvs.openbsd.org2019/05/08 06:40:57
> >
> > Modified files:
> > sys/kern   : vfs_bio.c vfs_biomem.c
> >
> > Log message:
> > Modify the buffer cache to always flip recovered DMA buffers high.
> >
> > This also modifies the backoff logic to only back off what is requested
> > and not a "mimimum" amount. Tested by me, benno@, tedu@ anda ports build
> > by naddy@.
> >
> > ok tedu@
> >
> > ---8<---
> >
> > However, I have since migrated away from using vnd(4)s since I was
> > able to find other solutions that worked for my use cases.  So I
> > may not be able to provide much additional information other than
> > what is contained in the above bug report.
> >
> > --
> > Bryan
> >
> > >
> > >
> >
> >
> Reproduction of BUG.
> 
> 
> # optional
> mkdir /tmpfs
> mount_mfs -o rw -s 2500M swap /tmpfs # i mounted through fstab so this line
> is not tested
> #the bug
> /bin/dd if=/dev/zero of=/tmpfs/img.dd count=0 bs=1 seek=25
> vnconfig vnd3 /tmpfs/img.dd
> printf "a a\n\n\n\nw\nq\n" | disklabel -E vnd3
> newfs vnd3a
> mount /dev/vnd3a /mnt
> cd /tmp && ftp https://cdn.openbsd.org/pub/OpenBSD/6.7/amd64/base67.tgz
> cd /mnt
> #will occur here (the mkdir was ub beedbuf state for a while ...
> for v in 1 2 3 4 5 6 7 8 9; do mkdir /tmp/$v; tar xzvf /tmp/base67.tgz -C
> /mnt/$v; done
> 
> Ready to test patches.
> 
> 

So, your problem is that you have your vnd created in an mfs
filesystem, when I run your test with the vnd backed by a regular
filesystem (withe softdep even) it works fine. 

The trouble happens when your VND has buffers cached in it's
"filesystem" but then is not flushing them out to the underlyin file
(vnode) that you have your vnd backed by.  On normal filesystems this
works fine, since vnd tells the lower layer to not cache the writes
and to do them syncrhonously, to avoid an explosion of delayed writes
and dependencies of buffers. 

The problem happens when we convert syncryonous bwrites to
asynchronous bdwrites if the fileystem is mounted ASYNC, which,
curiously, MFS always is (I don't know why, it doesn't really make any
sense, and I might even look at changing that) All the writes you do
end up being delayed anc chewing up more buffer space. And they are
all tied to one vnode (your image).  once you exhaust the buffer
space, the cleaner runs, but as you have noticed can't clean out your
vnode until the syncer runs (every 60 seconds).  This is why your
thing "takes a long time", and things stall in need buffer. softdep
has deep dark voodoo in it to avoid this problem and therefore when I
use a softdep filesystem instead of an ASYNC filesystem it works. 

Anyway, what's below fixes your issue on my machine. I'm not sure I'm
happy that it's the final fix but it does fix it. there are many other
dragons lurking in here.

Index: sys/kern/vfs_bio.c
===
RCS file: /cvs/src/sys/kern/vfs_bio.c,v
retrieving revision 1.200
diff -u -p -u -p -r1.200 vfs_bio.c
--- sys/kern/vfs_bio.c  29 Apr 2020 02:25:48 -  1.200
+++ sys/kern/vfs_bio.c  29 Jun 2020 15:18:21 -
@@ -706,8 +706,14 @@ bwrite(struct buf *bp)
 */
async = ISSET(bp->b_flags, B_ASYNC);
if (!async && mp && ISSET(mp->mnt_flag, MNT_ASYNC)) {
-   bdwrite(bp);
-   return (0);
+   /*
+* Don't convert writes from VND on async filesystems
+* that already have delayed writes in the upper layer.
+*/
+   if (!ISSET(bp->b_flags, B_NOCACHE)) {
+   bdwrite(bp);
+   return (0);
+   }
}
 
/*

Re: Stuck in Needbuf state, trying to understand (6.7)

[Bob Beck]

No. 

I know *exactly* what needbuf is but to attempt to diagnose what your
problem is we need exact details. especially:

1) The configuration of your system including all the details of the filesystems
you have mounted, all options used, etc. 

2) The script you are using to generate the problem (Not a paraphrasing of what
you think the script does) What filesystems it is using. 



On Sat, Jun 27, 2020 at 08:09:18PM -0400, sven falempin wrote:
> On Fri, Jun 26, 2020 at 7:35 PM sven falempin 
> wrote:
> 
> >
> >
> > On Fri, Jun 26, 2020 at 5:22 PM Stuart Henderson 
> > wrote:
> >
> >> On 2020/06/26 15:30, sven falempin wrote:
> >> > behavior confirmed on current.
> >> >
> >> > Once the process stalls,  ( could be anything writing to the vnconfig
> >> disk,
> >> > cp , umount )
> >> > a few other calls like df , or ps, etc may hang, never the same
> >> > sp or mp kernel, reproduced on today's snapshots.
> >>
> >> vnconfig is used as part of "make release", many builds are done every
> >> week using this so it's not a general problem with vnconfig.
> >>
> >> Can you show some commands or a script to trigger the behaviour?
> >>
> >
> > the perl script use the system to call :
> >
> > vnconfig.
> > mount.
> > umount. <- saw hanged
> > cp.<- saw hanged
> > tar.<- saw hanged
> > svn up.<- saw hanged
> > and dd.
> > newfs.
> >
> > really nothing fancy, only stuff writing to disk got stuck.
> >
> > At one point it does a chroot but it never hangs near that , most of the
> > time it hangs before.
> >
> > The script has been used like 1000 times on 6.0 and maybe twice more on
> > 6.4.
> >
> > I have absolutely no idea what the 'needbuf' of top is .
> >
> > the script hangs at random position , always writing into vnconfig.
> >
> > I have no idea how to reproduce outside the perl script , so maybe it is
> > related
> > to some devious perl stdin/stdout buffer .
> >
> > Nevertheless there's like a 5% chance that's the script will work( slowly )
> >
> > Most of the system call are inside a routine to log
> >
> > sub debug_system {
> >   $logger->debug('running: '.join(' ', @_));
> >   return system(@_);
> > }
> >
> > so i can easily put things inside to try to understand the issue.
> >
> > It is really a strange behavior, and the device must be shut down
> > electrically.
> > Something really odd, i run syslogd on a buffer, and syslogc buffer is
> > stuck too
> > when the device stuck (but it supposed to be mostly already allocated
> > memory ).
> >
> > It's really like the vm does not want to give anymore bucket (<- i
> > don't know what i m talking about here,
> > but i looks like that anything that doesn't malloc is ok , computer reply
> > to ping , can do a few things for a while , and then complete
> > hang )
> >
> > I ran the 6.7 release on a VM somewhere and another device with many perl
> > script and they work.
> >
> > Only this fails 95% of the time and is VERY VERY slow when ok.
> > compared to what i saw in /usr/src the vnconfig is big ,  ( forgot to copy
> > df -h  ),
> > like 2GB
> >
> 
> 
> i put ktrace in front of the perl system call
> 
> An di was able to recover a 800MB trace
> 
> $ kdump -f ./trace.out | tail -20
> kdump: realloc: Cannot allocate memory
>  25955 UNKNOWN(1634890859)
>  72466 ? CALL  syscall()
> 
> 
> could that be of some use ?
> 
> 
> -- 
> --
> -
> Knowing is not enough; we must apply. Willing is not enough; we must do

Re: drop addtrust from cert.pem?

[Bob Beck]

On Mon, Jun 01, 2020 at 06:04:17PM +0100, Stuart Henderson wrote:
> OK to drop the expired AddTrust cert from cert.pem?

yes, thanks.

> 
> I checked against the firefox set, there are no new/removed certs that
> work with libressl there. There are now two with GENERALIZEDTIME notAfter
> dates from before 2050 that don't work though (I only remember seeing one
> of those when I last looked).. but that is a separate issue.
> 
> /C=EE/O=AS Sertifitseerimiskeskus/CN=EE Certification Centre Root 
> CA/emailAddress=p...@sk.ee
> /C=PL/O=Unizeto Technologies S.A./OU=Certum Certification Authority/CN=Certum 
> Trusted Network CA 2

I suspect these can safely be dropped too.

> 
> 
> Index: cert.pem
> ===
> RCS file: /cvs/src/lib/libcrypto/cert.pem,v
> retrieving revision 1.20
> diff -u -p -r1.20 cert.pem
> --- cert.pem  10 Apr 2020 12:13:17 -  1.20
> +++ cert.pem  1 Jun 2020 16:59:23 -
> @@ -350,58 +350,6 @@ LysRJyU3eExRarDzzFhdFPFqSBX/wge2sY0PjlxQ
>  LnPqZih4zR0Uv6CPLy64Lo7yFIrM6bV8+2ydDKXhlg==
>  -END CERTIFICATE-
>  
> -### AddTrust AB
> -
> -=== /C=SE/O=AddTrust AB/OU=AddTrust External TTP Network/CN=AddTrust 
> External CA Root
> -Certificate:
> -Data:
> -Version: 3 (0x2)
> -Serial Number: 1 (0x1)
> -Signature Algorithm: sha1WithRSAEncryption
> -Validity
> -Not Before: May 30 10:48:38 2000 GMT
> -Not After : May 30 10:48:38 2020 GMT
> -Subject: C=SE, O=AddTrust AB, OU=AddTrust External TTP Network, 
> CN=AddTrust External CA Root
> -X509v3 extensions:
> -X509v3 Subject Key Identifier: 
> -AD:BD:98:7A:34:B4:26:F7:FA:C4:26:54:EF:03:BD:E0:24:CB:54:1A
> -X509v3 Key Usage: 
> -Certificate Sign, CRL Sign
> -X509v3 Basic Constraints: critical
> -CA:TRUE
> -X509v3 Authority Key Identifier: 
> -
> keyid:AD:BD:98:7A:34:B4:26:F7:FA:C4:26:54:EF:03:BD:E0:24:CB:54:1A
> -DirName:/C=SE/O=AddTrust AB/OU=AddTrust External TTP 
> Network/CN=AddTrust External CA Root
> -serial:01
> -
> -SHA1 Fingerprint=02:FA:F3:E2:91:43:54:68:60:78:57:69:4D:F5:E4:5B:68:85:18:68
> -SHA256 
> Fingerprint=68:7F:A4:51:38:22:78:FF:F0:C8:B1:1F:8D:43:D5:76:67:1C:6E:B2:BC:EA:B4:13:FB:83:D9:65:D0:6D:2F:F2
> --BEGIN CERTIFICATE-
> -MIIENjCCAx6gAwIBAgIBATANBgkqhkiG9w0BAQUFADBvMQswCQYDVQQGEwJTRTEU
> -MBIGA1UEChMLQWRkVHJ1c3QgQUIxJjAkBgNVBAsTHUFkZFRydXN0IEV4dGVybmFs
> -IFRUUCBOZXR3b3JrMSIwIAYDVQQDExlBZGRUcnVzdCBFeHRlcm5hbCBDQSBSb290
> -MB4XDTAwMDUzMDEwNDgzOFoXDTIwMDUzMDEwNDgzOFowbzELMAkGA1UEBhMCU0Ux
> -FDASBgNVBAoTC0FkZFRydXN0IEFCMSYwJAYDVQQLEx1BZGRUcnVzdCBFeHRlcm5h
> -bCBUVFAgTmV0d29yazEiMCAGA1UEAxMZQWRkVHJ1c3QgRXh0ZXJuYWwgQ0EgUm9v
> -dDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALf3GjPm8gAELTngTlvt
> -H7xsD821+iO2zt6bETOXpClMfZOfvUq8k+0DGuOPz+VtUFrWlymUWoCwSXrbLpX9
> -uMq/NzgtHj6RQa1wVsfwTz/oMp50ysiQVOnGXw94nZpAPA6sYapeFI+eh6FqUNzX
> -mk6vBbOmcZSccbNQYArHE504B4YCqOmoaSYYkKtMsE8jqzpPhNjfzp/haW+710LX
> -a0Tkx63ubUFfclpxCDezeWWkWaCUN/cALw3CknLa0Dhy2xSoRcRdKn23tNbE7qzN
> -E0S3ySvdQwAl+mG5aWpYIxG3pzOPVnVZ9c0p10a3CitlttNCbxWyuHv77+ldU9U0
> -WicCAwEAAaOB3DCB2TAdBgNVHQ4EFgQUrb2YejS0Jvf6xCZU7wO94CTLVBowCwYD
> -VR0PBAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8wgZkGA1UdIwSBkTCBjoAUrb2YejS0
> -Jvf6xCZU7wO94CTLVBqhc6RxMG8xCzAJBgNVBAYTAlNFMRQwEgYDVQQKEwtBZGRU
> -cnVzdCBBQjEmMCQGA1UECxMdQWRkVHJ1c3QgRXh0ZXJuYWwgVFRQIE5ldHdvcmsx
> -IjAgBgNVBAMTGUFkZFRydXN0IEV4dGVybmFsIENBIFJvb3SCAQEwDQYJKoZIhvcN
> -AQEFBQADggEBALCb4IUlwtYj4g+WBpKdQZic2YR5gdkeWxQHIzZlj7DYd7usQWxH
> -YINRsPkyPef89iYTx4AWpb9a/IfPeHmJIZriTAcKhjW88t5RxNKWt9x+Tu5w/Rw5
> -6wwCURQtjr0W4MHfRnXnJK3s9EK0hZNwEGe6nQY1ShjTK3rMUUKhemPR5ruhxSvC
> -Nr4TDea9Y355e6cJDUCrat2PisP29owaQgVR1EX1n6diIWgVIEM8med8vSTYqZEX
> -c4g/VhsxOBi0cQ+azcgOno4uG+GMmIPLHzHxREzGBHNJdmAPx/i9F4BrLunMTA5a
> -mnkPIAou1Z5jJh5VkpTYghdae9C8x49OhgQ=
> --END CERTIFICATE-
> -
>  ### AffirmTrust
>  
>  === /C=US/O=AffirmTrust/CN=AffirmTrust Commercial
> 

Re: drop addtrust from cert.pem?

[Bob Beck]

On Mon, Jun 01, 2020 at 07:17:28PM +0200, Theo Buehler wrote:
> On Mon, Jun 01, 2020 at 06:04:17PM +0100, Stuart Henderson wrote:
> > OK to drop the expired AddTrust cert from cert.pem?
> 
> Thanks for taking care of this (and for checking the firefox set). I see
> no reason to keep it.
> 
> ok
> 
> > I checked against the firefox set, there are no new/removed certs that
> > work with libressl there. There are now two with GENERALIZEDTIME notAfter
> > dates from before 2050 that don't work though (I only remember seeing one
> > of those when I last looked).. but that is a separate issue.
> > 
> > /C=EE/O=AS Sertifitseerimiskeskus/CN=EE Certification Centre Root 
> > CA/emailAddress=p...@sk.ee
> > /C=PL/O=Unizeto Technologies S.A./OU=Certum Certification 
> > Authority/CN=Certum Trusted Network CA 2
> 
> Ugh...

Re: smtpd: make smarthost to use SNI when relaying

[Bob Beck]

looks good to me

ok beck@

On Sun, May 31, 2020 at 03:38:00PM +0200, Sebastien Marie wrote:
> Hi,
> 
> updated diff after millert@ and beck@ remarks:
> - use union to collapse in_addr + in6_addr
> - doesn't allocate buffer and directly use s->relay->domain->name
> 
> Thanks.
> -- 
> Sebastien Marie
> 
> 
> diff 73b535ef4537e8454483912fc3420bc304759e96 /home/semarie/repos/openbsd/src
> blob - d384692a0e43de47d645142a6b99e72b7d83b687
> file + usr.sbin/smtpd/mta_session.c
> --- usr.sbin/smtpd/mta_session.c
> +++ usr.sbin/smtpd/mta_session.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1604,6 +1605,10 @@ mta_cert_init_cb(void *arg, int status, const char *na
>   struct mta_session *s = arg;
>   void *ssl;
>   char *xname = NULL, *xcert = NULL;
> + union {
> + struct in_addr in4;
> + struct in6_addr in6;
> + } addrbuf;
>  
>   if (s->flags & MTA_WAIT)
>   mta_tree_pop(_tls_init, s->id);
> @@ -1623,6 +1628,22 @@ mta_cert_init_cb(void *arg, int status, const char *na
>   free(xcert);
>   if (ssl == NULL)
>   fatal("mta: ssl_mta_init");
> +
> + /*
> +  * RFC4366 (SNI): Literal IPv4 and IPv6 addresses are not
> +  * permitted in "HostName".
> +  */
> + if (s->relay->domain->as_host == 1) {
> + if (inet_pton(AF_INET, s->relay->domain->name, ) != 1 &&
> + inet_pton(AF_INET6, s->relay->domain->name, ) != 1) 
> {
> + log_debug("%016"PRIx64" mta tls setting SNI name=%s",
> + s->id, s->relay->domain->name);
> + if (SSL_set_tlsext_host_name(ssl, 
> s->relay->domain->name) == 0)
> + log_warnx("%016"PRIx64" mta tls setting SNI 
> failed",
> +s->id);
> + }
> + }
> +
>   io_start_tls(s->io, ssl);
>  }
>  
> 

Re: smtpd: make smarthost to use SNI when relaying

[Bob Beck]

On Sat, May 30, 2020 at 05:40:43PM +0200, Sebastien Marie wrote:
> Hi,
> 
> I am looking to make smtpd to set SNI (SSL_set_tlsext_host_name) when 
> connecting
> to smarthost when relaying mail.
> 
> After digging a bit in libtls (to stole the right code) and smtpd (to see 
> where
> to put the stolen code), I have the following diff:
> 
> 
> diff 73b535ef4537e8454483912fc3420bc304759e96 /home/semarie/repos/openbsd/src
> blob - d384692a0e43de47d645142a6b99e72b7d83b687
> file + usr.sbin/smtpd/mta_session.c
> --- usr.sbin/smtpd/mta_session.c
> +++ usr.sbin/smtpd/mta_session.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1604,6 +1605,8 @@ mta_cert_init_cb(void *arg, int status, const char *na
>   struct mta_session *s = arg;
>   void *ssl;
>   char *xname = NULL, *xcert = NULL;
> + struct in_addr addrbuf4;
> + struct in6_addr addrbuf6;
>  
>   if (s->flags & MTA_WAIT)
>   mta_tree_pop(_tls_init, s->id);
> @@ -1623,6 +1626,24 @@ mta_cert_init_cb(void *arg, int status, const char *na
>   free(xcert);
>   if (ssl == NULL)
>   fatal("mta: ssl_mta_init");
> +
> + /*
> +  * RFC4366 (SNI): Literal IPv4 and IPv6 addresses are not
> +  * permitted in "HostName".
> +  */
> + if (s->relay->domain->as_host == 1) {
> + xname = xstrdup(s->relay->domain->name);

This allocation appears to be unnecessary.  I believe you should be
able to simply check with inet_pton, and then use
s->relay->domain->name

On a strictly smtpd front, it seems odd to have somthing called
domain->name possibly being an ip address in text form. It would seem
prudent to keep these things separate as we resolve things. (domain->ip, or
domain->mxip if we have resolved down to that might be better) but
that's a separate issue and larger change.

> + if (inet_pton(AF_INET, xname, ) != 1 &&
> + inet_pton(AF_INET6, xname, ) != 1) {
> + log_info("%016"PRIx64" mta setting SNI name=%s",
> + s->id, xname);
> + if (SSL_set_tlsext_host_name(ssl, xname) == 0)
> + log_warnx("%016"PRIx64" mta setting SNI failed",
> +s->id);
> + }
> + free(xname);
> + }
> +
>   io_start_tls(s->io, ssl);
>  }
>  
> 
> 
> For what I understood:
> 
> mta_cert_init_cb() function is responsable to prepare a connection. the SSL
> initialization (SSL_new() call) occured in ssl_mta_init() which was just 
> called,
> so it seems it is the right place to call SSL_set_tlsext_host_name().
> 
> We just need the hostname to configure it.
> 
> Regarding mta_session structure, relay->domain->as_host is set to 1 when the
> domain is linked to smarthost configuration (or when the mx is ip address I
> think). And in smarthost case, the domain->name is the hostname. For SNI, we 
> are
> excluding ip, so I assume it should copte with domain->name as ip.
> 
> Does someone with better understanding of smtpd code source could confirm the
> approch is right and comment ?
> 
> Please note I have only tested it on simple configuration.
> 
> Thanks.
> -- 
> Sebastien Marie
> 

Re: official ports vs DEBUG_PACKAGES

[Bob Beck]

> (iirc python does something strange)

Inconcievable!

Re: official ports vs DEBUG_PACKAGES

[Bob Beck]

On Fri, May 29, 2020 at 06:14:44PM +0200, Marc Espie wrote:
> In a trace:
> 
> > > > #3  0x15e48c95459e in WebVfx::shutdown ()
> > > >  at /usr/obj/ports/webvfx-1.2.0/webvfx-1.2.0/webvfx/webvfx.cpp:193
> 
> Now, this is NOT the default location for WRKOBJDIR, but we are shipping
> packages with debug information...
> 
> This could be a bit cumbersome, if in order to debug locally, you have
> to make patch NO_DEPENDS=Yes, and then you figure out you have to mimic the
> "official" setup for ports, which does not even match the default.
> 
> There are about 3 solutions to that:
> - change the bulk machines to /usr/ports/pobj
> - change the ports default to /usr/obj/ports

It's not realpathing is it? correct me if I am wrong but would
making /usr/ports/pobj a symlink to /usr/obj/ports on the bulk 
machines and running them with the default config just "fix" this? 

> - have some magic I don't know in ELF handling that would allow to either
> tweak the default location/introduce ${WRKOBJDIR} in that debug info.
> 

Re: nsd 4.3.1

[Bob Beck]

> On May 8, 2020, at 03:00, Stuart Henderson  wrote:
> 
> On 2020/05/08 06:58, Florian Obser wrote:
>> I'm running this for about 2 weeks or so.
>> Tests, OKs?
> 
> Just off to look at a radio link in a church tower that I suspect a pigeon
> may have knocked out of alignment,


This is possibly one of the most English outages I have ever heard of




> I'll put this on some machines when I get
> back, just wanted to comment:
> 
>> - I'm adding RELNOTES, it helps my process of merging upstream
>> - I forgot to update ChangeLog previously, so there are unrelated
>> changes in there as well. I'll first bring in the ChangeLog and
>> RELNOTES changes up to 4.2.4 and then update to 4.3.1 on top.
> 
> Yes please, I found this very helpful when doing Unbound updates.
> 
> I wonder if testers will notice what's different about responses (I think
> it's a good thing).
> 

Recent 'ftplist' changes visible in the installer

[Bob Beck]

So, as some of you know the installer hits ftp.openbsd.org during the
install process to query a CGI to provide you with a list of nearby mirrors
and some other useful things. 

I've recently made some changes to modernize and improve this after
the retirement of the GEO:IP modules and increasing fun with using maxmind
without hitting an API server.  While it's true that if you install from
a mirror or hit the cgi, we know where you came from, I'm not super comfortable
handing that information to a third party for no good reason. 

Fortunately with the help of fcambus@, I managed to switch over to
the free dbip databases, which can be installed from ports and kept local
to the machine.  With the help of afresh1@ we managed to get self contained
timezone information as well, based on some earlier work by andy hook.

Which is long way to say, there should be improvements in what
mirrors you are offered in the installer, especially if you are in some of
the more isolated places lacking an openbsd mirror in your country. 

If you can try it out, and you see anything problematic, please let
me know. 

Thanks

-Bob

Re: suggest to run rpki-client hourly

[Bob Beck]

On Mon, Apr 13, 2020 at 09:23:23PM -0600, Todd C. Miller wrote:
> On Mon, 13 Apr 2020 20:27:30 -0600, Bob Beck wrote:
> 
> > In my hearts desire I'd love for "R" to be chosen for each line once at 
> > start
> > up. (so in
> > the above example the things are randomly distributed).  but not sure how 
> > har
> > d that is..
> >
> > If it saves code and effort I really think this is only useful for hours 
> > and 
> > minutes
> 
> Here's a version that uses a suggestion from Theo to support ranges
> like "0~30" to mean a random number between 0 and 30 and just a
> bare "~" to mean a random value in the valid range for that field.
> 
> The random values are chosen when the file is parsed which means
> that on reload due to crontab edit they will change.  I was trying
> to avoid that initially but now I don't think it is a big deal.

Like this one plenty.  I think it's ok the values change on reload. 

> 
>  - todd
> 
> Index: usr.sbin/cron/entry.c
> ===
> RCS file: /cvs/src/usr.sbin/cron/entry.c,v
> retrieving revision 1.49
> diff -u -p -u -r1.49 entry.c
> --- usr.sbin/cron/entry.c 13 Jun 2018 11:27:30 -  1.49
> +++ usr.sbin/cron/entry.c 14 Apr 2020 01:52:13 -
> @@ -450,13 +450,30 @@ static int
>  get_range(bitstr_t *bits, int low, int high, const char *names[],
> int ch, FILE *file)
>  {
> - /* range = number | number "-" number [ "/" number ]
> + /* range = number | number* "~" number* | number "-" number ["/" number]
>*/
>  
>   int i, num1, num2, num3;
>  
> + /* XXX - parse ~, X~Y, X~ and ~Y */
> +
> + if (ch == '~') {
> + /* '~' means a random number [high, low]
> +  */
> + num1 = arc4random_uniform(high - low + 1) + low;
> + ch = get_char(file);
> + if (ch == EOF)
> + return (EOF);
> +
> + if (EOF == set_element(bits, low, high, num1)) {
> + unget_char(ch, file);
> + return (EOF);
> + }
> + return (ch);
> + }
> +
>   if (ch == '*') {
> - /* '*' means "first-last" but can still be modified by /step
> + /* '*' means "high-low" but can still be modified by /step
>*/
>   num1 = low;
>   num2 = high;
> @@ -464,30 +481,45 @@ get_range(bitstr_t *bits, int low, int h
>   if (ch == EOF)
>   return (EOF);
>   } else {
> - ch = get_number(, low, names, ch, file, ",- \t\n");
> + ch = get_number(, low, names, ch, file, ",-~ \t\n");
>   if (ch == EOF)
>   return (EOF);
>  
> - if (ch != '-') {
> - /* not a range, it's a single number.
> -  */
> - if (EOF == set_element(bits, low, high, num1)) {
> - unget_char(ch, file);
> - return (EOF);
> - }
> - return (ch);
> - } else {
> - /* eat the dash
> + switch (ch) {
> + case '-':
> + case '~':
> + num3 = ch;
> +
> + /* eat the dash/tilde
>*/
>   ch = get_char(file);
>   if (ch == EOF)
>   return (EOF);
>  
> - /* get the number following the dash
> + /* get the number following the dash/tilde
>*/
>   ch = get_number(, low, names, ch, file, "/, \t\n");
>   if (ch == EOF || num1 > num2)
>   return (EOF);
> +
> + /* if it's a standard range, we're done here.
> +  */
> + if (num3 == '-')
> + break;
> +
> + /* get a random number in the range [num1, num2]
> +  */
> + num3 = num1;
> + num1 = arc4random_uniform(num2 - num3 + 1) + num3;
> + /* FALLTHROUGH */
> + default:
> + /* not a range, it's a single number.
> +  */
> + if (EOF == set_element(bits, low, high, num1)) {
> + unget_char(ch, file);
> + return (EOF);
> + }
> + return (ch);
>   }
>   }
>  
> 

Re: suggest to run rpki-client hourly

[Bob Beck]

A couple thoughts: 

1) I'm not sure how useful random months or days of the week are, but for 
consistency maybe?
2) if I do something like

R * * * * /usr/local/bin/thing1
R * * * * /usr/local/bin/thing2
R * * * * /usr/local/bin/thing3
...
R * * * * /usr/local/bin/thing1000

I still have the thundering herd problem a bit, in that all my things fire at 
the same R. 

In my hearts desire I'd love for "R" to be chosen for each line once at 
startup. (so in
the above example the things are randomly distributed).  but not sure how hard 
that is..

If it saves code and effort I really think this is only useful for hours and 
minutes


On Mon, Apr 13, 2020 at 12:54:34PM -0600, Todd C. Miller wrote:
> On Mon, 13 Apr 2020 10:00:52 -0600, Bob Beck wrote:
> 
> > +1000. a new random time chosen at cron start. 
> >
> > We see this all the time, and it would be a lot better than the hacks for 
> > all
> >  the things
> >
> > "R" for random sounds good to me
> 
> How about this?  If you guys like it I'll update the man page too.
> So far only tested with a crontab entry like:
> 
> R * * * * date
> 
>  - todd
> 
> Index: usr.sbin/cron/cron.c
> ===
> RCS file: /cvs/src/usr.sbin/cron/cron.c,v
> retrieving revision 1.78
> diff -u -p -u -r1.78 cron.c
> --- usr.sbin/cron/cron.c  11 Feb 2020 12:42:01 -  1.78
> +++ usr.sbin/cron/cron.c  13 Apr 2020 16:25:44 -
> @@ -129,6 +129,7 @@ main(int argc, char *argv[])
>   syslog(LOG_INFO, "(CRON) STARTUP (%s)", CRON_VERSION);
>   }
>  
> + init_random();
>   load_database();
>   scan_atjobs(_database, NULL);
>   set_time(TRUE);
> Index: usr.sbin/cron/entry.c
> ===
> RCS file: /cvs/src/usr.sbin/cron/entry.c,v
> retrieving revision 1.49
> diff -u -p -u -r1.49 entry.c
> --- usr.sbin/cron/entry.c 13 Jun 2018 11:27:30 -  1.49
> +++ usr.sbin/cron/entry.c 13 Apr 2020 18:49:56 -
> @@ -54,6 +54,12 @@ static const char *ecodes[] = {
>   "out of memory"
>  };
>  
> +typedef  enum r_type {
> + r_minute, r_hour, r_dom, r_month, r_dow
> +} rtype_e;
> +
> +static int rand_vals[5];
> +
>  static const char *MonthNames[] = {
>   "Jan", "Feb", "Mar", "Apr", "May", "Jun",
>   "Jul", "Aug", "Sep", "Oct", "Nov", "Dec",
> @@ -70,6 +76,8 @@ static int  get_list(bitstr_t *, int, int
>   get_number(int *, int, const char *[], int, FILE *, const char 
> *),
>   set_element(bitstr_t *, int, int, int);
>  
> +static int   set_random(bitstr_t *, int, int, FILE *);
> +
>  void
>  free_entry(entry *e)
>  {
> @@ -187,10 +195,15 @@ load_entry(FILE *file, void (*error_func
>   goto eof;
>   }
>   } else {
> - if (ch == '*')
> - e->flags |= MIN_STAR;
> - ch = get_list(e->minute, FIRST_MINUTE, LAST_MINUTE,
> -   NULL, ch, file);
> + if (ch == 'R') {
> + ch = set_random(e->minute, rand_vals[r_minute], ch,
> + file);
> + } else {
> + if (ch == '*')
> + e->flags |= MIN_STAR;
> + ch = get_list(e->minute, FIRST_MINUTE, LAST_MINUTE,
> +   NULL, ch, file);
> + }
>   if (ch == EOF) {
>   ecode = e_minute;
>   goto eof;
> @@ -199,10 +212,14 @@ load_entry(FILE *file, void (*error_func
>   /* hours
>*/
>  
> - if (ch == '*')
> - e->flags |= HR_STAR;
> - ch = get_list(e->hour, FIRST_HOUR, LAST_HOUR,
> -   NULL, ch, file);
> + if (ch == 'R') {
> + ch = set_random(e->hour, rand_vals[r_hour], ch, file);
> + } else {
> + if (ch == '*')
> + e->flags |= HR_STAR;
> + ch = get_list(e->hour, FIRST_HOUR, LAST_HOUR,
> +   NULL, ch, file);
> + }
>   if (ch == EOF) {
>   ecode = e_hour;
>   goto eof;
> @@ -211,10 +228,15 @@ load_entry(FILE *file, void (*error_func
>   /* DOM (days of month)
>*/
>  
> - 

Re: suggest to run rpki-client hourly

[Bob Beck]

On Mon, Apr 13, 2020 at 09:56:52AM -0600, Todd C. Miller wrote:
> On Mon, 13 Apr 2020 09:37:14 -0600, "Theo de Raadt" wrote:
> 
> > While I understand what RANDOM is trying to do, I am not a fan.  I've
> > thought often of an improvement, where the minute marker in a crontab
> > file could be a letter 'R', which indicates the specific random value
> > for this cron start.  That value would be selected at cron start, and
> > remain constant for the runtime of cron.
> 
> I was thinking the same thing, but using '?' as the character.  It
> doesn't really matter what we choose, the implementation should be
> straight-forward.

+1000. a new random time chosen at cron start. 

We see this all the time, and it would be a lot better than the hacks for all 
the things

"R" for random sounds good to me

> 
>  - todd
> 

Re: fts and unveil issue

[Bob Beck]

yes you are seeing the limitation of 6.4 unveil as mentioned at the bottom
of the man page.   this should be fixed in current

On Sun, Feb 3, 2019 at 03:29 Kristaps Dzonsons  wrote:

> When I unveil(2), fts doesn't behave well.  But only in a subtle way.
> Enclosed is a demonstration.  I found this with openrsync, which unveils
> before using fts_open to scan for files.
>
> When run with a directory with only empty subdirectories or just files,
> this works fine.  But when run with a directory that contains other
> non-empty directories, the fts_read fails in the nested directories.
>
> This is on stock OpenBSD 6.4, syspatched, amd64.
>
> For example, consider the following abridged output (to fit into this
> e-mail window):
>
> % find ~/tmp/test -ls
> drwxr-xr-x3  /home/kristaps/tmp/test
> drwxr-xr-x3  /home/kristaps/tmp/test/test2
> -rw-r--r--1  /home/kristaps/tmp/test/test2/test2
> drwxr-xr-x2  /home/kristaps/tmp/test/test2/test3
> -rw-r--r--1  /home/kristaps/tmp/test/test1
> % gcc -W -Wall -Wextra -g foo.c
> % ./a.out /home/kristaps/tmp/test/
> a.out: /home/kristaps/tmp/test/
> a.out: /home/kristaps/tmp/test/test2
> a.out: /home/kristaps/tmp/test/test2/test2
> a.out: /home/kristaps/tmp/test/test2/test3
> a.out: /home/kristaps/tmp/test/test2/test3
> a.out: /home/kristaps/tmp/test/test2
> a.out: /home/kristaps/tmp/test/test1
> a.out: /home/kristaps/tmp/test/
> a.out: TRYING AGAIN.
> a.out: /home/kristaps/tmp/test/
> a.out: /home/kristaps/tmp/test/test2
> a.out: /home/kristaps/tmp/test/test2/test2: no stat
> a.out: ...but regular stat works
>
> So the first nested child fails (regardless of whether it's a file or
> directory, by the way).  But a regular stat still works.
>
> The same happens if I use unveil("/", "r").
>

Re: unveil spamlogd

[Bob Beck]

ok beck@ as well

On Wed, Oct 24, 2018 at 06:13 Todd C. Miller  wrote:

> On Wed, 24 Oct 2018 08:05:11 +0100, Ricardo Mestre wrote:
>
> > The only file that spamlogd needs to access after calling pledge is
> > PATH_SPAMD_DB, so unveil it with O_RDWR permissions.
>
> Looks good.  OK millert@
>
>  - todd
>

Re: Reuse VM ids.

[Bob Beck]

works here and I like it.  but probably for after unlock

On Sun, Oct 7, 2018 at 22:11 Mischa Peters  wrote:

> No idea if the code works yet.
> Hopefully I can try later. But love the idea.
>
> Mischa
>
> > On 8 Oct 2018, at 04:31, Ori Bernstein  wrote:
> >
> > Keep a list of known vms, and reuse the VM IDs. This means that when
> using
> > '-L', the IP addresses of the VMs are stable.
> >
> > diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
> > index af12b790002..522bae32501 100644
> > --- usr.sbin/vmd/config.c
> > +++ usr.sbin/vmd/config.c
> > @@ -61,7 +61,10 @@ config_init(struct vmd *env)
> >if (what & CONFIG_VMS) {
> >if ((env->vmd_vms = calloc(1, sizeof(*env->vmd_vms))) == NULL)
> >return (-1);
> > +if ((env->vmd_known = calloc(1, sizeof(*env->vmd_known))) ==
> NULL)
> > +return (-1);
> >TAILQ_INIT(env->vmd_vms);
> > +TAILQ_INIT(env->vmd_known);
> >}
> >if (what & CONFIG_SWITCHES) {
> >if ((env->vmd_switches = calloc(1,
> > diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
> > index 18a5e0d3d5d..732691b4381 100644
> > --- usr.sbin/vmd/vmd.c
> > +++ usr.sbin/vmd/vmd.c
> > @@ -1169,6 +1169,27 @@ vm_remove(struct vmd_vm *vm, const char *caller)
> >free(vm);
> > }
> >
> > +static uint32_t
> > +claim_vmid(const char *name)
> > +{
> > +struct name2id *n2i = NULL;
> > +
> > +TAILQ_FOREACH(n2i, env->vmd_known, entry)
> > +if (strcmp(n2i->name, name) == 0)
> > +return n2i->id;
> > +
> > +if (++env->vmd_nvm == 0)
> > +fatalx("too many vms");
> > +if ((n2i = calloc(1, sizeof(struct name2id))) == NULL)
> > +fatalx("could not alloc vm name");
> > +n2i->id = env->vmd_nvm;
> > +if (strlcpy(n2i->name, name, sizeof(n2i->name)) >=
> sizeof(n2i->name))
> > +fatalx("overlong vm name");
> > +TAILQ_INSERT_TAIL(env->vmd_known, n2i, entry);
> > +
> > +return n2i->id;
> > +}
> > +
> > int
> > vm_register(struct privsep *ps, struct vmop_create_params *vmc,
> > struct vmd_vm **ret_vm, uint32_t id, uid_t uid)
> > @@ -1300,11 +1321,8 @@ vm_register(struct privsep *ps, struct
> vmop_create_params *vmc,
> >vm->vm_cdrom = -1;
> >vm->vm_iev.ibuf.fd = -1;
> >
> > -if (++env->vmd_nvm == 0)
> > -fatalx("too many vms");
> > -
> >/* Assign a new internal Id if not specified */
> > -vm->vm_vmid = id == 0 ? env->vmd_nvm : id;
> > +vm->vm_vmid = (id == 0) ? claim_vmid(vcp->vcp_name) : id;
> >
> >log_debug("%s: registering vm %d", __func__, vm->vm_vmid);
> >TAILQ_INSERT_TAIL(env->vmd_vms, vm, vm_entry);
> > diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> > index b7c012854e8..86fad536e59 100644
> > --- usr.sbin/vmd/vmd.h
> > +++ usr.sbin/vmd/vmd.h
> > @@ -276,6 +276,13 @@ struct vmd_user {
> > };
> > TAILQ_HEAD(userlist, vmd_user);
> >
> > +struct name2id {
> > +charname[VMM_MAX_NAME_LEN];
> > +int32_tid;
> > +TAILQ_ENTRY(name2id)entry;
> > +};
> > +TAILQ_HEAD(name2idlist, name2id);
> > +
> > struct address {
> >struct sockaddr_storage ss;
> >int prefixlen;
> > @@ -300,6 +307,7 @@ struct vmd {
> >
> >uint32_t vmd_nvm;
> >struct vmlist*vmd_vms;
> > +struct name2idlist*vmd_known;
> >uint32_t vmd_nswitches;
> >struct switchlist*vmd_switches;
> >struct userlist*vmd_users;
> >
> > --
> >Ori Bernstein
> >
>
>

Nuke PLEDGE_STAT for further pledge/unveil disentaglement.

[Bob Beck]

So this gets rid of unveil's PLEDGE_STAT.

Instead we use UNVEIL_INSPECT which is set by the stat and access opeerations
that are needed for realpath() type traversals that effectively call stat/access
for each component of a pathname before doing a final operation on the end. 

The intended semantic of UNVEIL_INSPECT (which is only used in the kernel) 
is to allow inspection of vnodes that are traversed on the way to an 
unveil'ed component - just like what PLEDGE_STAT did. 

This also removes the use of PLEDGE_STATLIE in unveil - theo and I had
discussed that this was probably fine in lubljana, but I never did it
then. I'll remove STATLIE later if we decide that's the way we
are going. 

Passes regress - realpath still works, etc. etc.

ok?

Index: kern/kern_pledge.c
===
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.239
diff -u -p -u -p -r1.239 kern_pledge.c
--- kern/kern_pledge.c  2 Aug 2018 15:34:07 -   1.239
+++ kern/kern_pledge.c  5 Aug 2018 17:45:52 -
@@ -608,14 +608,14 @@ pledge_namei(struct proc *p, struct name
switch (p->p_pledge_syscall) {
case SYS_access:
/* tzset() needs this. */
-   if ((ni->ni_pledge == (PLEDGE_RPATH | PLEDGE_STAT)) &&
+   if (ni->ni_pledge == PLEDGE_RPATH &&
strcmp(path, "/etc/localtime") == 0) {
ni->ni_cnd.cn_flags |= BYPASSUNVEIL;
return (0);
}
 
/* when avoiding YP mode, getpw* functions touch this */
-   if (ni->ni_pledge == (PLEDGE_RPATH | PLEDGE_STAT) &&
+   if (ni->ni_pledge == PLEDGE_RPATH &&
strcmp(path, "/var/run/ypbind.lock") == 0) {
if (p->p_p->ps_pledge & PLEDGE_GETPW) {
ni->ni_cnd.cn_flags |= BYPASSUNVEIL;
@@ -713,7 +713,7 @@ pledge_namei(struct proc *p, struct name
break;
case SYS_readlink:
/* Allow /etc/malloc.conf for malloc(3). */
-   if ((ni->ni_pledge == (PLEDGE_RPATH | PLEDGE_STAT)) &&
+   if ((ni->ni_pledge == PLEDGE_RPATH) &&
strcmp(path, "/etc/malloc.conf") == 0) {
ni->ni_cnd.cn_flags |= BYPASSUNVEIL;
return (0);
@@ -721,7 +721,7 @@ pledge_namei(struct proc *p, struct name
break;
case SYS_stat:
/* DNS needs /etc/resolv.conf. */
-   if ((ni->ni_pledge == (PLEDGE_RPATH | PLEDGE_STAT)) &&
+   if ((ni->ni_pledge == PLEDGE_RPATH) &&
(p->p_p->ps_pledge & PLEDGE_DNS) &&
strcmp(path, "/etc/resolv.conf") == 0) {
ni->ni_cnd.cn_flags |= BYPASSUNVEIL;
@@ -732,9 +732,9 @@ pledge_namei(struct proc *p, struct name
 
/*
 * Ensure each flag of ni_pledge has counterpart allowing it in
-* ps_pledge. discard PLEDGE_STAT as it is unveil(2) stuff.
+* ps_pledge.
 */
-   if ((ni->ni_pledge & ~PLEDGE_STAT) & ~p->p_p->ps_pledge)
+   if (ni->ni_pledge & ~p->p_p->ps_pledge)
return (pledge_fail(p, EPERM, (ni->ni_pledge & 
~p->p_p->ps_pledge)));
 
/* continue, and check unveil if present */
Index: kern/kern_unveil.c
===
RCS file: /cvs/src/sys/kern/kern_unveil.c,v
retrieving revision 1.11
diff -u -p -u -p -r1.11 kern_unveil.c
--- kern/kern_unveil.c  5 Aug 2018 14:23:57 -   1.11
+++ kern/kern_unveil.c  5 Aug 2018 17:45:52 -
@@ -379,13 +379,20 @@ unveil_add_vnode(struct process *pr, str
return (uv);
 }
 
-void
+int
 unveil_add_traversed_vnodes(struct proc *p, struct nameidata *ndp)
 {
/*
-* add the traversed vnodes with 0 flags if they
-* are not already present.
+* Add the traversed vnodes with the UNVEIL_INSPECT flag
+* if they are not already present to allow traversal
+* operations such as access and stat. This lets
+* TOCTOU fans that call access on all components of
+* an unveil'ed path before the final operation
+* work.
 */
+   int ret = 0;
+   struct unveil *uv;
+
if (ndp->ni_tvpsize) {
size_t i;
 
@@ -394,10 +401,15 @@ unveil_add_traversed_vnodes(struct proc 
if (unveil_lookup(vp, p) == NULL) {
vref(vp);
vp->v_uvcount++;
-   unveil_add_vnode(p->p_p, vp);
+   uv = unveil_add_vnode(p->p_p, vp);
+   if (uv != NULL)
+   uv->uv_flags = UNVEIL_INSPECT;
+   else
+   ret = E2BIG;
}
}
}
+   return 

Re: unveil: incomplete unveil_flagmatch semantic

[Bob Beck]

> Some examples that will need consideration for unveil(2):
> - mount(2)
> - unmount(2)
> - quotactl(2)
> - chroot(2)
> - getfh(2)
> - acct(2)
> - coredump()
> - loadfirmware() - I think ifconfig(1) could make the kernel loading a
>   firmware for some network card
> 
> so having ni_unveil separated from ni_pledge could be good to be able to
> manage these namei() calls in unveiled programs.
> 

And yes, I am in violent agreement with this :)

this lets us have a cleaner separation and unveil things that aren't 
pledgeable. 

Re: unveil: incomplete unveil_flagmatch semantic

[Bob Beck]

> On Sat, Aug 04, 2018 at 10:40:11AM -0600, Bob Beck wrote:
> > On Fri, Aug 03, 2018 at 06:31:00AM +0200, Sebastien Marie wrote:
> > > On Thu, Aug 02, 2018 at 03:42:03PM +0200, Sebastien Marie wrote:
> > > > On Mon, Jul 30, 2018 at 07:55:35AM -0600, Bob Beck wrote:
> > > > > yeah the latter will be the way to go
> > > > > 
> > > > 
> > 
> > > > new diff with direct lookup using an indirection table.
> > > > 
> > > 
> > > new (emergency) version with PLEDGE_CHOWN consideration for unveil(2).
> > > 
> > > sorry for having missed it.
> > >  
> > 
> > All good because you gave me inspiration, after I ran your diff. 
> > 
> > I tied unveil to the pledge flags when I first did it because it was
> > convenient - I think this thig with chmod (and the awkwardness of
> > PLEDGE_STAT, etc. etc.) just shows that this was a decision of
> > convienience in the short term that is going to bite us in the long
> > term. 
> > 
> > The lookup table is clever, but is frankly, voodoo :) I don't like
> > trying to follow the logic of what maps to what and be concerned
> > about what flags are where just for the sake of this, and it
> > makes things ugly to read.
> > 
> > I think I would rather add my own char to the namei structure, 
> > and set it appropriately in the same places that pledge does. IMO
> > this makes looking at the source code for system calls much clearer
> > int the kernel - rather than trying to fathom in your head how a
> > combination of pledge flags will turn into unveil. 
> 
> it seems to me it is a bit duplicating annotations: annotating syscalls
> for pledge, and annotating syscalls for unveil, while pledge has just
> more granuality. but I can understand the problem with an additional
> level for translating pledge-flags to unveil-flags.
> 
> in consequence, you will have to change some bits in man page, as "r"
> will not be anymore "corresponding to the pledge(2) promise rpath" :)
> 
> > So this is a somewhat "minimal" diff tha puts the flags in 
> > namei.h, and checks them as per your change, but rather
> > than using a lookup table just expressly sets them
> > for each system call appropriately.. it passes regress
> > as is. 
> > 
> > I think after doing this I can probably go in an get rid of
> > the awkward PLEDGE_STAT and simplify BYPASS considerably
> > as well, but I will do that separately. 
> > 
> > ok?
> 
> some comments inlined.
> 
> and one important for starting: you should consider (ni_unveil==0) as
> deny by default, instead of allow by default. eventually having an
> panic(9) or an assert(9) (but it should be too early for now in case we
> want it).
> 
> it is important because else if we miss an explicit initialisation (the
> struct is zeroed, ni_unveil=0 by default), it will be an allowed
> operation by default and we will *not* see the problem. with deny or a
> panic(9) we will not miss it: someone will complain.
> 
> for a panic(9), it could be done in namei(), eventually just after
> pledge_namei() call, to avoid checking the variable too often.
> 
> 
> another important thing I just realized: not all filesystem operations
> were pledeable, and some haven't ni_pledge because the function was
> unreachable while pledged (but pledge_namei() has a panic for such
> cases).
> 
> Some examples that will need consideration for unveil(2):
> - mount(2)
> - unmount(2)
> - quotactl(2)
> - chroot(2)
> - getfh(2)
> - acct(2)
> - coredump()
> - loadfirmware() - I think ifconfig(1) could make the kernel loading a
>   firmware for some network card
> 
> so having ni_unveil separated from ni_pledge could be good to be able to
> manage these namei() calls in unveiled programs.
> 
> 
> > Index: kern/kern_unveil.c
> > ===
> > RCS file: /cvs/src/sys/kern/kern_unveil.c,v
> > retrieving revision 1.9
> > diff -u -p -u -p -r1.9 kern_unveil.c
> > --- kern/kern_unveil.c  30 Jul 2018 15:16:27 -  1.9
> > +++ kern/kern_unveil.c  4 Aug 2018 16:13:07 -
> > @@ -40,6 +40,11 @@
> >  #define UNVEIL_MAX_VNODES  128
> >  #define UNVEIL_MAX_NAMES   128
> >  
> > +#defineUNVEIL_READ 0x01
> > +#defineUNVEIL_WRITE0x02
> > +#defineUNVEIL_CREATE   0x04
> > +#defineUNVEIL_EXEC 0x08
> > +
> 
> the flags are duplicated with namei.h. I think namei.h is the right
> place, and there could be removed from here.
> 
> 

Re: unveil: incomplete unveil_flagmatch semantic

[Bob Beck]

> > +   nd.ni_unveil = 0; /* XXX No flags == allow it */
> 
> see my comment about ni_unveil != 0.
> 
> as you still have check on (ni_pledge & PLEDGE_STAT), it should be still
> ok.
> 

It doesn't actually do this yt.. this comment was a reminder for me
and should have had allow it? for my dealig with PLEDGE_STAT afterwards

I'm intend on making another flag for that the "you have to
be able to access it" a-la PLEDGE_STAT which was a hack - and
clean that up in a separate diff. so no, 0 flags won't be "allow it"

Re: unveil: incomplete unveil_flagmatch semantic

[Bob Beck]

On Fri, Aug 03, 2018 at 06:31:00AM +0200, Sebastien Marie wrote:
> On Thu, Aug 02, 2018 at 03:42:03PM +0200, Sebastien Marie wrote:
> > On Mon, Jul 30, 2018 at 07:55:35AM -0600, Bob Beck wrote:
> > > yeah the latter will be the way to go
> > > 
> > 

> > new diff with direct lookup using an indirection table.
> > 
> 
> new (emergency) version with PLEDGE_CHOWN consideration for unveil(2).
> 
> sorry for having missed it.
>  

All good because you gave me inspiration, after I ran your diff. 

I tied unveil to the pledge flags when I first did it because it was
convenient - I think this thig with chmod (and the awkwardness of
PLEDGE_STAT, etc. etc.) just shows that this was a decision of
convienience in the short term that is going to bite us in the long
term. 

The lookup table is clever, but is frankly, voodoo :) I don't like
trying to follow the logic of what maps to what and be concerned
about what flags are where just for the sake of this, and it
makes things ugly to read.

I think I would rather add my own char to the namei structure, 
and set it appropriately in the same places that pledge does. IMO
this makes looking at the source code for system calls much clearer
int the kernel - rather than trying to fathom in your head how a
combination of pledge flags will turn into unveil. 

So this is a somewhat "minimal" diff tha puts the flags in 
namei.h, and checks them as per your change, but rather
than using a lookup table just expressly sets them
for each system call appropriately.. it passes regress
as is. 

I think after doing this I can probably go in an get rid of
the awkward PLEDGE_STAT and simplify BYPASS considerably
as well, but I will do that separately. 

ok?


Index: dev/diskmap.c
===
RCS file: /cvs/src/sys/dev/diskmap.c,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 diskmap.c
--- dev/diskmap.c   4 Jul 2018 12:42:30 -   1.22
+++ dev/diskmap.c   3 Aug 2018 02:38:26 -
@@ -85,6 +85,7 @@ diskmapioctl(dev_t dev, u_long cmd, cadd
 
NDINIT(, 0, 0, UIO_SYSSPACE, devname, p);
ndp.ni_pledge = PLEDGE_RPATH;
+   ndp.ni_unveil = UNVEIL_READ;
if ((error = vn_open(, fp0->f_flag, 0)) != 0)
goto invalid;
 
Index: kern/exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.145
diff -u -p -u -p -r1.145 exec_elf.c
--- kern/exec_elf.c 20 Jul 2018 21:57:26 -  1.145
+++ kern/exec_elf.c 3 Aug 2018 02:38:26 -
@@ -332,6 +332,7 @@ elf_load_file(struct proc *p, char *path
 
NDINIT(, LOOKUP, FOLLOW | LOCKLEAF, UIO_SYSSPACE, path, p);
nd.ni_pledge = PLEDGE_RPATH;
+   nd.ni_unveil = UNVEIL_READ;
if ((error = namei()) != 0) {
return (error);
}
Index: kern/kern_exec.c
===
RCS file: /cvs/src/sys/kern/kern_exec.c,v
retrieving revision 1.200
diff -u -p -u -p -r1.200 kern_exec.c
--- kern/kern_exec.c20 Jul 2018 21:57:26 -  1.200
+++ kern/kern_exec.c3 Aug 2018 02:38:26 -
@@ -275,6 +275,7 @@ sys_execve(struct proc *p, void *v, regi
 
NDINIT(, LOOKUP, NOFOLLOW, UIO_USERSPACE, SCARG(uap, path), p);
nid.ni_pledge = PLEDGE_EXEC;
+   nid.ni_unveil = UNVEIL_EXEC;
 
/*
 * initialize the fields of the exec package.
Index: kern/kern_ktrace.c
===
RCS file: /cvs/src/sys/kern/kern_ktrace.c,v
retrieving revision 1.98
diff -u -p -u -p -r1.98 kern_ktrace.c
--- kern/kern_ktrace.c  20 Jun 2018 10:48:55 -  1.98
+++ kern/kern_ktrace.c  3 Aug 2018 02:38:26 -
@@ -513,6 +513,7 @@ sys_ktrace(struct proc *p, void *v, regi
cred = p->p_ucred;
NDINIT(, LOOKUP, FOLLOW, UIO_USERSPACE, fname, p);
nd.ni_pledge = PLEDGE_CPATH | PLEDGE_WPATH;
+   nd.ni_unveil = UNVEIL_CREATE | UNVEIL_WRITE;
if ((error = vn_open(, FWRITE|O_NOFOLLOW, 0)) != 0)
return error;
vp = nd.ni_vp;
Index: kern/kern_unveil.c
===
RCS file: /cvs/src/sys/kern/kern_unveil.c,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 kern_unveil.c
--- kern/kern_unveil.c  30 Jul 2018 15:16:27 -  1.9
+++ kern/kern_unveil.c  4 Aug 2018 16:13:07 -
@@ -40,6 +40,11 @@
 #define UNVEIL_MAX_VNODES  128
 #define UNVEIL_MAX_NAMES   128
 
+#defineUNVEIL_READ 0x01
+#defineUNVEIL_WRITE0x02
+#defineUNVEIL_CREATE   0x04
+#defineUNVEIL_EXEC 0x08
+
 static inline int
 unvname_compare(const struct unvname *n1, const struct unvname *n2)
 {
@@ -50,7 +55,7 @@ unvname_compare(const struct unvname *n1
 }
 
 struct unvname *
-unvname_new(

Re: unveil: incomplete unveil_flagmatch semantic

[Bob Beck]

yeah the latter will be the way to go

On Mon, Jul 30, 2018 at 06:02 Sebastien Marie  wrote:

> Hi,
>
> I think unveil_flagmatch() isn't complete and/or has not the right
> semantic.
>
> A bit of internals for starting (I will speak about ni_pledge, people
> that know what it is and how it works with pledge/unveil could go to
> "what is the problem" part).
>
> unveil(2) works with the syscall annotation introduced for pledge(2).
>
> When kernel needs to resolv a name to vnode, it used namei() function.
> For that it initializes a special structure used as namei() argument:
> `struct nameidata`. This struct has a field `ni_pledge` used to let vfs
> subsystem know what kind of syscalls called it. This way, underline
> subsystem doesn't have to know all syscalls, and could work on them by
> "group".
>
> For example, when you call open(2), depending the flags argument used,
> ni_pledge will contain PLEDGE_RPATH, PLEDGE_WPATH, or PLEDGE_CPATH. The
> subsystem has a quick and accurate view of the intented usage of the
> vnode.
>
> pledge(2) uses it to check that you have the promise of intent to use,
> and unveil(2) uses it too in a similar way, in particular in
> unveil_flagmatch() to check if the process has the accurate permission
> for this particular vnode.
>
>
>
> Now, what is the problem with unveil_flagmatch() ?
>
> If I exclude the PLEDGE_STAT stuff from the equation (I am not still
> convinced it is the right way to do it - but it isn't the question for
> now), unveil_flagmatch() has a default policy to allow the operation,
> and only check for some flags in ni_pledge to deny it.
>
> For simple syscall like open(2) there is no problem. The possible
> value of ni_pledge are composed with only PLEDGE_RPATH, PLEDGE_WPATH, or
> PLEDGE_CPATH.
>
> But for some others syscalls, it isn't the case.
>
> For example, chmod(2) will set ni_pledge to "PLEDGE_FATTR |
> PLEDGE_RPATH". For pledge(2), it means you must have "fattr" (capability
> to change attribute on the node) and "rpath" (capability to read the
> node) promise to use such syscall.
>
> As unveil(2) doesn't have the "fattr" concept, and unveil_flagmatch()
> will check only for PLEDGE_RPATH, PLEDGE_WPATH, PLEDGE_CPATH and
> PLEDGE_EXEC, we end with unsound policy: you could use chmod(2) with just
> "r" on unveiled part of the filesystem.
>
> Some others flags could occurs in ni_pledge:
> - PLEDGE_CHOWN: chown(2) family
> - PLEDGE_DPATH: mknod(2), mkfifo(2)
> - PLEDGE_FATTR: utimes(2) family, chmod(2) family, truncate(2), chflags(2)
> - PLEDGE_TTY:   revoke(2)
> - PLEDGE_UNIX:  bind(2), connect(2)
>
> unveil_flagmatch() has a special case for "" flag: it means deny.
> but as soon as you have "r", "w", "x" or "c", you have an allow policy
> by default. Check will be done only for a part of ni_pledge.
>
>
> I see basically two possibilities:
> - extending unveil(2) permissions to match pledge(2) flags - but I don't
>   like it, unveil(2) should be kept simple.
>
> - having a separate namespace for unveil and pledge flags (to avoid
>   confusion), and translating all pledge flags to unveil flags.
>
>   PLEDGE_RPATH => UNVEIL_READ
>   PLEDGE_WPATH => UNVEIL_WRITE
>   PLEDGE_CPATH => UNVEIL_CREATE
>   PLEDGE_EXEC  => UNVEIL_EXEC
>   PLEDGE_CHOWN => UNVEIL_WRITE
>   PLEDGE_DPATH => UNVEIL_CREATE
>   PLEDGE_FATTR => UNVEIL_WRITE
>   PLEDGE_TTY   => UNVEIL_WRITE
>   PLEDGE_UNIX  => UNVEIL_READ|UNVEIL_WRITE (requiring both)
>   any others   => panic(9)
>
> This way, we could be really exhaustive in unveil_flagmatch() without
> having to bother for future PLEDGE flag addition (as we will panic(9) if
> some developer doesn't add it where intented).
>
> Thanks.
> --
> Sebastien Marie
>

Re: unveil: incorrect type flags on unvname_new()

[Bob Beck]

ok beck@

On Mon, Jul 16, 2018 at 15:53 Sebastien Marie  wrote:

> Hi,
>
> While reviewing unveil(2) code, I found an incorrect type on
> unvname_new() function: flags argument should be uint64_t.
>
> It is called by unveil_add_name() which uses uint64_t for flags, and
> store the value in struct unvname un_flags member which is uint64_t.
>
> Thanks.
> --
> Sebastien Marie
>
>
> Index: kern/kern_unveil.c
> ===
> RCS file: /cvs/src/sys/kern/kern_unveil.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 kern_unveil.c
> --- kern/kern_unveil.c  13 Jul 2018 13:47:41 -  1.2
> +++ kern/kern_unveil.c  16 Jul 2018 13:08:40 -
> @@ -50,7 +50,7 @@ unvname_compare(const struct unvname *n1
>  }
>
>  struct unvname *
> -unvname_new(const char *name, size_t size, int flags)
> +unvname_new(const char *name, size_t size, uint64_t flags)
>  {
> struct unvname *ret = malloc(sizeof(struct unvname), M_PROC,
> M_WAITOK);
> ret->un_name = malloc(size, M_PROC, M_WAITOK);
>

Re: const qualifiers for EVP_*

[Bob Beck]

ok

On Sat, May 12, 2018 at 13:14 Theo Buehler  wrote:

> Here's another straightforward batch. As usual, it's been tested in a
> bulk by sthen and there was no fallout.
>
> Index: lib/libcrypto/asn1/ameth_lib.c
> ===
> RCS file: /var/cvs/src/lib/libcrypto/asn1/ameth_lib.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 ameth_lib.c
> --- lib/libcrypto/asn1/ameth_lib.c  21 Jan 2017 04:31:25 -
> 1.16
> +++ lib/libcrypto/asn1/ameth_lib.c  12 May 2018 18:42:51 -
> @@ -299,7 +299,7 @@ EVP_PKEY_asn1_get0_info(int *ppkey_id, i
>  }
>
>  const EVP_PKEY_ASN1_METHOD*
> -EVP_PKEY_get0_asn1(EVP_PKEY *pkey)
> +EVP_PKEY_get0_asn1(const EVP_PKEY *pkey)
>  {
> return pkey->ameth;
>  }
> Index: lib/libcrypto/evp/evp.h
> ===
> RCS file: /var/cvs/src/lib/libcrypto/evp/evp.h,v
> retrieving revision 1.59
> diff -u -p -r1.59 evp.h
> --- lib/libcrypto/evp/evp.h 2 May 2018 15:51:41 -   1.59
> +++ lib/libcrypto/evp/evp.h 12 May 2018 18:42:51 -
> @@ -617,7 +617,8 @@ int EVP_DigestSignFinal(EVP_MD_CTX *ctx,
>
>  int EVP_DigestVerifyInit(EVP_MD_CTX *ctx, EVP_PKEY_CTX **pctx,
>  const EVP_MD *type, ENGINE *e, EVP_PKEY *pkey);
> -int EVP_DigestVerifyFinal(EVP_MD_CTX *ctx, unsigned char *sig, size_t
> siglen);
> +int EVP_DigestVerifyFinal(EVP_MD_CTX *ctx, const unsigned char *sig,
> +size_t siglen);
>
>  int EVP_OpenInit(EVP_CIPHER_CTX *ctx, const EVP_CIPHER *type,
>  const unsigned char *ek, int ekl, const unsigned char *iv, EVP_PKEY
> *priv);
> @@ -866,12 +867,12 @@ int EVP_PKEY_encrypt_old(unsigned char *
>  int EVP_PKEY_type(int type);
>  int EVP_PKEY_id(const EVP_PKEY *pkey);
>  int EVP_PKEY_base_id(const EVP_PKEY *pkey);
> -int EVP_PKEY_bits(EVP_PKEY *pkey);
> +int EVP_PKEY_bits(const EVP_PKEY *pkey);
>  int EVP_PKEY_size(EVP_PKEY *pkey);
>  int EVP_PKEY_set_type(EVP_PKEY *pkey, int type);
>  int EVP_PKEY_set_type_str(EVP_PKEY *pkey, const char *str, int len);
>  int EVP_PKEY_assign(EVP_PKEY *pkey, int type, void *key);
> -void *EVP_PKEY_get0(EVP_PKEY *pkey);
> +void *EVP_PKEY_get0(const EVP_PKEY *pkey);
>
>  #ifndef OPENSSL_NO_RSA
>  struct rsa_st;
> @@ -995,7 +996,7 @@ int EVP_PKEY_asn1_get0_info(int *ppkey_i
>  const char **pinfo, const char **ppem_str,
>  const EVP_PKEY_ASN1_METHOD *ameth);
>
> -const EVP_PKEY_ASN1_METHOD* EVP_PKEY_get0_asn1(EVP_PKEY *pkey);
> +const EVP_PKEY_ASN1_METHOD* EVP_PKEY_get0_asn1(const EVP_PKEY *pkey);
>  EVP_PKEY_ASN1_METHOD* EVP_PKEY_asn1_new(int id, int flags, const char
> *pem_str,
>  const char *info);
>  void EVP_PKEY_asn1_copy(EVP_PKEY_ASN1_METHOD *dst,
> Index: lib/libcrypto/evp/evp_pkey.c
> ===
> RCS file: /var/cvs/src/lib/libcrypto/evp/evp_pkey.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 evp_pkey.c
> --- lib/libcrypto/evp/evp_pkey.c29 Jan 2017 17:49:23 -
> 1.19
> +++ lib/libcrypto/evp/evp_pkey.c12 May 2018 18:42:51 -
> @@ -181,7 +181,8 @@ EVP_PKEY_get_attr_by_NID(const EVP_PKEY
>  }
>
>  int
> -EVP_PKEY_get_attr_by_OBJ(const EVP_PKEY *key, ASN1_OBJECT *obj, int
> lastpos)
> +EVP_PKEY_get_attr_by_OBJ(const EVP_PKEY *key, const ASN1_OBJECT *obj,
> +int lastpos)
>  {
> return X509at_get_attr_by_OBJ(key->attributes, obj, lastpos);
>  }
> Index: lib/libcrypto/evp/m_sigver.c
> ===
> RCS file: /var/cvs/src/lib/libcrypto/evp/m_sigver.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 m_sigver.c
> --- lib/libcrypto/evp/m_sigver.c29 Jan 2017 17:49:23 -  1.6
> +++ lib/libcrypto/evp/m_sigver.c12 May 2018 18:42:51 -
> @@ -166,7 +166,7 @@ EVP_DigestSignFinal(EVP_MD_CTX *ctx, uns
>  }
>
>  int
> -EVP_DigestVerifyFinal(EVP_MD_CTX *ctx, unsigned char *sig, size_t siglen)
> +EVP_DigestVerifyFinal(EVP_MD_CTX *ctx, const unsigned char *sig, size_t
> siglen)
>  {
> EVP_MD_CTX tmp_ctx;
> unsigned char md[EVP_MAX_MD_SIZE];
> Index: lib/libcrypto/evp/p_lib.c
> ===
> RCS file: /var/cvs/src/lib/libcrypto/evp/p_lib.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 p_lib.c
> --- lib/libcrypto/evp/p_lib.c   14 Apr 2018 07:09:21 -  1.21
> +++ lib/libcrypto/evp/p_lib.c   12 May 2018 18:42:51 -
> @@ -85,7 +85,7 @@
>  static void EVP_PKEY_free_it(EVP_PKEY *x);
>
>  int
> -EVP_PKEY_bits(EVP_PKEY *pkey)
> +EVP_PKEY_bits(const EVP_PKEY *pkey)
>  {
> if (pkey && pkey->ameth && pkey->ameth->pkey_bits)
> return pkey->ameth->pkey_bits(pkey);
> @@ -277,7 +277,7 @@ EVP_PKEY_assign(EVP_PKEY *pkey, int type
>  }
>
>  void *
> -EVP_PKEY_get0(EVP_PKEY *pkey)
> +EVP_PKEY_get0(const EVP_PKEY *pkey)
>  {
> return pkey->pkey.ptr;
>  }
> Index: lib/libcrypto/x509/x509.h
> 

Re: Anyone can suggest a BitCoin processor to the OpenBSD Foundation? BitPay has become terrible

[Bob Beck]

So, related to this topic, Apparently BitPay has now fixed us up again.

I have put the button back on the web site, if anyone wants to try a
bitcoin donation is is supposed to be possible again

Anyone can suggest a BitCoin processor to the OpenBSD Foundation? BitPay has become terrible

[Bob Beck]

So, as some of you may know, the OpenBSD Foundation has accepted BitCoin
donations
for some time via BitPay.com

BitPay was convenient for us since they will sell the BTC donations
immediately, and
convert to Canadian Dollars.  We then periodically get bank transfers of
the balance,
and this works great.

Obviously, for money laundering rules, we do need to provide them with some
documentation to prove the foundation's legitimacy, which we have
repatedly, and
this used to be not a problem.

Lately they seem to have decided in spite of being in posession of
1) A copy of two of our personal passports
2) A copy of the Canadian Federal Incorporation documents for a not for
profit
3) A copy of the Electric Bill for the address of record
4) A copy of an invoice sent to the foundation at the address of record
5) A copy of a bank statement from the foundation

That they seem to be suspending our access to BitPay - this in spite of this
being the only documents they request from us and then they seem to say
they won't work - because apparently they don't understand Canada our
countries outside of the USA.

So in short, bitPay has always been a bit of a pain for us, but now seems
to be
unable to be used by us.

So, what I'm looking for is opinions on an online processor.  Yes I am
aware they
charge fees, I am looking for convenience, basically, the ability to put a
button
on the foundation donation page that links to them. which will take a
donation
in BTC, convert it for us, and get it to us at a Canadian bank.  What are
all you people using?

For a number of reasons, we do *not* want to hold a bitCoin wallet
ourselves, and
be in the business of selling bitcoins, so please don't suggest this, and
No, we can't
have a bank account outside of Canada, so please don't suggest that either
:)

Re: libressl: crash in DES_fcrypt

[Bob Beck]

why AA?  why not just choose two random ascii salt chars at that point?  or
since this is effectively a failure case encrypt a random ascii salt and
random string?

using AA will produce a usable result based on the original string.
 encrypting a random string with a random salt means the failure return
wont be accidentally used

On Wed, Dec 13, 2017 at 06:51 Theo de Raadt  wrote:

> I still don't understand.
>
> This feels like fail-open.
>
> > I would like to commit this fix.  I tried to avoid the crash in
> > libcrypto with least possible impact for the DES_fcrypt() API.
> >
> > ok?
> >
> > bluhm
> >
> > On Tue, Dec 05, 2017 at 05:20:49PM +0100, Alexander Bluhm wrote:
> > > On Fri, Oct 27, 2017 at 01:50:26AM +0200, Jan Engelhardt wrote:
> > > > #include 
> > > > int main(void) {
> > > > char salt[3] = {0xf8, 0xd0, 0x00};
> > > > char out[32];
> > > > DES_fcrypt("foo", salt, out);
> > > > }
> > >
> > > This program produces a Segmentation fault in OpenBSD current.
> > >
> > > > openssl 1.1.x has it fixed (but 1.0.2l does not!) - their commit
> > > > seems to be 6493e4801e9edbe1ad1e256d4ce9cd55c8aa2242 in
> > > > https://github.com/openssl/openssl .
> > >
> > > Their fix changes the semantics of DES_crypt() and DES_fcrypt().
> > > They may fail and return NULL now.  This was never the case before
> > > so we may expect that programs do not check it.  With DES_fcrypt()
> > > it is very likely that some uninitilaized content of the return
> > > string is used.
> > >
> > > So I have extended the workaround that was there already.  Read the
> > > comment above the fix.  If the salt does not consist of two ascii
> > > characters, replace it with "AA".  This gives a safe result in all
> > > cases.
> > >
> > > ok?
> > >
> > > bluhm
> > >
> > > Index: lib/libcrypto/des/fcrypt.c
> > > ===
> > > RCS file: /data/mirror/openbsd/cvs/src/lib/libcrypto/des/fcrypt.c,v
> > > retrieving revision 1.12
> > > diff -u -p -r1.12 fcrypt.c
> > > --- lib/libcrypto/des/fcrypt.c  26 Dec 2016 21:30:10 -
> 1.12
> > > +++ lib/libcrypto/des/fcrypt.c  5 Dec 2017 16:03:57 -
> > > @@ -78,9 +78,15 @@ char *DES_fcrypt(const char *buf, const
> > >  * crypt to "*".  This was found when replacing the crypt in
> > >  * our shared libraries.  People found that the disabled
> > >  * accounts effectively had no passwd :-(. */
> > > -   x=ret[0]=((salt[0] == '\0')?'A':salt[0]);
> > > +   x = salt[0];
> > > +   if (x == 0 || x >= sizeof(con_salt))
> > > +   x = 'A';
> > > +   ret[0] = x;
> > > Eswap0=con_salt[x]<<2;
> > > -   x=ret[1]=((salt[1] == '\0')?'A':salt[1]);
> > > +   x = (salt[0] == '\0') ? 'A' : salt[1];
> > > +   if (x == 0 || x >= sizeof(con_salt))
> > > +   x = 'A';
> > > +   ret[1] = x;
> > > Eswap1=con_salt[x]<<6;
> > >  /* EAY
> > >  r=strlen(buf);
> > > Index: regress/lib/libcrypto/des/destest.c
> > > ===
> > > RCS file:
> /data/mirror/openbsd/cvs/src/regress/lib/libcrypto/des/destest.c,v
> > > retrieving revision 1.3
> > > diff -u -p -r1.3 destest.c
> > > --- regress/lib/libcrypto/des/destest.c 30 Oct 2015 15:58:40
> -  1.3
> > > +++ regress/lib/libcrypto/des/destest.c 5 Dec 2017 16:02:18 -
> > > @@ -749,18 +749,38 @@ plain[8+4], plain[8+5], plain[8+6], plai
> > > }
> > > printf("\n");
> > > printf("fast crypt test ");
> > > -   str=crypt("testing","ef");
> > > +   str=DES_crypt("testing","ef");
> > > if (strcmp("efGnQx2725bI2",str) != 0)
> > > {
> > > printf("fast crypt error, %s should be
> efGnQx2725bI2\n",str);
> > > err=1;
> > > }
> > > -   str=crypt("bca76;23","yA");
> > > +   str=DES_crypt("bca76;23","yA");
> > > if (strcmp("yA1Rp/1hZXIJk",str) != 0)
> > > {
> > > printf("fast crypt error, %s should be
> yA1Rp/1hZXIJk\n",str);
> > > err=1;
> > > }
> > > +   str = DES_crypt("testing", "\202B");
> > > +   if (strncmp("AB",str,2) != 0) {
> > > +   printf("salt %s first non ascii not replaced with A\n",
> str);
> > > +   err = 1;
> > > +   }
> > > +   str = DES_crypt("testing", "B\202");
> > > +   if (strncmp("BA",str,2) != 0) {
> > > +   printf("salt %s second non ascii not replaced with A\n",
> str);
> > > +   err = 1;
> > > +   }
> > > +   str = DES_crypt("testing", "\0B");
> > > +   if (strncmp("AA",str,2) != 0) {
> > > +   printf("salt %s first NUL not replaced with AA\n", str);
> > > +   err = 1;
> > > +   }
> > > +   str = DES_crypt("testing", "B");
> > > +   if (strncmp("BA",str,2) != 0) {
> > > +   printf("salt %s second NUL not replaced with A\n", str);
> > > +   err = 1;
> > > +   }
> > > printf("\n");
> > > return(err);
> > > }
> >
>
>

Re: iked, don't return NULL in print_host

[Bob Beck]

ok beck@

On Wed, Nov 29, 2017 at 02:17:21AM +0100, Claudio Jeker wrote:
> On Wed, Nov 29, 2017 at 01:59:06AM +0100, Claudio Jeker wrote:
> > Seen in my log file:
> > Nov 28 17:47:22 dramaqueen iked: vfprintf %s NULL in "%s: %s %s from %s to
> > %s ms gid %u, %ld bytes%s"
> > 
> > and
> > 
> > Nov 29 01:02:39 dramaqueen iked[49967]: ikev2_msg_send: IKE_SA_INIT
> > request from (null) to 62.48.30.5:500 msgid 0, 438 bytes
> > 
> > The problem seems to be in print_host so try to not return NULL in there.
> > Maybe we could return something else but this is a start IMO.
> 
> beck@ prefers to just print unknown instead  of the gai_strerror -- guess
> that is more sensible.
> 
> -- 
> :wq Claudio
> 
> Index: util.c
> ===
> RCS file: /cvs/src/sbin/iked/util.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 util.c
> --- util.c9 Jan 2017 14:49:21 -   1.33
> +++ util.c29 Nov 2017 01:16:21 -
> @@ -654,8 +654,8 @@ print_host(struct sockaddr *sa, char *bu
>  
>   if (getnameinfo(sa, sa->sa_len,
>   buf, len, NULL, 0, NI_NUMERICHOST) != 0) {
> - buf[0] = '\0';
> - return (NULL);
> + strlcpy(buf, "unknown", len);
> + return (buf);
>   }
>  
>   if ((port = socket_getport(sa)) != 0) {
> 

Official OpenBSD 6.2 CD set up for auction on Ebay

[Bob Beck]

So, the only 6.2 set to be produced is up for auction, featuring hand-drawn
artwork by Theo.

Artisanally Made in Canada!

All proceeds of the sale to fund OpenBSD development.

Go have a look at
http://www.ebay.ca/itm/Official-OpenBSD-6-2-CD-Set/253265944606

Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Bob Beck]

effectivelyu providing a limitless OCSP staple is kind of stupid - you may
as well simply *not staple*

On Wed, Sep 6, 2017 at 8:23 AM, Bob Beck <b...@obtuse.com> wrote:

> I'm not super inclined to make this "flexible" unless we see this used int
> the wild, which I have not. We are more restrictive than
> OpenSSL in many areas.
>
> On Wed, Sep 6, 2017 at 1:31 AM, Andreas Bartelt <o...@bartula.de> wrote:
>
>> On 09/06/17 04:40, Bob Beck wrote:
>>
>>> Andreas where are you seeing this as being a real issue - who is shipping
>>> out OCSP responses without a next update field?
>>>
>>>
>> I've noticed this while playing with a local CA and a corresponding OCSP
>> responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is
>> optional. If these arguments are not explicitly provided, the next update
>> field will not be set.
>>
>>
>>
>>>
>>> On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt <o...@bartula.de>
>>> wrote:
>>>
>>> ocspcheck effectively treats a missing nextUpdate like an error, i.e., it
>>>> always provides a warning and no staplefile is written out. According to
>>>> RFC 6960, the nextUpdate field is optional. The following patch should
>>>> handle this case more gracefully and include a suitable debug message
>>>> only
>>>> in case -vv is specified.
>>>>
>>>> OK?
>>>>
>>>> Index: src/usr.sbin/ocspcheck/ocspcheck.c
>>>> ===
>>>> RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
>>>> retrieving revision 1.21
>>>> diff -u -p -u -r1.21 ocspcheck.c
>>>> --- src/usr.sbin/ocspcheck/ocspcheck.c  8 May 2017 20:15:34 -
>>>>   1.21
>>>> +++ src/usr.sbin/ocspcheck/ocspcheck.c  2 Sep 2017 17:09:00 -
>>>> @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size
>>>>   {
>>>>  ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL,
>>>> *nextupd =
>>>> NULL;
>>>>  const unsigned char **p = (const unsigned char **)
>>>> -   int status, cert_status=0, crl_reason=0;
>>>> +   int status, cert_status=0, crl_reason=0, next_update=0;
>>>>  time_t now, rev_t = -1, this_t, next_t;
>>>>  OCSP_RESPONSE *resp;
>>>>  OCSP_BASICRESP *bresp;
>>>> @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size
>>>>  return 0;
>>>>  }
>>>>  if ((next_t = parse_ocsp_time(nextupd)) == -1) {
>>>> -   warnx("unable to parse next update time in OCSP reply");
>>>> -   return 0;
>>>> +   if (verbose >= 2)
>>>> +   fprintf(stderr, "Optional timestamp for next
>>>> update not included in OCSP reply\n");
>>>>  }
>>>> +   else
>>>> +   next_update = 1;
>>>>
>>>>  /* Don't allow this update to precede next update */
>>>> -   if (this_t >= next_t) {
>>>> +   if (next_update == 1 && this_t >= next_t) {
>>>>  warnx("Invalid OCSP reply: this update >= next
>>>> update");
>>>>  return 0;
>>>>  }
>>>> @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size
>>>>  /*
>>>>   * Check that next update is still valid
>>>>   */
>>>> -   if (next_t < now - JITTER_SEC) {
>>>> +   if (next_update == 1 && next_t < now - JITTER_SEC) {
>>>>  warnx("Invalid OCSP reply: reply has expired (%s)",
>>>>  ctime(_t));
>>>>  return 0;
>>>> @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size
>>>>
>>>>  vspew("OCSP response validated from %s\n", host);
>>>>  vspew("This Update: %s", ctime(_t));
>>>> -   vspew("Next Update: %s", ctime(_t));
>>>> +   if (next_update == 1)
>>>> +   vspew("Next Update: %s", ctime(_t));
>>>>  return 1;
>>>>   }
>>>>
>>>>
>>>>
>>>
>>
>

Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Bob Beck]

I'm not super inclined to make this "flexible" unless we see this used int
the wild, which I have not. We are more restrictive than
OpenSSL in many areas.

On Wed, Sep 6, 2017 at 1:31 AM, Andreas Bartelt <o...@bartula.de> wrote:

> On 09/06/17 04:40, Bob Beck wrote:
>
>> Andreas where are you seeing this as being a real issue - who is shipping
>> out OCSP responses without a next update field?
>>
>>
> I've noticed this while playing with a local CA and a corresponding OCSP
> responder on my LAN. For openssl ocsp, the -nmin or -ndays argument is
> optional. If these arguments are not explicitly provided, the next update
> field will not be set.
>
>
>
>>
>> On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt <o...@bartula.de> wrote:
>>
>> ocspcheck effectively treats a missing nextUpdate like an error, i.e., it
>>> always provides a warning and no staplefile is written out. According to
>>> RFC 6960, the nextUpdate field is optional. The following patch should
>>> handle this case more gracefully and include a suitable debug message
>>> only
>>> in case -vv is specified.
>>>
>>> OK?
>>>
>>> Index: src/usr.sbin/ocspcheck/ocspcheck.c
>>> ===
>>> RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
>>> retrieving revision 1.21
>>> diff -u -p -u -r1.21 ocspcheck.c
>>> --- src/usr.sbin/ocspcheck/ocspcheck.c  8 May 2017 20:15:34 -
>>>   1.21
>>> +++ src/usr.sbin/ocspcheck/ocspcheck.c  2 Sep 2017 17:09:00 -
>>> @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size
>>>   {
>>>  ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd
>>> =
>>> NULL;
>>>  const unsigned char **p = (const unsigned char **)
>>> -   int status, cert_status=0, crl_reason=0;
>>> +   int status, cert_status=0, crl_reason=0, next_update=0;
>>>  time_t now, rev_t = -1, this_t, next_t;
>>>  OCSP_RESPONSE *resp;
>>>  OCSP_BASICRESP *bresp;
>>> @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size
>>>  return 0;
>>>  }
>>>  if ((next_t = parse_ocsp_time(nextupd)) == -1) {
>>> -   warnx("unable to parse next update time in OCSP reply");
>>> -   return 0;
>>> +   if (verbose >= 2)
>>> +   fprintf(stderr, "Optional timestamp for next
>>> update not included in OCSP reply\n");
>>>  }
>>> +   else
>>> +   next_update = 1;
>>>
>>>  /* Don't allow this update to precede next update */
>>> -   if (this_t >= next_t) {
>>> +   if (next_update == 1 && this_t >= next_t) {
>>>  warnx("Invalid OCSP reply: this update >= next update");
>>>  return 0;
>>>  }
>>> @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size
>>>  /*
>>>   * Check that next update is still valid
>>>   */
>>> -   if (next_t < now - JITTER_SEC) {
>>> +   if (next_update == 1 && next_t < now - JITTER_SEC) {
>>>  warnx("Invalid OCSP reply: reply has expired (%s)",
>>>  ctime(_t));
>>>  return 0;
>>> @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size
>>>
>>>  vspew("OCSP response validated from %s\n", host);
>>>  vspew("This Update: %s", ctime(_t));
>>> -   vspew("Next Update: %s", ctime(_t));
>>> +   if (next_update == 1)
>>> +   vspew("Next Update: %s", ctime(_t));
>>>  return 1;
>>>   }
>>>
>>>
>>>
>>
>

Re: [patch] ocspcheck: nextUpdate is optional according to RFC 6960

[Bob Beck]

Andreas where are you seeing this as being a real issue - who is shipping
out OCSP responses without a next update field?



On Sat, Sep 2, 2017 at 11:28 AM, Andreas Bartelt  wrote:

> ocspcheck effectively treats a missing nextUpdate like an error, i.e., it
> always provides a warning and no staplefile is written out. According to
> RFC 6960, the nextUpdate field is optional. The following patch should
> handle this case more gracefully and include a suitable debug message only
> in case -vv is specified.
>
> OK?
>
> Index: src/usr.sbin/ocspcheck/ocspcheck.c
> ===
> RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
> retrieving revision 1.21
> diff -u -p -u -r1.21 ocspcheck.c
> --- src/usr.sbin/ocspcheck/ocspcheck.c  8 May 2017 20:15:34 -
>  1.21
> +++ src/usr.sbin/ocspcheck/ocspcheck.c  2 Sep 2017 17:09:00 -
> @@ -368,7 +368,7 @@ validate_response(char *buf, size_t size
>  {
> ASN1_GENERALIZEDTIME *revtime = NULL, *thisupd = NULL, *nextupd =
> NULL;
> const unsigned char **p = (const unsigned char **)
> -   int status, cert_status=0, crl_reason=0;
> +   int status, cert_status=0, crl_reason=0, next_update=0;
> time_t now, rev_t = -1, this_t, next_t;
> OCSP_RESPONSE *resp;
> OCSP_BASICRESP *bresp;
> @@ -447,12 +447,14 @@ validate_response(char *buf, size_t size
> return 0;
> }
> if ((next_t = parse_ocsp_time(nextupd)) == -1) {
> -   warnx("unable to parse next update time in OCSP reply");
> -   return 0;
> +   if (verbose >= 2)
> +   fprintf(stderr, "Optional timestamp for next
> update not included in OCSP reply\n");
> }
> +   else
> +   next_update = 1;
>
> /* Don't allow this update to precede next update */
> -   if (this_t >= next_t) {
> +   if (next_update == 1 && this_t >= next_t) {
> warnx("Invalid OCSP reply: this update >= next update");
> return 0;
> }
> @@ -481,7 +483,7 @@ validate_response(char *buf, size_t size
> /*
>  * Check that next update is still valid
>  */
> -   if (next_t < now - JITTER_SEC) {
> +   if (next_update == 1 && next_t < now - JITTER_SEC) {
> warnx("Invalid OCSP reply: reply has expired (%s)",
> ctime(_t));
> return 0;
> @@ -489,7 +491,8 @@ validate_response(char *buf, size_t size
>
> vspew("OCSP response validated from %s\n", host);
> vspew("This Update: %s", ctime(_t));
> -   vspew("Next Update: %s", ctime(_t));
> +   if (next_update == 1)
> +   vspew("Next Update: %s", ctime(_t));
> return 1;
>  }
>
>

Re: [PATCH 0/2] SMALL_TIME_T follow-ups (was Re: [PATCH] allow notAfter after 2038 with 32-bit time_t)

[Bob Beck]

> 
> With the new define (SMALL_TIME_T) enabled, a 32-bit time_t build  
> using "openssl s_client -connect" can successfully connect to a server  
> and verify its certificate chain when one or more notAfter dates after  
> 2038 are present.
> 
> However, using "nc -c" fails to connect to the same server.
> 
> The reason being that libtls also needs to clamp the notAfter date.

I've just committed a change that enables libtls to use it. so
nc and libtls should work on broken computers now. 

I still think karma for me doing this is I will be killed by an
embedded 32 bit linux device in 2038...

 -Bob

Re: [PATCH] allow notAfter after 2038 with 32-bit time_t

[Bob Beck]

On Thu, May 18, 2017 at 7:31 AM, Kyle J. McKay  wrote:

> RFC 5280 section 4.1.2.5 states:
>
> To indicate that a certificate has no well-defined expiration date,
> the notAfter SHOULD be assigned the GeneralizedTime value of
> 1231235959Z.
>
>
True enough.



> Unfortunately, if sizeof(time_t) == 4, -12-31T23:59:59Z cannot be
> represented as a time_t value causing valid certificates to be rejected
> just because the notAfter value is after 2038-01-19T03:14:07Z.
>

Correct

So, I'll ask - what is the platform you are using that needs this? OpenBSD
does not, nor do most modern unix systems - So what platform are we doing
this for?

I'm not asking it to be nasty, but to put it in a slightly different
context "OMG, windows 98 does not support this, but if you add this code I
can support windows 98". You and I both know what your answer would
probably be for the person with Windows 98, which is "Here's a nickel kid,
get a modern operating system".

We ripped out support for a lot of moribund platforms for a reason.  What
platform are you using with 32 bit time where this is an issue?  I'm not
saying no, I'm saying "convince me this matters for anything relevant
please", and I am open to being convinced.


Fix this problem by disabling the restriction in the X509_cmp_time function
> and "wrap" far in the future notAfter values to 2038-01-19T03:14:07Z in the
> tls_get_peer_cert_times function.
>
> With both of these changes certificates with "no well-defined expiration
> date" as specified by RFC 5280 are again accepted on platforms where the
> sizeof(time_t) == 4.
>
> In general, there's no reason that a notAfter value should not be wrapped
> to 2038-01-19T03:14:07Z on a system with a 32-bit time_t.  The system
> itself
> can never have a time after 2038-01-19T03:14:07Z because of the size of the
> time_t type and so wrapping a notAfter date that is after
> 2038-01-19T03:14:07Z
> to 2038-01-19T03:14:07Z can never result in any additional certificates
> being
> accepted on such a system.
>
> Signed-off-by: Kyle J. McKay 
> ---
>
> For those using the libressl-2.5.4.tar.gz distribution, an equivalent
> patch that updates the tarball files instead can be found here:
>
>   https://gist.github.com/7d4d59bbae9e4d18444b86aa79d6f350
>
> Without this patch (or an equivalent), libressl-portable is not a viable
> alternative to OpenSSL on systems with a 32-bit time_t.
>
> It rejects valid TLS connections to any site that contains a notAfter date
> after 2038-01-19T03:14:07Z in any certificate in the chain.
>
> Besides the special case date mentioned above, there are many root
> certificates
> already in use today that have notAfter dates beyond 2038-01-19.
>
> These patches have been tested with libressl-portable 2.5.4 on a system
> with
> a 32-bit time_t.  With both of these patches the "nc -c" command
> successfully
> connects to sites with certificates that include notAfter dates after
> 2038-01-19.
>
> If either patch is omitted, "nc -c" fails to connect.
>
>  src/lib/libcrypto/x509/x509_vfy.c | 3 ++-
>  src/lib/libtls/tls_conninfo.c | 8 ++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/lib/libcrypto/x509/x509_vfy.c
> b/src/lib/libcrypto/x509/x509_vfy.c
> index d8c09a12..c59bd258 100644
> --- a/src/lib/libcrypto/x509/x509_vfy.c
> +++ b/src/lib/libcrypto/x509/x509_vfy.c
> @@ -1882,7 +1882,8 @@ X509_cmp_time(const ASN1_TIME *ctm, time_t *cmp_time)
>  * a time_t. A time_t must be sane if you care about times after
>  * Jan 19 2038.
>  */
> -   if ((time1 = timegm()) == -1)
> +   if (((time1 = timegm()) == -1) &&
> +   ((sizeof(time_t) != 4) || tm1.tm_year < 138))
> goto out;
>
> if (gmtime_r(, ) == NULL)
> diff --git a/src/lib/libtls/tls_conninfo.c b/src/lib/libtls/tls_conninfo.c
> index 5cdd0f77..a59b4ba2 100644
> --- a/src/lib/libtls/tls_conninfo.c
> +++ b/src/lib/libtls/tls_conninfo.c
> @@ -142,8 +142,12 @@ tls_get_peer_cert_times(struct tls *ctx, time_t
> *notbefore,
> goto err;
> if ((*notbefore = timegm(_tm)) == -1)
> goto err;
> -   if ((*notafter = timegm(_tm)) == -1)
> -   goto err;
> +   if ((*notafter = timegm(_tm)) == -1) {
> +   if (sizeof(time_t) == 4 && after_tm.tm_year >= 138)
> +   *notafter = 2147483647;
> +   else
> +   goto err;
> +   }
>
> return (0);
>
> ---
>
>

Re: Better handling of short reads

[Bob Beck]

> As you all might have gathered by now Amit has jumped the gun
> but was wrong to do so.  His setup is not affected by this change.
> That was expected so please don't get distracted by this as I'm
> still looking forward to replies to the original set of changes.
> beck@?
> 
> > diff --git sys/kern/vfs_bio.c sys/kern/vfs_bio.c
> > index 95bc80bc0e6..9316e6e0eb2 100644
> > --- sys/kern/vfs_bio.c
> > +++ sys/kern/vfs_bio.c
> > @@ -534,10 +534,27 @@ bread_cluster_callback(struct buf *bp)
> >  */
> > buf_fix_mapping(bp, newsize);
> > bp->b_bcount = newsize;
> > }
> >  
> > +   /* Invalidate read-ahead buffers if read short */
> > +   if (bp->b_resid > 0) {
> > +   printf("read %ld resid %ld\n", bp->b_bcount, bp->b_resid);

Should the printf actually be here?  I'm not thinking this thing 
spewing dmesg like a banshee if we get short reads is really going
to help anything

> > +   for (i = 0; xbpp[i] != NULL; i++)
> > +   continue;
> > +   for (i = i - 1; i != 0; i--) {
> > +   if (xbpp[i]->b_bufsize <= bp->b_resid) {
> > +   bp->b_resid -= xbpp[i]->b_bufsize;
> > +   SET(xbpp[i]->b_flags, B_INVAL);
> > +   } else if (bp->b_resid > 0) {
> > +   bp->b_resid = 0;
> > +   SET(xbpp[i]->b_flags, B_INVAL);
> > +   } else
> > +   break;
> > +   }
> > +   }
> > +
> > for (i = 1; xbpp[i] != 0; i++) {
> > if (ISSET(bp->b_flags, B_ERROR))
> > SET(xbpp[i]->b_flags, B_INVAL | B_ERROR);
> > biodone(xbpp[i]);
> > }
> 

Re: Better handling of short reads

[Bob Beck]

 - ok mike, I'm looking at it..   Allow me a short while to beat my
head against a wall for a bit to get it into readahead mode...


On Wed, Jun 14, 2017 at 3:56 AM, Mike Belopuhov  wrote:

> On Thu, Jun 08, 2017 at 11:55 +0200, Mike Belopuhov wrote:
> > On Wed, Jun 07, 2017 at 23:04 -0500, Amit Kulkarni wrote:
> > > On Wed, 7 Jun 2017 21:27:27 -0500
> > > Amit Kulkarni  wrote:
> > >
> > > > On Thu, 8 Jun 2017 01:57:25 +0200
> > > > Mike Belopuhov  wrote:
> > > >
> > > > > On Wed, Jun 07, 2017 at 18:35 -0500, Amit Kulkarni wrote:
> > > > > > Wow, please get this in!!!
> > > > > >
> > > > > > This fixes cvs update on hard disks, to go much much faster.
> When I am
> > > > > > updating the entire set of cvs trees: www, src, xenocara, ports,
> I can
> > > > > > still use firefox and have it perfectly usable. There's a night
> and
> > > > > > day improvement, before and after. Thanks for debugging and
> fixing
> > > > > > this.
> > > > > >
> > > > >
> > > > > What kind of broken hardware do you have that this diff helps you?
> > > > > Can you show us your dmesg?
> > > > >
> > >
> > > Please ignore previous dmesg, it was incomplete.
> > >
> >
> > Are you 100% sure this diff changes anything for you?
> > Can you please try the one below.  It adds a printf.
> >
>
> As you all might have gathered by now Amit has jumped the gun
> but was wrong to do so.  His setup is not affected by this change.
> That was expected so please don't get distracted by this as I'm
> still looking forward to replies to the original set of changes.
> beck@?
>
> > diff --git sys/kern/vfs_bio.c sys/kern/vfs_bio.c
> > index 95bc80bc0e6..9316e6e0eb2 100644
> > --- sys/kern/vfs_bio.c
> > +++ sys/kern/vfs_bio.c
> > @@ -534,10 +534,27 @@ bread_cluster_callback(struct buf *bp)
> >*/
> >   buf_fix_mapping(bp, newsize);
> >   bp->b_bcount = newsize;
> >   }
> >
> > + /* Invalidate read-ahead buffers if read short */
> > + if (bp->b_resid > 0) {
> > + printf("read %ld resid %ld\n", bp->b_bcount, bp->b_resid);
> > + for (i = 0; xbpp[i] != NULL; i++)
> > + continue;
> > + for (i = i - 1; i != 0; i--) {
> > + if (xbpp[i]->b_bufsize <= bp->b_resid) {
> > + bp->b_resid -= xbpp[i]->b_bufsize;
> > + SET(xbpp[i]->b_flags, B_INVAL);
> > + } else if (bp->b_resid > 0) {
> > + bp->b_resid = 0;
> > + SET(xbpp[i]->b_flags, B_INVAL);
> > + } else
> > + break;
> > + }
> > + }
> > +
> >   for (i = 1; xbpp[i] != 0; i++) {
> >   if (ISSET(bp->b_flags, B_ERROR))
> >   SET(xbpp[i]->b_flags, B_INVAL | B_ERROR);
> >   biodone(xbpp[i]);
> >   }
>
>

Re: ocspcheck size_t printing

[Bob Beck]

You are correct. 

Patch committed.  Thanks!

-Bob


On Mon, May 08, 2017 at 08:20:57PM +0200, Jonas 'Sortie' Termansen wrote:
> Hi,
> 
> When upgrading to libressl-2.5.4 I noticed a couple -Wformat errors due
> to this code assuming size_t is of type long when it was actually int on
> this 32-bit system. Here's a patch against cvs that fixes the issue and
> also prints the variableas unsigned type.
> 
> Jonas
> 
> Index: ocspcheck.c
> ===
> RCS file: /cvs/src/usr.sbin/ocspcheck/ocspcheck.c,v
> retrieving revision 1.20
> diff -u -r1.20 ocspcheck.c
> --- ocspcheck.c   27 Mar 2017 23:59:08 -  1.20
> +++ ocspcheck.c   8 May 2017 18:15:51 -
> @@ -564,7 +564,7 @@
>   if ((request = ocsp_request_new_from_cert(certfile, nonce)) == NULL)
>   exit(1);
>  
> - dspew("Built an %ld byte ocsp request\n", request->size);
> + dspew("Built an %zu byte ocsp request\n", request->size);
>  
>   if ((host = url2host(request->url, , )) == NULL)
>   errx(1, "Invalid OCSP url %s from %s", request->url,
> @@ -601,7 +601,7 @@
>   dspew("Server at %s returns:\n", host);
>   for (i = 0; i < httphsz; i++)
>   dspew("   [%s]=[%s]\n", httph[i].key, httph[i].val);
> - dspew("   [Body]=[%ld bytes]\n", hget->bodypartsz);
> + dspew("   [Body]=[%zu bytes]\n", hget->bodypartsz);
>   if (hget->bodypartsz <= 0)
>   errx(1, "No body in reply from %s", host);
>  
> 

Official OpenBSD 6.1 CD !

[Bob Beck]

So.  There *Is* an official OpenBSD 6.1 CD

Just One.

If you are interested, please bid on ebay :

http://www.ebay.com/itm/The-only-Official-OpenBSD-6-1-CD-set-to-be-made-For-auction-for-the-project-/252910718452?hash=item3ae2a74df4:g:SJQAAOSwrhBZBqkd

(It's a pretty cool little CD set!)

Re: explicit_bzero after readpassphrase

[Bob Beck]

On Mon, May 01, 2017 at 04:07:27PM -0600, Theo de Raadt wrote:
> 
> Let me stop here and ask if the pattern is: "always explicit_bzero
> a password field once it is used"?  It might make sense, but some
> of these are heading straight to exit immediately.  Is it too much
> to do it then, or is the worry these code patterns might get copied
> elsewhere?
> 

I would fall on the side of "It could get copied elsewhere or hoisted 
for other reasons (like pledge)" so do it anyway. 

Re: patch: mv(1): Add -p flag to preserve time stamps for moved directories

[Bob Beck]

> Note that I have noatime on this FS.

then turn that off, or understand that things will not behave as you expect 
them to with it on. 

Re: httpd/libtls: TLS client certificate revocation checking

[Bob Beck]

There will be some libtls api additions post 6.1 to get the peer cert in
PEM format

In the meantime, testing snaps prior to 6.1 should be the priority. not a
talkathon.

On Sat, Apr 1, 2017 at 10:49 Joerg Sonnenberger  wrote:

> On Sat, Apr 01, 2017 at 07:53:05PM +1030, Jack Burton wrote:
> > One common example of that happening is when a cert gets revoked because
> > its private key has been lost/stolen and the user needs a new cert
> > associated with the same identity. An even more common example is when
> > a cert expires & gets renewed.
>
> If you are using certificate revocation, I think you should do the check
> as early as possible. That means in httpd in this case. Nothing later in
> the stack should have to care about expired or revoked certificates --
> it just adds complexity and the danger of someone forgetting about it.
>
> Which mechanisms to support (i.e. CRLs or OCSP) is a completely
> different topic.
>
> Joerg
>
>

Re: regarding OpenSSL License change

[Bob Beck]

On Thu, Mar 23, 2017 at 17:48 Bob Beck <b...@obtuse.com> wrote:

> Honestly, anyone who gets one of these should say no
>
> what would you all think if people quietly took derived works of software
> licensed under one license and took silence as assent to relicense
>
> Does this mean that with an unanswered email i can now release my re
> licensed as ISC version of gcc?  or the linux kernel?
>
> This sort of action just means that any software you write can be
> plagiarized against your will if you did not find out about it in time.
>
> thats really not cool
>
> If you write software this is not a world you want to live in.   Even if
> it does mean a anyone who can fork a github repo could get rid of the GPL
> after a period of non response from an author (dont go on vacation). As
> much as I might not agree with the GPL personally, I respect someones right
> to release thier work under a license and have it respected. without having
> to constantly answer emails and click web links telling people no
>
>
>
> On Thu, Mar 23, 2017 at 10:58 Theo de Raadt <dera...@openbsd.org> wrote:
>
> > > The start suggests they want to privately collect sufficient consensus
> > > to pass their agenda.  They appear to be considering all actions in
> > > the tree (including mine) on equal grounds.
> >
> > I already sent them a clear "NO, i explicitly object to relicensing
> > any of my contributions."
> >
> > If any of you care about the possibility of merging future OpenSSL
> > improvements to LibreSSL and OpenBSD, i suggest you do the same.
> >
> > Similarly, if any of you dislike publishing their own code under Apache
> 2.
>
> There has been no discussion amongst the greater community of
> developers as to which license to take.  Apache 2 has come as an edict
> from Rich Salz.
>
> There has also been no statement from the original authorship that this
> is the way to go.
>
> I suspect there is a lack of approval from some, and manufacturing
> consent in volume is the approach being taken.
>
>
> Apparently lawyers are being paid to help them push this through.  Is
> that being paid for by donations people gave after Heartbleed?  Is
> this why people donated?
>
>

Re: regarding OpenSSL License change

[Bob Beck]

Honestly, anyone who gets one of these should say no

what would you all think if people quietly took derived works of software
licensed under one license and took silence as assent to relicense

Does this mean that with an unanswered email i can now release my re
licensed as ISC version of gcc?  or the linux kernel?

This sort of action just means that any software you write can be
plagiarized against your will if you did not find out about it in time.

thats really not cool

If you write software this is not a world you want to live in.   Even if it
does mean a anyone who can fork a github repo could get rid of the GPL
after a period of non response from an author (dont go on vacation). As
much as I might not agree with the GPL personally, I respect someones right
to release thier work under a license and have it respected. without having
to constantly answer emails and click web links telling people no



On Thu, Mar 23, 2017 at 10:58 Theo de Raadt  wrote:

> > > The start suggests they want to privately collect sufficient consensus
> > > to pass their agenda.  They appear to be considering all actions in
> > > the tree (including mine) on equal grounds.
> >
> > I already sent them a clear "NO, i explicitly object to relicensing
> > any of my contributions."
> >
> > If any of you care about the possibility of merging future OpenSSL
> > improvements to LibreSSL and OpenBSD, i suggest you do the same.
> >
> > Similarly, if any of you dislike publishing their own code under Apache
> 2.
>
> There has been no discussion amongst the greater community of
> developers as to which license to take.  Apache 2 has come as an edict
> from Rich Salz.
>
> There has also been no statement from the original authorship that this
> is the way to go.
>
> I suspect there is a lack of approval from some, and manufacturing
> consent in volume is the approach being taken.
>
>
> Apparently lawyers are being paid to help them push this through.  Is
> that being paid for by donations people gave after Heartbleed?  Is
> this why people donated?
>
>

Re: tlsv1 alert decrypt error

[Bob Beck]

And as joel mentioned, a fix is already arriving for this - there was a bug
in SSLv2 compatible handshake initiation,
and Paypal still has it enabled... (yeeuch)

On Mon, Mar 6, 2017 at 3:48 PM, Bob Beck <b...@obtuse.com> wrote:

>
> Move it to tech@ from misc.. not libressl.. libressl is not special ;)
>
> On Mon, Mar 6, 2017 at 3:21 PM, Kirill Miazine <k...@krot.org> wrote:
>
>> Moving to libressl@ from misc@, as it's a LibreSSL issue.
>>
>> * Joel Sing [2017-03-05 23:01]:
>>
>> On Thursday 02 March 2017 13:28:08 Kirill Miazine wrote:
>>>
>>>> Recently I've noticed a number of error messages in my Exim mail log:
>>>>
>>>> TLS error on connection from mx1.slc.paypal.com (mx0.slc.paypal.com
>>>> )
>>>> [173.0.84.226] \ (SSL_accept): error:1403741B:SSL
>>>> routines:ACCEPT_SR_KEY_EXCH:tlsv1 alert decrypt error TLS client
>>>> disconnected cleanly (rejected our certificate?)
>>>>
>>>
>>> This is most likely the same issue as that reported on the libressl@
>>> mailing
>>> list a day or so ago - expect a fix to arrive shortly.
>>>
>>
>> I rebuilt exim on latest snapshot (OpenBSD 6.1-beta (GENERIC.MP) #213:
>> Mon Mar  6 12:31:59 MST 2017) and the error looks different now:
>>
>> TLS error on connection from mx0.phx.paypal.com [66.211.168.230] \
>>(SSL_accept): error:14039119:SSL routines:ACCEPT_SR_CERT_VRFY:decryption
>> \
>>failed or bad record mac
>>
>>
>> --
>>-- Kirill Miazine <k...@krot.org>
>>
>>
>

Re: tlsv1 alert decrypt error

[Bob Beck]

Move it to tech@ from misc.. not libressl.. libressl is not special ;)

On Mon, Mar 6, 2017 at 3:21 PM, Kirill Miazine  wrote:

> Moving to libressl@ from misc@, as it's a LibreSSL issue.
>
> * Joel Sing [2017-03-05 23:01]:
>
> On Thursday 02 March 2017 13:28:08 Kirill Miazine wrote:
>>
>>> Recently I've noticed a number of error messages in my Exim mail log:
>>>
>>> TLS error on connection from mx1.slc.paypal.com (mx0.slc.paypal.com)
>>> [173.0.84.226] \ (SSL_accept): error:1403741B:SSL
>>> routines:ACCEPT_SR_KEY_EXCH:tlsv1 alert decrypt error TLS client
>>> disconnected cleanly (rejected our certificate?)
>>>
>>
>> This is most likely the same issue as that reported on the libressl@
>> mailing
>> list a day or so ago - expect a fix to arrive shortly.
>>
>
> I rebuilt exim on latest snapshot (OpenBSD 6.1-beta (GENERIC.MP) #213:
> Mon Mar  6 12:31:59 MST 2017) and the error looks different now:
>
> TLS error on connection from mx0.phx.paypal.com [66.211.168.230] \
>(SSL_accept): error:14039119:SSL routines:ACCEPT_SR_CERT_VRFY:decryption
> \
>failed or bad record mac
>
>
> --
>-- Kirill Miazine 
>
>

Re: Scheduler ping-pong with preempt()

[Bob Beck]

Go for it mpi.. move forward.

ok beck@

On Mon, Feb 6, 2017 at 7:48 AM, Martin Pieuchot  wrote:

> On 24/01/17(Tue) 13:35, Martin Pieuchot wrote:
> > Userland threads are preempt()'d when hogging a CPU or when processing
> > an AST.  Currently when such a thread is preempted the scheduler looks
> > for an idle CPU and puts it on its run queue.  That means the number of
> > involuntary context switch often result in a migration.
> >
> > This is not a problem per se and one could argue that if another CPU
> > is idle it makes sense to move.  However with the KERNEL_LOCK() moving
> > to another CPU won't necessarily allows the preempt()'d thread to run.
> > It's even worse, it increases contention.
> >
> > If you add to this behavior the fact that sched_choosecpu() prefers idle
> > CPUs in a linear order, meaning CPU0 > CPU1 > .. > CPUN, you'll
> > understand that the set of idle CPUs will change every time preempt() is
> > called.
> >
> > I believe this behavior affects kernel threads by side effect, since
> > the set of idle CPU changes every time a thread is preempted.  With this
> > diff the 'softnet' thread didn't move on a 2 CPUs machine during simple
> > benchmarks.  Without, it plays ping-pong between CPU.
> >
> > The goal of this diff is to reduce the number of migrations.  You
> > can compare the value of 'sched_nomigrations' and 'sched_nmigrations'
> > with and without it.
> >
> > As usual, I'd like to know what's the impact of this diff on your
> > favorite benchmark.  Please test and report back.
>
> I only got positive test results so I'd like to commit the diff below.
>
> ok?
>
> Index: kern/sched_bsd.c
> ===
> RCS file: /cvs/src/sys/kern/sched_bsd.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 sched_bsd.c
> --- kern/sched_bsd.c25 Jan 2017 06:15:50 -  1.44
> +++ kern/sched_bsd.c6 Feb 2017 14:47:28 -
> @@ -329,7 +329,6 @@ preempt(struct proc *newp)
> SCHED_LOCK(s);
> p->p_priority = p->p_usrpri;
> p->p_stat = SRUN;
> -   p->p_cpu = sched_choosecpu(p);
> setrunqueue(p);
> p->p_ru.ru_nivcsw++;
> mi_switch();
>
>

Re: Password corruption in adduser

[Bob Beck]

ok beck@
On Sun, Feb 5, 2017 at 22:53 Theo Buehler  wrote:

> On Sun, Feb 05, 2017 at 09:47:35PM -0800, Philip Guenther wrote:
> > On Sun, 5 Feb 2017, John McGuigan wrote:
> > > I've noticed something strange in adduser -- when attempting to add a
> > > user completely though command line argument it seems to corrupt the
> > > entry in /etc/master.passwd.
> > >
> > > Example:
> > >
> > > $ echo "HorseBatteryStaple" | encrypt
> > > $2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2
> > >
> > > # adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh \
> > > -message no -batch some.user "" "Some User" \
> > > $2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2
> > > Added user ``some.user''
> > ...
> > > some.user:b/bin/ksh9/9uoOrbTRaf//3ZprAb9k.hOpfe9vYVqjf1a:5000:5000:: \
> > > 0:0:Some User:/home/some.user:/bin/ksh
> > >
> > > As you can see the password entry gets corrupted with a 'b/bin/ksh...'
> >
> > Let's see what the adduser command is seeing by passing that all to
> 'echo'
> > instead:
> >
> > # echo \
> > > adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh \
> > > -message no -batch some.user "" "Some User" \
> > > $2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2
> > adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh
> -message no -batch some.user  Some User b/bin/ksh9/FGXw.9oNjr3BLTS7DJp5n4M2
> > #
> >
> > Ah, so the expansion is happening *outside* of adduser...in the shell.
> > Yes, the shell does variable expansion even if the dollar-sign is in the
> > middle of a word, so it's expanding the variables
> >   $2  --> ""
> >   $0  --> "/bin/ksh"
> >   $ssZSLC6laHsTS7O2FwJ4Mufw6mSS   --> ""
> >
> >
> > > Behavior *is* present when hash is wrapped in "
> >
> > Sure, because double-quotes only stop file-globbing and field splitting
> > and not variable expansion.  You need single quotes for that:
> >
> > # echo \
> > > adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh \
> > > -message no -batch some.user "" "Some User" \
> > > '$2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2'
> > adduser -silent -noconfig -uid_start 5000 -group USER -shell ksh
> -message no -batch some.user  Some User
> $2b$09$ssZSLC6laHsTS7O2FwJ4Mufw6mSS/FGXw.9oNjr3BLTS7DJp5n4M2
> > #
>
> The adduser.8 manual page has an example with no quotes in it, so we
> should fix that.  Also, let's use a new hash using $2b$ instead of $2a$.
>
> Index: adduser.8
> ===
> RCS file: /var/cvs/src/usr.sbin/adduser/adduser.8,v
> retrieving revision 1.44
> diff -u -p -r1.44 adduser.8
> --- adduser.8   24 Dec 2015 16:54:37 -  1.44
> +++ adduser.8   6 Feb 2017 05:49:00 -
> @@ -373,7 +373,7 @@ The password has been created using
>  .Xr encrypt 1 :
>  .Bd -literal -offset indent
>  # adduser -batch falken guest,staff,beer 'Prof. Falken' \e
> -$2a$06$1Sdjxjoxg4cNmT6zAxriGOLgdLXQ3HdJ2dKBbzEk68jSrO1EtLJ3C
> +'$2b$10$aOadQNznQ1YJFnqNaRRneOvYvZAEO7atYiTND3EsLf6afHT5t1UIK'
>  .Ed
>  .Pp
>  Create user
>
>

Re: netcat: IPv6 address support for proxy

[Bob Beck]

ok beck@

On Sun, Feb 05, 2017 at 12:27:19AM +0100, Jeremie Courreges-Anglas wrote:
> 
> The colons used in IPv6 addresses conflicts with the proxy port
> specification.  Do the right thing for -x ::1:8080, [::1] and
> [::1]:8080.
> 
> ok?
> 
> 
> Index: netcat.c
> ===
> RCS file: /d/cvs/src/usr.bin/nc/netcat.c,v
> retrieving revision 1.171
> diff -u -p -p -u -r1.171 netcat.c
> --- netcat.c  30 Nov 2016 07:56:23 -  1.171
> +++ netcat.c  4 Feb 2017 23:24:42 -
> @@ -148,8 +148,8 @@ main(int argc, char *argv[])
>   struct servent *sv;
>   socklen_t len;
>   struct sockaddr_storage cliaddr;
> - char *proxy;
> - const char *errstr, *proxyhost = "", *proxyport = NULL;
> + char *proxy, *proxyport = NULL;
> + const char *errstr;
>   struct addrinfo proxyhints;
>   char unix_dg_tmp_socket_buf[UNIX_DG_TMP_SOCKET_SIZE];
>   struct tls_config *tls_cfg = NULL;
> @@ -426,15 +426,29 @@ main(int argc, char *argv[])
>   if (family == AF_UNIX)
>   errx(1, "no proxy support for unix sockets");
>  
> - /* XXX IPv6 transport to proxy would probably work */
> - if (family == AF_INET6)
> - errx(1, "no proxy support for IPv6");
> -
>   if (sflag)
>   errx(1, "no proxy support for local source address");
>  
> - proxyhost = strsep(, ":");
> - proxyport = proxy;
> + if (*proxy == '[') {
> + ++proxy;
> + proxyport = strchr(proxy, ']');
> + if (proxyport == NULL)
> + errx(1, "missing closing bracket in proxy");
> + *proxyport++ = '\0';
> + if (*proxyport == '\0')
> + /* Use default proxy port. */
> + proxyport = NULL;
> + else {
> + if (*proxyport == ':')
> + ++proxyport;
> + else
> + errx(1, "garbage proxy port delimiter");
> + }
> + } else {
> + proxyport = strrchr(proxy, ':');
> + if (proxyport != NULL)
> + *proxyport++ = '\0';
> + }
>  
>   memset(, 0, sizeof(struct addrinfo));
>   proxyhints.ai_family = family;
> @@ -617,7 +631,7 @@ main(int argc, char *argv[])
>   }
>   if (xflag)
>   s = socks_connect(host, portlist[i], hints,
> - proxyhost, proxyport, proxyhints, socksv,
> + proxy, proxyport, proxyhints, socksv,
>   Pflag);
>   else
>   s = remote_connect(host, portlist[i], hints);
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

Re: Update for US Holidays.

[Bob Beck]

On Sat, Feb 04, 2017 at 01:52:14PM -0700, Bob Beck wrote:
> 
> Presented without further comment. 
> 
> ok? 
> 

Or maybe this is more appropriate:

Index: calendar.history
===
RCS file: /cvs/src/usr.bin/calendar/calendars/calendar.history,v
retrieving revision 1.80
diff -u -p -u -p -r1.80 calendar.history
--- calendar.history19 Nov 2016 12:41:22 -  1.80
+++ calendar.history4 Feb 2017 21:28:55 -
@@ -475,6 +475,7 @@
 12/12  First wireless message sent across Atlantic by Marconi, 1901
 12/13  Dartmouth College chartered, 1769
 12/14  Portugal joins the United Nations, 1955
+12/14  World totals the cost of running embedded 32 bit Linux, 1901
 12/15  Argo Merchant oil spill, 1976
 12/15  James Naismith invents basketball, Canada, 1891
 12/16  Pokemon episode (Electric Soldier Porygon) triggers attacks of

Re: Update for US Holidays.

[Bob Beck]

On Sat, Feb 04, 2017 at 12:59:53PM -0800, Philip Guenther wrote:
> On Sat, Feb 4, 2017 at 12:52 PM, Bob Beck <b...@obtuse.com> wrote:
> >
> > Presented without further comment.
> >
> > ok?
> 
> NACK.  Obsolete 32bit time_t OSes can track their own damn holidays.

But how will I remember to be appropriately devotional if my embedded
linux pacemaker didn't kill me the previous day?  :)

Update for US Holidays.

[Bob Beck]

Presented without further comment. 

ok? 

Index: calendar.usholiday
===
RCS file: /cvs/src/usr.bin/calendar/calendars/calendar.usholiday,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 calendar.usholiday
--- calendar.usholiday  19 Jan 2015 18:07:47 -  1.9
+++ calendar.usholiday  4 Feb 2017 20:50:41 -
@@ -33,6 +33,7 @@
 11/SunFirstDaylight Saving Time ends; clocks move back (1st Sunday in 
November)
 11/11  Veterans' Day
 11/ThuFourth   Thanksgiving Day (4th Thursday in November)
+12/14  "National Day of Patriotic Devotion" (After 1901, for 32 bit time_t 
systems)
 12/21* Winter Solstice
 12/24  Christmas Eve
 12/25  Christmas

Re: specify curves via ecdhe statement in httpd.conf

[Bob Beck]

try connecting with openbsd nc rather than s-client

On Sat, Feb 4, 2017 at 09:13 Bob Beck <b...@obtuse.com> wrote:

>
> On Sat, Feb 4, 2017 at 07:51 Andreas Bartelt <o...@bartula.de> wrote:
>
> On 02/04/17 05:26, Joel Sing wrote:
> > On Wednesday 01 February 2017 15:41:29 Andreas Bartelt wrote:
> >> Hello,
> >>
> >> after reading the LibreSSL accouncement from today, I assumed that
> >> specifying ecdhe "auto" in /etc/httpd.conf would enable X25519, P-256
> >> and P-384 on current.
> >
> > This is correct.
> >
> >> I've noticed that "auto" enables only curves x25519 and P-256 (which is
> >> what I'd want to use - but somehow unexpected with regard to the
> >> announcement).
> >
> > Why do you believe this is the case?
> >
>
> Tested with a build of today's current:
> - httpd started with ecdhe "auto" in /etc/httpd.conf
> - then trying to connect via openssl s_client with -groups P-384 option
> doesn't negotiate a cipher suite.
>
> However, specifying -groups P-256 works. I don't know how to specify
> x25519 with OpenBSD's openssl s_client (it's not yet listed in openssl
> ecparam -list_curves output) but SSL Labs successfully negotiates via
> x25519 and P-256 (but not P-384). P-384 doesn't seem to be enabled with
> "auto".
>
> Another confusing test result:
> - httpd started with ecdhe "secp384r1" (P-384)
> - then trying to connect via openssl s_client with -groups P-384 option
> also doesn't negotiate a cipher suite!
>
> However, SSL Labs successfully connects to httpd and confirms support
> for secp384r1.
>
> Can you reproduce this?
>
> >> Diff is attached which clarifies the meaning of "auto" in httpd.conf.5.
> >
> > There are some documentation improvements that could be used here,
> however the
> > meaning of auto for httpd.conf.5 needs to refer to the meaning of "auto"
> for
> > libtls (currently tls_config_set_ecdhecurve()). Otherwise libtls changes
> and
> > httpd becomes out of date.
> >
> >> There currently seems to be no way to explicitly specify x25519, or to
> >> specify multiple colon separated curves with the ecdhe statement. Would
> >> it make sense to change semantics and make the ecdhe statement in
> >> httpd.conf consistent with the recent changes to openssl s_client
> >> -groups (e.g., to also allow more common names like P-256 instead of
> >> prime256v1)?
> >
> > Yes - tls_config_set_ecdhecurve() needs to change to accept the same
> colon
> > separate list of priority ordered curve names, that
> SSL_set1_curves_list()
> > accepts.
> >
> >
>
>

OpenBSD errata, Jan 31, 2017

[Bob Beck]

An issue has been identified whereby httpd(8) could be subject to a denial
of service attack. Repeated crafted requests could be made from a client
using file-range requests, making the server consume excessive amounts of
memory.

This issue has been fixed in current. For 5.9 and 6.0 the following errata
will disable range header processing in httpd(8) to prevent the problem.

Thanks to Pierre Kim  for reporting
the issue.

https://ftp.openbsd.org/pub/OpenBSD/patches/6.0/common/017_httpd.patch.sig

https://ftp.openbsd.org/pub/OpenBSD/patches/5.9/common/034_httpd.patch.sig

err with multiple TLS sites but one OCSP?

[Bob Beck]

Sooo.. 

Pretty sure mlucas has uncovered a problem with the ocsp interface. 

Basically I didn't attach it to the keypair, (yes Joel, I think you
told me so) so it only works with the master keypair.. OK, but the
problem is that it also returns the staple for other keypairs which is
wrong. 

This attaches the ocsp staple to the keypair, rather than the config. 

It does not yet add a way to change it for keypairs other than the
master - that will require an API change - but with this change
it should not return an incorrect ocsp staple for the non primary
keypair. I'll deal with the API change separately after pestering
joel about it a bit.

ok?

Index: tls_config.c
===
RCS file: /cvs/src/lib/libtls/tls_config.c,v
retrieving revision 1.34
diff -u -p -u -p -r1.34 tls_config.c
--- tls_config.c24 Jan 2017 01:48:05 -  1.34
+++ tls_config.c28 Jan 2017 21:40:14 -
@@ -101,6 +101,26 @@ tls_keypair_set_key_mem(struct tls_keypa
return set_mem(>key_mem, >key_len, key, len);
 }
 
+static int
+tls_keypair_set_ocsp_staple_file(struct tls_keypair *keypair,
+struct tls_error *error, const char *ocsp_file)
+{
+   if (keypair->ocsp_staple != NULL)
+   explicit_bzero(keypair->ocsp_staple, keypair->ocsp_staple_len);
+   return tls_config_load_file(error, "ocsp", ocsp_file,
+   >ocsp_staple, >ocsp_staple_len);
+}
+
+static int
+tls_keypair_set_ocsp_staple_mem(struct tls_keypair *keypair,
+const uint8_t *staple, size_t len)
+{
+   if (keypair->ocsp_staple != NULL)
+   explicit_bzero(keypair->ocsp_staple, keypair->ocsp_staple_len);
+   return set_mem(>ocsp_staple, >ocsp_staple_len, staple,
+   len);
+}
+
 static void
 tls_keypair_clear(struct tls_keypair *keypair)
 {
@@ -241,7 +261,6 @@ tls_config_free(struct tls_config *confi
free((char *)config->ca_mem);
free((char *)config->ca_path);
free((char *)config->ciphers);
-   free(config->ocsp_staple);
 
free(config);
 }
@@ -664,14 +683,14 @@ tls_config_verify_client_optional(struct
 int
 tls_config_set_ocsp_staple_file(struct tls_config *config, const char 
*staple_file)
 {
-   return tls_config_load_file(>error, "OCSP", staple_file,
-   >ocsp_staple, >ocsp_staple_len);
+   return tls_keypair_set_ocsp_staple_file(config->keypair, >error,
+   staple_file);
 }
 
 int
 tls_config_set_ocsp_staple_mem(struct tls_config *config, char *staple, size_t 
len)
 {
-   return set_mem(>ocsp_staple, >ocsp_staple_len, staple, 
len);
+   return tls_keypair_set_ocsp_staple_mem(config->keypair, staple, len);
 }
 
 int
Index: tls_internal.h
===
RCS file: /cvs/src/lib/libtls/tls_internal.h,v
retrieving revision 1.52
diff -u -p -u -p -r1.52 tls_internal.h
--- tls_internal.h  26 Jan 2017 12:56:37 -  1.52
+++ tls_internal.h  28 Jan 2017 21:07:25 -
@@ -51,6 +51,8 @@ struct tls_keypair {
size_t cert_len;
char *key_mem;
size_t key_len;
+   char *ocsp_staple;
+   size_t ocsp_staple_len;
 };
 
 #define TLS_MIN_SESSION_TIMEOUT (4)
@@ -83,8 +85,6 @@ struct tls_config {
int ecdhecurve;
struct tls_keypair *keypair;
int ocsp_require_stapling;
-   char *ocsp_staple;
-   size_t ocsp_staple_len;
uint32_t protocols;
unsigned char session_id[TLS_MAX_SESSION_ID_LENGTH];
int session_lifetime;
Index: tls_ocsp.c
===
RCS file: /cvs/src/lib/libtls/tls_ocsp.c,v
retrieving revision 1.10
diff -u -p -u -p -r1.10 tls_ocsp.c
--- tls_ocsp.c  27 Jan 2017 07:03:27 -  1.10
+++ tls_ocsp.c  28 Jan 2017 21:42:22 -
@@ -332,17 +332,19 @@ tls_ocsp_stapling_cb(SSL *ssl, void *arg
if ((ctx = SSL_get_app_data(ssl)) == NULL)
goto err;
 
-   if (ctx->config->ocsp_staple == NULL ||
-   ctx->config->ocsp_staple_len == 0)
+   if (ctx->config->keypair->ocsp_staple == NULL ||
+   ctx->config->keypair->ocsp_staple == NULL ||
+   ctx->config->keypair->ocsp_staple_len == 0)
return SSL_TLSEXT_ERR_NOACK;
 
-   if ((ocsp_staple = malloc(ctx->config->ocsp_staple_len)) == NULL)
+   if ((ocsp_staple = malloc(ctx->config->keypair->ocsp_staple_len)) ==
+   NULL)
goto err;
 
-   memcpy(ocsp_staple, ctx->config->ocsp_staple,
-   ctx->config->ocsp_staple_len);
+   memcpy(ocsp_staple, ctx->config->keypair->ocsp_staple,
+   ctx->config->keypair->ocsp_staple_len);
if (SSL_set_tlsext_status_ocsp_resp(ctx->ssl_conn, ocsp_staple,
-   ctx->config->ocsp_staple_len) != 1)
+   ctx->config->keypair->ocsp_staple_len) != 1)
goto err;
 
ret = SSL_TLSEXT_ERR_OK;

Re: err with multiple TLS sites but one OCSP?

[Bob Beck]

On Fri, Jan 27, 2017 at 15:23 Stuart Henderson <s...@spacehopper.org> wrote:

> On 2017/01/27 22:09, Bob Beck wrote:
>
> > I think you have more issues than ocsp. if thats the same host you can't
>
> > have two different tls certs on the same ip.   and you have them both on
>
> > *443
>
> >
>
> > try using a separate ip for each
>
>
>
> Wasn't SNI support added to httpd already?
>
> hmmm. right. but i bet itll work with explicit separate ip's.  stapling on
> the other hand will be per config. so thats probably whats fighting.
> separate ip would confirm that.


> im tired. ill look at it tomorrow unless someone else does
>
>
>

Re: err with multiple TLS sites but one OCSP?

[Bob Beck]

I think you have more issues than ocsp. if thats the same host you can't
have two different tls certs on the same ip.   and you have them both on
*443

try using a separate ip for each



On Fri, Jan 27, 2017 at 15:03 Michael W. Lucas <mwlu...@michaelwlucas.com>
wrote:

> On Fri, Jan 27, 2017 at 09:53:25PM +0000, Bob Beck wrote:
>
> >On Fri, Jan 27, 2017 at 14:12 Michael W. Lucas
>
> >  Or a misconfiguration. Â show configs
>
>
>
>
>
> Configs follow.
>
>
>
> # cat /etc/httpd.conf
>
> include "/etc/sites/www3.conf"
>
> include "/etc/sites/www4.conf"
>
>
>
> www3.conf:
>
>
>
> server "www3.mwlucas.org" {
>
>listen on * port 80
>
>block return 302 "https://$SERVER_NAME$REQUEST_URI;
>
> }
>
>
>
>
>
> server "www3.mwlucas.org" {
>
> alias tarpit.mwlucas.org
>
> listen on * tls port 443
>
> hsts
>
> # TLS certificate and key files created with acme-client(1)
>
> tls certificate "/etc/ssl/acme/www3/www3.fullchain.pem"
>
> tls key "/etc/ssl/acme/www3/www3.key"
>
> tls ocsp "/etc/ssl/acme/www3/www3.der"
>
> tcp nodelay
>
>
>
>location "/.well-known/acme-challenge/*" {
>
>root "/acme"
>
>root strip 2
>
>}
>
> }
>
>
>
>
>
> www4:
>
>
>
> server "www4.mwlucas.org" {
>
> alias bill.mwlucas.org
>
> alias auction.mwlucas.org
>
> listen on * port 80
>
>
>
>location "/.well-known/acme-challenge/*" {
>
>root "/acme"
>
>root strip 2
>
>}
>
>
>
>
>
> block return 301 "https://$DOCUMENT_URI;
>
> }
>
>
>
> server "www4.mwlucas.org" {
>
> alias bill.mwlucas.org
>
> alias auction.mwlucas.org
>
> root "/www4"
>
> listen on * tls port 443
>
> hsts
>
> # TLS certificate and key files created with acme-client(1)
>
> tls certificate "/etc/ssl/acme/www4/www4.fullchain.pem"
>
> tls key "/etc/ssl/acme/www4/www4.key"
>
> #   tls ocsp "/etc/ssl/acme/www4/www4.der"
>
> tcp nodelay
>
>location "/.well-known/acme-challenge/*" {
>
>root "/acme"
>
>root strip 2
>
>}
>
>
>
> }
>
>
>
>
>
>
>
>
>
> --
>
> Michael W. LucasTwitter @mwlauthor
>
> nonfiction: https://www.michaelwlucas.com/
>
> fiction: https://www.michaelwarrenlucas.com/
>
> blog: http://blather.michaelwlucas.com/
>
>

Re: err with multiple TLS sites but one OCSP?

[Bob Beck]

On Fri, Jan 27, 2017 at 14:12 Michael W. Lucas 
wrote:

> On Fri, Jan 27, 2017 at 02:50:29PM -0500, Michael W. Lucas wrote:
>
> > On Fri, Jan 27, 2017 at 06:49:06PM +, Stuart Henderson wrote:
>
> > > That looks like a web server bug, it shouldn't return a staple
>
>
> Or a misconfiguration.  show configs
>
>
> > > in that case.  What software are you using for that?
>
> >
>
> > 
>
> >
>
> > OpenBSD httpd, of course. amd64 snapshot downloaded yesterday from
>
> > ftp3.usa.openbsd.org.
>
>
>
> To be clear, that's a "How the hell could I forget to include that?"
>
> facepalm, not anything about Stuart asking the question...
>
>
>
> --
>
> Michael W. LucasTwitter @mwlauthor
>
> nonfiction: https://www.michaelwlucas.com/
>
> fiction: https://www.michaelwarrenlucas.com/
>
> blog: http://blather.michaelwlucas.com/
>
>
>
>

Re: Allow install from https server w/ self signed cert

[Bob Beck]

On Sat, Jan 07, 2017 at 03:52:04PM -0700, Theo de Raadt wrote:
> > What workarounds would be reasonable and approriate? and does it
> > make sense for OpenBSD to support such scenarios out-of-the-box to
> > promote wider adoption of better software?
> 
> If you want buy the OpenBSD-installer-for-drones, contact me offline.
> That featureset didn't make it into the free software version.


But out GeoLocation shit in the installer will just find that it's
located on a netblock that is registered 5000 feet over Pakistan and
direct it to a local public mirror!  Definately have to disable
that feature from the free software version.

Re: Allow install from https server w/ self signed cert

[Bob Beck]

On Sat, Jan 07, 2017 at 05:42:24PM -0500, Jacob L. Leifman wrote:

> Most of the time I agree with this particular attitude and it is indeed 
> appropriate for the OP case. However, there some major networks such as 
> various governments (or for example .mil) that do not participate in 
> the public PKI but run their own PKI infrastructure. What effect will 
> the installer's checks have in that environment? What workarounds would 
> be reasonable and approriate? and does it make sense for OpenBSD to 
> support such scenarios out-of-the-box to promote wider adoption of 
> better software?

That's not a good reason, since in the case of what I am describing they
would *still be depending on the public PKI infrastrucure* to publish
their own list of signers..  No.. they aren't going to do that.. Sorry, 
Unless you're mailing me from a .mil address I think you might
be imagining this usage case. 

they're probably building from source, or have the wherewithall to roll
their own bsd.rd if they care about doing this. 

Re: Allow install from https server w/ self signed cert

[Bob Beck]

On Fri, Jan 06, 2017 at 10:48:37AM -0500, RD Thrush wrote:
> On 01/06/17 06:28, Stuart Henderson wrote:
> > Related to this (and particularly thinking about autoinstalls),
> > would it make sense to allow explicit protocols in the hostname?
> > 
> > some.host -> https with http fallback
> > http://some.host/ -> http only
> > https://some.host/ -> https only, no fallback
> 
> That would totally work for my install problem.
> 
> FWIW, instead of running a patched install.sub, "rm /etc/ssl/cert.pem" makes 
> the install bypass the https attempt.
> 

Note, if you're upgrading or otherwise have a way to et a cert.pem bundle onto 
there to *replace*
the default, you could always drop the signer for your private self-signed 
server into the cert.pem
bundle, at which point it would be accepted as trusted. 

of course if you're just installing you have an interesting chicken and egg 
problem, unless
you put it somewhere on an https site that does have a real certificate, drop 
out of the
installer and do

ftp -o /tmp/mysigner.pem https://my.secure.site/mysigner.pem
cat /tmp/mysigner.pem >> /etc/ssl/cert.pem

then continue the install, and you're good. 

Almost wonder if it's worth an extra question in the installer to ask
for an https address to retrieve a certficiate bundle to be appended to cert.pem
for the install...

Re: acme-client use configuration file [1 of 5]

[Bob Beck]

No objection in principle.. although since some of us depend on this we
might either need warning and/or a small period of overlap where the old
stuff works and then we can move to the new stuff without things blowing
up.

On Sun, Jan 1, 2017 at 1:59 PM, Sebastian Benoit  wrote:

> start using the configuration file and delete command line arguments:
>
> -a agreement-> agreement url ...
> -c certdir  -> domain certificate "path"
> -f accountkey   -> account key "path"
> -k domainkey-> domain key "path"
> -s authority-> sign with "name"
>
> new argument:
> -f configfile
>
> the changes needed to use the new configuration are local to main.c for
> now.
> While the configuration could be passed directly to netproc(), keyproc()
> etc,
> the diff is smaller this way.
>
> This also removes the multidir (-m) mode for now - specify different paths
> in
> each domain {} block instead.
>
> diff --git usr.sbin/acme-client/Makefile usr.sbin/acme-client/Makefile
> index 55e0b0e..eae13ed 100644
> --- usr.sbin/acme-client/Makefile
> +++ usr.sbin/acme-client/Makefile
> @@ -13,6 +13,6 @@ CFLAGS+=  -W -Wall -I${.CURDIR}
>  CFLAGS+=   -Wstrict-prototypes -Wmissing-prototypes
>  CFLAGS+=   -Wmissing-declarations
>  CFLAGS+=   -Wshadow -Wpointer-arith
> -CFLAGS+=   -Wsign-compare
> +CFLAGS+=   -Wsign-compare -Wunused
>
>  .include 
> diff --git usr.sbin/acme-client/acme-client.1 usr.sbin/acme-client/acme-
> client.1
> index 526c11f..6f38573 100644
> --- usr.sbin/acme-client/acme-client.1
> +++ usr.sbin/acme-client/acme-client.1
> @@ -22,15 +22,10 @@
>  .Nd ACME client
>  .Sh SYNOPSIS
>  .Nm acme-client
> -.Op Fl bFmNnrv
> -.Op Fl a Ar agreement
> +.Op Fl bFNnrv
>  .Op Fl C Ar challengedir
> -.Op Fl c Ar certdir
> -.Op Fl f Ar accountkey
> -.Op Fl k Ar domainkey
> -.Op Fl s Ar authority
> +.Op Fl f Ar configfile
>  .Ar domain
> -.Op Ar altnames
>  .Sh DESCRIPTION
>  The
>  .Nm
> @@ -39,8 +34,6 @@ Automatic Certificate Management Environment (ACME)
> client.
>  .Pp
>  The options are as follows:
>  .Bl -tag -width Ds
> -.It Fl a Ar agreement
> -Use an alternative user agreement URL.
>  .It Fl b
>  Back up all certificates in the certificate directory.
>  This only happens if a remove or replace operation is possible.
> @@ -58,67 +51,21 @@ Any given backup uses the same Epoch time for all
> three certificates.
>  If there are no certificates in place, this option does nothing.
>  .It Fl C Ar challengedir
>  The directory to register challenges.
> -.It Fl c Ar certdir
> -The directory to store public certificates.
>  .It Fl F
>  Force updating the certificate signature even if it's too soon.
> -.It Fl f Ar accountkey
> -The account private key.
> -This was either made with a previous client or with
> -.Fl n .
> -.It Fl k Ar domainkey
> -The private key for the domain.
> -This may also be created with
> -.Fl N .
> -.It Fl m
> -Append
> -.Ar domain
> -to all default paths except the challenge path
> -.Pq i.e. those that are overridden by Fl c , k , f .
> -Thus,
> -.Ar foo.com
> -as the initial domain would make the default domain private key into
> -.Pa /etc/ssl/acme/private/foo.com/privkey.pem .
> -This is useful in setups with multiple domain sets.
> +.It Fl f Ar configfile
> +Specify an alternative configuration file.
>  .It Fl N
>  Create a new RSA domain key if one does not already exist.
>  .It Fl n
>  Create a new RSA account key if one does not already exist.
>  .It Fl r
>  Revoke the X509 certificate found in the certificates.
> -.It Fl s Ar authority
> -ACME
> -.Ar authority
> -to talk to.
> -Currently the following authorities are available:
> -.Pp
> -.Bl -tag -width "letsencrypt-staging" -compact
> -.It Cm letsencrypt
> -Let's Encrypt authority
> -.It Cm letsencrypt-staging
> -Let's Encrypt staging authority
> -.El
> -.Pp
> -The default is
> -.Cm letsencrypt .
>  .It Fl v
>  Verbose operation.
>  Specify twice to also trace communication and data transfers.
>  .It Ar domain
>  The domain name.
> -The only difference between this and
> -.Ar altnames
> -is that it's put into the certificate's
> -.Li CN
> -field and it uses the
> -.Qq main
> -domain when specifying
> -.Fl m .
> -.It Ar altnames
> -Alternative names
> -.Pq Dq SAN
> -for the domain name.
> -The number of SAN entries is limited to 100 or so.
>  .El
>  .Pp
>  Public certificates are by default placed in
> @@ -175,7 +122,7 @@ as in the
>  .Sx Challenges
>  section:
>  .Pp
> -.Dl # acme-client -vNn foo.com www.foo.com smtp.foo.com
> +.Dl # acme-client -vNn www.foo.com
>  .Pp
>  A daily
>  .Xr cron 8
> @@ -183,7 +130,7 @@ job can renew the certificates:
>  .Bd -literal -offset indent
>  #! /bin/sh
>
> -acme-client foo.com www.foo.com smtp.foo.com
> +acme-client www.foo.com
>
>  if [ $? -eq 0 ]
>  then
> diff --git usr.sbin/acme-client/chngproc.c usr.sbin/acme-client/chngproc.c
> index 4cb7f33..3e931da 100644
> --- usr.sbin/acme-client/chngproc.c
> +++ usr.sbin/acme-client/chngproc.c
> @@ -27,7 +27,7 @@
>  

Re: libtls syslogd pledge abort

[Bob Beck]

> Or do not call tls_configure_ssl_verify() if verification is turned
> off.

This makes sense to me. 

> 
> Index: lib/libtls/tls_client.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/lib/libtls/tls_client.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 tls_client.c
> --- lib/libtls/tls_client.c   26 Dec 2016 16:20:58 -  1.38
> +++ lib/libtls/tls_client.c   29 Dec 2016 22:56:23 -
> @@ -195,7 +195,9 @@ tls_connect_common(struct tls *ctx, cons
>   }
>   }
>  
> - if (tls_configure_ssl_verify(ctx, ctx->ssl_ctx, SSL_VERIFY_PEER) == -1)
> + if (ctx->config->verify_cert &&
> + (tls_configure_ssl_verify(ctx, ctx->ssl_ctx,
> +  SSL_VERIFY_PEER) == -1))
>   goto err;
>  
>   if (SSL_CTX_set_tlsext_status_cb(ctx->ssl_ctx, tls_ocsp_verify_cb) != 
> 1) {
> 

ok beck@

> I would prefer the fix in libtls as
> - this problem may also affect other daemons
> - avoid to do unnecsessary stuff
> - syslogd could run on a system without cert.pem
> 
> comments? ok?
> 
> bluhm

Re: httpd(8)/proc.c: use less fds on startup

[Bob Beck]

This is now working on www.openbsd.org.  I upgraded my 
6.0 system to current today off the latest snap and httpd would
not start, same problem. 

This diff lets current httpd start again. 

ok beck@


On Tue, Oct 04, 2016 at 11:54:37PM +0200, Rafael Zalamena wrote:
> On Tue, Oct 04, 2016 at 07:46:52PM +0200, Rafael Zalamena wrote:
> > This diff makes proc.c daemons to use less file descriptors on startup,
> > this way we increase the number of child we can have considerably. This
> > also improves the solution on a bug reported in bugs@
> > "httpd errors out with 'too many open files'". 
> > 
> > To achieve that I delayed the socket distribution and made a minimal
> > socket allocation in proc_init(), so only the necessary children socket
> > are allocated and passed with proc_exec(). After the event_init() is called
> > we call proc_connect() which creates the socketpair() and immediatly after
> > each call we already sends them without accumulating.
> > 
> > Note: We still have to calculate how many fds we will want to have and
> >   then limit the daemon prefork configuration.
> > 
> > ok?
> > 
> 
> Paul de Weerd still found problems with the diff, because the httpd(8)
> would not exit successfully with '-n' flag. It happened because the
> new proc_connect() code tried to write fds to children process that
> did not start caused by the ps_noaction flag. (thanks Paul!)
> 
> This new diff just adds a check for ps_noaction in proc_init() and
> proc_connect() so it doesn't try to do anything if we are not really
> going to start the daemon. Also I removed the ps_noaction from proc_run()
> since we are not going to get to this point anymore.
> 
> ok?
> 
> 
> Index: proc.c
> ===
> RCS file: /home/obsdcvs/src/usr.sbin/httpd/proc.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 proc.c
> --- proc.c28 Sep 2016 12:01:04 -  1.27
> +++ proc.c4 Oct 2016 21:50:43 -
> @@ -36,8 +36,6 @@
>  
>  void  proc_exec(struct privsep *, struct privsep_proc *, unsigned int,
>   int, char **);
> -void  proc_connectpeer(struct privsep *, enum privsep_procid, int,
> - struct privsep_pipes *);
>  void  proc_setup(struct privsep *, struct privsep_proc *, unsigned int);
>  void  proc_open(struct privsep *, int, int);
>  void  proc_accept(struct privsep *, int, enum privsep_procid,
> @@ -147,72 +145,18 @@ proc_exec(struct privsep *ps, struct pri
>  }
>  
>  void
> -proc_connectpeer(struct privsep *ps, enum privsep_procid id, int inst,
> -struct privsep_pipes *pp)
> -{
> - unsigned int i, j;
> - struct privsep_fdpf;
> -
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Parent is already connected with everyone. */
> - if (i == PROC_PARENT)
> - continue;
> -
> - for (j = 0; j < ps->ps_instances[i]; j++) {
> - /* Don't send socket to child itself. */
> - if (i == (unsigned int)id &&
> - j == (unsigned int)inst)
> - continue;
> - if (pp->pp_pipes[i][j] == -1)
> - continue;
> -
> - pf.pf_procid = i;
> - pf.pf_instance = j;
> - proc_compose_imsg(ps, id, inst, IMSG_CTL_PROCFD,
> - -1, pp->pp_pipes[i][j], , sizeof(pf));
> - pp->pp_pipes[i][j] = -1;
> - }
> - }
> -}
> -
> -/* Inter-connect all process except with ourself. */
> -void
>  proc_connect(struct privsep *ps)
>  {
> - unsigned int src, i, j;
> - struct privsep_pipes*pp;
> - struct imsgev   *iev;
> -
> - /* Listen on appropriate pipes. */
> - src = privsep_process;
> - pp = >ps_pipes[src][ps->ps_instance];
> -
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Don't listen to ourself. */
> - if (i == src)
> - continue;
> -
> - for (j = 0; j < ps->ps_instances[i]; j++) {
> - if (pp->pp_pipes[i][j] == -1)
> - continue;
> -
> - iev = >ps_ievs[i][j];
> - imsg_init(>ibuf, pp->pp_pipes[i][j]);
> - event_set(>ev, iev->ibuf.fd, iev->events,
> - iev->handler, iev->data);
> - event_add(>ev, NULL);
> - }
> - }
> + unsigned int src, dst;
>  
> - /* Exchange pipes between process. */
> - for (i = 0; i < PROC_MAX; i++) {
> - /* Parent is already connected with everyone. */
> - if (i == PROC_PARENT)
> - continue;
> + /* Don't distribute any sockets if we are not really going to run. */
> + if (ps->ps_noaction)
> + return;
>  
> - for (j = 0; j < ps->ps_instances[i]; j++)
> - 

Re: rebound quantum entanglement

[Bob Beck]

BTW I'm not picking on you.. my DNS setup blew up this week for local
resolution and I've been dealing with the fallout - so the topic
is relatively near and dear to my heart.

On Wed, Sep 14, 2016 at 10:07 PM, Bob Beck <b...@obtuse.com> wrote:

>
> Yep.  and now you need to solve the problem that when prepending
> 127.0.0.1, and hitting rebound, which in turn is going to only grab the
> first dns server from my resolv.conf instead of all of them, that it now
> doubles my failure time when the first dns server doesn't respond (once for
> libc asking rebound, which asks only the first server it found, which
> fails) then falls back to libc asking resolv.conf which again... asks the
> first server, and fails again.
>
> So the problem here is that it's going to be great when things are working
> but become a failure multiplier when something breaks.
>
> Of course - perhaps you could query them all in parallel to mitigate this?
>   Rebound might need to become a real boy if we're going to do something
> like this.  If rebound were a "real boy" it would be it easier to say "just
> use rebound or if it's not there just use resolv.conf normally
>
> then nothing changes at *all* when it's not there.
>
>
> On Wed, Sep 14, 2016 at 8:39 PM, Ted Unangst <t...@tedunangst.com> wrote:
>
>> Ted Unangst wrote:
>> > Bob Beck wrote:
>> > > how is rebound going to handle a change in resolv.conf? thats still a
>> > > problem here
>> >
>> > oh, that's easy. it watches the file for changes. i never quite got
>> around to
>> > that, but it's another five lines.
>>
>> ok, so it's a net +15 lines, including blanks.
>>
>> Index: rebound.8
>> ===
>> RCS file: /cvs/src/usr.sbin/rebound/rebound.8,v
>> retrieving revision 1.4
>> diff -u -p -r1.4 rebound.8
>> --- rebound.8   4 Dec 2015 04:50:43 -   1.4
>> +++ rebound.8   15 Sep 2016 00:57:21 -
>> @@ -33,9 +33,7 @@ The options are as follows:
>>  .Bl -tag -width Ds
>>  .It Fl c Ar config
>>  Specify an alternative configuration file, instead of the default
>> -.Pa /etc/rebound.conf .
>> -At present, the config file consists of a single line containing the next
>> -hop DNS server.
>> +.Pa /etc/resolv.conf .
>>  .Nm
>>  will reload the configuration file when sent a SIGHUP signal.
>>  .It Fl d
>> @@ -46,8 +44,8 @@ does not
>>  into the background.
>>  .El
>>  .Sh FILES
>> -.Bl -tag -width "/etc/rebound.confXX" -compact
>> -.It Pa /etc/rebound.conf
>> +.Bl -tag -width "/etc/resolv.confXX" -compact
>> +.It Pa /etc/resolv.conf
>>  Default
>>  .Nm
>>  configuration file.
>> Index: rebound.c
>> ===
>> RCS file: /cvs/src/usr.sbin/rebound/rebound.c,v
>> retrieving revision 1.70
>> diff -u -p -r1.70 rebound.c
>> --- rebound.c   1 Sep 2016 10:57:24 -   1.70
>> +++ rebound.c   15 Sep 2016 02:30:46 -
>> @@ -33,10 +33,12 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #define MINIMUM(a,b) (((a)<(b))?(a):(b))
>>
>> @@ -455,34 +457,51 @@ fail:
>>  }
>>
>>  static int
>> -readconfig(FILE *conf, union sockun *remoteaddr)
>> +readconfig(int conffd, union sockun *remoteaddr)
>>  {
>> +   const char ns[] = "nameserver";
>> char buf[1024];
>> +   char *p;
>> struct sockaddr_in *sin = >i;
>> struct sockaddr_in6 *sin6 = >i6;
>> +   FILE *conf;
>> +   int rv = -1;
>>
>> -   if (fgets(buf, sizeof(buf), conf) == NULL)
>> -   return -1;
>> -   buf[strcspn(buf, "\n")] = '\0';
>> +   conf = fdopen(conffd, "r");
>>
>> -   memset(remoteaddr, 0, sizeof(*remoteaddr));
>> -   if (inet_pton(AF_INET, buf, >sin_addr) == 1) {
>> -   sin->sin_len = sizeof(*sin);
>> -   sin->sin_family = AF_INET;
>> -   sin->sin_port = htons(53);
>> -   return AF_INET;
>> -   } else if (inet_pton(AF_INET6, buf, >sin6_addr) == 1) {
>> -   sin6->sin6_len = sizeof(*sin6);
>> -   sin6->sin6_family = AF_INET6;
>> -   sin6->sin6_port = htons(53);
>> -   return AF_INET6;
>> -   } else {
&

Re: rebound quantum entanglement

[Bob Beck]

Yep.  and now you need to solve the problem that when prepending 127.0.0.1,
and hitting rebound, which in turn is going to only grab the first dns
server from my resolv.conf instead of all of them, that it now doubles my
failure time when the first dns server doesn't respond (once for libc
asking rebound, which asks only the first server it found, which fails)
then falls back to libc asking resolv.conf which again... asks the first
server, and fails again.

So the problem here is that it's going to be great when things are working
but become a failure multiplier when something breaks.

Of course - perhaps you could query them all in parallel to mitigate this?
  Rebound might need to become a real boy if we're going to do something
like this.  If rebound were a "real boy" it would be it easier to say "just
use rebound or if it's not there just use resolv.conf normally

then nothing changes at *all* when it's not there.


On Wed, Sep 14, 2016 at 8:39 PM, Ted Unangst <t...@tedunangst.com> wrote:

> Ted Unangst wrote:
> > Bob Beck wrote:
> > > how is rebound going to handle a change in resolv.conf? thats still a
> > > problem here
> >
> > oh, that's easy. it watches the file for changes. i never quite got
> around to
> > that, but it's another five lines.
>
> ok, so it's a net +15 lines, including blanks.
>
> Index: rebound.8
> ===
> RCS file: /cvs/src/usr.sbin/rebound/rebound.8,v
> retrieving revision 1.4
> diff -u -p -r1.4 rebound.8
> --- rebound.8   4 Dec 2015 04:50:43 -   1.4
> +++ rebound.8   15 Sep 2016 00:57:21 -
> @@ -33,9 +33,7 @@ The options are as follows:
>  .Bl -tag -width Ds
>  .It Fl c Ar config
>  Specify an alternative configuration file, instead of the default
> -.Pa /etc/rebound.conf .
> -At present, the config file consists of a single line containing the next
> -hop DNS server.
> +.Pa /etc/resolv.conf .
>  .Nm
>  will reload the configuration file when sent a SIGHUP signal.
>  .It Fl d
> @@ -46,8 +44,8 @@ does not
>  into the background.
>  .El
>  .Sh FILES
> -.Bl -tag -width "/etc/rebound.confXX" -compact
> -.It Pa /etc/rebound.conf
> +.Bl -tag -width "/etc/resolv.confXX" -compact
> +.It Pa /etc/resolv.conf
>  Default
>  .Nm
>  configuration file.
> Index: rebound.c
> ===
> RCS file: /cvs/src/usr.sbin/rebound/rebound.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 rebound.c
> --- rebound.c   1 Sep 2016 10:57:24 -   1.70
> +++ rebound.c   15 Sep 2016 02:30:46 -
> @@ -33,10 +33,12 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #define MINIMUM(a,b) (((a)<(b))?(a):(b))
>
> @@ -455,34 +457,51 @@ fail:
>  }
>
>  static int
> -readconfig(FILE *conf, union sockun *remoteaddr)
> +readconfig(int conffd, union sockun *remoteaddr)
>  {
> +   const char ns[] = "nameserver";
> char buf[1024];
> +   char *p;
> struct sockaddr_in *sin = >i;
> struct sockaddr_in6 *sin6 = >i6;
> +   FILE *conf;
> +   int rv = -1;
>
> -   if (fgets(buf, sizeof(buf), conf) == NULL)
> -   return -1;
> -   buf[strcspn(buf, "\n")] = '\0';
> +   conf = fdopen(conffd, "r");
>
> -   memset(remoteaddr, 0, sizeof(*remoteaddr));
> -   if (inet_pton(AF_INET, buf, >sin_addr) == 1) {
> -   sin->sin_len = sizeof(*sin);
> -   sin->sin_family = AF_INET;
> -   sin->sin_port = htons(53);
> -   return AF_INET;
> -   } else if (inet_pton(AF_INET6, buf, >sin6_addr) == 1) {
> -   sin6->sin6_len = sizeof(*sin6);
> -   sin6->sin6_family = AF_INET6;
> -   sin6->sin6_port = htons(53);
> -   return AF_INET6;
> -   } else {
> -   return -1;
> +   while (fgets(buf, sizeof(buf), conf) != NULL) {
> +   buf[strcspn(buf, "\n")] = '\0';
> +
> +   if (strncmp(buf, ns, strlen(ns)) != 0)
> +   continue;
> +   p = buf + strlen(ns) + 1;
> +   while (isspace((unsigned char)*p))
> +   p++;
> +
> +   /* this will not end well */
> +   if (strcmp(p, "127.0.0.1") == 0)
> +   continue;
> +
> +   memset(remoteaddr, 0, sizeof(*remoteaddr));
> +   if (inet_pton(AF_INET, p, >sin_addr) == 1) {
> +

Re: rebound quantum entanglement

[Bob Beck]

wont this also mean if it is not running i have to wait for the localhost
attempt to fail before the resolver moves on? (ASR_STATE_NEXT_NS, etc) so i
slow everything down for a timeout?

dont get me wrong, it is an interesting direction, but I think maybe get
the rest of the five line changes into rebound to make it useful and then
look at libc which might need slightly more cleverness than just adding
localhost unconditionally.

On Wednesday, 14 September 2016, Ted Unangst <t...@tedunangst.com> wrote:

> Bob Beck wrote:
> > how is rebound going to handle a change in resolv.conf? thats still a
> > problem here
>
> oh, that's easy. it watches the file for changes. i never quite got around
> to
> that, but it's another five lines.
>

Re: rebound quantum entanglement

[Bob Beck]

how is rebound going to handle a change in resolv.conf? thats still a
problem here

On Wednesday, 14 September 2016, Ted Unangst  wrote:

> So the plan is for rebound to be the 'system' resolver, with libc talking
> to
> rbeound and rebound talking to the cloud. The main wrinkle is how does
> rebound
> find the cloud? rebound.conf, but dhclient doesn't know anything about
> rebound.conf, preferring to edit resolv.conf. But if rebound reads
> resolv.conf, what does libc read? This has been a bit of a tangle until
> now,
> especially in scenarios like upgrades where rebound may not even be
> running.
>
> And so I present the following diff to enable a smooth transition. It's
> 'quantum' because it works whether or not rebound is running. No need to
> open
> the box.
>
> 1. rebound reads resolv.conf. This remains the config file for upstream
> DNS.
>
> 2. libc now prepends its nameserver list with localhost, thus always
> searching
> for rebound. If it's not running, we just continue down the list.
>
> This covers the basic use case, where enabling rebound now requires no
> additional work. No need to edit dhclient.conf, etc. It also works on
> ramdisks. It also works with a mix of old and new binaries. Once you flip
> resolv.conf back to upstream, old binaries will bypass rebound, but that's
> ok.
> The new rebound checks to make sure it's not stuck in a time loop, which is
> never good.
>
> I also note this improves the situation for people who have been using
> unbound
> as a local cache, too. Just enable unbound and libc will use it
> automatically.
>
> Particular edge case: if resolv.conf has no nameservers, then the localhost
> default is not prepended. So libc won't try talking to rebound if it's
> specifically configured not to (chroot).
>
>
> Index: lib/libc/asr/asr.c
> ===
> RCS file: /cvs/src/lib/libc/asr/asr.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 asr.c
> --- lib/libc/asr/asr.c  18 Jun 2016 15:25:28 -  1.54
> +++ lib/libc/asr/asr.c  15 Sep 2016 00:42:30 -
> @@ -549,6 +549,15 @@ pass0(char **tok, int n, struct asr_ctx
> return;
> if (n != 2)
> return;
> +   /* prepend localhost to list */
> +   if (ac->ac_nscount == 0) {
> +   if (asr_parse_nameserver((struct sockaddr *),
> "127.0.0.1"))
> +   return;
> +   if ((ac->ac_ns[ac->ac_nscount] = calloc(1,
> ss.ss_len)) == NULL)
> +   return;
> +   memmove(ac->ac_ns[ac->ac_nscount], ,
> ss.ss_len);
> +   ac->ac_nscount += 1;
> +   }
> if (asr_parse_nameserver((struct sockaddr *), tok[1]))
> return;
> if ((ac->ac_ns[ac->ac_nscount] = calloc(1, ss.ss_len)) ==
> NULL)
> Index: usr.sbin/rebound/rebound.8
> ===
> RCS file: /cvs/src/usr.sbin/rebound/rebound.8,v
> retrieving revision 1.4
> diff -u -p -r1.4 rebound.8
> --- usr.sbin/rebound/rebound.8  4 Dec 2015 04:50:43 -   1.4
> +++ usr.sbin/rebound/rebound.8  15 Sep 2016 00:57:21 -
> @@ -33,9 +33,7 @@ The options are as follows:
>  .Bl -tag -width Ds
>  .It Fl c Ar config
>  Specify an alternative configuration file, instead of the default
> -.Pa /etc/rebound.conf .
> -At present, the config file consists of a single line containing the next
> -hop DNS server.
> +.Pa /etc/resolv.conf .
>  .Nm
>  will reload the configuration file when sent a SIGHUP signal.
>  .It Fl d
> @@ -46,8 +44,8 @@ does not
>  into the background.
>  .El
>  .Sh FILES
> -.Bl -tag -width "/etc/rebound.confXX" -compact
> -.It Pa /etc/rebound.conf
> +.Bl -tag -width "/etc/resolv.confXX" -compact
> +.It Pa /etc/resolv.conf
>  Default
>  .Nm
>  configuration file.
> Index: usr.sbin/rebound/rebound.c
> ===
> RCS file: /cvs/src/usr.sbin/rebound/rebound.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 rebound.c
> --- usr.sbin/rebound/rebound.c  1 Sep 2016 10:57:24 -   1.70
> +++ usr.sbin/rebound/rebound.c  15 Sep 2016 00:53:26 -
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #define MINIMUM(a,b) (((a)<(b))?(a):(b))
>
> @@ -457,28 +458,41 @@ fail:
>  static int
>  readconfig(FILE *conf, union sockun *remoteaddr)
>  {
> +   const char ns[] = "nameserver";
> char buf[1024];
> +   char *p;
> struct sockaddr_in *sin = >i;
> struct sockaddr_in6 *sin6 = >i6;
>
> -   if (fgets(buf, sizeof(buf), conf) == NULL)
> -   return -1;
> -   buf[strcspn(buf, "\n")] = '\0';
> +   while (fgets(buf, sizeof(buf), conf) != NULL) {
> +   buf[strcspn(buf, "\n")] = '\0';
>
> -   memset(remoteaddr, 0, sizeof(*remoteaddr));
> -

Re: reduce double caching in mfs

[Bob Beck]

I really dislike "CHEAP".

and it almost seems like these should actually be NOCACHE.. why the heck
can't they be?


On Thu, Sep 8, 2016 at 7:49 PM, Ted Unangst  wrote:

> Currently, the bufcache doesn't know that mfs is backed by memory. All i/o
> to
> mfs ends up being double cached, once in the userland process and again in
> the
> kernel bufcache. This is wasteful. In particular, it means one can't use
> mfs
> to increase the effective size of the buffer cache. Reading or writing to
> mfs
> will push out buffers used for disk caching. (I think you can even end up
> with
> triple buffering when mfs starts swapping...)
>
> This is mostly inherent to the design of mfs. But with a small tweak to the
> buffer cache, we can improve the situation. This introduces the concept of
> 'cheap' buffers, a hint to the buffer cache that they are easily replaced.
> (There's a 'nocache' flag already, but it's not suitable here.) When mfs
> finishes with a buf, it marks it cheap. Then it goes onto a special queue
> that
> gets chewed up before we start looking at the regular caches. We still
> cache
> some number of cheap buffers to prevent constant memory copying.
>
> With this diff, I've confirmed that reading/writing large files to mfs
> doesn't
> flush the cache, but performance appears about the same. (Of particular
> note,
> my bufcache is big enough to cache all of src/, but not src/ and obj/. With
> obj/ on mfs, src never gets flushed.)
>
>
> Index: kern/vfs_bio.c
> ===
> RCS file: /cvs/src/sys/kern/vfs_bio.c,v
> retrieving revision 1.176
> diff -u -p -r1.176 vfs_bio.c
> --- kern/vfs_bio.c  4 Sep 2016 10:51:24 -   1.176
> +++ kern/vfs_bio.c  8 Sep 2016 18:31:52 -
> @@ -93,7 +93,10 @@ int bd_req;  /* Sleep point for cleaner
>
>  #define NUM_CACHES 2
>  #define DMA_CACHE 0
> +#define CHEAP_LIMIT 256
>  struct bufcache cleancache[NUM_CACHES];
> +struct bufqueue cheapqueue;
> +u_int cheapqueuelen;
>  struct bufqueue dirtyqueue;
>
>  void
> @@ -1297,6 +1300,7 @@ bufcache_init(void)
> TAILQ_INIT([i].coldqueue);
> TAILQ_INIT([i].warmqueue);
> }
> +   TAILQ_INIT();
> TAILQ_INIT();
>  }
>
> @@ -1329,6 +1333,12 @@ bufcache_getcleanbuf(int cachenum, int d
>
> splassert(IPL_BIO);
>
> +   /* try cheap queue if over limit */
> +   if (discard || cheapqueuelen > CHEAP_LIMIT) {
> +   if ((bp = TAILQ_FIRST()))
> +   return bp;
> +   }
> +
> /* try  cold queue */
> while ((bp = TAILQ_FIRST(>coldqueue))) {
> if ((!discard) &&
> @@ -1356,6 +1366,8 @@ bufcache_getcleanbuf(int cachenum, int d
> /* buffer is cold - give it up */
> return bp;
> }
> +   if ((bp = TAILQ_FIRST()))
> +   return bp;
> if ((bp = TAILQ_FIRST(>warmqueue)))
> return bp;
> if ((bp = TAILQ_FIRST(>hotqueue)))
> @@ -1410,6 +1422,13 @@ bufcache_take(struct buf *bp)
> pages = atop(bp->b_bufsize);
> struct bufcache *cache = [bp->cache];
> if (!ISSET(bp->b_flags, B_DELWRI)) {
> +   if (ISSET(bp->b_flags, B_CHEAP)) {
> +   TAILQ_REMOVE(, bp, b_freelist);
> +   cheapqueuelen--;
> +   CLR(bp->b_flags, B_CHEAP);
> +   return;
> +   }
> +
>  if (ISSET(bp->b_flags, B_COLD)) {
> queue = >coldqueue;
> } else if (ISSET(bp->b_flags, B_WARM)) {
> @@ -1462,11 +1481,17 @@ bufcache_release(struct buf *bp)
> struct bufqueue *queue;
> int64_t pages;
> struct bufcache *cache = [bp->cache];
> +
> pages = atop(bp->b_bufsize);
> KASSERT(ISSET(bp->b_flags, B_BC));
> KASSERT((ISSET(bp->b_flags, B_DMA) && bp->cache == 0)
> || ((!ISSET(bp->b_flags, B_DMA)) && bp->cache > 0));
> if (!ISSET(bp->b_flags, B_DELWRI)) {
> +   if (ISSET(bp->b_flags, B_CHEAP)) {
> +   TAILQ_INSERT_TAIL(, bp, b_freelist);
> +   cheapqueuelen++;
> +   return;
> +   }
> int64_t *queuepages;
> if (ISSET(bp->b_flags, B_WARM | B_COLD)) {
> SET(bp->b_flags, B_WARM);
> Index: sys/buf.h
> ===
> RCS file: /cvs/src/sys/sys/buf.h,v
> retrieving revision 1.103
> diff -u -p -r1.103 buf.h
> --- sys/buf.h   23 May 2016 09:31:28 -  1.103
> +++ sys/buf.h   8 Sep 2016 17:20:12 -
> @@ -221,12 +221,14 @@ struct bufcache {
>  #defineB_COLD  0x0100  /* buffer is on the cold
> queue */
>  #defineB_BC0x0200  /* buffer is managed by
> the cache */
>  #defineB_DMA 

Re: [PATCH] Callback-based interface to libtls

[Bob Beck]

I am in agreement in principle, but please coordinate with bcook@ and/or
jsing@ who were possibly doing
some related adjustments.



On Mon, Sep 5, 2016 at 4:44 AM, Ted Unangst <t...@tedunangst.com> wrote:

> Bob Beck wrote:
> > >
> > > Agreed, I was also a bit unclear on payload at first (though it grew on
> > > me over time, so I didn't change it). Here's an update with the
> > > parameter renamed and better documented.
> > >
> > > ok?
> >
> > Yeah. I'm good with this
> >
> > IMO get it in so we can tweak it in tree.
>
> first tweak: the context has a type: struct tls *, so use it.
>
> Index: tls.h
> ===
> RCS file: /cvs/src/lib/libtls/tls.h,v
> retrieving revision 1.37
> diff -u -p -r1.37 tls.h
> --- tls.h   4 Sep 2016 14:15:44 -   1.37
> +++ tls.h   5 Sep 2016 10:42:50 -
> @@ -44,9 +44,9 @@ extern "C" {
>  struct tls;
>  struct tls_config;
>
> -typedef ssize_t (*tls_read_cb)(void *_ctx, void *_buf, size_t _buflen,
> +typedef ssize_t (*tls_read_cb)(struct tls *_ctx, void *_buf, size_t
> _buflen,
>  void *_cb_arg);
> -typedef ssize_t (*tls_write_cb)(void *_ctx, const void *_buf,
> +typedef ssize_t (*tls_write_cb)(struct tls *_ctx, const void *_buf,
>  size_t _buflen, void *_cb_arg);
>
>  int tls_init(void);
> Index: tls_init.3
> ===
> RCS file: /cvs/src/lib/libtls/tls_init.3,v
> retrieving revision 1.71
> diff -u -p -r1.71 tls_init.3
> --- tls_init.3  4 Sep 2016 16:37:18 -   1.71
> +++ tls_init.3  5 Sep 2016 10:43:43 -
> @@ -189,13 +189,13 @@
>  .Ft "int"
>  .Fn tls_connect_socket "struct tls *ctx" "int s" "const char *servername"
>  .Ft "int"
> -.Fn tls_connect_cbs "struct tls *ctx" "ssize_t (*tls_read_cb)(void *ctx,
> void *buf, size_t buflen, void *cb_arg)" "ssize_t (*tls_write_cb)(void
> *ctx, const void *buf, size_t buflen, void *cb_arg)" "void *cb_arg" "const
> char *servername"
> +.Fn tls_connect_cbs "struct tls *ctx" "ssize_t (*tls_read_cb)(struct tls
> *ctx, void *buf, size_t buflen, void *cb_arg)" "ssize_t
> (*tls_write_cb)(struct tls *ctx, const void *buf, size_t buflen, void
> *cb_arg)" "void *cb_arg" "const char *servername"
>  .Ft "int"
>  .Fn tls_accept_fds "struct tls *tls" "struct tls **cctx" "int fd_read"
> "int fd_write"
>  .Ft "int"
>  .Fn tls_accept_socket "struct tls *tls" "struct tls **cctx" "int socket"
>  .Ft "int"
> -.Fn tls_accept_cbs "struct tls *ctx" "struct tls **cctx" "ssize_t
> (*tls_read_cb)(void *ctx, void *buf, size_t buflen, void *cb_arg)" "ssize_t
> (*tls_write_cb)(void *ctx, const void *buf, size_t buflen, void *cb_arg)"
> "void *cb_arg"
> +.Fn tls_accept_cbs "struct tls *ctx" "struct tls **cctx" "ssize_t
> (*tls_read_cb)(struct *ctx, void *buf, size_t buflen, void *cb_arg)"
> "ssize_t (*tls_write_cb)(struct tls *ctx, const void *buf, size_t buflen,
> void *cb_arg)" "void *cb_arg"
>  .Ft "int"
>  .Fn tls_handshake "struct tls *ctx"
>  .Ft "ssize_t"
>

Re: hexdump(1): strlen + calloc + snprintf == asprintf

[Bob Beck]

ok beck@

On Sun, Sep 4, 2016 at 9:54 AM, Theo Buehler  wrote:

> use the libc interface instead of rolling it by hand.
>
> Index: parse.c
> ===
> RCS file: /var/cvs/src/usr.bin/hexdump/parse.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 parse.c
> --- parse.c 24 Aug 2016 03:13:45 -  1.21
> +++ parse.c 24 Aug 2016 05:23:47 -
> @@ -147,8 +147,7 @@ add(const char *fmt)
> for (savep = ++p; *p != '"';)
> if (*p++ == 0)
> badfmt(fmt);
> -   tfu->fmt = strndup(savep, p - savep);
> -   if (tfu->fmt == NULL)
> +   if ((tfu->fmt = strndup(savep, p - savep)) == NULL)
> err(1, NULL);
> escape(tfu->fmt);
> p++;
> @@ -219,7 +218,6 @@ rewrite(FS *fs)
> char *p1, *p2;
> char savech, *fmtp, cs[4];
> int nconv, prec;
> -   size_t len;
>
> nextpr = NULL;
> prec = 0;
> @@ -408,10 +406,8 @@ rewrite(FS *fs)
>  */
> savech = *p2;
> p1[0] = '\0';
> -   len = strlen(fmtp) + strlen(cs) + 1;
> -   if ((pr->fmt = calloc(1, len)) == NULL)
> +   if (asprintf(>fmt, "%s%s", fmtp, cs) == -1)
> err(1, NULL);
> -   snprintf(pr->fmt, len, "%s%s", fmtp, cs);
> *p2 = savech;
> pr->cchar = pr->fmt + (p1 - fmtp);
> fmtp = p2;
>
>

Re: [PATCH] Callback-based interface to libtls

[Bob Beck]

On Sun, Sep 04, 2016 at 05:26:24AM -0500, Brent Cook wrote:
> On Sun, Sep 04, 2016 at 05:57:54AM -0400, Ted Unangst wrote:
> > Brent Cook wrote:
> > > @@ -246,14 +252,18 @@ An already existing socket can be upgrad
> > >  .Fn tls_connect_socket .
> > >  Alternatively, a secure connection can be established over a pair of 
> > > existing
> > >  file descriptors by calling
> > > -.Fn tls_connect_fds .
> > > +.Fn tls_connect_fds . Using
> > > +.Fn tls_connect_cbs , read and write callbacks can be specified to 
> > > handle the
> > > +actual data transfer.
> >
> > I think we need just a wee bit more documentation. payload is not the 
> > clearest
> > name. It sounds like connection data. I think cookie? Or cbarg? Is it
> > necessary to pass the tls context to the callback? I think that's unusual.
> >
> > read callback should be more like:
> >
> > ssize_t (*read_cb)(void *buf, size_t buflen, void *cbarg);
> 
> Agreed, I was also a bit unclear on payload at first (though it grew on
> me over time, so I didn't change it). Here's an update with the
> parameter renamed and better documented.
> 
> ok?

Yeah. I'm good with this

IMO get it in so we can tweak it in tree. 

ok beck@

and don't forget to bump all the minors of all the things


> 
> Index: Makefile
> ===
> RCS file: /cvs/src/lib/libtls/Makefile,v
> retrieving revision 1.23
> diff -u -p -u -p -r1.23 Makefile
> --- Makefile  30 Mar 2016 06:38:43 -  1.23
> +++ Makefile  4 Sep 2016 10:23:42 -
> @@ -13,6 +13,7 @@ LDADD+= -L${BSDOBJDIR}/lib/libssl/ssl -l
>  HDRS=tls.h
> 
>  SRCS=tls.c \
> + tls_bio_cb.c \
>   tls_client.c \
>   tls_config.c \
>   tls_conninfo.c \
> Index: shlib_version
> ===
> RCS file: /cvs/src/lib/libtls/shlib_version,v
> retrieving revision 1.20
> diff -u -p -u -p -r1.20 shlib_version
> --- shlib_version 31 Aug 2016 23:05:30 -  1.20
> +++ shlib_version 4 Sep 2016 10:23:42 -
> @@ -1,2 +1,2 @@
>  major=11
> -minor=3
> +minor=4
> Index: tls.c
> ===
> RCS file: /cvs/src/lib/libtls/tls.c,v
> retrieving revision 1.48
> diff -u -p -u -p -r1.48 tls.c
> --- tls.c 22 Aug 2016 17:12:35 -  1.48
> +++ tls.c 4 Sep 2016 10:23:42 -
> @@ -424,6 +424,10 @@ tls_reset(struct tls *ctx)
>   tls_sni_ctx_free(sni);
>   }
>   ctx->sni_ctx = NULL;
> +
> + ctx->read_cb = NULL;
> + ctx->write_cb = NULL;
> + ctx->cb_arg = NULL;
>  }
> 
>  int
> Index: tls.h
> ===
> RCS file: /cvs/src/lib/libtls/tls.h,v
> retrieving revision 1.35
> diff -u -p -u -p -r1.35 tls.h
> --- tls.h 22 Aug 2016 14:58:26 -  1.35
> +++ tls.h 4 Sep 2016 10:23:42 -
> @@ -44,6 +44,11 @@ extern "C" {
>  struct tls;
>  struct tls_config;
> 
> +typedef ssize_t (*tls_read_cb)(void *_ctx, void *_buf, size_t _buflen,
> +void *_cb_arg);
> +typedef ssize_t (*tls_write_cb)(void *_ctx, const void *_buf,
> +size_t _buflen, void *_cb_arg);
> +
>  int tls_init(void);
> 
>  const char *tls_config_error(struct tls_config *_config);
> @@ -102,12 +107,16 @@ void tls_free(struct tls *_ctx);
>  int tls_accept_fds(struct tls *_ctx, struct tls **_cctx, int _fd_read,
>  int _fd_write);
>  int tls_accept_socket(struct tls *_ctx, struct tls **_cctx, int _socket);
> +int tls_accept_cbs(struct tls *_ctx, struct tls **_cctx,
> +tls_read_cb _read_cb, tls_write_cb _write_cb, void *_cb_arg);
>  int tls_connect(struct tls *_ctx, const char *_host, const char *_port);
>  int tls_connect_fds(struct tls *_ctx, int _fd_read, int _fd_write,
>  const char *_servername);
>  int tls_connect_servername(struct tls *_ctx, const char *_host,
>  const char *_port, const char *_servername);
>  int tls_connect_socket(struct tls *_ctx, int _s, const char *_servername);
> +int tls_connect_cbs(struct tls *_ctx, tls_read_cb _read_cb,
> +tls_write_cb _write_cb, void *_cb_arg, const char *_servername);
>  int tls_handshake(struct tls *_ctx);
>  ssize_t tls_read(struct tls *_ctx, void *_buf, size_t _buflen);
>  ssize_t tls_write(struct tls *_ctx, const void *_buf, size_t _buflen);
> Index: tls_bio_cb.c
> ===
> RCS file: tls_bio_cb.c
> diff -N tls_bio_cb.c
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ tls_bio_cb.c  4 Sep 2016 10:23:42 -
> @@ -0,0 +1,224 @@
> +/* $ID$ */
> +/*
> + * Copyright (c) 2016 Tobias Pape 
> + *
> + * 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 

Re: [PATCH] Callback-based interface to libtls

[Bob Beck]

On Sun, Sep 04, 2016 at 05:57:54AM -0400, Ted Unangst wrote:
> Brent Cook wrote:
> > @@ -246,14 +252,18 @@ An already existing socket can be upgrad
> >  .Fn tls_connect_socket .
> >  Alternatively, a secure connection can be established over a pair of 
> > existing
> >  file descriptors by calling
> > -.Fn tls_connect_fds .
> > +.Fn tls_connect_fds . Using
> > +.Fn tls_connect_cbs , read and write callbacks can be specified to handle 
> > the
> > +actual data transfer.
> 
> I think we need just a wee bit more documentation. payload is not the clearest
> name. It sounds like connection data. I think cookie? Or cbarg? Is it
> necessary to pass the tls context to the callback? I think that's unusual.

Yes you need the ctx