Re: Unmounting with MNT_DOOMED flag can lead to a kernel panic

[Todd C. Miller]

On Wed, 13 Jul 2016 03:26:34 +0200, Alexander Hall wrote:

> Then wouldn't EINVAL be a reasonable response? Am I missing something? 

We typically ignore flags that don't make sense.  For example,
chmod(2) doesn't return an error if you pass in a mode with the
directory bit set, it just masks it out.  Since unmount(2) uses the
same flags as mount(2) it seems reasonable to just ignore the ones
that don't make sense.

 - todd

Re: Unmounting with MNT_DOOMED flag can lead to a kernel panic

[Alexander Hall]

On July 12, 2016 8:55:50 PM GMT+02:00, "Todd C. Miller" 
 wrote:
>On Tue, 12 Jul 2016 07:22:57 -1000, Tim Newsham wrote:
>
>> Here's another root-only (unless kern.usermount is set) panic issue. 
>We
>> exercise it through tmpfs but it might be more general than that. 
>We're
>> not sure what the proper remediation should be here.
>
>The only valid flag for umount(2) is MNT_FORCE.

Then wouldn't EINVAL be a reasonable response? Am I missing something? 

/Alexander 

>
> - todd
>
>Index: vfs_syscalls.c
>===
>RCS file: /cvs/src/sys/kern/vfs_syscalls.c,v
>retrieving revision 1.261
>diff -u -p -u -r1.261 vfs_syscalls.c
>--- vfs_syscalls.c 6 Jul 2016 19:26:35 -   1.261
>+++ vfs_syscalls.c 12 Jul 2016 18:55:09 -
>@@ -412,7 +412,7 @@ sys_unmount(struct proc *p, void *v, reg
>   if (vfs_busy(mp, VB_WRITE|VB_WAIT))
>   return (EBUSY);
> 
>-  return (dounmount(mp, SCARG(uap, flags), p, vp));
>+  return (dounmount(mp, SCARG(uap, flags) & MNT_FORCE, p, vp));
> }
> 
> /*

Re: always reset mt state in wsmouse_mt_init

[Mark Kettenis]

> From: Ulf Brosziewski 
> Date: Tue, 12 Jul 2016 23:17:34 +0200
> 
> It seems that if an MT device is disabled and reenabled,
> remnants of the previous MT state can lead to problems.
> wsmouse_mt_init should always reset that state completely
> (thanks to jcs@ for help).
> 
> OK?

Makes sense to me.

> Index: dev/wscons/wsmouse.c
> ===
> RCS file: /cvs/src/sys/dev/wscons/wsmouse.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 wsmouse.c
> --- dev/wscons/wsmouse.c  5 Jul 2016 19:33:14 -   1.31
> +++ dev/wscons/wsmouse.c  12 Jul 2016 21:01:17 -
> @@ -1266,11 +1266,8 @@ wsmouse_mt_init(struct device *sc, int n
>   &((struct wsmouse_softc *) sc)->input;
>   int n, size;
>  
> - if (num_slots == input->mt.num_slots
> - && (!tracking == ((input->flags & MT_TRACKING) == 0)))
> - return (0);
> -
>   free_mt_slots(input);
> + memset(>mt, 0, sizeof(struct mt_state));
>  
>   if (tracking)
>   input->flags |= MT_TRACKING;
> 
> 

always reset mt state in wsmouse_mt_init

[Ulf Brosziewski]

It seems that if an MT device is disabled and reenabled,
remnants of the previous MT state can lead to problems.
wsmouse_mt_init should always reset that state completely
(thanks to jcs@ for help).

OK?


Index: dev/wscons/wsmouse.c
===
RCS file: /cvs/src/sys/dev/wscons/wsmouse.c,v
retrieving revision 1.31
diff -u -p -r1.31 wsmouse.c
--- dev/wscons/wsmouse.c5 Jul 2016 19:33:14 -   1.31
+++ dev/wscons/wsmouse.c12 Jul 2016 21:01:17 -
@@ -1266,11 +1266,8 @@ wsmouse_mt_init(struct device *sc, int n
&((struct wsmouse_softc *) sc)->input;
int n, size;
 
-   if (num_slots == input->mt.num_slots
-   && (!tracking == ((input->flags & MT_TRACKING) == 0)))
-   return (0);
-
free_mt_slots(input);
+   memset(>mt, 0, sizeof(struct mt_state));
 
if (tracking)
input->flags |= MT_TRACKING;

Re: Unmounting with MNT_DOOMED flag can lead to a kernel panic

[Alexander Bluhm]

On Tue, Jul 12, 2016 at 12:55:50PM -0600, Todd C. Miller wrote:
> On Tue, 12 Jul 2016 07:22:57 -1000, Tim Newsham wrote:
> 
> > Here's another root-only (unless kern.usermount is set) panic issue.  We
> > exercise it through tmpfs but it might be more general than that.  We're
> > not sure what the proper remediation should be here.
> 
> The only valid flag for umount(2) is MNT_FORCE.

OK bluhm@

> 
>  - todd
> 
> Index: vfs_syscalls.c
> ===
> RCS file: /cvs/src/sys/kern/vfs_syscalls.c,v
> retrieving revision 1.261
> diff -u -p -u -r1.261 vfs_syscalls.c
> --- vfs_syscalls.c6 Jul 2016 19:26:35 -   1.261
> +++ vfs_syscalls.c12 Jul 2016 18:55:09 -
> @@ -412,7 +412,7 @@ sys_unmount(struct proc *p, void *v, reg
>   if (vfs_busy(mp, VB_WRITE|VB_WAIT))
>   return (EBUSY);
>  
> - return (dounmount(mp, SCARG(uap, flags), p, vp));
> + return (dounmount(mp, SCARG(uap, flags) & MNT_FORCE, p, vp));
>  }
>  
>  /*

Unsigned variables can't be < 0

[Tom Cosgrove]

... and size_t is unsigned.


Index: sys/dev/wscons/wsmouse.c
===
RCS file: /home/OpenBSD/cvs/src/sys/dev/wscons/wsmouse.c,v
retrieving revision 1.31
diff -u -p -U8 -r1.31 wsmouse.c
--- sys/dev/wscons/wsmouse.c5 Jul 2016 19:33:14 -   1.31
+++ sys/dev/wscons/wsmouse.c12 Jul 2016 15:12:15 -
@@ -1319,17 +1319,17 @@ wsmouse_init_scaling(struct wsmouseinput
 void
 wsmouse_set_param(struct device *sc, size_t param, int value)
 {
struct wsmouseinput *input =
&((struct wsmouse_softc *) sc)->input;
struct wsmouseparams *params = >params;
int *p;
 
-   if (param < 0 || param > WSMPARAM_LASTFIELD) {
+   if (param > WSMPARAM_LASTFIELD) {
printf("wsmouse_set_param: invalid parameter type\n");
return;
}
 
p = (int *) (((void *) params) + param);
*p = value;
 
if (IS_WSMFLTR_PARAM(param)) {

Re: read(2) on directories

[Chris Cappuccio]

Todd C. Miller [todd.mil...@courtesan.com] wrote:
> On Tue, 12 Jul 2016 12:47:46 -0700, Chris Cappuccio wrote:
> 
> > I've personally always liked being able to cat / read() a directory
> > since it gives you a peek behind the curtain and reflects the
> > reality of how the filesystem is constructed. 
> 
> You haven't been able to do that on OpenBSD for a very long time.
> 

I've been in a deep depression ever since :)

Re: read(2) on directories

[Todd C. Miller]

On Tue, 12 Jul 2016 12:47:46 -0700, Chris Cappuccio wrote:

> I've personally always liked being able to cat / read() a directory
> since it gives you a peek behind the curtain and reflects the
> reality of how the filesystem is constructed. 

You haven't been able to do that on OpenBSD for a very long time.

 - todd

Re: read(2) on directories

[Chris Cappuccio]

Todd C. Miller [todd.mil...@courtesan.com] wrote:
> >From source inspection, Net and Free appear to allow read(2) of
> dirs to succeed.  However, since Linux, Mac OS X and Solaris have
> the EISDIR behavior I think it is probably safe from a portability
> standpoint.
> 
> We're long past the days when opendir(3)/readdir(3) used read(2)...
> 
> HP-UX and AIX still allow reads of directories but no one cares
> about them ;-)
> 

I've personally always liked being able to cat / read() a directory
since it gives you a peek behind the curtain and reflects the
reality of how the filesystem is constructed. 

Re: read(2) on directories

[Todd C. Miller]

>From source inspection, Net and Free appear to allow read(2) of
dirs to succeed.  However, since Linux, Mac OS X and Solaris have
the EISDIR behavior I think it is probably safe from a portability
standpoint.

We're long past the days when opendir(3)/readdir(3) used read(2)...

HP-UX and AIX still allow reads of directories but no one cares
about them ;-)

 - todd

Re: read(2) on directories

[Todd C. Miller]

On Tue, 12 Jul 2016 21:23:51 +0200, Mark Kettenis wrote:

> What do other BSDs (including Mac OS X) do?

Mac OS X returns EISDIR.

 - todd

Re: read(2) on directories

[Tim Newsham]

On Tue, Jul 12, 2016 at 9:19 AM, Todd C. Miller 
wrote:

> Do you know what other systems return EISDIR for read(2) of a
> directory?
>

Linux:
>>> os.open("/", 0)
3
>>> os.read(3, 1024)
Traceback (most recent call last):
  File "", line 1, in 
OSError: [Errno 21] Is a directory


>
>  - todd
>
>


-- 
Tim Newsham | www.thenewsh.com/~newsham | @newshtwit | thenewsh.blogspot.com

Re: read(2) on directories

[Mark Kettenis]

> From: j...@wxcvbn.org (Jeremie Courreges-Anglas)
> Date: Tue, 12 Jul 2016 21:10:37 +0200
> 
> Sending this now to get opinions, but I do not plan any change for 6.0.
> 
> Since I started to use OpenBSD I've always found confusing that
> 
>   cat /directory/
> 
> doesn't error out.  I initially assumed that it was historical behavior,
> but, as hinted by Theo, in rev. 1.1 the behavior was actually to return
> the raw directory entry.
> 
> Both behaviors match POSIX, which allows a third possibility as an XSI
> extension: fail with EISDIR.  I think this is the most useful behavior;
> dumping binary junk is useless, and returning en empty read can hide
> errors.
> 
> I haven't performed extensive testing but if base/xenocara/ports bulk
> builds show errors I can fix them.  The question is, is it worth it?
> 
> Comments / objections?

What do other BSDs (including Mac OS X) do?


> Index: lib/libc/sys/read.2
> ===
> RCS file: /cvs/src/lib/libc/sys/read.2,v
> retrieving revision 1.35
> diff -u -p -r1.35 read.2
> --- lib/libc/sys/read.2   5 Feb 2015 02:33:09 -   1.35
> +++ lib/libc/sys/read.2   9 Jul 2016 17:20:39 -
> @@ -152,13 +152,15 @@ is not a valid file or socket descriptor
>  Part of
>  .Fa buf
>  points outside the process's allocated address space.
> -.It Bq Er EIO
> -An I/O error occurred while reading from the file system.
>  .It Bq Er EINTR
>  A read from a slow device
>  (i.e. one that might block for an arbitrary amount of time)
>  was interrupted by the delivery of a signal
>  before any data arrived.
> +.It Bq Er EIO
> +An I/O error occurred while reading from the file system.
> +.It Bq Er EISDIR
> +The underlying file is a directory.
>  .El
>  .Pp
>  In addition,
> Index: sys/kern/vfs_vnops.c
> ===
> RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 vfs_vnops.c
> --- sys/kern/vfs_vnops.c  19 Jun 2016 11:54:33 -  1.85
> +++ sys/kern/vfs_vnops.c  9 Jul 2016 17:20:39 -
> @@ -336,11 +336,13 @@ vn_read(struct file *fp, off_t *poff, st
>   if (vp->v_type != VCHR && count > LLONG_MAX - *poff)
>   return (EINVAL);
>  
> + if (vp->v_type == VDIR)
> + return (EISDIR);
> +
>   vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
>   uio->uio_offset = *poff;
> - if (vp->v_type != VDIR)
> - error = VOP_READ(vp, uio,
> - (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0, cred);
> + error = VOP_READ(vp, uio, (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0,
> + cred);
>   *poff += count - uio->uio_resid;
>   VOP_UNLOCK(vp, p);
>   return (error);
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 
> 

Re: read(2) on directories

[Todd C. Miller]

Do you know what other systems return EISDIR for read(2) of a
directory?

 - todd

read(2) on directories

[Jeremie Courreges-Anglas]

Sending this now to get opinions, but I do not plan any change for 6.0.

Since I started to use OpenBSD I've always found confusing that

  cat /directory/

doesn't error out.  I initially assumed that it was historical behavior,
but, as hinted by Theo, in rev. 1.1 the behavior was actually to return
the raw directory entry.

Both behaviors match POSIX, which allows a third possibility as an XSI
extension: fail with EISDIR.  I think this is the most useful behavior;
dumping binary junk is useless, and returning en empty read can hide
errors.

I haven't performed extensive testing but if base/xenocara/ports bulk
builds show errors I can fix them.  The question is, is it worth it?

Comments / objections?


Index: lib/libc/sys/read.2
===
RCS file: /cvs/src/lib/libc/sys/read.2,v
retrieving revision 1.35
diff -u -p -r1.35 read.2
--- lib/libc/sys/read.2 5 Feb 2015 02:33:09 -   1.35
+++ lib/libc/sys/read.2 9 Jul 2016 17:20:39 -
@@ -152,13 +152,15 @@ is not a valid file or socket descriptor
 Part of
 .Fa buf
 points outside the process's allocated address space.
-.It Bq Er EIO
-An I/O error occurred while reading from the file system.
 .It Bq Er EINTR
 A read from a slow device
 (i.e. one that might block for an arbitrary amount of time)
 was interrupted by the delivery of a signal
 before any data arrived.
+.It Bq Er EIO
+An I/O error occurred while reading from the file system.
+.It Bq Er EISDIR
+The underlying file is a directory.
 .El
 .Pp
 In addition,
Index: sys/kern/vfs_vnops.c
===
RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
retrieving revision 1.85
diff -u -p -r1.85 vfs_vnops.c
--- sys/kern/vfs_vnops.c19 Jun 2016 11:54:33 -  1.85
+++ sys/kern/vfs_vnops.c9 Jul 2016 17:20:39 -
@@ -336,11 +336,13 @@ vn_read(struct file *fp, off_t *poff, st
if (vp->v_type != VCHR && count > LLONG_MAX - *poff)
return (EINVAL);
 
+   if (vp->v_type == VDIR)
+   return (EISDIR);
+
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
uio->uio_offset = *poff;
-   if (vp->v_type != VDIR)
-   error = VOP_READ(vp, uio,
-   (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0, cred);
+   error = VOP_READ(vp, uio, (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0,
+   cred);
*poff += count - uio->uio_resid;
VOP_UNLOCK(vp, p);
return (error);


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: Unmounting with MNT_DOOMED flag can lead to a kernel panic

[Todd C. Miller]

On Tue, 12 Jul 2016 07:22:57 -1000, Tim Newsham wrote:

> Here's another root-only (unless kern.usermount is set) panic issue.  We
> exercise it through tmpfs but it might be more general than that.  We're
> not sure what the proper remediation should be here.

The only valid flag for umount(2) is MNT_FORCE.

 - todd

Index: vfs_syscalls.c
===
RCS file: /cvs/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.261
diff -u -p -u -r1.261 vfs_syscalls.c
--- vfs_syscalls.c  6 Jul 2016 19:26:35 -   1.261
+++ vfs_syscalls.c  12 Jul 2016 18:55:09 -
@@ -412,7 +412,7 @@ sys_unmount(struct proc *p, void *v, reg
if (vfs_busy(mp, VB_WRITE|VB_WAIT))
return (EBUSY);
 
-   return (dounmount(mp, SCARG(uap, flags), p, vp));
+   return (dounmount(mp, SCARG(uap, flags) & MNT_FORCE, p, vp));
 }
 
 /*

Unmounting with MNT_DOOMED flag can lead to a kernel panic

[Tim Newsham]

Here's another root-only (unless kern.usermount is set) panic issue.  We
exercise it through tmpfs but it might be more general than that.  We're
not sure what the proper remediation should be here.



/*
 * unmount_panic.c
 *Demonstrate a panic through the unmount system call.
 *
 * gcc -g unmount_panic.c -o unmount_panic
 */

#ifdef BUG_WRITEUP //---
Unmounting with MNT_DOOMED flag can lead to a kernel panic

Impact:
Root users or users on systems with kern.usermount set to true can
trigger a kernel panic when unmounting a filesystem.

Description:
When the unmount system call is called with the MNT_DOOMED flag
set, it does not sync vnodes. This can lead to a condition where
there is still a vnode on the mnt_vnodelist, which triggers a
panic in dounmount().

if (!LIST_EMPTY(>mnt_vnodelist))
panic("unmount: dangling vnode");

This conditoin can only be triggered by users who are allowed
to unmount a filesystem. Normally this is the root user, but
if the kern.usernmount sysctl variable has been set to true,
any user could trigger this panic.

Reproduction:
Run the attached unmount_panic.c program.  It will mount a new
tmpfs on /mnt, open a file on it, and then unmount /mnt with
the MNT_DOOMED flag. This will lead to a panic of "unmount: dangling vnode".
NCC Group was able to reproduce this issue on OpenBSD 5.9 release
running amd64.

Recommendation:
TBD

Reported: 2016-07-12
Fixed:   notyet

#endif // BUG_WRITEUP ---


#include 
#include 
#include 
#include 
#include 

void xperror(int cond, char *msg)
{
if(cond) {
perror(msg);
exit(1);
}
}

int main(int argc, char **argv)
{
struct tmpfs_args args = { TMPFS_ARGS_VERSION, 0, 0, 0, 0, 0 };
int x, fd;

x = mount("tmpfs", "/mnt", 0, );
xperror(x == -1, "mount");

fd = open("/mnt/somefile", O_RDWR | O_CREAT, 0666);
xperror(fd == -1, "/mnt/somefile");

x = unmount("/mnt", MNT_DOOMED);
xperror(fd == -1, "unmount");

printf("no crash!\n");
return 0;
}


-- 
Tim Newsham
Distinguished Security Engineer, Security Consulting
NCC Group
Tim.Newsham@nccgroup.trust | PGP: B415 550D BEE9 07DB B4C9  F96C 8EFE CB2F 402D 
3DF0

Re: Introduce M_ACAST

[Jeremie Courreges-Anglas]

Martin Pieuchot  writes:

> Turning the IPv6 forwarding path MP-safe implies, in a first step, to
> defer local delivery to a context serialized by the KERNEL_LOCK().  This
> is the same design as for IPv4.
>
> In order to keep discarding TCP-over-Anycast I'm using an mbuf(9) flag
> to remember that a packet has been received on such address.  This is
> necessary because we do not want to pass extra arguments when enqueuing
> an mbuf.
>
> This reuse the recently unused M_FILDROP.
>
> ok?

ok jca@

-- 
jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: libcrypto: explicitly initialize constant

[Brent Cook]

On Tue, Jul 12, 2016 at 6:41 AM, Miod Vallat  wrote:

> >> Noted by VS2013, const values should be initialized (though I think
> >> the 'static' should also implicitly zero).
> >
> > this sounds like the compiler doesn't know C?
>
> He is talking about Visual Studio. The C part of that piece of shit
> pretending to be a compiler only supports a subset of C89.
>
>
​Well, it's more of a C++ compiler pretending to be a C compiler :)

This is the only const in the LibreSSL tree that's not explicitly
initialized though, right or wrong.​ There are plenty more things it
complains about, if we wanted to mull over the full list. Possibly a few
bits of gold amongst the slag.

Re: Auto tunnel - RFC4213

[Jeremie Courreges-Anglas]

Martin Pieuchot  writes:

> By default we have a route to reject compatible addresses:
>
> ::/96 ::1UGRS 0  0 32768 8 lo0  
>
> But the corresponding check in ip6_input() is still commented because it
> is "stronger than RFC1933".  However since 1996 this RFC has been
> obsoleted twice and the newer one, RFC4213 says:
>
>   The following changes have been performed since RFC 2893:
>
> - Removed automatic tunneling and use of IPv4-compatible
>   addresses.
>
> - [...]
>
>
> Then later it explicitly documents:
>
>   After the decapsulation, the node MUST silently discard a packet with
>   an invalid IPv6 source address. The list of invalid source addresses
>   SHOULD include at least:
>
> - all the IPv4-compatible IPv6 addresses [RFC3513] (::/96),
>   excluding the unspecified address for Duplicate Address Detection
>   (::/128)
>
> - [...]
>
>
> Do I understand correctly that it is time to enable this check?

I think so, ok jca@.

> Index: netinet6/ip6_input.c
> ===
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.162
> diff -u -p -r1.162 ip6_input.c
> --- netinet6/ip6_input.c  6 Jul 2016 15:50:00 -   1.162
> +++ netinet6/ip6_input.c  12 Jul 2016 09:17:04 -
> @@ -299,20 +299,17 @@ ip6_input(struct mbuf *m)
>   ip6stat.ip6s_badscope++;
>   goto bad;
>   }
> -#if 0
> +
>   /*
>* Reject packets with IPv4 compatible addresses (auto tunnel).
>*
> -  * The code forbids auto tunnel relay case in RFC1933 (the check is
> -  * stronger than RFC1933).  We may want to re-enable it if mech-xx
> -  * is revised to forbid relaying case.
> +  * The code forbids automatic tunneling as per RFC4213.
>*/
>   if (IN6_IS_ADDR_V4COMPAT(>ip6_src) ||
>   IN6_IS_ADDR_V4COMPAT(>ip6_dst)) {
>   ip6stat.ip6s_badscope++;
>   goto bad;
>   }
> -#endif
>  
>   /*
>* If the packet has been received on a loopback interface it
>

-- 
jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: libcrypto: explicitly initialize constant

[Miod Vallat]

>> Noted by VS2013, const values should be initialized (though I think
>> the 'static' should also implicitly zero).
>
> this sounds like the compiler doesn't know C?

He is talking about Visual Studio. The C part of that piece of shit
pretending to be a compiler only supports a subset of C89.

Introduce M_ACAST

[Martin Pieuchot]

Turning the IPv6 forwarding path MP-safe implies, in a first step, to
defer local delivery to a context serialized by the KERNEL_LOCK().  This
is the same design as for IPv4.

In order to keep discarding TCP-over-Anycast I'm using an mbuf(9) flag
to remember that a packet has been received on such address.  This is
necessary because we do not want to pass extra arguments when enqueuing
an mbuf.

This reuse the recently unused M_FILDROP.

ok?

Index: sys/sys/mbuf.h
===
RCS file: /cvs/src/sys/sys/mbuf.h,v
retrieving revision 1.215
diff -u -p -r1.215 mbuf.h
--- sys/sys/mbuf.h  13 Jun 2016 21:24:43 -  1.215
+++ sys/sys/mbuf.h  12 Jul 2016 09:35:58 -
@@ -184,7 +184,7 @@ struct mbuf {
 /* mbuf pkthdr flags, also in m_flags */
 #define M_VLANTAG  0x0020  /* ether_vtag is valid */
 #define M_LOOP 0x0040  /* for Mbuf statistics */
-#define M_FILDROP  0x0080  /* dropped by bpf filter */
+#define M_ACAST0x0080  /* received as IPv6 anycast */
 #define M_BCAST0x0100  /* send/received as link-level 
broadcast */
 #define M_MCAST0x0200  /* send/received as link-level 
multicast */
 #define M_CONF 0x0400  /* payload was encrypted (ESP-transport) */
@@ -197,13 +197,13 @@ struct mbuf {
 #ifdef _KERNEL
 #define M_BITS \
 ("\20\1M_EXT\2M_PKTHDR\3M_EOR\4M_EXTWR\5M_PROTO1\6M_VLANTAG\7M_LOOP" \
-"\10M_FILDROP\11M_BCAST\12M_MCAST\13M_CONF\14M_AUTH\15M_TUNNEL" \
+"\10M_ACAST\11M_BCAST\12M_MCAST\13M_CONF\14M_AUTH\15M_TUNNEL" \
 "\16M_ZEROIZE\17M_COMP\20M_LINK0")
 #endif
 
 /* flags copied when copying m_pkthdr */
 #defineM_COPYFLAGS 
(M_PKTHDR|M_EOR|M_PROTO1|M_BCAST|M_MCAST|M_CONF|M_COMP|\
-M_AUTH|M_LOOP|M_TUNNEL|M_LINK0|M_VLANTAG|M_FILDROP|\
+M_AUTH|M_LOOP|M_TUNNEL|M_LINK0|M_VLANTAG|M_ACAST|\
 M_ZEROIZE)
 
 /* Checksumming flags */
Index: sys/netinet6/ip6_input.c
===
RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.162
diff -u -p -r1.162 ip6_input.c
--- sys/netinet6/ip6_input.c6 Jul 2016 15:50:00 -   1.162
+++ sys/netinet6/ip6_input.c12 Jul 2016 09:37:16 -
@@ -197,7 +197,7 @@ ip6_input(struct mbuf *m)
 #if NPF > 0
struct in6_addr odst;
 #endif
-   int srcrt = 0, isanycast = 0;
+   int srcrt = 0;
u_int rtableid = 0;
 
ifp = if_get(m->m_pkthdr.ph_ifidx);
@@ -456,7 +456,7 @@ ip6_input(struct mbuf *m)
struct in6_ifaddr *ia6 =
ifatoia6(ip6_forward_rt.ro_rt->rt_ifa);
if (ia6->ia6_flags & IN6_IFF_ANYCAST)
-   isanycast = 1;
+   m->m_flags |= M_ACAST;
/*
 * packets to a tentative, duplicated, or somehow invalid
 * address must not be accepted.
@@ -558,7 +558,7 @@ ip6_input(struct mbuf *m)
}
 
/* draft-itojun-ipv6-tcp-to-anycast */
-   if (isanycast && nxt == IPPROTO_TCP) {
+   if (ISSET(m->m_flags, M_ACAST) && (nxt == IPPROTO_TCP)) {
if (m->m_len >= sizeof(struct ip6_hdr)) {
icmp6_error(m, ICMP6_DST_UNREACH,
ICMP6_DST_UNREACH_ADDR,
Index: share/man/man9/mbuf.9
===
RCS file: /cvs/src/share/man/man9/mbuf.9,v
retrieving revision 1.99
diff -u -p -r1.99 mbuf.9
--- share/man/man9/mbuf.9   8 Apr 2016 10:01:12 -   1.99
+++ share/man/man9/mbuf.9   12 Jul 2016 09:50:48 -
@@ -294,10 +294,8 @@ protocol-specific.
 variable is valid.
 .It Dv M_LOOP
 for mbuf statistics.
-.It Dv M_FILDROP
-dropped by
-.Xr bpf 4
-filter.
+.It Dv M_ACAST
+received as IPv6 anycast.
 .It Dv M_BCAST
 packet send/received as link-level broadcast.
 .It Dv M_MCAST

Re: Auto tunnel - RFC4213

[Martin Pieuchot]

On 12/07/16(Tue) 11:33, Claudio Jeker wrote:
> On Tue, Jul 12, 2016 at 11:28:47AM +0200, Martin Pieuchot wrote:
> > By default we have a route to reject compatible addresses:
> > 
> > ::/96 ::1UGRS   0  0 32768 8 lo0  
> > 
> > But the corresponding check in ip6_input() is still commented because it
> > is "stronger than RFC1933".  However since 1996 this RFC has been
> > obsoleted twice and the newer one, RFC4213 says:
> > 
> >   The following changes have been performed since RFC 2893:
> > 
> > - Removed automatic tunneling and use of IPv4-compatible
> >   addresses.
> > 
> > - [...]
> > 
> > 
> > Then later it explicitly documents:
> > 
> >   After the decapsulation, the node MUST silently discard a packet with
> >   an invalid IPv6 source address. The list of invalid source addresses
> >   SHOULD include at least:
> > 
> > - all the IPv4-compatible IPv6 addresses [RFC3513] (::/96),
> >   excluding the unspecified address for Duplicate Address Detection
> >   (::/128)
> > 
> > - [...]
> > 
> > 
> > Do I understand correctly that it is time to enable this check?
> 
> Would you then remove the ::/96 reject route from the routing table?

I think this should be a second discussion.  We also have a route for
IPv4-mapped IPv6 addresses & have a similar check enabled in ip6_input():

:::0.0.0.0/96::1  UGRS0   0 32768 8 lo0  

Now routes also prevent any user from sending packets to such destinations. 
Note that we don't have similar checks in ip6_output().

> Or is this more a belt and suspender kind of thing? 

It is.

> > Index: netinet6/ip6_input.c
> > ===
> > RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> > retrieving revision 1.162
> > diff -u -p -r1.162 ip6_input.c
> > --- netinet6/ip6_input.c6 Jul 2016 15:50:00 -   1.162
> > +++ netinet6/ip6_input.c12 Jul 2016 09:17:04 -
> > @@ -299,20 +299,17 @@ ip6_input(struct mbuf *m)
> > ip6stat.ip6s_badscope++;
> > goto bad;
> > }
> > -#if 0
> > +
> > /*
> >  * Reject packets with IPv4 compatible addresses (auto tunnel).
> >  *
> > -* The code forbids auto tunnel relay case in RFC1933 (the check is
> > -* stronger than RFC1933).  We may want to re-enable it if mech-xx
> > -* is revised to forbid relaying case.
> > +* The code forbids automatic tunneling as per RFC4213.
> >  */
> > if (IN6_IS_ADDR_V4COMPAT(>ip6_src) ||
> > IN6_IS_ADDR_V4COMPAT(>ip6_dst)) {
> > ip6stat.ip6s_badscope++;
> > goto bad;
> > }
> > -#endif
> >  
> > /*
> >  * If the packet has been received on a loopback interface it
> > 
> 
> -- 
> :wq Claudio
> 

Re: Auto tunnel - RFC4213

[Claudio Jeker]

On Tue, Jul 12, 2016 at 11:28:47AM +0200, Martin Pieuchot wrote:
> By default we have a route to reject compatible addresses:
> 
> ::/96 ::1UGRS 0  0 32768 8 lo0  
> 
> But the corresponding check in ip6_input() is still commented because it
> is "stronger than RFC1933".  However since 1996 this RFC has been
> obsoleted twice and the newer one, RFC4213 says:
> 
>   The following changes have been performed since RFC 2893:
> 
> - Removed automatic tunneling and use of IPv4-compatible
>   addresses.
> 
> - [...]
> 
> 
> Then later it explicitly documents:
> 
>   After the decapsulation, the node MUST silently discard a packet with
>   an invalid IPv6 source address. The list of invalid source addresses
>   SHOULD include at least:
> 
> - all the IPv4-compatible IPv6 addresses [RFC3513] (::/96),
>   excluding the unspecified address for Duplicate Address Detection
>   (::/128)
> 
> - [...]
> 
> 
> Do I understand correctly that it is time to enable this check?

Would you then remove the ::/96 reject route from the routing table?
Or is this more a belt and suspender kind of thing? 
 
> Index: netinet6/ip6_input.c
> ===
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.162
> diff -u -p -r1.162 ip6_input.c
> --- netinet6/ip6_input.c  6 Jul 2016 15:50:00 -   1.162
> +++ netinet6/ip6_input.c  12 Jul 2016 09:17:04 -
> @@ -299,20 +299,17 @@ ip6_input(struct mbuf *m)
>   ip6stat.ip6s_badscope++;
>   goto bad;
>   }
> -#if 0
> +
>   /*
>* Reject packets with IPv4 compatible addresses (auto tunnel).
>*
> -  * The code forbids auto tunnel relay case in RFC1933 (the check is
> -  * stronger than RFC1933).  We may want to re-enable it if mech-xx
> -  * is revised to forbid relaying case.
> +  * The code forbids automatic tunneling as per RFC4213.
>*/
>   if (IN6_IS_ADDR_V4COMPAT(>ip6_src) ||
>   IN6_IS_ADDR_V4COMPAT(>ip6_dst)) {
>   ip6stat.ip6s_badscope++;
>   goto bad;
>   }
> -#endif
>  
>   /*
>* If the packet has been received on a loopback interface it
> 

-- 
:wq Claudio

Auto tunnel - RFC4213

[Martin Pieuchot]

By default we have a route to reject compatible addresses:

::/96 ::1UGRS   0  0 32768 8 lo0  

But the corresponding check in ip6_input() is still commented because it
is "stronger than RFC1933".  However since 1996 this RFC has been
obsoleted twice and the newer one, RFC4213 says:

  The following changes have been performed since RFC 2893:

- Removed automatic tunneling and use of IPv4-compatible
  addresses.

- [...]


Then later it explicitly documents:

  After the decapsulation, the node MUST silently discard a packet with
  an invalid IPv6 source address. The list of invalid source addresses
  SHOULD include at least:

- all the IPv4-compatible IPv6 addresses [RFC3513] (::/96),
  excluding the unspecified address for Duplicate Address Detection
  (::/128)

- [...]


Do I understand correctly that it is time to enable this check?

Index: netinet6/ip6_input.c
===
RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.162
diff -u -p -r1.162 ip6_input.c
--- netinet6/ip6_input.c6 Jul 2016 15:50:00 -   1.162
+++ netinet6/ip6_input.c12 Jul 2016 09:17:04 -
@@ -299,20 +299,17 @@ ip6_input(struct mbuf *m)
ip6stat.ip6s_badscope++;
goto bad;
}
-#if 0
+
/*
 * Reject packets with IPv4 compatible addresses (auto tunnel).
 *
-* The code forbids auto tunnel relay case in RFC1933 (the check is
-* stronger than RFC1933).  We may want to re-enable it if mech-xx
-* is revised to forbid relaying case.
+* The code forbids automatic tunneling as per RFC4213.
 */
if (IN6_IS_ADDR_V4COMPAT(>ip6_src) ||
IN6_IS_ADDR_V4COMPAT(>ip6_dst)) {
ip6stat.ip6s_badscope++;
goto bad;
}
-#endif
 
/*
 * If the packet has been received on a loopback interface it

Race in ARP

[Martin Pieuchot]

dlg@ could reproduce a panic by running dhclient in a loop on one of his
machines.

Turns out that there's a race between arplookup() and arpcache() inside
in_arpinput().  If another CPU removes the ARP entry from the table, via
RTM_DELETE, it will free the ARP storage.  That means we cannot update
an ARP cache without holding the KERNEL_LOCK().

Diff below should prevent the race.  A better solution would be to delay
the pool_put() until we call rtfree(9), but this needs more work.

Index: netinet/if_ether.c
===
RCS file: /cvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.217
diff -u -p -u -1 -1 -r1.217 if_ether.c
--- netinet/if_ether.c  11 Jul 2016 09:23:06 -  1.217
+++ netinet/if_ether.c  12 Jul 2016 08:36:18 -
@@ -201,23 +201,23 @@ arp_rtrequest(struct ifnet *ifp, int req
}
if (ifa) {
KASSERT(ifa == rt->rt_ifa);
rt->rt_expire = 0;
}
break;
 
case RTM_DELETE:
if (la == NULL)
break;
LIST_REMOVE(la, la_list);
-   rt->rt_llinfo = 0;
+   rt->rt_llinfo = NULL;
rt->rt_flags &= ~RTF_LLINFO;
la_hold_total -= ml_purge(>la_ml);
pool_put(_pool, la);
}
 }
 
 /*
  * Broadcast an ARP request. Caller specifies:
  * - arp header source ip address
  * - arp header target ip address
  * - arp header source ethernet address
@@ -499,23 +499,28 @@ in_arpinput(struct ifnet *ifp, struct mb
/* Do we have an ARP cache for the sender? Create if we are target. */
rt = arplookup(, target, 0, rdomain);
 
/* Check sender against our interface addresses. */
if (rtisvalid(rt) && ISSET(rt->rt_flags, RTF_LOCAL) &&
rt->rt_ifidx == ifp->if_index && isaddr.s_addr != INADDR_ANY) {
inet_ntop(AF_INET, , addr, sizeof(addr));
log(LOG_ERR, "duplicate IP address %s sent from ethernet "
"address %s\n", addr, ether_sprintf(ea->arp_sha));
itaddr = isaddr;
} else if (rt != NULL) {
-   if (arpcache(ifp, ea, rt))
+   int error;
+
+   KERNEL_LOCK();
+   error = arpcache(ifp, ea, rt);
+   KERNEL_UNLOCK();
+   if (error)
goto out;
}
 
if (op == ARPOP_REQUEST) {
uint8_t *eaddr;
 
if (target) {
/* We already have all info for the reply */
eaddr = LLADDR(ifp->if_sadl);
} else {
rtfree(rt);
@@ -541,23 +546,31 @@ out:
 int
 arpcache(struct ifnet *ifp, struct ether_arp *ea, struct rtentry *rt)
 {
struct llinfo_arp *la = (struct llinfo_arp *)rt->rt_llinfo;
struct sockaddr_dl *sdl = satosdl(rt->rt_gateway);
struct in_addr *spa = (struct in_addr *)ea->arp_spa;
char addr[INET_ADDRSTRLEN];
struct ifnet *rifp;
unsigned int len;
int changed = 0;
 
+   KERNEL_ASSERT_LOCKED();
KASSERT(sdl != NULL);
+
+   /*
+* This can happen if the entry has been deleted by another CPU
+* after we found it.
+*/
+   if (la == NULL)
+   return (0);
 
if (sdl->sdl_alen > 0) {
if (memcmp(ea->arp_sha, LLADDR(sdl), sdl->sdl_alen)) {
if (ISSET(rt->rt_flags, RTF_PERMANENT_ARP|RTF_LOCAL)) {
inet_ntop(AF_INET, spa, addr, sizeof(addr));
log(LOG_WARNING, "arp: attempt to overwrite "
   "permanent entry for %s by %s on %s\n", addr,
   ether_sprintf(ea->arp_sha), ifp->if_xname);
return (-1);
} else if (rt->rt_ifidx != ifp->if_index) {
 #if NCARP > 0

Re: client certificate support in syslogd

[Kapetanakis Giannis]

On 12/07/16 02:28, Alexander Bluhm wrote:
> On Mon, Jun 27, 2016 at 05:10:14PM +0300, Kapetanakis Giannis wrote:
>> new version with all changes
> 
> I have polished the diff a bit and would like to commit it.
> 
> ok?
> 
> bluhm

Nice,

One question. Since you've already changed to tls_config_set_XXX_file for the 
server side
https://www.marc.info/?l=openbsd-tech=146784645120595=2
would it be ok to use those functions for the client as well
instead of tls_load_file && tls_config_set_XXX_mem ?

G

> 
> Index: usr.sbin/syslogd/syslogd.8
> ===
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.8,v
> retrieving revision 1.40
> diff -u -p -r1.40 syslogd.8
> --- usr.sbin/syslogd/syslogd.831 Mar 2016 15:53:25 -  1.40
> +++ usr.sbin/syslogd/syslogd.811 Jul 2016 22:07:22 -
> @@ -42,7 +42,9 @@
>  .Op Fl 46dFhnuV
>  .Op Fl a Ar path
>  .Op Fl C Ar CAfile
> +.Op Fl c Ar cert_file
>  .Op Fl f Ar config_file
> +.Op Fl k Ar key_file
>  .Op Fl m Ar mark_interval
>  .Op Fl p Ar log_socket
>  .Op Fl S Ar listen_address
> @@ -81,6 +83,11 @@ PEM encoded file containing CA certifica
>  validation;
>  the default is
>  .Pa /etc/ssl/cert.pem .
> +.It Fl c Ar cert_file
> +PEM encoded file containing the client certificate for TLS connection
> +to a remote host.
> +The default is not to use a client certificate for the connection
> +to a syslog server.
>  .It Fl d
>  Enable debugging to the standard output,
>  and do not disassociate from the controlling terminal.
> @@ -93,6 +100,11 @@ the default is
>  .Pa /etc/syslog.conf .
>  .It Fl h
>  Include the hostname when forwarding messages to a remote host.
> +.It Fl k Ar key_file
> +PEM encoded file containing the client private key for TLS connection
> +to a remote host.
> +This option has to be used together with
> +.Fl c Ar cert_file .
>  .It Fl m Ar mark_interval
>  Select the number of minutes between
>  .Dq mark
> Index: usr.sbin/syslogd/syslogd.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.208
> diff -u -p -r1.208 syslogd.c
> --- usr.sbin/syslogd/syslogd.c6 Jul 2016 19:29:13 -   1.208
> +++ usr.sbin/syslogd/syslogd.c11 Jul 2016 23:06:48 -
> @@ -225,6 +225,8 @@ structtls *server_ctx;
>  struct   tls_config *client_config, *server_config;
>  const char *CAfile = "/etc/ssl/cert.pem"; /* file containing CA certificates 
> */
>  int  NoVerify = 0;   /* do not verify TLS server x509 certificate */
> +char *ClientCertfile = NULL;
> +char *ClientKeyfile = NULL;
>  int  tcpbuf_dropped = 0; /* count messages dropped from TCP or TLS */
>  
>  #define CTL_READING_CMD  1
> @@ -353,7 +355,8 @@ main(int argc, char *argv[])
>   int  ch, i;
>   int  lockpipe[2] = { -1, -1}, pair[2], nullfd, fd;
>  
> - while ((ch = getopt(argc, argv, "46a:C:dFf:hm:np:S:s:T:U:uV")) != -1)
> + while ((ch = getopt(argc, argv, "46a:C:c:dFf:hk:m:np:S:s:T:U:uV"))
> + != -1)
>   switch (ch) {
>   case '4':   /* disable IPv6 */
>   Family = PF_INET;
> @@ -369,6 +372,9 @@ main(int argc, char *argv[])
>   case 'C':   /* file containing CA certificates */
>   CAfile = optarg;
>   break;
> + case 'c':   /* file containing client certificate */
> + ClientCertfile = optarg;
> + break;
>   case 'd':   /* debug */
>   Debug++;
>   break;
> @@ -381,6 +387,9 @@ main(int argc, char *argv[])
>   case 'h':   /* RFC 3164 hostnames */
>   IncludeHostname = 1;
>   break;
> + case 'k':   /* file containing client key */
> + ClientKeyfile = optarg;
> + break;
>   case 'm':   /* mark interval */
>   MarkInterval = strtonum(optarg, 0, 365*24*60, );
>   if (errstr)
> @@ -582,6 +591,31 @@ main(int argc, char *argv[])
>   free(p);
>   close(fd);
>   }
> + if (ClientCertfile && ClientKeyfile) {
> + uint8_t *cert, *key;
> + size_t certlen, keylen;
> +
> + cert = tls_load_file(ClientCertfile, , NULL);
> + if (cert == NULL) {
> + logerror("load client TLS cert failed");
> + } else if (tls_config_set_cert_mem(client_config, cert,
> + certlen) == -1) {
> + logerror("set client TLS cert failed");
> + } else {
> +