Re: LibreSSL 2.2.2 release

[Theo de Raadt]

 I'm wondering out loud if these versions should follow the openbsd shlib
 major minor numbers.  That is where we are careful about semantic
 versioning for api change/add/remove

One note.

If that is decided, on occasion libressl-portable could skip a release
number.  Probably fine for everyone.

Re: get rid of em_realign()

[Theo de Raadt]

 Regarding other strict-alignment architectures, em(4) is probably one
 of the more popular gigabit ethernet options for those architectures
 that have PCI slots.  I don't think any of these machines are severely
 memory starved, but memory might be limited to something like 256MB of
 physical memory.

I have a few of these setups.  I'll keep an eye on it after it is commited.

Re: Brainy: User-Triggerable Kernel Memory Leak in execve()

[Theo de Raadt]

 Awful lot of noise wherein people tell someone else what they should
 need to do with their time and their code.
 
 
 To the best of my knowledge, we've cited and/or thanked Maxime in the
 commits fixing the issues he's found, and we're glad to continue to
 receive his reports, whether or not they include patches.  My
 apologies if we've failed to do so.

Thanks for saying that Philip.

I would like to point out the noise is coming from *users* -- not from
actual developers in the project.

Re: Less strict requirement for USB_REQUEST ioctl

[Theo de Raadt]

 ludovic coues schreef op 2015-08-10 23:29:
  2015-08-10 22:56 GMT+02:00 Martin Pieuchot m...@openbsd.org:
  On 30/07/15(Thu) 12:18, Ludovic Coues wrote:
  Right now, an USB_REQUEST ioctl will fail with an EBADF error if done
  on a device opened as read-only. With this patch, the ioctl will fail
  only if the USB_REQUEST is a write request.
  
  Do you need this to be able to use your usbdevs-like tool as a normal
  use with the w bit on a /dev/usb node?
  
  
  In the case a user have the read right on the node without the write
  right, this patch would allow the normal use of my usbdevs-like tools
  for this user.
  
  In a system with default permission, this won't change anything as
  root and wheel have read + write permission on /dev/usb nodes and
  everybody else have nothing.
 
 Well, the point is that the USB_REQUEST ioctl sends a command to the 
 device.  In that sense, it always involves a write to the device even
 if the command issued is a read command.  The fact that there is only 
 data transfer from the device to the host doesn't mean it doesn't affect
 the state of the device.
 
 I think the current code is right.  If you open the node read-only, you 
 only get to see the kernel state for the device.  If you open with write 
 permission, you get full control over the device.

Essentially the OP wants to completely remove the security model, running
unknown lines of code in the kernel, and in the firmware of the actual
device, causing unknown responses to trigger bugs on the underside of
the USB stack.

Then why not open it as root?  Oh, because you have no root privilege
to exercise those code paths on purpose.  That's the whole idea of this
seperation.

Do you understand?

Re: Brainy: User-Triggerable Kernel Memory Leak in execve()

[Theo de Raadt]

 I am also of the opinion that if somebody/a method can discover bugs,
 they should report them. And if they can't, that method should be
 disclosed to allow others to continue their work.

And my opinion is that Sam should back his opinions with lots of
money.

Re: Brainy: User-Triggerable Kernel Memory Leak in execve()

[Theo de Raadt]

 It feels wasteful to develop a seemingly comprehensive and modular code
 scanner which will inherently find heaps of bugs, and then not release
 it or allow others to work with it.

Sam, since you think throwing opinions out there is valuable

Let me give me yours.

I think you should talk privately to Maxime and find out how much money
he wants from you, to release his tool.

Maxime, I suggest you take Sam for all he is worth.

That's my opinion in this situation.  I came to this opinion because
Sam feels so incredibly entitled.

Re: Possible fix for i217 problem

[Theo de Raadt]

 On 2015/08/04 22:40, Stefan Fritsch wrote:
  someone mentioned to me the i217-LM problems that were reported on misc 
  end of May. It is possible that the patch below helps.
 
 This fixes my Dell poweredge T20:
 em0 at pci0 dev 25 function 0 Intel I217-LM rev 0x04: msi, address 
 f8:b1:56:...
 
 And doesn't break my X220:
 em0 at pci0 dev 25 function 0 Intel 82579LM rev 0x04: msi, address 
 f0:de:f1:...
 
 Readers with I217 / I218 / 82579 devices, please test, especially if network
 is currently WORKING for you. We know it fixes various issues but the 
 important
 thing is that this isn't at the expense of other systems.

This diff will be in the next snapshots, for rapid testing before release.

The most important goal ensuring this does not cause a regression for
other chips.  Don't test the ones this hopes to fix.  Test the ones it
shouldn't affect!!!

Re: softdep by default on AMD64

[Theo de Raadt]

There is no way this diff is going in.

When softdep is 100% reliable, then we can talk.

Enabling it prematurely is ridiculous.  Considering the defects
are clearly described as lockups, disk space corruption -- with
such a suggestion I must ask --who's side are you on??  

 There was a great discussion about softdep recently:
 
   https://marc.info/?t=14216401691r=1w=2
 
 It needs extra memory, so the FAQ warns against its use on really old
 architectures.
 
 tedu@ described the two main deterrents:
 
   https://marc.info/?l=openbsd-miscm=142294185000751w=2
 
 Might it be worth making default for AMD64? The performance gain is big
 in many cases.
 
 
 
 Index: editor.c
 ===
 RCS file: /cvs/src/sbin/disklabel/editor.c,v
 retrieving revision 1.295
 diff -u -p -r1.295 editor.c
 --- editor.c  8 May 2015 12:15:50 -   1.295
 +++ editor.c  24 Jul 2015 00:16:08 -
 @@ -51,6 +51,12 @@
  #define NUMBOOT 0
  #endif
  
 +#if defined(__amd64__)
 +#define MNT_OPTS rw,softdep
 +#else
 +#define MNT_OPTS rw
 +#endif
 +
  /* structure to describe a portion of a disk */
  struct diskchunk {
   u_int64_t start;
 @@ -1892,9 +1898,10 @@ mpsave(struct disklabel *lp)
   if (fstype == FS_SWAP) {
   fprintf(fp, %s%c none swap sw\n, bdev, 'a'+j);
   } else if (mi[i].mountpoint) {
 - fprintf(fp, %s%c %s %s rw 1 %d\n, bdev,
 + fprintf(fp, %s%c %s %s %s 1 %d\n, bdev,
   'a' + j, mi[i].mountpoint,
 - fstypesnames[fstype], j == 0 ? 1 : 2);
 + fstypesnames[fstype], MNT_OPTS,
 + j == 0 ? 1 : 2);
   }
   }
   fclose(fp);
 

x1

Re: [patch] Remove archaic manual sizing from dump(8)

[Theo de Raadt]

 On 2015-07-23, Michael McConville mmcco...@sccs.swarthmore.edu wrote:
 
  --- sbin/dump/main.c23 May 2015 05:17:20 -  1.56
  +++ sbin/dump/main.c15 Jun 2015 23:16:10 -
  @@ -115,7 +115,7 @@ main(int argc, char *argv[])
  usage();
   
  obsolete(argc, argv);
  -   while ((ch = getopt(argc, argv, 0123456789aB:b:cd:f:h:ns:ST:uWw)) != 
  -1)
  +   while ((ch = getopt(argc, argv, 0123456789AB:b:cd:f:h:ns:ST:uWw)) != 
  -1)
 
 This will break lots of backup scripts.  Should -a continue to be
 supported for some time?

Definately.  At minimum, as a no-op.  Though if it previously acted as any
sort of override over other selected options, that might be needed too.

Re: OpenBSD::Tame perl wrapper for tame(2)

[Theo de Raadt]

This is extremely premature.

The tame() in my devtree already has major incompatible changes.

Re: Patch to add -l flag to cat(1)

[Theo de Raadt]

  So, what's the story with the -l option?  What change/fix in OpenBSD
  base requires it?
 
 Nothing requires it explicitly, more the question of if having the
 ability for cat to set exclusive locks on its stdout so that multiple
 calls to the same cat command will cause the the output to be arranged
 in sequence is useful.

Yes, and maybe the same option should be added to every command!

Re: [patch] Disklabel message tweak

[Theo de Raadt]

This is another trivial patch, but I've always found the disklabel
message No label changes confusing. For example, if you print (p), add
a label (a), write (w), print to check your changes (p), and then quit
(q), it seems odd to be told No label changes. 



Index: sbin/disklabel/editor.c
===
RCS file: /cvs/src/sbin/disklabel/editor.c,v
retrieving revision 1.295
diff -u -p -r1.295 editor.c
--- sbin/disklabel/editor.c8 May 2015 12:15:50 -   1.295
+++ sbin/disklabel/editor.c17 Jul 2015 02:54:59 -
@@ -373,7 +373,7 @@ editor(int f)
*/
   if (!dflag  !aflag 
   memcmp(lab, newlab, sizeof(newlab)) == 0) {
-  puts(No label changes.);
+  puts(No further label changes.);
   /* Save mountpoint info. */
   mpsave(newlab);
   goto done;

The phrase No label changes. was selected because it is true
(there is nothing to save).  I don't see what further adds to
make this more accurate or understandable.

Re: Get Ruby 2.2 test suite passing

[Theo de Raadt]

 Ted Unangst wrote:
  Jeremy Evans wrote:
   As an aside, crypt(passwd, $2) returns : instead of NULL.  I'm not
   sure if that's a security issue, but I think it is and we should fix it.
   I'll see if I can get a patch for that and send it to tech@.
  
  This is a weird edge case where niels decided to make bcrypt() work
  differently than crypt(). i don't really know why. I think null is the safer
  return, and we should probably switch. we don't have code that looks for :
  (and certainly no third party code ever does), but there is code that checks
  for null.
  
 
 like this.
 
 
 Index: bcrypt.c
 ===
 RCS file: /cvs/src/lib/libc/crypt/bcrypt.c,v
 retrieving revision 1.52
 diff -u -p -r1.52 bcrypt.c
 --- bcrypt.c  28 Jan 2015 23:33:52 -  1.52
 +++ bcrypt.c  18 Jul 2015 00:29:34 -
 @@ -385,12 +385,9 @@ char *
  bcrypt(const char *pass, const char *salt)
  {
   static chargencrypted[BCRYPT_HASHSPACE];
 - static chargerror[2];
  
 - /* How do I handle errors ? Return ':' */
 - strlcpy(gerror, :, sizeof(gerror));
   if (bcrypt_hashpass(pass, salt, gencrypted, sizeof(gencrypted)) != 0)
 - return gerror;
 + return NULL;
  
   return gencrypted;
  }

This feels so much safer

Re: Get Ruby 2.2 test suite passing

[Theo de Raadt]

 The only objection I can see is something stupid that does not check
 the error condition, derefs NULL, drops a core file in an insecure
 place, and therefore leaks information. 

 To my mind this is a buggy program, combined with an insecure configuration,
 and we shouldn't be trying to save people from their own stupid and make it
 worse.. 

I am hoping to see that happen!

Re: Get Ruby 2.2 test suite passing

[Theo de Raadt]

 my perspective is: absent clear knowledge of what programs are doing, attempts
 to second guess them in a library function are perilous. let us be standards
 compliant, and then at least any resulting holes are clearly the program's
 fault.

such programs always deference the pointer.

So I agree strongly with a NULL error, rather than something hacky like
errno modification.

tame(2) WIP

[Theo de Raadt]

I have been working for a while on a subsystem to restrict programs
into a reduced feature operating model.

Other people have made such systems in the past, but I have never been
happy with them.  I don't think I am alone.

Generally there are two models of operation.  The first model requires
a major rewrite of application software for effective use
(ie. capsicum).  The other model in common use lacks granularity, and
allows or denies an operation throughout the entire lifetime of a
process.  As a result, they lack differentiation between program
initialization versus main servicing loop.  systrace had the same
problem.  My observation is that programs need a large variety of
calls during initialization, but few in their main loops.

Some BPF-style approaches have showed up.  So you need to write a
program to observe your program, to keep things secure?  That is
insane.

So I asked myself if I could invent a simple system call, which people
would place directly into programs, between initialization and
main-loop.

Secondly, I wondered what kind of semantics such programs would need.
Not just directly themselves, but for DNS and other macro operations.

Anyways, enough explanation.  A manual page follows.

Then the kernel diff.

Finally, a sample of 29 userland programs protected to various
degrees by using it:
cat pax ps dmesg ping ping6 dc diff finger from id kdump
logger script sed signify uniq w wc whois arp authpf bgpd
httpd ntpd relayd syslogd tcpdump traceroute

Not all these are perfect, but it shows the trend.  The changes
are fairly simple.  In the simplest non-network programs, network
access is disabled.  In simple network programs, file access goes
away.  That is the trend.

Sometimes a program is easily modified, making it better, because
the integration of tame hints at an improvement which will make it
tighter under tame.  sed is an example...


TAME(2)   System Calls Manual  TAME(2)

NAME
 tame - restrict system operations

SYNOPSIS
 #include sys/tame.h

 int
 tame(int flags);

DESCRIPTION
 The current process is forced into a restricted-service operating mode.
 A few subsets are available, roughly described as computation, memory
 management, read-write operations on file descriptors, opening of files,
 networking.  In general, these modes were selected by studying the
 operation of many programs using libc and other such interfaces.

 Use of tame in an application will require at least some study and
 understanding of the interfaces called.

 Subsequent calls to tame() can reduce abilities further, but abilities
 can never be regained.

 A process which attempts a restricted operation is killed with SIGKILL.
 If TAME_ABORT is set, then a non-blockable SIGABRT is delivered instead,
 possibly resulting in a core(5) file.

 A flags value of 0 restricts the process to the _exit(2) system call.
 This can be used for pure computation operating on memory shared with
 another process.

 All TAME_* options below (with the exception of TAME_ABORT) permit the
 following system calls:

   clock_getres(2), clock_gettime(2), fchdir(2), getdtablecount(2),
   getegid(2), geteuid(2), getgid(2), getgroups(2), getitimer(2),
   getlogin(2), getpgid(2), getpgrp(2), getpid(2), getppid(2),
   getresgid(2), getresuid(2), getrlimit(2), getsid(2), getthrid(2),
   gettimeofday(2), getuid(2), getuid(2), issetugid(2), nanosleep(2),
   sendsyslog(2), setitimer(2), sigaction(2), sigprocmask(2),
   sigreturn(2), umask(2), wait4(2).

 Calls allowed with restrictions include:
   sysctl(3) A small set of read-only operations are allowed,
 sufficient to support: getifaddrs(3),
 getdomainname(3), gethostname(3), system sensor
 readings.
   access(2) May check for existance of /etc/localtime.
   adjtime(2)Read-only, for ntpd(8).
   open(2)   May open /etc/localtime, any files below
 /usr/share/zoneinfo and files ending in libc.cat in
 below the directory /usr/share/nls/.
   readlink(2)   May operate on /etc/malloc.conf.
   tame(2)   Can only reduce permissions.

 The flags are specified as a bitwise OR of the following values:

   TAME_MALLOC   To allow use of the malloc(3) family of functions,
 the following system calls are permitted:

 getentropy(2), madvise(2), minherit(2), mmap(2),
 mprotect(2), mquery(2), munmap(2).

   TAME_RW   The following system calls are permitted to allow
 most types of IO operations on previously allocated
 file descriptors, including libevent or handwritten
 

Re: tame(1), like nice(1) but for permissions

[Theo de Raadt]

 Sorry, should have made things clearer. I just meant that chroot was 
 a bad comparison. I can't see any sane use of a tame(1) at the
 moment.

No, no no, Ted's comments are completely valid.

You cannot replace the narrow chroot calls in the privsep daemons with
chroot(8) run externally.

Any attempts to do so would degrade security.  Same here with tame.

Same with your pkg building tools.

Re: tame(1), like nice(1) but for permissions

[Theo de Raadt]

 On Mon, Jul 20, 2015 at 12:04:43PM -0400, Ted Unangst wrote:
  chroot is probably the best comparision. yes, we provide a chroot(1), but
 There is no chroot(1). :p
 
  practically nothing uses it. everything is instead calling chroot(2) on its
  own. the things that do use chroot(1) are doing so for specialized namespace
  reasons, not for sandboxing.
 
 I have a huge counter-example: dpb.
 Specifically, chroot(8) does the nice usercontext thingies that would be
 cumbersome to do from perl.

chroot was only used as a partial example.

I have the same concerns with tame(1).

First, it is very premature.  Secondly, TAME_EXEC is a very nasty semantic.

Most importantly the purpose of tame is to allow a programmer to seperate
their initial-setup from the main-loop processing.  By tagging the unix
feature-set into a simple effect classifications, it also guides the
programming of general purpose unix tools, guiding them towards privdrop,
privsep; or if they have no specific priv-slit happening, at minimum it
constraints most to files-only or network-only behaviours.

From the outside, a regular user is not going to know the system features
and semantics that a program uses, not in a detailed fashion.

tame -a firefox doesn't work.  Is tame broken?

We don't need that kind of grief.

Re: doas failsafe

[Theo de Raadt]

 Ability to define alias in the doas config file might be nice. Just
 like ssh with the ssh_config file.

I have always wanted a .lsrc file, which would allow me to override
the special options for ls, as well.  That's kind of what you are
talking about, right?

No, I think you are serious.

And so let's say code is written to support an alias.  And let's
say it has a bug.  It runs as setuid root.

Do you understand what happens next?

If you want software to do everything, eventually it will do everything
including what you cannot afford.

Re: doas failsafe

[Theo de Raadt]

 Less code running with setuid root, the better.

That is the entire point.

Re: change from vt220 to pccon0 for AMD/Intel consoles

[Theo de Raadt]

 There are pccon* terminal descriptions for AMD/Intel PC consoles
 in /etc/termcap.I have been using them on various computers
 since 2011 without problems.
 
 I suggest to use pccon0 instead of vt220 by default
 for amd64 and i386because vt220 has not good support
 of navigation and function keys oftypical PC keyboard.
 
 History of pccon*:
 http://marc.info/?l=ncurses-bugm=131825802104588w=2
 http://marc.info/?l=openbsd-miscm=132125411729722w=2

Sorry, no.

Because, as soon as you ssh from such a console to another operating
system, you are now operating under the rules of their termcap
entries.  Which are not portable, because the push to distribute
modern termcap files into all-OS slowed down considerably in the
recent years.

vt220 is the baseline the kernel is trying to emulate.  I am pretty
sure such a baseline is more important to people than the fancy
extensions.

I actually wish kernel code implemented xterm.

Re: axphy(4): new dumb driver for axe(4) phys

[Theo de Raadt]

> First I added a custom reset function, but it turned out the problem was 
> elsewhere. So the current driver is just pretty printf'ing the model and 
> the phy.
> 
> And you're right, it's not worth getting this in tree so forget about 
> this diff.

correct.  ukphy is named a bit strangely.

 ukphy - generic/unknown IEEE 802.3u Ethernet PHY


It is better to think of this as the "generic" driver, that
handles all the phy's which don't require special handling.
 

Re: does anoybody use ul?

[Theo de Raadt]

> > From: "Ted Unangst" 
> > Date: Fri, 23 Oct 2015 03:47:56 -0400
> > 
> > ul appears somewhat useless for its intended purpose.
> > 
> > echo _xxx_ | ul does not result in underlined text in an xterm, so I doubt
> > many people are using this.
> > 
> > Unlike, say, mandoc, it can't output Greek letters. I also imagine most 
> > people
> > have moved on to some form of markdown for their other text markup needs.
> > 
> > Will anyone miss it?
> 
> I doubt anybody ever uses it directly, but I've seen it used in
> scripts.  And even though POSIX doesn't mention it, it is present on
> pretty much any UNIX-like OS.  I think removing it would be wrong.

printf '_\bc_\bh_\ba_\bn_\bg_\be_\b _\bi_\bs_\b _\bb_\ba_\bd_\b!\b' | ul

:-)

Re: documenting multiple standards

[Theo de Raadt]

>>From wcrtomb(3):
>
> The wcrtomb() function conforms to ISO/IEC 9899/AMD1:1995 (``ISO C90,
> Amendment 1'').  The restrict qualifier is added at ISO/IEC 9899/1999
> (``ISO C99'').
>
>This wording is confusing. Is it implying that we don't use a restrict
>qualifier? (We do.)
>
>If a standard changes, do we have to document how the old standard
>differs from what we currently follow? That seems awfully complicated.
>Can we just document the latest standard, or "the earliest standard
>that doesn't appreciably differ from the latest standard"?
>
>What's the guiding practice here?

Man pages should documnet what people need, trying to avoid ambiguity
or overpreciseness.  True or false:

The wcrtomb() function conforms to ISO/IEC 9899/1999 (``ISO C99'').

rip6query(8)

[Theo de Raadt]

does anyone use rip6query?

we don't have a ripquery

I cannot imagine the use for this too.  Either you are using rip6
protocol, or you aren't.  Does this perhaps still exist because
route6d isn't like our other daemons with a "fib-update" mode?

Re: utf8 hack for ls

[Theo de Raadt]

> Damien Miller wrote:
> > rather than scattering hacks in each program that needs to
> > output utf8 to the console, how about making something
> > for libutil that they all can use?
> 
> Yes, that is certainly the plan, but I think it's easier to see what's needed
> if we convert a few programs first to identify common functionality. Also,
> diffs that touch libutil and ls are a pain to test.

of course.  so imagine adding an API.  then changing it, cranking major,
changing it, cranking major, etc.

imsg eventually ended up in libutil, but that was after ~12 programs had
put design pressure on it from seperate directions.

The noises in the berlin room and icb lead me to believe we are nowhere near
some 'encompassing API' that would cover all cases.  as in, such API already
exist, but are not being considered...

Re: sed: better error message

[Theo de Raadt]

> Jérémie Courrèges-Anglas wrote:
> > Michael McConville  writes:
> > > It looks like it can be pretty easily replaced with calls to err(3),
> > > errx(3), warn(3), warnx(3), etc.
> > 
> > Not sure about this, you'd have to repeat the same code over and over to
> > print the line number and file name.
> 
> Good point. I'll look at the code more.
> 
> > > However, it'd be easiest to rename the function to error() first.
> > >
> > > Thoughts?
> > 
> > Makes sense to me.
> > 
> > However, using "error" instead of "sed_err" would save you two
> > characters and avoid some of the >80 chars long lines introduced by your
> > diff.  Those should be fixed.
> 
> 'error' seemed a little too general. ok for s_err?

hundreds of programs use error() just fine because it isn't available
in the system namespace.

Re: boot from softraid, backspace in passphrase prompt

[Theo de Raadt]

Since people are typing blind, can you add support for ^U as well?

> I want correct typing mistakes when booting from softraid crypto disks.
> Can we handle at least the backspace key, plz^Hease? :)
> 
> diff --git sys/arch/amd64/stand/libsa/softraid.c 
> sys/arch/amd64/stand/libsa/softraid.c
> index 336865a..801cec4 100644
> --- sys/arch/amd64/stand/libsa/softraid.c
> +++ sys/arch/amd64/stand/libsa/softraid.c
> @@ -557,6 +557,10 @@ sr_crypto_decrypt_keys(struct sr_boot_volume *bv)
>   c = cngetc();
>   if (c == '\r' || c == '\n')
>   break;
> + else if (c == '\b') {
> + i = i > 0 ? i - 2 : -1;
> + continue;
> + }
>   passphrase[i] = (c & 0xff);
>   }
>   passphrase[i] = 0;
> diff --git sys/arch/i386/stand/libsa/softraid.c 
> sys/arch/i386/stand/libsa/softraid.c
> index 336865a..801cec4 100644
> --- sys/arch/i386/stand/libsa/softraid.c
> +++ sys/arch/i386/stand/libsa/softraid.c
> @@ -557,6 +557,10 @@ sr_crypto_decrypt_keys(struct sr_boot_volume *bv)
>   c = cngetc();
>   if (c == '\r' || c == '\n')
>   break;
> + else if (c == '\b') {
> + i = i > 0 ? i - 2 : -1;
> + continue;
> + }
>   passphrase[i] = (c & 0xff);
>   }
>   passphrase[i] = 0;
> diff --git sys/arch/sparc64/stand/ofwboot/softraid.c 
> sys/arch/sparc64/stand/ofwboot/softraid.c
> index 1dadfdb..7654194 100644
> --- sys/arch/sparc64/stand/ofwboot/softraid.c
> +++ sys/arch/sparc64/stand/ofwboot/softraid.c
> @@ -572,6 +572,10 @@ sr_crypto_decrypt_keys(struct sr_boot_volume *bv)
>   c = getchar();
>   if (c == '\r' || c == '\n')
>   break;
> + else if (c == '\b') {
> + i = i > 0 ? i - 2 : -1;
> + continue;
> + }
>   passphrase[i] = (c & 0xff);
>   }
>   passphrase[i] = 0;
> 

Re: pledge(2) and spamd(8)

[Theo de Raadt]

> I was just trying to pledge(2) spamd(8), nevertheless came across 2 
> priviliges kern_pledge.c is missing for this to work.
>
> First spamd(8) needs to read sysctl kern.maxfiles in order to see if it 
> can launch with that value or not, and second if the multicast options 
> are passed as parameters then it also needs IP_MULTICAST_TTL since 
> spamd(8) calls setsockopt(2) with that option set:

I am not a fan of this approach.  Your diff is very close to

pledge("everything")

That is a very small stopgap against a problem.  Though you now have a
list of things being done in one process, and good argument for someone
to refactor this into privsep..

My gut reaction is to not allow these two operations.  I normally wait
until I see evidence other programs need such operations, while being
very strongly protected from pledge.  Not seeing that here.

Re: pledge(2) and spamd(8)

[Theo de Raadt]

> Also, I wonder what the point of having a sanity check against
> kern.maxfiles at all is, especially with the arbitrary-feeling
> additional rule of "maxcon may not exceed kern.maxfiles - 200". It feels
> redundant to me, and it sort of makes a promise of protection it can't
> uphold.

That code predates the addition of getdtablecount().  Maybe someone
can think about improving it.

> I mean, it's a system-wide limit and spamd has no control over the
> resource usage of the rest of the system, so it still has to (hopefully
> gracefully) handle the case when it runs up against either kern.maxfiles
> or RLIMIT_NOFILE.

Graceful recovery can include the heaviest programs avoiding getting
to that point.  Shrug.  These are just descriptors.

But honestly, a lot of programs don't know how to cope correctly with
ENFILE and EMFILE.  benno and I put a lot of effort into improving this
about 3 years ago for the most sensitive programs, but fixing all the
programs is a harder job.

> Removing that check at least simplifies that part of spamd's pledge().

Yes that covers up a scratch on one deck chair.

Re: pair(4) (was: connect routing domains on layer 2)

[Theo de Raadt]

> On 10/24/15 06:46, Reyk Floeter wrote:
> > vether doesn't help as it is not transmitting any traffic.
> > in other words, "vether is a bridge endpoint" "pair is a bridge link"
> This may be a dead topic, but doesn't bridge_output() transmit for 
> vether(4)?
> Or am I missing the point entirely?
> 
> pair(4) does look very useful as a "cable". I just wonder why bridge(4)
> doesn't act more like a physical switch which would accept the single
> endpoint of a vether(4)

That is answered in the manual page.

Re: __predict_false for pledge

[Theo de Raadt]

> On Mon, Oct 26, 2015 at 8:46 AM, Michael McConville  wrote:
> > We have a pretty strong guarantee that it can only happen once per
> > process...
> ...
> > --- sys/sys/syscall_mi.h9 Oct 2015 01:17:18 -   1.11
> > +++ sys/sys/syscall_mi.h26 Oct 2015 15:13:44 -
> > @@ -72,7 +72,8 @@ mi_syscall(struct proc *p, register_t co
> > if (lock)
> > KERNEL_LOCK();
> > pledged = (p->p_p->ps_flags & PS_PLEDGE);
> > -   if (pledged && !(tval = pledge_check(p, code))) {
> > +   if (__predict_false(
> > +   pledged && !(tval = pledge_check(p, code {
> 
> I disagree.  That's the code used on every syscall, not just once per
> process and pledged is true for *most* of the processes on a -current
> box.  No, that doesn't mean we should do __predict_true() there.

I have no idea what it will do on all our platforms.

> In general, __predict_{true,false} should be left in the tool box and
> only pulled out after detailed dives into code paths involved.  For
> all my banging on the project, I think I've used them in *two* places.

I suspect there is more benefit to be gained through actual tested
refactoring (with assumption: systrace is almost never used).

Re: __predict_false for pledge

[Theo de Raadt]

> Not sure how people feel about these annotations. This is a pretty
> classic use case, though.

No, the classic case is when the condition is a single variable, rather
than a condition "always true && rarely true".

Re: move cron socket to /var/run/cron.sock (pledge)

[Theo de Raadt]

> Grmbl.  I've hard a hard time trying to understand *why* this would be
> needed.  The answer is pledge(2), who makes chmod(2) fail with EPERM
> instead of killing the process.
> 
> I find this confusing.  IMO pledge(2) should let the kernel do the
> appropriate security checks for chown(2).

Cannot.  pledge handles *chown() at a realistic level.

Otherwise, we'd need pledge checks in every function reachable
by VOP_SETATTR.

Re: chmod(1) -f flag is still used?

[Theo de Raadt]

Your process here is really strange.  You see something, then you
write a diff before answering your own question, then you send a mail
asking a question with the diff already present as an investment.

In OpenBSD, we have a 20 year old repository that explains why a
change was made.  Before the 20 years, there are older repositories to
view.  There are also many other resources which might document why -f
existed in the past.  Without review of those, it is wrong to simply
delete it.

Use 'cvs annnoate' and look at the history for that line in the
file.

   revision 1.26
   date: 2010/01/12 19:30:53;  author: jasper;  state: Exp;  lines: +2 -2;
   sync comment with reality, -f won't be documented.
   
   as discussed with jmc@, otto@ and millert@
   ok otto@ millert@

So at least 4 people discussed it.  You didn't discover that?

Look at -r1.1 of chmod.c

case 'f':   /* XXX: undocumented. */
fflag = 1;

That still does not explain it.  But did you get there?

There are older repositories and documents to review.

But at least now there are two questions and you can dig further
rather than spending the time writing a diff.  Yet you did it first.
How bizzare.

Your diff is pointless, without first know the answers to the questions,
isn't it?

It think it is nice that people are trying to help; but if you are
modifying binaries often used in scripts, the standard of work has to
be very high.

> Digging around the tree (Yes, i want try to contribute if possible),
> i found this small piece of code:
> 
> 
> >case 'f':/* no longer documented. */
> > fflag = 1;
> > break;
> 
> And the usage of this flag in two conditional statement in a logic
> OR:
> 
> >if (ischmod) {
> > if (!fchmodat(AT_FDCWD, p->fts_accpath, oct ? omode :
> > getmode(set, p->fts_statp->st_mode), atflags)
> > || fflag)
> > continue;
> >} else if (!ischflags) {
> > if (!fchownat(AT_FDCWD, p->fts_accpath, uid, gid,
> > atflags) || fflag)
> > continue;
> 
> I checked the man pages related to chmod(1), chgrp(1), chflags(1) and
> chown(8) and as the comment stated, nowhere the option is mentioned.
> There is a motivation why the unused -f option is still there? As the
> option is not documented anymore, this mean that fflag will be false
> in every documented use case.
> Anyway, attached a diff usable if the option can be removed.
> 
> Regards
> 
> --UT4aJajqd8PRde7T
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: attachment; filename="f_option.diff"
> 
> Index: chmod.c
> ===
> RCS file: /cvs/src/bin/chmod/chmod.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 chmod.c
> --- chmod.c   9 Oct 2015 01:37:06 -   1.38
> +++ chmod.c   12 Nov 2015 20:51:28 -
> @@ -62,7 +62,7 @@ main(int argc, char *argv[])
>   unsigned long val;
>   int oct;
>   mode_t omode;
> - int Hflag, Lflag, Rflag, ch, fflag, fts_options, hflag, rval, atflags;
> + int Hflag, Lflag, Rflag, ch, fts_options, hflag, rval, atflags;
>   uid_t uid;
>   gid_t gid;
>   u_int32_t fclear, fset;
> @@ -79,8 +79,8 @@ main(int argc, char *argv[])
>  
>   uid = (uid_t)-1;
>   gid = (gid_t)-1;
> - Hflag = Lflag = Rflag = fflag = hflag = 0;
> - while ((ch = getopt(argc, argv, "HLPRXfghorstuwx")) != -1)
> + Hflag = Lflag = Rflag = hflag = 0;
> + while ((ch = getopt(argc, argv, "HLPRXghorstuwx")) != -1)
>   switch (ch) {
>   case 'H':
>   Hflag = 1;
> @@ -96,9 +96,6 @@ main(int argc, char *argv[])
>   case 'R':
>   Rflag = 1;
>   break;
> - case 'f':   /* no longer documented. */
> - fflag = 1;
> - break;
>   case 'h':
>   hflag = 1;
>   break;
> @@ -261,12 +258,11 @@ done:
>  
>   if (ischmod) {
>   if (!fchmodat(AT_FDCWD, p->fts_accpath, oct ? omode :
> - getmode(set, p->fts_statp->st_mode), atflags)
> - || fflag)
> + getmode(set, p->fts_statp->st_mode), atflags))
>   continue;
>   } else if (!ischflags) {
>   if (!fchownat(AT_FDCWD, p->fts_accpath, uid, gid,
> - atflags) || fflag)
> + atflags))
>   continue;
>   } else {
>   if (!chflagsat(AT_FDCWD, p->fts_accpath, oct ? fset :
> 
> --UT4aJajqd8PRde7T--
> 

pledge telnet

[Theo de Raadt]

I really want to delete telnet entirely, but there are still occasions
when someone might want to use it on an intranet.  Other telnet tools
are probably worse shape.

This adds two pledge calls.

The subshell and skey support are removed (you can use ^Z), and you
cannot start a new telnet session.  Only the first session.  (That is
not handled nicely yet... I'm looking into it).

Index: Makefile
===
RCS file: /cvs/src/usr.bin/telnet/Makefile,v
retrieving revision 1.33
diff -u -p -u -r1.33 Makefile
--- Makefile20 Jul 2014 05:29:39 -  1.33
+++ Makefile13 Nov 2015 08:16:12 -
@@ -32,7 +32,7 @@
 
 PROG=  telnet
 
-CFLAGS+=-DKLUDGELINEMODE -DSKEY
+CFLAGS+=-DKLUDGELINEMODE
 CFLAGS+= -Wall
 LDADD+=-lcurses
 DPADD= ${LIBCURSES}
Index: commands.c
===
RCS file: /cvs/src/usr.bin/telnet/commands.c,v
retrieving revision 1.74
diff -u -p -u -r1.74 commands.c
--- commands.c  26 Oct 2015 00:33:03 -  1.74
+++ commands.c  13 Nov 2015 08:27:43 -
@@ -49,11 +49,6 @@
 #include 
 #include 
 
-#ifdef SKEY
-#include 
-#define PATH_SKEY  "/usr/bin/skey"
-#endif
-
 char   *hostname;
 
 typedef struct {
@@ -64,37 +59,9 @@ typedef struct {
 } Command;
 
 static char line[256];
-static char saveline[256];
 static int margc;
 static char *margv[20];
 
-#ifdef SKEY
-int
-skey_calc(int argc, char **argv)
-{
-   int status;
-
-   if(argc != 3) {
-   printf("usage: %s sequence challenge\n", argv[0]);
-   return 0;
-   }
-
-   switch(fork()) {
-   case 0:
-   execv(PATH_SKEY, argv);
-   exit (1);
-   case -1:
-   err(1, "fork");
-   break;
-   default:
-   (void) wait();
-   if (WIFEXITED(status))
-   return (WEXITSTATUS(status));
-   return (0);
-   }
-}
-#endif
-
 static void
 makeargv(void)
 {
@@ -103,12 +70,6 @@ makeargv(void)
 
 margc = 0;
 cp = line;
-if (*cp == '!') {  /* Special case shell escape */
-   strlcpy(saveline, line, sizeof(saveline)); /* save for shell command */
-   *argp++ = "!";  /* No room in string to get this */
-   margc++;
-   cp++;
-}
 while ((c = *cp)) {
int inquote = 0;
while (isspace((unsigned char)c))
@@ -779,7 +740,6 @@ static struct setlist Setlist[] = {
 #endif
 { "escape","character to escape back to telnet command mode", 0, 
 },
 { "rlogin", "rlogin escape character", 0,  },
-{ "tracefile", "file to write trace information to", SetNetTrace, (cc_t 
*)NetTraceFile},
 { " ", "" },
 { " ", "The following need 'localchars' to be toggled true", 0, 0 },
 { "flushoutput", "character to cause an Abort Output", 0,  },
@@ -1243,52 +1203,6 @@ telnetsuspend(int unused1, char *unused2
 return 1;
 }
 
-int
-shell(int argc, char *argv[])
-{
-long oldrows, oldcols, newrows, newcols, err;
-
-setcommandmode();
-
-err = (TerminalWindowSize(, ) == 0) ? 1 : 0;
-switch(vfork()) {
-case -1:
-   perror("Fork failed\r\n");
-   break;
-
-case 0:
-   {
-   /*
-* Fire up the shell in the child.
-*/
-   char *shellp, *shellname;
-
-   shellp = getenv("SHELL");
-   if (shellp == NULL)
-   shellp = "/bin/sh";
-   if ((shellname = strrchr(shellp, '/')) == 0)
-   shellname = shellp;
-   else
-   shellname++;
-   if (argc > 1)
-   execl(shellp, shellname, "-c", [1], (char *)NULL);
-   else
-   execl(shellp, shellname, (char *)NULL);
-   perror("Execl");
-   _exit(1);
-   }
-default:
-   (void)wait((int *)0);   /* Wait for the shell to complete */
-
-   if (TerminalWindowSize(, ) && connected &&
-   (err || ((oldrows != newrows) || (oldcols != newcols {
-   sendnaws();
-   }
-   break;
-}
-return 1;
-}
-
 static void
 close_connection(void)
 {
@@ -2012,6 +1926,7 @@ tn(int argc, char *argv[])
connected++;
break;
 }
+
 freeaddrinfo(res0);
 if (net < 0) {
return 0;
@@ -2029,6 +1944,7 @@ tn(int argc, char *argv[])
user = NULL;
}
 }
+
 if (user) {
env_define("USER", user);
env_export("USER");
@@ -2056,10 +1972,6 @@ static char
slchelp[] = "change state of special charaters ('slc ?' for more)",
displayhelp[] = "display operating parameters",
zhelp[] =   "suspend telnet",
-#ifdef SKEY
-   skeyhelp[] ="compute response to s/key challenge",
-#endif
-   shellhelp[] =   "invoke a subshell",
envhelp[] = "change environment variables ('environ ?' for more)",
modestring[] = "try to enter line or 

Re: pledge telnet

[Theo de Raadt]

> > I really want to delete telnet entirely,
> 
> I often use it for testing unencrypted SMTP and HTTP across the
> Internet.  Which tool would you recommend for that purpose?

nc(1).

> You might wish to cross-check these three points though:
> 
>  * Does "inet" actually allow the following setsockopt()
>arguments: SO_DEBUG, SO_RTABLE, IP_TOS, IPV6_TCLASS?
>I know nothing about socket options.

I think all of those should be deleted, except perhaps SO_RTABLE,
which should be done line in nc(1).

>  * Does ioctl(TIOCGWINSZ) in TerminalWindowSize(), sys_bsd.c
>require "ioctl"?  The pledge(2) manual seems to say so.

"tty"

>  * Do getpwnam() and getpwuid() in tn(), commands.c
>require "getpw"?  The pledge(2) manual seems to say so,
>or is "rpath" sufficient here and YP irrelevant?

Adding "getpw" at the initial pledge; the later one is main
loop and never needs it again.

Few other changes coming on top of that.

Re: pledge("stdio") for arch(1)/machine(1)

[Theo de Raadt]

> This straightforward pledge("stdio") is one of the last uncommitted ones
> from Theo's big 'tame in userland' diff and seems to have been
> overlooked so far.

I think arch.c gains no value from being pledged.  It adds a system
call to a program which makes no user-input decisions.

Re: pledge telnet

[Theo de Raadt]

> > > > I really want to delete telnet entirely,
> > > 
> > > I often use it for testing unencrypted SMTP and HTTP across the
> > > Internet.  Which tool would you recommend for that purpose?
> > 
> > nc(1).
> 
> I use telnet fairly often for connecting to things like crappy switches,
> crappy routers, APs of varying crappiness, etc. nc -t isn't close to being
> good enough for this, also with nc it's difficult to send things like ^C
> (even worse, if you use it much you forget about this and end up killing
> your connection). I wouldn't mind having it removed from base, but would
> need to go in ports unless nc gets a lot of polishing.

This conversation is going far too meta.

I am not deleting telnet.  That is why I am going through the effort
of shrinking it, and then pledging it.

pledge is even more relevant when dealing with bowls of unmaintained
spaghetti.

> > > You might wish to cross-check these three points though:
> > > 
> > >  * Does "inet" actually allow the following setsockopt()
> > >arguments: SO_DEBUG, SO_RTABLE, IP_TOS, IPV6_TCLASS?
> > >I know nothing about socket options.
> > 
> > I think all of those should be deleted, except perhaps SO_RTABLE,
> > which should be done line in nc(1).
> 
> TOS/TCLASS are somewhat important, nc and ssh in client mode also
> use them. IP_TOS is permitted by "inet". IPV6_TCLASS was missed but
> should be added, it's the IPv6 equivalent to IP_TOS.
> 
> Index: kern_pledge.c
> ===
> RCS file: /cvs/src/sys/kern/kern_pledge.c,v
> retrieving revision 1.106
> diff -u -p -r1.106 kern_pledge.c
> --- kern_pledge.c 10 Nov 2015 04:30:59 -  1.106
> +++ kern_pledge.c 13 Nov 2015 17:11:20 -
> @@ -1275,6 +1275,7 @@ pledge_sockopt(struct proc *p, int set, 
>   break;
>   case IPPROTO_IPV6:
>   switch (optname) {
> + case IPV6_TCLASS:
>   case IPV6_UNICAST_HOPS:
>   case IPV6_RECVHOPLIMIT:
>   case IPV6_PORTRANGE:

Yes, I decided they should stay.  Commited the same diff 1 minute ago
already.

Re: pledge telnet

[Theo de Raadt]

> > On 2015/11/13 09:59, Theo de Raadt wrote:
> > > > > I really want to delete telnet entirely,
> > > >
> > > > I often use it for testing unencrypted SMTP and HTTP across the
> > > > Internet.  Which tool would you recommend for that purpose?
> > >
> > > nc(1).
> > I use telnet fairly often for connecting to things like crappy switches,
> > crappy routers, APs of varying crappiness, etc. nc -t isn't close to being
> > good enough for this, also with nc it's difficult to send things like ^C
> > (even worse, if you use it much you forget about this and end up killing
> > your connection). I wouldn't mind having it removed from base, but would
> > need to go in ports unless nc gets a lot of polishing.
> 
> I always thought of telnet as a kind of discipline over the wire. There are
> even extensions (like RFC 2217) well-fitting discipline model.

Like a horse buggy in the inside lane of a 4-lane highway, there are going
to fatalities.

"discipline" applies to the user of this code -- it means "avoid any and all
unnecessary use".

> >From other hand, nc(1) is a "raw" tool with decent client-server model.
> 
> Is there any possibility to run nc(1) as a privsep server, and a telnet(1) as
> a client, talking to nc(1) server via IMSG (instead of doing network stuff
> directly)?

What's the goal.  To continue the lifetime of telne?  To make the nc code
more complicated and fragile?  Those are the only outcomes I see.

Re: pledge telnet

[Theo de Raadt]

> It is similar to (optional) XMODEM/ZMODEM disciplines over serial, IMO.

No, it is similar to  over the INTERNET, because the INTERNET
is nothing at all like a serial line, the later generally being nicely
contained to a single room.

> The goal is to delete classic telnet entirely and make it an
> (optional) discipline frontend for nc(1). In "telnet mode" nc(1)
> will only attach discipline and let user  use flow control features (like ^C).

You have a diff?

> It is not about extending a lifetime of telnet, it is about making telnet 
> truly
> optional by making it a discipline (or flow control protocol), not a separate
> tool.

If you can do it without adding *any complexity* to nc, fine.

Except I know you can't do that, it will add substantial complexity.
So this seems like a pointless discussion.  nc is already more than
complex enough.  Probably best to focus on making it more secure,
before making it support the stone age.

Re: pledge telnet

[Theo de Raadt]

> Can telnet be extended to coexist with nc -F? Manual only mentions ssh.

Please don't email while driving your horse buggy.

Re: give cron a sensible default max load_avg for batch jobs

[Theo de Raadt]

> This patch changes the default setting to 1.5 *
> (number_of_cpus_in_system) instead, which I find better matches modern
> behaviour.

A larger number is sensible in this position.

I would propose 8.  I don't agree with a calculation like that; the
amount of work a system can do should not be calculated like that.

Re: pledge route(8) with '-n' flag

[Theo de Raadt]

> On 16/11/15(Mon) 10:03, Ricardo Mestre wrote:
> > Hello!
> > 
> > Like Benoit said, monitor still needs dns all the time, but since pledge
> > was being called there again with dns pledge then I thought it wouldn't
> > abort. Taking that into consideration and looking at it a little bit
> > more, how about this?
> 
> IMHO this doesn't make sense if I explicitly say "route -n monitor"  I'd
> prefer not to have any address resolved
> 
> Otherwise I just say "route monitor".
> 
> I'd add that on the list of bugs discovered by pledge(2).

Yes... the monitor command should honour the -n flag.

Re: Reducing compilation warnings in imsg.c on FreeSBD

[Theo de Raadt]

> Both OSs define getdtablesize() as sysconf(_SC_OPEN_MAX). sysconf(3)
> returns a long, so it seems more correct to make getdtablesize(3) return
> a long or ssize_t. The return value has to be signed because sysconf(3)
> is specified by POSIX to return -1 and set errno on failure.

Don't be ridiculous, getdtablesize() is older than you are.  We are
not changing the type it returns.

Re: Reducing compilation warnings in imsg.c on FreeSBD

[Theo de Raadt]

> > Both OSs define getdtablesize() as sysconf(_SC_OPEN_MAX). sysconf(3)
> > returns a long, so it seems more correct to make getdtablesize(3) return
> > a long or ssize_t. The return value has to be signed because sysconf(3)
> > is specified by POSIX to return -1 and set errno on failure.
> 
> No, fds are ints so returning a long from getdtablesize(3) would be
> misleading and useless.

I want to return to this point.

We cannot change the types of old fundamentals without incurring
many new problems.

  int dtablesize = getdtablesize();

Hello new compiler warning everytime.

Re: pledge mv(1)

[Theo de Raadt]

> If only rename(2)'ing then it only needs "stdio rpath cpath",
> nevertheless if we need to copy to a different partition it also needs
> "wpath fattr" for writing and chmod/chown operations, and finally "proc
> exec" are needed due to (extracted directly from mv(1)'s man page) ->
> "Should the rename(2) call fail because the source and destination are
> on different file systems, mv will use cp(1) and rm(1) to accomplish the
> move.".
> 
> PS: It's not possible to reduce only to "stdio rpath cpath" since
> rename(2) happens way before copy() or fastcopy() functions and if it
> fails it will call copy() anyway which needs further permissions.

We are going to skip doing mv, until other refactorings get done:

mv.c:   execl(_PATH_CP, "cp", "-PRp", "--", from, to, (char *)NULL);
mv.c:   execl(_PATH_RM, "rm", "-rf", "--", from, (char *)NULL);

It is quite horrible...

Re: Better ASR support in ospfd

[Theo de Raadt]

> On Mon, Oct 26, 2015 at 04:40:12PM +0100, Claudio Jeker wrote:
> > ospfd has some issues with self-originated networks and building summary
> > entries for those in case the router is an ABR (area border router).
> > This diff should hopefully fix all of the troubles. It changes a bit the
> > way we do nexthop calculation in the SPF/rib calculation to make sure we
> > handle self-originated networks correctly. As a side-effect it should also
> > remove the behaviour where ospfd added a OSPF route for all those
> > self-originated routes. Also the way we track active areas is changed to
> > be actually the way it should be.
> > 
> > Please test this on all ospfd setups and check for issues.
> 
> I guess nobody is using ospfd anymore.

yup, we should remove it.

Re: move cron socket to /var/run/cron.sock

[Theo de Raadt]

> There's limited backward compatibility so you can run a new crontab
> with an older cron daemon.

Why?

That makes no sense.  This kind of compat bubbles up.  It should
be deleted within a week, so why bother writing it...

Re: pledge audioctl

[Theo de Raadt]

> I am trying to add pledge(2) to audioctl(1),
> but it gets SIGABRT'ed under any pledge promises.
> (Indeed, I have pledged everything in a desperate attempt.)
> 
> Looking at gdb and a ktrace, /dev/audioctl gets opened fine,
> but then it fails on an ioctl in getinfo()
> 
>  23472 audioctl CALL  ioctl(3,AUDIO_GETDEV,0x10d3bed03a20)
>  23472 audioctl PLDG  ioctl, "ioctl", errno 1 Operation not permitted
>  23472 audioctl PSIG  SIGABRT SIG_DFL code <1054761850>
>  23472 audioctl NAMI  "audioctl.core"
> 
> Obviously, "ioctl" is in the pledge call.
> Is it simply that AUDIO_GETDEV is not included in the ioctl pledge?

...

> Is there a way to pledge audio stuff like this with current pledge?

No.  Much of ioctl is considered attack surface, and we will look into
handling those things after the less intense parts of the tree are
correctly annotated/managed by pledge.

Priority is on the more procedural code.  More than 80% done...

Re: printf(3) wording

[Theo de Raadt]

> > > > i don;t know how these implementations work, so it's hard to say.
> > > > perhaps they are interpolated. maybe use cvs to track down the author
> > > > and ask them?
> > > > 
> > > > whatever the outcome, if you want to change this text you probably want
> > > > to adjust a few more:
> > > > 
> > > > /usr/src/lib/libc/gen/err.3:for later interpolation by the
> > > > /usr/src/lib/libc/gen/setproctitle.3:for later interpolation by
> > > > /usr/src/lib/libc/gen/syslog.3:for later interpolation by
> > > > /usr/src/lib/libc/stdio/printf.3:for later interpolation by
> > > 
> > > Hm, probably just my English;
> > > sorry for the noise.
> > > 
> > >   Jan
> > > 
> > 
> > not neccessarily. the author may have been confused too. who knows. i
> > had to look up "interpolation" myself. it's a word i'd prefer to avoid
> > in man pages if we can ;)
> 
> interpretation is the correct word to use.

another option is "expansion"

Re: [PATCH] rcs: buf_free/rcsnum_free

[Theo de Raadt]

> > I think it should be moved into Attic. It's not like we've been nice to
> > the pcc tree-import either after it lacked attention.
> 
> I agree, it has been unlinked from the build for more than 5 years.

I don't agree.  I still have some hope.

Yes, it has problems.  But go look at the code in gnu/usr.bin/cvs

There were good reasons to start the effort...

Re: Tweaks to malloc(3) manpage

[Theo de Raadt]

> 1. I don't see much reason to mention calloc() as an alternative to
> reallocarray() when it's the worse option.

calloc() still remains the portable option.  Something should probably
still be mentioned here, otherwise people fall back to unchecked
malloc -- no matter what is stated further below regarding overflow
checks.

> 2. Use size > 0 when testing overflow.

That feels wrong.  The size variables in some code contexts may
mistakenly be signed, yet assigned an unsigned value from a caller.
You are trying to fight a losing battle with undefined behaviour
handling in modern compilers, and programmers who don't have time to
use static analysis tools.  In case size is signed, it would be better
to FAIL the overflow check as happens currently, rather multiply a
negative number, end up with a result, malloc, and then screw things up.

Re: Drop register keyword from less(1)

[Theo de Raadt]

less is code imported on a regular basis from upstream.
Look at the commit log.

> Every one of these is in a var declaration, so a megadiff is probably
> the easiest way to do it.
> 
> ok?
> 
> 
> Index: brac.c
> ===
> RCS file: /cvs/src/usr.bin/less/brac.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 brac.c
> --- brac.c25 Apr 2014 13:38:21 -  1.6
> +++ brac.c1 Nov 2015 19:19:16 -
> @@ -25,13 +25,13 @@
>   */
>   public void
>  match_brac(obrac, cbrac, forwdir, n)
> - register int obrac;
> - register int cbrac;
> + int obrac;
> + int cbrac;
>   int forwdir;
>   int n;
>  {
> - register int c;
> - register int nest;
> + int c;
> + int nest;
>   POSITION pos;
>   int (*chget)();
>  
> Index: ch.c
> ===
> RCS file: /cvs/src/usr.bin/less/ch.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 ch.c
> --- ch.c  25 Apr 2014 13:38:21 -  1.10
> +++ ch.c  1 Nov 2015 19:19:16 -
> @@ -144,11 +144,11 @@ static int ch_addbuf();
>   int
>  ch_get()
>  {
> - register struct buf *bp;
> - register struct bufnode *bn;
> - register int n;
> - register int slept;
> - register int h;
> + struct buf *bp;
> + struct bufnode *bn;
> + int n;
> + int slept;
> + int h;
>   POSITION pos;
>   POSITION len;
>  
> @@ -410,8 +410,8 @@ end_logfile()
>   public void
>  sync_logfile()
>  {
> - register struct buf *bp;
> - register struct bufnode *bn;
> + struct buf *bp;
> + struct bufnode *bn;
>   int warned = FALSE;
>   BLOCKNUM block;
>   BLOCKNUM nblocks;
> @@ -448,9 +448,9 @@ sync_logfile()
>  buffered(block)
>   BLOCKNUM block;
>  {
> - register struct buf *bp;
> - register struct bufnode *bn;
> - register int h;
> + struct buf *bp;
> + struct bufnode *bn;
> + int h;
>  
>   h = BUFHASH(block);
>   FOR_BUFS_IN_CHAIN(h, bn)
> @@ -468,7 +468,7 @@ buffered(block)
>   */
>   public int
>  ch_seek(pos)
> - register POSITION pos;
> + POSITION pos;
>  {
>   BLOCKNUM new_block;
>   POSITION len;
> @@ -537,8 +537,8 @@ ch_end_seek()
>   public int
>  ch_beg_seek()
>  {
> - register struct bufnode *bn;
> - register struct bufnode *firstbn;
> + struct bufnode *bn;
> + struct bufnode *firstbn;
>  
>   /*
>* Try a plain ch_seek first.
> @@ -595,7 +595,7 @@ ch_tell()
>   public int
>  ch_forw_get()
>  {
> - register int c;
> + int c;
>  
>   if (thisfile == NULL)
>   return (EOI);
> @@ -658,7 +658,7 @@ ch_setbufspace(bufspace)
>   public void
>  ch_flush()
>  {
> - register struct bufnode *bn;
> + struct bufnode *bn;
>  
>   if (thisfile == NULL)
>   return;
> @@ -725,8 +725,8 @@ ch_flush()
>   static int
>  ch_addbuf()
>  {
> - register struct buf *bp;
> - register struct bufnode *bn;
> + struct buf *bp;
> + struct bufnode *bn;
>  
>   /*
>* Allocate and initialize a new buffer and link it 
> @@ -750,7 +750,7 @@ ch_addbuf()
>   static void
>  init_hashtbl()
>  {
> - register int h;
> + int h;
>  
>   for (h = 0;  h < BUFHASH_SIZE;  h++)
>   {
> @@ -765,7 +765,7 @@ init_hashtbl()
>   static void
>  ch_delbufs()
>  {
> - register struct bufnode *bn;
> + struct bufnode *bn;
>  
>   while (ch_bufhead != END_OF_CHAIN)
>   {
> Index: charset.c
> ===
> RCS file: /cvs/src/usr.bin/less/charset.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 charset.c
> --- charset.c 25 Apr 2014 13:38:21 -  1.10
> +++ charset.c 1 Nov 2015 19:19:16 -
> @@ -134,9 +134,9 @@ public int binattr = AT_STANDOUT;
>  ichardef(s)
>   char *s;
>  {
> - register char *cp;
> - register int n;
> - register char v;
> + char *cp;
> + int n;
> + char v;
>  
>   n = 0;
>   v = 0;
> @@ -189,11 +189,11 @@ ichardef(s)
>   */
>   static int
>  icharset(name, no_error)
> - register char *name;
> + char *name;
>   int no_error;
>  {
> - register struct charset *p;
> - register struct cs_alias *a;
> + struct charset *p;
> + struct cs_alias *a;
>  
>   if (name == NULL || *name == '\0')
>   return (0);
> @@ -233,7 +233,7 @@ icharset(name, no_error)
>   static void
>  ilocale()
>  {
> - register int c;
> + int c;
>  
>   for (c = 0;  c < (int) sizeof(chardef);  c++)
>   {
> Index: cmdbuf.c
> ===
> RCS file: /cvs/src/usr.bin/less/cmdbuf.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 cmdbuf.c
> --- cmdbuf.c  25 Apr 2014 13:38:21 -  1.7
> +++ cmdbuf.c  1 Nov 2015 19:19:16 -
> @@ -525,7 +525,7 @@ cmd_ichar(cs, clen)
>   static int
>  

Re: tcpdump: stop building SLIP support

[Theo de Raadt]

Why?

We don't have decnet code either, yet it is nice to sniff it if you
see it.

> Some time ago mpi removed the sl(4) driver, IIUC this makes a bunch of
> code useless in tcpdump.
> 
> The diff below only removes the define.  If people prefer, I could also
> kill some noise by removing the useless code in print-sl.c, leaving only
> the stub functions.
> 
> Comments / ok?
> 
> Index: Makefile
> ===
> RCS file: /cvs/src/usr.sbin/tcpdump/Makefile,v
> retrieving revision 1.59
> diff -u -p -p -u -r1.59 Makefile
> --- Makefile  14 Oct 2015 04:55:17 -  1.59
> +++ Makefile  4 Nov 2015 18:05:23 -
> @@ -28,7 +28,7 @@ CFLAGS+=-Wall -I${.CURDIR}/../../sbin/pf
>  # for pcap-int.h
>  CFLAGS+=-I${.CURDIR}/../../lib/libpcap
>  
> -CFLAGS+=-DCSLIP -DPPP -DHAVE_FDDI -DETHER_SERVICE -DHAVE_NET_SLIP_H 
> -DHAVE_ETHER_NTOHOST -DINET6
> +CFLAGS+=-DCSLIP -DPPP -DHAVE_FDDI -DETHER_SERVICE -DHAVE_ETHER_NTOHOST 
> -DINET6
>  
>  LDADD+=  -lpcap -ll -lcrypto
>  DPADD+=  ${LIBL} ${LIBPCAP} ${LIBCRYPTO}
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

Re: chgrp(1) & chown(8): mark -h and -R as mutually exclusive

[Theo de Raadt]

I don't think it makes it clearer; it makes it more confusing.

The usage messages of programs are not a sufficent grammer to exactly
describe what conflicts with what.  Taken too far, it would bewilder
newcomers.

> the command line arguments -h and -R for chgrp and chown are mutually
> exclusive. The patch below changes the markup and the usage strings to
> make this clearer.
> 
> Cheers,
> Frederic
> 
> Index: bin/chmod/chgrp.1
> ===
> RCS file: /cvs/src/bin/chmod/chgrp.1,v
> retrieving revision 1.16
> diff -u -p -r1.16 chgrp.1
> --- bin/chmod/chgrp.1 21 Jan 2014 22:35:44 -  1.16
> +++ bin/chmod/chgrp.1 5 Nov 2015 17:09:11 -
> @@ -40,9 +40,7 @@
>  .Nd change group
>  .Sh SYNOPSIS
>  .Nm chgrp
> -.Op Fl h
> -.Oo
> -.Fl R
> +.Oo Fl h | R
>  .Op Fl H | L | P
>  .Oc
>  .Ar group
> Index: bin/chmod/chmod.c
> ===
> RCS file: /cvs/src/bin/chmod/chmod.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 chmod.c
> --- bin/chmod/chmod.c 9 Oct 2015 01:37:06 -   1.38
> +++ bin/chmod/chmod.c 5 Nov 2015 17:09:11 -
> @@ -350,11 +350,11 @@ usage(void)
>   __progname, ischmod ? "mode" : "flags");
>   else
>   fprintf(stderr,
> - "usage: %s [-h] [-R [-H | -L | -P]] %s file ...\n",
> + "usage: %s [-h | -R [-H | -L | -P]] %s file ...\n",
>   __progname, ischown ? "owner[:group]" : "group");
>   if (ischown)
>   fprintf(stderr,
> - "   %s [-h] [-R [-H | -L | -P]] :group file ...\n",
> + "   %s [-h | -R [-H | -L | -P]] :group file ...\n",
>   __progname);
>   exit(1);
>  }
> Index: bin/chmod/chown.8
> ===
> RCS file: /cvs/src/bin/chmod/chown.8,v
> retrieving revision 1.19
> diff -u -p -r1.19 chown.8
> --- bin/chmod/chown.8 14 Sep 2015 20:06:58 -  1.19
> +++ bin/chmod/chown.8 5 Nov 2015 17:09:11 -
> @@ -37,17 +37,13 @@
>  .Nd change file owner and group
>  .Sh SYNOPSIS
>  .Nm chown
> -.Op Fl h
> -.Oo
> -.Fl R
> +.Oo Fl h | R
>  .Op Fl H | L | P
>  .Oc
>  .Ar owner Ns Op : Ns Ar group
>  .Ar
>  .Nm chown
> -.Op Fl h
> -.Oo
> -.Fl R
> +.Oo Fl h | R
>  .Op Fl H | L | P
>  .Oc
>  .Pf : Ar group
> 

Re: save less

[Theo de Raadt]

>On Sat, Nov 07, 2015 at 12:12:55AM -0500, Michael McConville wrote:
>> Ted Unangst wrote:
>> > less has a peculiar estrdup function. unlike ecalloc etc., it only
>> > prints an error but doesn't quit. But the callers don't seem to check
>> > for null. And in many places they call a function called save()
>> > instead.
>> > 
>> > It is clearer to make estrdup() quit and use it everywhere.
>> 
>> ok mmcc@
>> 
>> That said, error()'s name very strongly implies that it quits, so we
>> should look into fixing that too.
>
>I don't think it implies anything of the sort, an error is still an
>error if it doesn't kill the program.

Feeling a lot of dejavu here.

Re: Drop register keyword from less(1)

[Theo de Raadt]

> I think we should move to the Illumos fork, it looks good. I've got it
> building easily enough, will take a look at porting our changes at some
> point.

I was the first one to bring it up with Todd about 4 weeks ago.
I am a big fan of this, becuase it will allow us to pledge^Wrefactor
it also.

Re: cvs(1) simplification

[Theo de Raadt]

> I can tell that rev can be VERY large... Wait, am I remembering these
> (Open)CVS internals all of a sudden again? :P

Some of us have memories wired for guilt.

Re: fix ksh histfile owner

[Theo de Raadt]

> ksh does a little dance to try and gift history files to their original owner
> if it's somehow running as a different user. this of course only works as
> root, and is probably a terrible idea.

When I found this with tame, I got worried about the implications of
how ksh is opening the file in the first place.  No *chown call should
be present in ksh!  

> ksh should simply refuse to open a history file that's owned by somebody else.

good policy.

ok deraadt

Re: reference syscall.h in pledge.2

[Theo de Raadt]

>Does it make sense to reference the syscall numbers in pledge(2)?

No not really.  By 5.9 release the kernel printf's will go away,
and people won't get such alerts.  Maybe they will get kernel log's,
but I will consider generating them with system call names.

Re: compress: tighter pledge

[Theo de Raadt]

> Here's an attempt to tighten compress/gzip's pledge:
> 
> Due to the use of fts(3), we always require rpath, even for
> gzip out.
> 
> We only write to stdio and never to any files...
> * if we are in cat mode (-c, zcat)
> * if we are in test mode (-t)
> * if there are no file arguments and there is no -o outfile
> 
> Does the logic sound correct?  It could use more testing.

Seems sound.

An even better plan would to recognize this stdin/out mode,
and entirely bypass that fts_open code.  Wow, that's quite
the mess isn't it.  I cannot figure out how to refactor it.
So many variables..

Re: pledge csh nice = death

[Theo de Raadt]

> It looks like csh would currently need to pledge("id") in order for the
> builtin nice to work --- setpriority() is called in three places
> depending on how nice is invoked.  However, adding "id" to a shell
> seems a bit scary.
> 
> Would it be preferable to mark
> [SYS_setpriority] = PLEDGE_PROC | PLEDGE_ID?

I prefer that, and will do that.

However I'd like a semantic where priority can only be adjusted downwards.
semarie was talking about that as well.  I'll commit PLEDGE_PROC | PLEDGE_ID,
and hope you two write such a diff...

Re: pledge(2) and exec

[Theo de Raadt]

> I am however curious to this patch. By pledging ksh with exec it appears 
> to me that once a pledged process is execve(2)d it looses it's already 
> made pledges.

Yes, because that is what it needs.

> This to me seems like 
> something that might be undesirable (find remote code execution->insert 
> exec of application->do some evil network activity)
> Is above observation correct or am I missing something?

It is entirely desirable.

It is what a shell does.  It runs other programs.  Hopefully those
other programs are also self-pledging.


You need to step back.  The only alternative you leave with your suggestion
is that a shell cannot start programs with full unix functionality.  What
kind of a system would that be?

Here's what pledge does bring to the table though.  Let's say a shell
like ksh has a bug inside it.  An attacker gets control of the guts of
ksh.  Now what can he do?  Lots.  But what can't he do.  He can't open
a socket.  There's lots he can't do.  He has to go through the exec path.
Less choices.

This means an attacker has to write more detailed attack code.

Let's look at ASLR.  What if the attacker can guess. Yes, ASLR is
useless.

Let's look at W^X, which is just a policy the programmer should follow,
not a mandatory mapping rule by the kernel.  Let's say there is a page
is incorrectly labelled by the programmer.  Game over. Right?

Let's look at the stack protector.  Some functions don't have enough
bytes in their frame, so for performance reasons they are not protected.
Game over, right?

Except, keeping it real, each of these things are cheap, but added up
in a large pile, they act as a mitigation.

About 15 programs in the tree are using "proc exec", either because they
are in an intentional shell, or because historically unix had lots of
!command support.  We'll also push back on this latter thing, to try to
shrink the usage.

But ksh is a shell, and so is tmux.  nicm and I have other plans for
tmux, but I doubt you'll ever see a privsep ksh.

Re: ftp: ctype interfaces need unsigned chars

[Theo de Raadt]

> Some isfoo(char) usages crept back into ftp

Hmm.  I wonder how we can keep these errors out of base.
Having to re-audit all the time is painful.

Re: npppd: simplify and lock down priv_open()

[Theo de Raadt]

> Currently, npppd's PRIVSEP_OPEN message (abstracted as priv_open()) 
> accepts arbitrary open() flags and passes a mode argument.  That 
> seems...unwise.
> 
> In particular, it never passes O_CREAT, so the mode argument isn't needed.  
> Indeed, the only open 'flags' it needs are O_RDONLY and O_RDWR.  If we 
> also permit O_NONBLOCK then the caller can skip using ioctl(FIONBIO) too.
> 
> So:
>  * kill the 'mode' argument to PRIVSEP_OPEN and priv_open()
>  * fail a PRIVSEP_OPEN call if it's passed any flags other than 
>O_ACCMODE or O_NONBLOCK
>  * paranoia: mask O_CREAT when calling open() with only two arguments
>  * instead of using ioctl(FIONBIO) after the fact, pass O_NONBLOCK to 
>priv_open()
> 
> I don't use npppd, so I only know this compiles and haven't actually 
> tested it in any way.

Looks like a good start.

I don't know the code either, but it is probably better if privsep's
had more narrow task-specific operations.  Like
open-specific-file-for-read, and open log-over-there.  Privsep
operations should be tightly specified, not very generic.

Then the child can't open anything.  It feels like this should be
PRIVSEP_GET_NEW_TUN (with the parent selecting which one)

Re: kdump perjury: syscall 5

[Theo de Raadt]

> Index: sys/kern/kern_pledge.c
> ===
> RCS file: /var/cvs/src/sys/kern/kern_pledge.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 kern_pledge.c
> --- sys/kern/kern_pledge.c9 Oct 2015 05:30:03 -   1.4
> +++ sys/kern/kern_pledge.c10 Oct 2015 06:07:42 -
> @@ -567,6 +567,11 @@ pledge_namei(struct proc *p, char *origp
>   strcmp(path, "/etc/localtime") == 0)
>   return (0);
>  
> + /* For {get,set}proto*() */
> + if ((p->p_pledgenote == TMN_RPATH) &&
> + strcmp(path, "/etc/protocols") == 0)
> + return (0);
> +
>   /* /usr/share/nls/../libc.cat has to succeed for strerror(3). */
>   if ((p->p_pledgenote == TMN_RPATH) &&
>   strncmp(path, "/usr/share/nls/",

I would only do this if the libc parsers for this are fully re-audited.

Please think carefully about the list of paths that are allowed, and why.
It's a tricky balance.

Re: ftp: ctype interfaces need unsigned chars

[Theo de Raadt]

> as well as this:
> 
> > --- tcpdump/print-ipsec.c
> > +++ /tmp/cocci-output-17550-499a71-print-ipsec.c
> > @@ -101,7 +101,7 @@ esp_init (char *espspec)
> > s[0] = espkey[2*i];
> > s[1] = espkey[2*i + 1];
> > s[2] = 0;
> > -   if (!isxdigit(s[0]) || !isxdigit(s[1])) {
> > +   if (!isxdigit((unsigned char)s[0]) || !isxdigit((unsigned 
> > char)s[1])) {
> 
> 
> For tcpdump/print-decnet.c, I think it's best to change the variable type, 
> as putchar() expects an int ("EOF or unsigned char") like isprint():
> 
> 
> --- tcpdump/print-decnet.c21 Aug 2015 02:07:32 -  1.14
> +++ tcpdump/print-decnet.c11 Oct 2015 03:25:02 -
> @@ -756,11 +756,11 @@ dnname_string(u_short dnaddr)
>  static void
>  pdata(u_char *dp, u_int maxlen)
>  {
> - char c;
> + int c;
>   u_int x = maxlen;
>  
>   while (x-- > 0) {
> - c = *dp++;
> + c = (unsigned char)*dp++;
>   if (isprint(c))
>   putchar(c);
>   else
> 
> 
> For tcpdump/smbutil.c...gaaahh.  Add the return of atoi() to a 
> pointer and then skip all digits?  That has *FUN* results with negative 
> numbers and numbers greater than the length of the buffer!  fdata1() needs 
> to be hit repeatedly with a big stick until it stops assuming that no one 
> makes errors.

Luckily, our tcpdump is privsep.

One day something like this is going to hurt very badly.  Poor other people.

Re: prefer dprintf() over snprintf()+write()

[Theo de Raadt]

> Actually, plain old printf should be OK in ping.c since buffering
> is disabled for ping -f.  If you want to keep dprintf(), I think
> we can lose the setbuf() call.  Whatever you decide, it would
> be nice to make ping6.c match.

No, disagree strongly.

ping is doing this inside a signal handler.  Our snprintf has been
verified (sans floating point) for use in signal handlers.  Other
libc/stdio functions did not receive any study, because their use in a
signal handler is unacceptable.

We should never place anything like this in signal handler.  Even if
unbuffered printf is safe, someone might copy this code to elsewhere
and not pick up on the nuanced use.  Also maybe it is safe on
OpenBSD...  it surely is not elsewhere...  and people do copy our
code.

Re: Permitting the override of MACHINE_ARCH in amd64/param.h

[Theo de Raadt]

> I was wondering if it would be possible to allow the override the
> definition of MACHINE_ARCH /__MACHINE_ARCH in amd64/param.h by wrapping
> them around ifndef statement.
> 
> Index: sys/arch/amd64/include/param.h
> ===
> RCS file: /cvs/src/sys/arch/amd64/include/param.h,v
> retrieving revision 1.22
> diff -u -p -u -r1.22 param.h
> --- sys/arch/amd64/include/param.h26 Mar 2013 05:04:10 -  1.22
> +++ sys/arch/amd64/include/param.h12 Oct 2015 15:17:03 -
> @@ -45,8 +45,12 @@
> 
>  #define  _MACHINEamd64
>  #define  MACHINE "amd64"
> +#ifndef _MACHINE_ARCH
>  #define  _MACHINE_ARCH   amd64
> +#endif
> +#ifndef MACHINE_ARCH
>  #define  MACHINE_ARCH"amd64"
> +#endif
>  #define  MID_MACHINE MID_AMD64
> 
>  #define  PAGE_SHIFT  12
> 
> Being able to override is useful in scenarios where you group arch name
> variants under a common alias, at the moment using such a mechanism
> fails under OpenBSD/amd64 as whatever is passed in during build time is
> overwritten by the inclusion of .

That makes absolutely no sense.  I don't see any purpose to this.  You
have failed to explain the usage case; I don't believe there is any
scenario where people benefit from more architecture divergence.

Re: prefer dprintf() over snprintf()+write()

[Theo de Raadt]

> Ah, I didn't realize that pinger() was still called via a signal
> handler in ping.  It looks like ping6 is better in this regard.

ping and ping6 need to be merged, as happened to traceroute.

First step: make all ping6's options match ping options.  Rename
ping6 options with wild abandon if ping has a similar option.
Don't encode backwards compat behaviour for those options in ping6;
throw them out the window.

However, each time ping6's guts are found to better, use the ping6
guts.  The engine is a bit better. 

This should have happened years ago already.  Every year of realtime,
this gets harder to pull off; there will be more resistance against
change.

Re: ttyname

[Theo de Raadt]

> When guenther@ switched isatty(3) to F_ISATTY, he forgot ttyname(3).

That was a change I made.

> With this, simple callers of ttyname(3) like tty(1) and who(1) no
> longer need pledge("tty").

That is correct, these programs could do without the ability to set
modes on the tty.  I think this is a good idea.  It may be hard to
document later; but we'll get there.

> Index: lib/libc/gen/ttyname.c
> ===
> RCS file: /cvs/src/lib/libc/gen/ttyname.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 ttyname.c
> --- lib/libc/gen/ttyname.c12 Sep 2015 14:56:50 -  1.15
> +++ lib/libc/gen/ttyname.c12 Oct 2015 10:58:35 -
> @@ -32,7 +32,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -68,7 +67,6 @@ int
>  ttyname_r(int fd, char *buf, size_t len)
>  {
>   struct stat sb;
> - struct termios ttyb;
>   DB *db;
>   DBT data, key;
>   struct {
> @@ -77,7 +75,7 @@ ttyname_r(int fd, char *buf, size_t len)
>   } bkey;
>  
>   /* Must be a terminal. */
> - if (tcgetattr(fd, ) < 0)
> + if (fcntl(fd, F_ISATTY) == -1)
>   return (errno);
>   /* Must be a character device. */
>   if (fstat(fd, ))
> Index: usr.bin/tty/tty.c
> ===
> RCS file: /cvs/src/usr.bin/tty/tty.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 tty.c
> --- usr.bin/tty/tty.c 9 Oct 2015 01:37:09 -   1.9
> +++ usr.bin/tty/tty.c 12 Oct 2015 11:02:51 -
> @@ -43,7 +43,7 @@ main(int argc, char *argv[])
>   int ch, sflag;
>   char *t;
>  
> - if (pledge("stdio rpath tty", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
>  
>   sflag = 0;
> Index: usr.bin/who/who.c
> ===
> RCS file: /cvs/src/usr.bin/who/who.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 who.c
> --- usr.bin/who/who.c 11 Oct 2015 23:29:56 -  1.25
> +++ usr.bin/who/who.c 12 Oct 2015 11:03:11 -
> @@ -74,7 +74,7 @@ main(int argc, char *argv[])
>  
>   setlocale(LC_ALL, "");
>  
> - if (pledge("stdio rpath getpw tty", NULL) == -1)
> + if (pledge("stdio rpath getpw", NULL) == -1)
>   err(1, "pledge");
>  
>   if (mytty = ttyname(0)) {
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 

Re: Char cleanup in lex(1)

[Theo de Raadt]

Our tree uses lex pretty much as-is from upstream, so this refactoring
isn't the way to go.

If you find an actual bug however.. fix them, using the established
practice.

> When scanning for is*() function uses with signed chars, I found that
> lex(1) uses an unecessary #define for unsigned char. The below diff
> removes it and fixes an undefined is*() usage or two as well.
> 
> 
> Index: ecs.c
> ===
> RCS file: /cvs/src/usr.bin/lex/ecs.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 ecs.c
> --- ecs.c 4 Jun 2003 17:34:44 -   1.6
> +++ ecs.c 12 Oct 2015 14:13:36 -
> @@ -107,9 +107,9 @@ int fwd[], bck[], num;
>  /* mkeccl - update equivalence classes based on character class xtions
>   *
>   * synopsis
> - *Char ccls[];
> + *unsigned char ccls[];
>   *int lenccl, fwd[llsiz], bck[llsiz], llsiz, NUL_mapping;
> - *void mkeccl( Char ccls[], int lenccl, int fwd[llsiz], int bck[llsiz],
> + *void mkeccl( unsigned char ccls[], int lenccl, int fwd[llsiz], int 
> bck[llsiz],
>   *   int llsiz, int NUL_mapping );
>   *
>   * ccls contains the elements of the character class, lenccl is the
> @@ -120,7 +120,7 @@ int fwd[], bck[], num;
>   */
>  
>  void mkeccl( ccls, lenccl, fwd, bck, llsiz, NUL_mapping )
> -Char ccls[];
> +unsigned char ccls[];
>  int lenccl, fwd[], bck[], llsiz, NUL_mapping;
>   {
>   int cclp, oldec, newec;
> Index: flexdef.h
> ===
> RCS file: /cvs/src/usr.bin/lex/flexdef.h,v
> retrieving revision 1.8
> diff -u -p -r1.8 flexdef.h
> --- flexdef.h 10 Oct 2015 05:47:54 -  1.8
> +++ flexdef.h 12 Oct 2015 14:13:36 -
> @@ -81,7 +81,6 @@
>  
>  /* Always be prepared to generate an 8-bit scanner. */
>  #define CSIZE 256
> -#define Char unsigned char
>  
>  /* Size of input alphabet - should be size of ASCII set. */
>  #ifndef DEFAULT_CSIZE
> @@ -633,7 +632,7 @@ extern int end_of_buffer_state;
>  
>  extern int lastccl, *cclmap, *ccllen, *cclng, cclreuse;
>  extern int current_maxccls, current_max_ccl_tbl_size;
> -extern Char *ccltbl;
> +extern unsigned char *ccltbl;
>  
>  
>  /* Variables for miscellaneous information:
> @@ -701,10 +700,10 @@ void flex_free PROTO((void*));
>   (char *) reallocate_array( (void *) array, size, sizeof( char ) )
>  
>  #define allocate_Character_array(size) \
> - (Char *) allocate_array( size, sizeof( Char ) )
> + (unsigned char *) allocate_array( size, sizeof( unsigned char ) )
>  
>  #define reallocate_Character_array(array,size) \
> - (Char *) reallocate_array( (void *) array, size, sizeof( Char ) )
> + (unsigned char *) reallocate_array( (void *) array, size, sizeof( 
> unsigned char ) )
>  
>  
>  /* Used to communicate between scanner and parser.  The type should really
> @@ -755,7 +754,7 @@ extern void ccl2ecl PROTO((void));
>  extern int cre8ecs PROTO((int[], int[], int));
>  
>  /* Update equivalence classes based on character class transitions. */
> -extern void mkeccl PROTO((Char[], int, int[], int[], int, int));
> +extern void mkeccl PROTO((unsigned char[], int, int[], int[], int, int));
>  
>  /* Create equivalence class for single character. */
>  extern void mkechar PROTO((int, int[], int[]));
> @@ -834,16 +833,16 @@ extern void bubble PROTO((int [], int));
>  extern void check_char PROTO((int c));
>  
>  /* Replace upper-case letter to lower-case. */
> -extern Char clower PROTO((int));
> +extern unsigned char clower PROTO((int));
>  
>  /* Returns a dynamically allocated copy of a string. */
>  extern char *copy_string PROTO((register const char *));
>  
>  /* Returns a dynamically allocated copy of a (potentially) unsigned string. 
> */
> -extern Char *copy_unsigned_string PROTO((register Char *));
> +extern unsigned char *copy_unsigned_string PROTO((register unsigned char *));
>  
>  /* Shell sort a character array. */
> -extern void cshell PROTO((Char [], int, int));
> +extern void cshell PROTO((unsigned char[], int, int));
>  
>  /* Finish up a block of data declarations. */
>  extern void dataend PROTO((void));
> @@ -858,7 +857,7 @@ extern void flexerror PROTO((const char[
>  extern void flexfatal PROTO((const char[]));
>  
>  /* Convert a hexadecimal digit string to an integer value. */
> -extern int htoi PROTO((Char[]));
> +extern int htoi PROTO((unsigned char[]));
>  
>  /* Report an error message formatted with one integer argument. */
>  extern void lerrif PROTO((const char[], int));
> @@ -886,10 +885,10 @@ extern void mkdata PROTO((int));/* gene
>  extern int myctoi PROTO((char []));
>  
>  /* Return character corresponding to escape sequence. */
> -extern Char myesc PROTO((Char[]));
> +extern unsigned char myesc PROTO((unsigned char[]));
>  
>  /* Convert an octal digit string to an integer value. */
> -extern int otoi PROTO((Char [] ));
> +extern int otoi PROTO((unsigned char[] ));
>  
>  /* Output a 

Re: tame userland diff

[Theo de Raadt]

> Assume you have a bad program1 and you write your tame(2)-ed program2 that 
> disallows execution of program1. But you also have to use my un-tame(2)-ed 
> program3 that allows execution of program1. How does your tame(2)-ed 
> program2 protect you now against executing program1 ? You still risk 
> executing it by using program3.

Crossing an execve boundary can be risky or not risky -- it depends,
and your brush is too broad when you paint it all safe.  Perhaps that
is true for programs which INTEND to execve, but tame is also there
for programs which were never supposed to execve at all, and then due
to a bug, suddenly try to do so.

> Besides, what if all programs are tame(2)-ed, does that 
> mean that no program will be able to call any other program anymore ?

Don't understand that sentence at all.

tame(2) is still incomplete.  There are secret planned features not
yet shared with the masses.  tame is designed to be flexible and
extensible; some features are coming which will help, but first we
should use the existing tame features to solve the programs which only
need them.

By talking -- instead of reviewing or providing diffs -- you are not
part of the solution.  You being precipitate, that gunk at the dark
bottom of the lake.  Rise up to the sunlight, review some of the diffs
that have been sent out, verify the ones that DO work, look for ones
that subly need more, and wait for future features needed by programs
which are a bit different.

Instead of handwaving.

> So unconditionally disallowing exec(2) for the purpose of avoiding bad 
> programs being run seems somewhat pointless to me. I don't think the attack 
> surface of those bad programs goes away, but maybe it's less visible.

So run something like this:

ftp -o - ftp://internet/file | col

1. That "|" is a fork + exec from your shell.
2. You're going to say you wouldn't do that.  Except this is a
   microscopic example of what you do every second of your day -- taking
   untrusted data and running it through potentially buggy system tools.
3. Go look at the 20+ bugs fixed in the last year in col.
4. col contains no execve, but watch someone execve out of it due to
   those bugs.

I do not want col to execve, so no tame feature allows execve at present.

> I don't really understand why exec(2) is treated differently from other 
> potentially dangerous operations. I think it has a legitimate use.

Gasp.

> (I'd like to use it as "exec + whitelist of permitted 'trusted' programs")

tame currently blocks all execve, and we won't go to such a list.  Because
that is not the problem in software.

> I see each program as a different entity, each having different tame(2)-ing 
> requirements. I'm not sure tame(2) inheritance is very useful from that 
> point of view.

tame inheritance over "fork" is definately useful, over 15 programs
commited in base area already using it effectively and carefully.
fork + kill, basically.

tame cannot be inherited over "exec", because tame does not currently
allow exec.

> The main reason for running another program is probably because some 
> functionality is needed not offered by the currently running program.
> I don't really see why the child program would need the same tame(2) 
> requirements, or a subset thereof. (maybe there are cases where this could 
> be the case, but I don't see a general usefulness)

tame prevents all use of execve at the moment, so what you say is not
relevant.

> Please correct me if I'm wrong, but I see tame(2) as a means to reduce the 
> attack surface of the program it is used in, not as a tool to reduce the 
> attack surface of another program it knows nothing about.

That sentence is correct.  But I don't agree with your earlier premises,
because we can do better than just this.

We don't want col to run execve.  This is a very common case.  The
case of wanting to allow execve is MUCH LESS COMMON.  We'll get there,
in short order, once the non-exec parts of the tree are taken care of.

You may not be aware, but people load fresh code into programs when
they attack them.

Be patient.  I am not adding all the planned layers to this subsystem
in one go.  There is something being designed for exec, and it will
show up when the time is right.

Re: tame userland diff

[Theo de Raadt]

> The problem of exec(2) is if we permit it (without herited tame flags)
> your program has a way to go out his expected behaviour. For example, if
> a tamed program has a bug that permit execution of code, the attacker
> would just has to do "exec(something-else)" to escape the imposed
> policy. Without exec(2), the attacker has just a limited subset of
> syscalls allowed.

Actually the attacker would do:

 copy address space to new program
 with "fattr" attribute, make it executable
 execve

That is an escape.

execve by default is wrong.  tame is starting at the bottom end,
allowing us to define small semantics which cover most programs.

As we move up the stack towards more complicated programs, other
tame features will show up.  Be patient

pledge in -current

[Theo de Raadt]

Many will have observed that pledge(2) usage is being pushed into the
source tree at a very rapid pace.

I'd like if everyone looks in their dmesg logs for pledge errors.  But
please don't immediately mail a report!  Instead, look for if someone
else reports an error in the same command.  If noone else does within
24 hours, then please inform tech@, or myself and semarie@

We're doing the best we can to test every usage case of the programs
we modify, but there are going to be some glitches, hopefully all
found & fixed quickly.

Thanks.

Oh and what has pledge become?  A very simple annotation system call a
program can do, to tell the kernel what it will do henceforth.  If it
breaks the rules, it gets killed (and you see those pledge messages).
It's a new kind of security system, just wait and see... :)

Re: Microsoft Now OpenBSD Foundation Gold Contributor

[Theo de Raadt]

 Kenneth R Westerback, 08 Jul 2015 10:13:
  The OpenBSD Foundation is happy to announce that Microsoft has made
  a significant financial donation to the Foundation. This donation
  is in recognition of the role of the Foundation in supporting the
  OpenSSH project. This donation makes Microsoft the first Gold level
  contributor in the OpenBSD Foundation's 2015 fundraising campaign.
 
 this is very impressive news, although for me for all
 the wrong reasons.
 
 sad world when even ms realises openssh's importance,
 (even while not supporting it out of box natively)
 while the other big open source players are just
 feeding off of it.  speechless.

Frantisek,

With respect, and at the same none at all due to what you say;

I know very little about you except what I see historically.  This
mailing list contains hundreds of people.  You have been around for a
long time.  You know the general agenda which is 20+ years old.  Yet
here you broadcast just your own, acting like it has any acceptance at
all; furthermore you seem to expect validation of your own agenda
without consideration of the fact that it is not at all like our
general agenda; in fact, you do not put any limiters to indicate so.

Therefore:

You are welcome to provide the same revenue, and I will personally
argue that the OpenBSD Foundation should return what Microsoft gave
them; my understanding being that it was without any strings.

So pony up all the cash, to match the efforts by hundreds over two
decades, and I will gladly validate your opinions as having some value
greater than negative to the greater community.

Luckily, saying that to you is just a prank.

My historical analysis indicates you are just a tiny/tinny loudmouth,
who accidentally hurts the people who have spent over two decades
building good software.  You have some hateful agenda entirely unlike
our own (by own, I consider the rough consensus of 500+ openbsd
developers over two decades).  Your comments are entirely
disassociated with the general project guidelines of writing software
which anyone can use, to advance the state of the art, without restriction.

To keep it simple:  Where are you coming from, Mr. Do-Nothing?

As a group (with me and others as the proxy) we do ask for companies
to help fund us from time to time, and this benefits everyone as a
result of the common advancements.  As for output, there is no
discrimination as to their cause, because that is not our
oversight/job.  The general guidelines for access to software is CLEAR
in the licence.

Yet you clearly state that we should have some restrictive agenda, Mr.
Do-Nothing-Except-State-Position.

In essence, you are a loudmouth and a leech.

I've been working on this community for two decades.

It is my right to call you out for your destructive words.  Few are
in the position where they can say your words cause harm.  Few have
as much insight.

You have zero insight. You only harbour a general hate and want to
change our direction.  You have no skin in the game.  When the BBQ
gets lit up, you are the just a sparrow flitting away down the alley
in search of a place to hide.

I'm looking at your record of postings in various forums.  You add
ZERO value.  You have spent almost a decade demanding we do more,
more, MORE, DO FUCKING FOR ME MORE DEVELOPERS, yet I've never seen
your name show up elsewhere of consequence to actually solve the costs
behind advancing the state of the art of free options.

Yet you feel qualified to tell us this pattern is wrong.

Unfortunately, it is a sad world filled with Holops who have done VERY
LITTLE, and then deride funding opportunities, very late in the game;
for what many of have simply done sans funding for decades.

The problem here is simple.  Holop has an specific agenda that is not
generally held!

For everyone following allong, please find the original IPF removal
commit message for guidance.  The standard is to advance the industry.
In general, we don't care about people who whine and try to apply
their agenda towards us, but in your case I must make an exception.

I think it is high time people like you FIND A MIRROR.  You are the
specific cause of the situation you so deride.  Frantisek, you have
absolutely no right to speak like you do.  You are a pure-form leech,
and leeches should not preach.  Please leave our userbase instantly; I
am afraid someone in this community might accidentally make a mistake
of fixing a bug you care about (and you made it clear you care about
nothing else except your own leechy agenda).

Frantisek, it would be easier if you just went away.  I hate writing
these mails, editing them, and creating a disaster.

Perhaps you should stop pissing off people who try hard by stabbing
yourself with a knife, repeatedly.  I don't know if that would work.
Maybe you will return as a zombie and say the same things again.

Re: Microsoft Now OpenBSD Foundation Gold Contributor

[Theo de Raadt]

 it flatters me somewhat that you read so much into
 my simple astonishment about a news item that does
 in most geek circles provoke the response no way,
 hell froze over.

I quote from your original mail:

this is very impressive news, although for me for all
the wrong reasons.

Have all the other sponsors contributed for only the right reasons
over the 20 years?

You did not show simple astonishment -- you judged a sponsor.

Others on the day of that donation also crossed similar lines, some
implying that the donation was been accepted for the wrong reasons.

This is our house, and you don't get to insult guests who bring a
generous green salad to the party.  That is how you become uninvited.

Re: tcpdump -A: really printable characters

[Theo de Raadt]

  Index: tcpdump.c
  ===
  RCS file: /cvs/src/usr.sbin/tcpdump/tcpdump.c,v
  retrieving revision 1.70
  diff -u -p -r1.70 tcpdump.c
  --- tcpdump.c   18 Apr 2015 18:28:38 -  1.70
  +++ tcpdump.c   11 Jul 2015 20:35:11 -
  @@ -603,8 +603,10 @@ default_print_ascii(const u_char *cp, un
  printf(\n);
  for (i = 0; i  length; i++) {
  c = cp[i];
  -   c = isprint(c) || isspace(c) ? c : '.';
  -   putchar(c);
  +   if (isprint(c) || c == '\t' || c == '\n' || c == '\r')
 
 does printing '\r' will allow overriding previously printed char on line ?
 
 $ echo 'bad thing\rgood thing'
 good thing

Hah, yeah pretty bad.

I sent a mail to naddy mentioning that a long time ago (feels like 10
years ago) we talked about using vis, but this would have made our
tcpdump far too different from others.  Not that it is very similar,
because of the baked-in privsep work.  Which reminds me... I have a
diff to send out...

tcpdump: make BIOCGSTATS a priv operation

[Theo de Raadt]

This moves the BIOCGSTATS ioctl operation done by the tcpdump process
(at ^C time, but not at -c count expiration) into a service provided
by the privsep monitor.

Had a bit of help from canacar, who wrote the original privsep code
with otto.

Will be needed for tame.

Index: privsep.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
retrieving revision 1.33
diff -u -p -u -r1.33 privsep.c
--- privsep.c   15 Mar 2015 00:41:28 -  1.33
+++ privsep.c   12 Jul 2015 05:42:29 -
@@ -20,6 +20,7 @@
 #include sys/types.h
 #include sys/socket.h
 #include sys/wait.h
+#include sys/ioctl.h
 
 #include netinet/in.h
 #include net/if.h
@@ -59,7 +60,8 @@ enum priv_state {
STATE_INIT, /* initial state */
STATE_BPF,  /* input file/device opened */
STATE_FILTER,   /* filter applied */
-   STATE_RUN   /* running and accepting network traffic */
+   STATE_RUN,  /* running and accepting network traffic */
+   STATE_EXIT  /* in the process of dying */
 };
 
 #define ALLOW(action)  (1  (action))
@@ -76,7 +78,8 @@ static const int allowed_max[] = {
ALLOW(PRIV_ETHER_NTOHOST) | ALLOW(PRIV_INIT_DONE),
/* RUN */   ALLOW(PRIV_GETHOSTBYADDR) | ALLOW(PRIV_ETHER_NTOHOST) |
ALLOW(PRIV_GETRPCBYNUMBER) | ALLOW(PRIV_GETLINES) |
-   ALLOW(PRIV_LOCALTIME)
+   ALLOW(PRIV_LOCALTIME) | ALLOW(PRIV_PCAP_STATS),
+   /* EXIT */  0
 };
 
 /*
@@ -87,7 +90,9 @@ static int allowed_ext[] = {
/* INIT */  ALLOW(PRIV_SETFILTER),
/* BPF */   ALLOW(PRIV_SETFILTER),
/* FILTER */ALLOW(PRIV_GETSERVENTRIES),
-   /* RUN */   ALLOW(PRIV_GETLINES) | ALLOW(PRIV_LOCALTIME)
+   /* RUN */   ALLOW(PRIV_GETLINES) | ALLOW(PRIV_LOCALTIME) |
+   ALLOW(PRIV_PCAP_STATS),
+   /* EXIT */  0
 };
 
 struct ftab {
@@ -120,6 +125,7 @@ static void impl_getserventries(int);
 static voidimpl_getprotoentries(int);
 static voidimpl_localtime(int fd);
 static voidimpl_getlines(int);
+static voidimpl_pcap_stats(int, int *);
 
 static voidtest_state(int, int);
 static voidlogmsg(int, const char *, ...);
@@ -186,6 +192,7 @@ priv_init(int argc, char **argv)
}
 
sigprocmask(SIG_SETMASK, oset, NULL);
+   signal(SIGINT, SIG_IGN);
 
/* Child - drop suid privileges */
gid = getgid();
@@ -303,6 +310,10 @@ priv_init(int argc, char **argv)
test_state(cmd, STATE_RUN);
impl_getlines(socks[0]);
break;
+   case PRIV_PCAP_STATS:
+   test_state(cmd, STATE_RUN);
+   impl_pcap_stats(socks[0], bpfd);
+   break;
default:
logmsg(LOG_ERR, [priv]: unknown command %d, cmd);
_exit(1);
@@ -390,8 +401,6 @@ impl_setfilter(int fd, char *cmdbuf, int
 
if (setfilter(*bpfd, fd, cmdbuf))
logmsg(LOG_DEBUG, [priv]: setfilter() failed);
-   close(*bpfd);   /* done with bpf descriptor */
-   *bpfd = -1;
 }
 
 static void
@@ -401,8 +410,6 @@ impl_init_done(int fd, int *bpfd)
 
logmsg(LOG_DEBUG, [priv]: msg PRIV_INIT_DONE received);
 
-   close(*bpfd);   /* done with bpf descriptor */
-   *bpfd = -1;
ret = 0;
must_write(fd, ret, sizeof(ret));
 }
@@ -581,6 +588,19 @@ impl_getlines(int fd)
fclose(fp);
 }
 
+static void
+impl_pcap_stats(int fd, int *bpfd)
+{
+   struct pcap_stat stats;
+
+   logmsg(LOG_DEBUG, [priv]: msg PRIV_PCAP_STATS received);
+
+   if (ioctl(*bpfd, BIOCGSTATS, stats) == -1)
+   write_zero(fd);
+   else
+   must_write(fd, stats, sizeof(stats));
+}
+
 void
 priv_init_done(void)
 {
@@ -738,6 +758,17 @@ priv_getlines(size_t sz)
 
write_command(priv_fd, PRIV_GETLINES);
must_write(priv_fd, sz, sizeof(size_t));
+}
+
+int
+priv_pcap_stats(struct pcap_stat *ps)
+{
+   if (priv_fd  0)
+   errx(1, %s: called from privileged portion, __func__);
+
+   write_command(priv_fd, PRIV_PCAP_STATS);
+   must_read(priv_fd, ps, sizeof(*ps));
+   return (0);
 }
 
 /* retrieve a line from a file, should be called repeatedly after calling
Index: privsep.h
===
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.h,v
retrieving revision 1.7
diff -u -p -u -r1.7 privsep.h
--- privsep.h   25 Aug 2009 06:59:17 -  1.7
+++ privsep.h   21 May 2015 01:27:53 -
@@ -37,7 +37,8 @@ enum cmd_types {
PRIV_GETPROTOENTRIES,   /* get the ip protocol entries table */
PRIV_LOCALTIME, /* return localtime */
PRIV_GETLINES,  /* get lines from a file */
-   PRIV_INIT_DONE  

Re: sndiod_flags=NO in /etc/rc.conf on recent snapshots

[Theo de Raadt]

> I only just noticed that, trying to watch a video while having a web
> browser open at the same time, I don't get any sound.
> 
> Only going through recent daily insecurity emails, had I found out that:
> 
> sndiod_flags=
> 
> in /etc/rc.conf, has been changed to:
> 
> sndiod_flags=NO
> 
> on snapshots from around the 10th November.
> 
> The source in CVS doesn't have that change. I couldn't find any
> information about it in the mailing list archives either.
> 
> What (if any) is the reason behind it?

Testing reactions.

Re: Pledge shutdown halt

[Theo de Raadt]

> 
> Anybody see this on shutdown?=C2=A0
> 
> shutdown -hp now
> 
>   *** FINAL System shutdown message from i...@ianm-openbsd.xxx.edu.au
> 
>   System going down IMMEDIATELY=C2=A0
> 
>   System shutdown time has arrived=20
> 
>   halt: revoke: Inappropriate ioctl for device

I think your kernel is very slightly out of sync, but will double
check the changes I made.

Re: MALLOC_STATS and pledge

[Theo de Raadt]

> On 2015/11/14 14:01, David CARLIER wrote:
> > Might be indeed, there are other use cases with valgrind and protexec
> > and so on ..., utrace might be a good suggestion too except maybe a
> > need for an exception in pledge's side then.
> 
> btw, valgrind currently ssems to be quite broken with pledge.

i'm glad that is phrased correctly :-)

valgrind has to cope with conditions on the ground.  in this case,
it will have to turn off pledge itself, because it needs so much
more.

Re: increase initialization time for serverworks sata

[Theo de Raadt]

> Don't tell me this fixes SATA on the xserve G5...

I did try that back in the day.  I cranked all the delays, and
nothing helped.  No interrupts came in, ever.

Re: MALLOC_STATS and pledge

[Theo de Raadt]

> I ve tried to discuss this point with Otto Moerbeek but he might be
> very busy so I throw the topic here if you do not mind ...
> 
> I have the habit to enable MALLOC_STATS but with the recent pledge
> feature, it s now difficult to debug some pledged applications with
> MALLOC_OPTIONS=D as, for malloc_dump, wpath and rpath are needed ...
> and it is fore sure not wishable to enable those in those userland
> apps.
> 
> Is there a way, somehow, to succeed ? For the moment I ve slighty
> modified malloc_dump to only write on stderr and then I redirect the
> output myself.

Yes, opening a file late under program control becomes undesireable.
I suspect utrace(2) becomes a good idea, then the data can show up in
ktrace.  'ktrace -t u' can be used to request just that information...

Re: MALLOC_STATS and pledge

[Theo de Raadt]

> An idea would be to open the fd at init time, which should be early
> enough for most cases (i.e. before the first pledge(2) call).  Big
> drawback is the open fd all the time until program exits. 

Keeping a fd open through libc runtime is not going to fly.  It isn't
just the fragility of it.  The risks of misuse are too high.

Re: [PATCH] doas authentication type

[Theo de Raadt]

  security model.
 
  How many users of that functionality will there be?
 
  We only need to concern ourselves with the cost; you have to justify
  the benefit.  How many people were doing this with sudo, and how many
  will need this with doas?
 
 
 While I understand it's a good idea to limit the possibilities of setuid 
 programs, this patch is not an overly complicated piece of code which 
 does network stuff or interacts with something the original programs 
 doesn't already use.

If I understand it right, it asks more of the bsd auth layer.

 Estimating the number of users which use the feature is quite hard, but 
 I think many people using a yubikey use that feature at some point.

Do you mean many -- as in 1% of openbsd users?  I'm going to suggest
less than 1% of openbsd users by far.  Kind of making the word many
meaningless.

Re: [PATCH] doas authentication type

[Theo de Raadt]

  Sorry, I think adding an option is too much. I just committed halex's o=
 riginal
  diff to only change the type. I thought he was going to do that by now.=
 
 
 
 Hi Ted,
 
 The thing is, my patch doesn't do the same thing at all as the one which
 adds auth-doas. My patch lets the user choose which authentication he
 wants, while the other patch lets the admin restrict which auth is used.
 
 There are 2 very different use cases for this. Let's take an example
 with yubikey.
 
 - The small patch is used by the admin to force yubikey for doas, while
 the user could login with a password.
 
 - My patch with the option lets the user choose. The example would be a
 server with an encrypted home directory. When everything is working
 correctly, the user can login with, for example, a ssh key and then use
 doas with a (non yubi) password. But if the server has crashed for
 whatever reason and /home is not mounted, the only way to login would be
 with the yubikey because the ssh key is not available and remote login
 with normal passwords is disabled. The option replicates how sudo was
 working.

doas is a one of the few setuid programs.  It should try to do a
little bit less functionality, because doing less is part of the
security model.

How many users of that functionality will there be?

We only need to concern ourselves with the cost; you have to justify
the benefit.  How many people were doing this with sudo, and how many
will need this with doas?

Re: syslogd optarg

[Theo de Raadt]

 On Thu, Aug 27, 2015 at 10:13:25AM -0600, Theo de Raadt wrote:
  Why not strdup?
 
 And now with strdup() as suggested by Theo.

ok, because such style is not really a leak.

 Index: usr.sbin/syslogd/syslogd.c
 ===
 RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
 retrieving revision 1.178
 diff -u -p -r1.178 syslogd.c
 --- usr.sbin/syslogd/syslogd.c25 Aug 2015 17:14:16 -  1.178
 +++ usr.sbin/syslogd/syslogd.c27 Aug 2015 16:28:22 -
 @@ -393,12 +393,16 @@ main(int argc, char *argv[])
   path_ctlsock = optarg;
   break;
   case 'T':   /* allow tcp and listen on address */
 - if (loghost_parse(optarg, NULL, listen_host,
 - listen_port) == -1)
 + if ((p = strdup(optarg)) == NULL)
 + err(1, strdup listen address);
 + if (loghost_parse(p, NULL, listen_host, listen_port)
 + == -1)
   errx(1, bad listen address: %s, optarg);
   break;
   case 'U':   /* allow udp only from address */
 - if (loghost_parse(optarg, NULL, bind_host, bind_port)
 + if ((p = strdup(optarg)) == NULL)
 + err(1, strdup bind address);
 + if (loghost_parse(p, NULL, bind_host, bind_port)
   == -1)
   errx(1, bad bind address: %s, optarg);
   break;

Re: [PATCH] doas authentication type

[Theo de Raadt]

  How many users of that functionality will there be?
 
  We only need to concern ourselves with the cost; you have to justify
  the benefit.  How many people were doing this with sudo, and how many
  will need this with doas?
 
 
 My current model is to use my yubikey when sudo'ing. Occasionally I find it
 easier to use my password, and I make use of sudo -a passwd. Perhaps the
 case for doas is to provide less features and if you need to be able to do
 that, it's best to use sudo from ports?

I didn't question whether you are using it.  I asked how many people.
Are you avoiding the question by answering 1?  Not all sensitive
software can do everything.  That's why the -backrub option from sudo
didn't make it to doas.

Using tame() in userland

[Theo de Raadt]

This is for those of you interested in tame, and skilled enough to
play along.

This is a set of almost 100 diffs to programs in the tree to use tame.
These have been done by myself, doug, florian, semarie, and a few
other people I forget.  I would make a rough guess these changes took
about 100 hours of developer time; so making programs use tame() is
pretty efficient.

None of these examples uses the path whitelist yet.

It is not perfect or final, but it shows the strategy for applying
them to the base.  It can make it through a 'make build'.  Feel free
to do tests, look for mistakes, or write diffs for other programs.

Be careful writing such diffs; you need to fully understand the
program and handle all cases.  Not all programs can be tamed, some
behaviours (like execve) are not compatible with features tame
can do.

Index: bin/cat/cat.c
===
RCS file: /cvs/src/bin/cat/cat.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 cat.c
--- bin/cat/cat.c   16 Jan 2015 06:39:28 -  1.21
+++ bin/cat/cat.c   26 Aug 2015 22:07:37 -
@@ -35,6 +35,7 @@
 
 #include sys/types.h
 #include sys/stat.h
+#include sys/tame.h
 
 #include ctype.h
 #include err.h
@@ -65,6 +66,8 @@ main(int argc, char *argv[])
int ch;
 
setlocale(LC_ALL, );
+
+   tame(TAME_STDIO | TAME_RPATH, NULL);
 
while ((ch = getopt(argc, argv, benstuv)) != -1)
switch (ch) {
Index: bin/chmod/chmod.c
===
RCS file: /cvs/src/bin/chmod/chmod.c,v
retrieving revision 1.34
diff -u -p -u -r1.34 chmod.c
--- bin/chmod/chmod.c   25 Jun 2015 02:04:08 -  1.34
+++ bin/chmod/chmod.c   26 Aug 2015 22:45:24 -
@@ -32,6 +32,7 @@
 
 #include sys/types.h
 #include sys/stat.h
+#include sys/tame.h
 
 #include err.h
 #include errno.h
@@ -69,6 +70,8 @@ main(int argc, char *argv[])
char *ep, *mode, *cp, *flags;
 
setlocale(LC_ALL, );
+
+   tame(TAME_STDIO | TAME_RPATH | TAME_WPATH | TAME_FATTR, NULL);
 
if (strlen(__progname)  2) {
ischown = __progname[2] == 'o';
Index: bin/dd/dd.c
===
RCS file: /cvs/src/bin/dd/dd.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 dd.c
--- bin/dd/dd.c 16 Jan 2015 06:39:31 -  1.21
+++ bin/dd/dd.c 26 Aug 2015 22:07:37 -
@@ -38,6 +38,7 @@
 #include sys/stat.h
 #include sys/ioctl.h
 #include sys/mtio.h
+#include sys/tame.h
 
 #include ctype.h
 #include err.h
@@ -148,6 +149,11 @@ setup(void)
pos_in();
if (out.offset)
pos_out();
+
+   if (in.fd == STDIN_FILENO  out.fd == STDOUT_FILENO)
+   tame(TAME_STDIO, NULL);
+   else
+   tame(TAME_STDIO | TAME_RW, NULL);
 
/*
 * Truncate the output file; ignore errors because it fails on some
Index: bin/df/df.c
===
RCS file: /cvs/src/bin/df/df.c,v
retrieving revision 1.52
diff -u -p -u -r1.52 df.c
--- bin/df/df.c 16 Jan 2015 06:39:31 -  1.52
+++ bin/df/df.c 27 Aug 2015 03:32:58 -
@@ -37,6 +37,7 @@
 
 #include sys/stat.h
 #include sys/mount.h
+#include sys/tame.h
 
 #include err.h
 #include errno.h
@@ -78,6 +79,9 @@ main(int argc, char *argv[])
int ch, i;
int width, maxwidth;
char *mntpt;
+
+   // but what about getfstat() ?
+   //tame(TAME_STDIO | TAME_RPATH, NULL);
 
while ((ch = getopt(argc, argv, hiklnPt:)) != -1)
switch (ch) {
Index: bin/echo/echo.c
===
RCS file: /cvs/src/bin/echo/echo.c,v
retrieving revision 1.8
diff -u -p -u -r1.8 echo.c
--- bin/echo/echo.c 14 Dec 2014 16:55:59 -  1.8
+++ bin/echo/echo.c 26 Aug 2015 22:07:37 -
@@ -30,6 +30,8 @@
  * SUCH DAMAGE.
  */
 
+#include sys/tame.h
+
 #include stdio.h
 #include string.h
 
@@ -38,6 +40,8 @@ int
 main(int argc, char *argv[])
 {
int nflag;
+
+   tame(TAME_STDIO, NULL);
 
/* This utility may NOT do getopt(3) option parsing. */
if (*++argv  !strcmp(*argv, -n)) {
Index: bin/expr/expr.c
===
RCS file: /cvs/src/bin/expr/expr.c,v
retrieving revision 1.20
diff -u -p -u -r1.20 expr.c
--- bin/expr/expr.c 11 Aug 2015 17:15:46 -  1.20
+++ bin/expr/expr.c 26 Aug 2015 22:07:37 -
@@ -6,6 +6,8 @@
  * Public domain.
  */
 
+#include sys/tame.h
+
 #include stdio.h
 #include stdlib.h
 #include string.h
@@ -499,6 +501,8 @@ main(int argc, char *argv[])
struct val *vp;
 
(void) setlocale(LC_ALL, );
+
+   tame(TAME_STDIO, NULL);
 
if (argc  1  !strcmp(argv[1], --))
argv++;
Index: bin/ls/ls.c
===
RCS file: /cvs/src/bin/ls/ls.c,v
retrieving 

Re: pool allocator names

[Theo de Raadt]

* pool_allocator_multi_ni: A multi page allocator that is *not* safe
for use in interrupts.  Also less efficient than
pool_allocator_single.  It allocates kva from kernel_map, which is
significantly more plentyful.

We are the knights who say non-interruptable.  Honestly, ni feels
a bit short here.

Re: [patch] cat's main never return

[Theo de Raadt]

 just saw that cat's *main* function does never return even though there is a 
 return value,
 exit(3) is called instead.
 Is there any reason why or is it just historically, cause it's a bit 
 confusing?
 If exit(3) is always called, than why not changing the return value to *void*?
 
 Other calls in *src/bin/* share the same behavior.

It really does not matter.  Coder's choice.  The result is the same.
You could hunt them all down, change them all, save a few code bytes,
but don't you dare introduce any bugs...

Re: [patch] return instead of exit(3) in src/bin/

[Theo de Raadt]

 As suggested by deraadt@ and tobias@ it might be better to use the
 *return* statement instead of exit(3) 
 inside the *main* function, to let the stack protector do its work.
 
 This diff removes such calls in all *src/bin/* tools, except those
 who already use it.  I think I didn't miss a call and didn't introduce
 any bugs.

So there is a downside.

In a more complex program with a large main() function, a call to
exit() is an explicit statement about termination, so that even if
someone refactors code to a subfunction, they must consider that
it carefully.

The return is not as explicit.

Now that you have done a few, please go review them and think about
that side.  Perhaps think about it in regarding larger programs;
the ones in bin tend to be very small.

libexec is a good area to come to the realization that firm rules
don't always result in an improvement.

Re: ksh sanatizing argv redundant

[Theo de Raadt]

> Martijn van Duren wrote:
> > Hello tech@,
> > 
> > I took a quick glance at ksh and one of the first things I noticed was 
> > that it uses some sanatizing code on argv. When looking at execve(2) I 
> > see that EINVAL or EFAULT are returned when argv isn't properly 
> > formatted. I've also verified this quickly by a small PoC and in 
> > sys/kern/kern_exec.c.
> > 
> > Would it make sense to remove the check all together?
> 
> I think this is ok. You used to have to worry about it, because the kernel let
> you exec something with empty argv. And there's still perhaps a portability
> concern. But old workarounds need to die sometime. I support removing this,
> but I'd like some one else to comment.

with fire

(or else  i wonder if doas needs this checking...)

Re: sendsyslog error log

[Theo de Raadt]

Seems good to me.

I wouldn't bother with disabling this in SMALL_KERNEL.   Space
is not short enough to complicate the code for ~200 bytes.

> To make syslog reliable, I want to see log messages about failed
> log atempts.  sendsyslog(2) seems a good place to detect and report
> the problem.
> 
> ok?
> 
> bluhm
> 
> Index: kern/subr_log.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_log.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 subr_log.c
> --- kern/subr_log.c   6 May 2015 08:52:17 -   1.30
> +++ kern/subr_log.c   14 Aug 2015 18:11:47 -
> @@ -83,6 +83,8 @@ int filt_logread(struct knote *kn, long 
>  struct filterops logread_filtops =
>   { 1, NULL, filt_logrdetach, filt_logread};
>  
> +int dosendsyslog(struct proc *, const char *, size_t, enum uio_seg);
> +
>  void
>  initmsgbuf(caddr_t buf, size_t bufsize)
>  {
> @@ -354,6 +356,40 @@ sys_sendsyslog(struct proc *p, void *v, 
>   syscallarg(const void *) buf;
>   syscallarg(size_t) nbyte;
>   } */ *uap = v;
> + int error;
> +#ifndef SMALL_KERNEL
> + static int dropped_count, orig_error;
> + int len;
> + char buf[64];
> +
> + if (dropped_count) {
> + len = snprintf(buf, sizeof(buf),
> + "<%d> sendsyslog: dropped %d message%s, error %d",
> + LOG_KERN|LOG_WARNING, dropped_count,
> + dropped_count == 1 ? "" : "s", orig_error);
> + error = dosendsyslog(p, buf, MIN((size_t)len, sizeof(buf) - 1),
> + UIO_SYSSPACE);
> + if (error) {
> + dropped_count++;
> + return (error);
> + }
> + dropped_count = 0;
> + }
> +#endif
> + error = dosendsyslog(p, SCARG(uap, buf), SCARG(uap, nbyte),
> + UIO_USERSPACE);
> +#ifndef SMALL_KERNEL
> + if (error) {
> + dropped_count++;
> + orig_error = error;
> + }
> +#endif
> + return (error);
> +}
> +
> +int
> +dosendsyslog(struct proc *p, const char *buf, size_t nbyte, enum uio_seg 
> sflg)
> +{
>  #ifdef KTRACE
>   struct iovec *ktriov = NULL;
>   int iovlen;
> @@ -369,11 +405,11 @@ sys_sendsyslog(struct proc *p, void *v, 
>   f = syslogf;
>   FREF(f);
>  
> - aiov.iov_base = (char *)SCARG(uap, buf);
> - aiov.iov_len = SCARG(uap, nbyte);
> + aiov.iov_base = (char *)buf;
> + aiov.iov_len = nbyte;
>   auio.uio_iov = 
>   auio.uio_iovcnt = 1;
> - auio.uio_segflg = UIO_USERSPACE;
> + auio.uio_segflg = sflg;
>   auio.uio_rw = UIO_WRITE;
>   auio.uio_procp = p;
>   auio.uio_offset = 0;
> 

Re: nfs pool diff

[Theo de Raadt]

>The only pool_get() call uses PR_WAITOK, and the pool_put() calls are
>only done from the nfsd main loop, so process context.

OK.  Thanks that explains how one makes sure..

>No I'm not an NFS hacker!

   3 kettenis

Actually lots of people are NFS hackers.

   1 aaron
   1 damien
   1 dlg
   1 doug
   1 espie
   1 hugh
   1 itojun
   1 kstailey
   1 markus
   1 mikeb
   1 mk
   1 naddy
   1 nordin
   1 provos
   1 ray
   1 rees
   1 smurph
   2 briggs
   2 florian
   2 graichen
   2 grange
   2 jmc
   2 pvalchev
   3 claudio
   3 fgsch
   3 hin
   3 jasper
   3 kettenis
   3 oga
   3 sf
   3 sturm
   4 henning
   4 jsing
   5 angelos
   6 avsm
   6 brad
   6 djm
   6 jason
   7 otto
   8 krw
   9 assar
   9 bluhm
   9 dm
  12 ericj
  12 mpi
  15 marius
  18 tholo
  19 nate
  21 pedro
  23 guenther
  25 ho
  25 miod
  26 beck
  28 jsg
  63 tedu
  64 millert
  72 mickey
  89 csapuntz
 123 art
 127 blambert
 144 niklas
 195 deraadt
 301 thib

63 people.

All of them to blame equally, I will say from a high position...

Re: sqlite 3.8.11.1

[Theo de Raadt]

> > Not attaching the diff because of the size, available at
> > http://rhaalovely.net/~landry/stuff/sqlite-3.8.11.1.diff.gz
> 
> And i forgot to mention that it went in an amd64 bulk build without
> fallout, thanks to ajacoutot@

So, one architecture has been tested.  Now there is a request for OK
to commit -- based only upon that one test.

That will put other people in the situation of being first to build on
all the other architectures.  If something breaks during build, then
who has to deal with that?  It seems like potential costs are being
foisted on others.  That is not our way.

I'm sorry, but I'm putting my foot down here.  The folk who wanted
sqlite in the base in the first place have to do sufficient testing.
Owning that part of the tree requires better process than a update
splats.

Since sqlite went into base, the standards are higher.  Meet that
standard.  Test it to same standard we use when doing library changes,
or leave it alone, or prepare to move it back out of base.

Re: sqlite 3.8.11.1

[Theo de Raadt]

This is a really stupid way to treat the base source tree.

> thanks to the hard work of jturner@, here's a 650kb gzipped update to
> sqlite 3.8.11.1, bumping shlib to 31.0. This is needed for upcoming
> firefox 41 update, but anyone is welcome to look into the update itself.
> 
> Not attaching the diff because of the size, available at
> http://rhaalovely.net/~landry/stuff/sqlite-3.8.11.1.diff.gz
> 
> Landry
> 

Re: Remove useless NULL casts from mv(1)

[Theo de Raadt]

>Michael McConville wrote:
>> Index: mv.c
>> ===
>> RCS file: /cvs/src/bin/mv/mv.c,v
>> retrieving revision 1.40
>> diff -u -p -r1.40 mv.c
>> --- mv.c 24 Aug 2015 00:10:59 -  1.40
>> +++ mv.c 14 Sep 2015 13:38:13 -
>> @@ -348,7 +348,7 @@ copy(char *from, char *to)
>>  pid_t pid;
>>  
>>  if ((pid = vfork()) == 0) {
>> -execl(_PATH_CP, "cp", "-PRp", "--", from, to, (char *)NULL);
>> +execl(_PATH_CP, "cp", "-PRp", "--", from, to, NULL);
>>  warn("%s", _PATH_CP);
>>  _exit(1);
>>  }
>> @@ -366,7 +366,7 @@ copy(char *from, char *to)
>>  return (1);
>>  }
>>  if (!(pid = vfork())) {
>> -execl(_PATH_RM, "cp", "-rf", "--", from, (char *)NULL);
>> +execl(_PATH_RM, "cp", "-rf", "--", from, NULL);
>>  warn("%s", _PATH_RM);
>>  _exit(1);
>>  }
>> 
>
>I don't know where we stand on this. It was previously necessary to have the
>cast because otherwise NULL would only get passed as an integer, not a pointer
>(long). We have fixed NULL in our headers, but the cast may still be the
>correct incantation for some systems. I think the best approach is to delete
>casts in other more obvious places, but leave these alone for now.

Placing more burden on people who make -portable versions of important
software from our base.