re: CVS commit: src/sys/kern

2018-12-03 Thread Christos Zoulas
On Dec 4, 10:20am, m...@eterna.com.au (matthew green) wrote:
-- Subject: re: CVS commit: src/sys/kern

| i just had an idea about a relatively simple hack to allow
| kvm tools to work sanely in kaslr space, even if they're not
| fully converted yet.
| 
| a secmodel overlay that has a way to allow a uid/gid combo
| to retrieve the addresses, not just root, and then have that
| combo set to */kvm.  then, kvm tools don't drop gid kvm until
| after doing sysctl.
| 
| this would restrict the sysctls to gid kvm.
| 
| we still would have to audit the tools to ensure they do not
| expose these addresses directly (ie, printf), but only use
| them internally, but until functional parity is achieved it
| would allow both security and usability today.
| 
| just an idea..

We already have the hooks for that: In proc_listener_cb() one can
add to KAUTH_REQ_PROCESS_CANSEE_KPTR a credentials check based on
two new sysctl's (kern.expose_address.uid, kern.expose_address.gid).
These can work as:

If kern.expose_address.enabled == 0, then nothing is allowed
If kern.expose_address.enabled == 1, then kern.expose_address.{uid,gid} are
consulted: if -1, all are allowed, else the euid/egid needs to match.

The changes to do this are trivial :-)

christos


Re: CVS commit: src/sys/kern

2018-12-03 Thread Robert Elz
Date:Mon, 3 Dec 2018 15:30:35 +0100
From:Maxime Villard 
Message-ID:  <03e1d0b4-d6b9-5f45-4770-ee6b161b8...@m00nbsd.net>

  | It shouldn't be hard to open new sysctls that don't leak stuff.

You really should stop using biased terminology.   Of course we don't
want the kernel to "leak stuff" - but the syscalls in question are not
doing that, they are providing useful information.   There is a difference.

The other fixes you have been making recently, where there are leaks,
where the kernel is returning data in padding fields of structs, etc, should
certainly all be fixed.  Those ones really are leaks.

The sysctls (and procfs/kernfs if they're relevant) are something entirely
different, they're doing what they were designed to do, and which is used,
probably in more ways than you'd think.

m...@eterna.com.au said:
  | we still would have to audit the tools to ensure they do not expose these
  | addresses directly (ie, printf), but only use them internally,

I disagree with that too - I want to be able to see (at least some of) those
values.   That is, I want to see where in the kernel a particular file struct
or inode/proc/... (etc) is stored, so I can get at it easily with a debugger,
and look and see what is happening with it inside the kernel.

Obviously I could chase it down with the debugger using the same methods
that the sysctl uses (using all the kernel internal data structs) to find what
I am looking for, but as the kernel has already done all that work, and can
make it available, I want it to.

I also want to be able to disable KASLR (in all its aspects) completely.
I don't care what the default is, I know how to build my own kernels, and
I change the defaults for all kinds of things - that's fine.  But I want all
of the kernel loaded at the VA's that the linker put things (all text and
data).  Obviously dynamic data goes wherever there is space available
when it is allocated ... that's why we have the sysctls that will tell us
where a particular object is located.

I also don't care if you don't want any of this information, and I don't
care if you add (optional) mechanisms so that none of it works on your
systems.   Just don't break mine in a way I cannot easily turn off.

kre



Re: CVS commit: src/sys/kern

2018-12-03 Thread Maxime Villard

Le 03/12/2018 à 15:24, matthew green a écrit :

the broken kvm tools


which ones?


this level of security


None of Linux, Windows, MacOS and more accept to leak pointers; it is not
a high level, it is a basic level.

And no, this change shouldn't stay as-is. It shouldn't be hard to open
new sysctls that don't leak stuff. All of that was already discussed.


Re: CVS commit: src/sys/kern

2018-12-03 Thread Martin Husemann
On Mon, Dec 03, 2018 at 12:54:26PM +0100, Maxime Villard wrote:
> In other words, 80% of KASLR is enabled by default, regardless of #ifdef
> KASLR.

I'd call that a bug.

> Therefore, it is wrong to add an ifdef, because in either case we
> don't want unpriv to retrieve kernel addresses. And we don't want that,
> for reasons that were already discussed more than two months ago.

There is a choice via sysctl and we are only talking about the default.
Not everyone wants security at the price of broken functionality always.

Martin


Re: CVS commit: src/bin/sh

2018-12-03 Thread Robert Elz
Date:Mon, 3 Dec 2018 10:53:29 +
From:"Martin Husemann" 
Message-ID:  <20181203105329.45433f...@cvs.netbsd.org>

  | Log Message:
  | Make pendingsigs forward declaration match the definition.

Grunge, sorry about that - I assume that's what went wrong with
the alpha build (I was waiting till the links to the logs appear to
see what happened with that...)

kre



Re: CVS commit: src/sys/kern

2018-12-03 Thread Maxime Villard

Le 03/12/2018 à 01:11, Christos Zoulas a écrit :

Module Name:src
Committed By:   christos
Date:   Mon Dec  3 00:11:02 UTC 2018

Modified Files:
src/sys/kern: files.kern init_sysctl.c

Log Message:
Expose addresses depending on the KASLR setting (from mrg@). Restores the
status quo of exposing kernel addresses if there is no KASLR.


To generate a diff of this commit:
cvs rdiff -u -r1.26 -r1.27 src/sys/kern/files.kern
cvs rdiff -u -r1.219 -r1.220 src/sys/kern/init_sysctl.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.


This is bad. GENERIC already has some KASLR enabled by default [1], and
with your change you're rendering that useless. Please revert.

[1] http://wiki.netbsd.org/security/kaslr/


Re: CVS commit: src/sys/kern

2018-12-03 Thread Martin Husemann
On Mon, Dec 03, 2018 at 06:39:18AM +0100, Maxime Villard wrote:
> This is bad. GENERIC already has some KASLR enabled by default [1], and
> with your change you're rendering that useless. Please revert.

Please describe in more details how this renders something useless,
if I read the change correctly it unbreaks things for the non-KASL part
w/o changing anything for kernels with KASL.

Martin


Re: CVS commit: src/sys/kern

2018-12-03 Thread Maxime Villard

Le 03/12/2018 à 13:07, Martin Husemann a écrit :

On Mon, Dec 03, 2018 at 12:54:26PM +0100, Maxime Villard wrote:

In other words, 80% of KASLR is enabled by default, regardless of #ifdef
KASLR.


I'd call that a bug.


No, it's a basic level of security. It is also the best randomization of
all of the BSDs, which can be combined with the GENERIC_KASLR conf that
provides the most advanced KASLR implementation available out there.


Therefore, it is wrong to add an ifdef, because in either case we
don't want unpriv to retrieve kernel addresses. And we don't want that,
for reasons that were already discussed more than two months ago.


There is a choice via sysctl and we are only talking about the default.


Yes, and if people want their kernel to leak a ton of stuff they can set
the sysctl to 1.

The real bug I see here is changing the behavior while it was already
discussed and agreed that we would prevent leaks by default. I proceeded
slowly, I added everything that was needed in TODO.kaslr and also on the
NetBSD-9 tasklist, and I even repeated several times that I would plug
the leaks by default, no one objected.

Yet all of a sudden what was agreed upon doesn't hold anymore, and
Christos has to come around and commit nonsense under the reason "from
mrg". This is bullshit.


re: CVS commit: src/sys/kern

2018-12-03 Thread matthew green
until all the broken kvm tools are fixed this change really
must stay as-is.  if someone truly wants this level of 
security they can choose it, but it's not OK to break basic
features by default in the name of security.


.mrg.


re: CVS commit: src

2018-12-03 Thread matthew green
"Maxime Villard" writes:
> Module Name:  src
> Committed By: maxv
> Date: Sun Dec  2 21:00:13 UTC 2018
> 
> Modified Files:
>   src/share/mk: bsd.sys.mk
>   src/sys/arch/amd64/conf: GENERIC
>   src/sys/arch/amd64/include: param.h
>   src/sys/conf: files ssp.mk
>   src/sys/kern: files.kern subr_pool.c sys_syscall.c
>   src/sys/sys: systm.h
>   src/sys/uvm: uvm_km.c
> Added Files:
>   src/sys/arch/amd64/include: kleak.h
>   src/sys/kern: subr_kleak.c
>   src/usr.sbin/kleak: Makefile kleak.c
> 
> Log Message:
> Introduce KLEAK, a new feature that can detect kernel information leaks.
> 
> It works by tainting memory sources with marker values, letting the data
> travel through the kernel, and scanning the kernel<->user frontier for
> these marker values. Combined with compiler instrumentation and rotation
> of the markers, it is able to yield relevant results with little effort.
> 
> We taint the pools and the stack, and scan copyout/copyoutstr. KLEAK is
> supported on amd64 only for now, but it is not complicated to add more
> architectures (just a matter of having the address of .text, and a stack
> unwinder).
> 
> A userland tool is provided, that allows to execute a command in rounds
> and monitor the leaks generated all the while.
> 
> KLEAK already detected directly 12 kernel info leaks, and prompted changes
> that in total fixed 25+ leaks.
> 
> Based on an idea developed jointly with Thomas Barabosch (of Fraunhofer
> FKIE).

extra thanks to Thomas and Max for this feature!

this is great work.


.mrg.


Re: CVS commit: src/sys/kern

2018-12-03 Thread Maxime Villard

Le 03/12/2018 à 11:49, Martin Husemann a écrit :

On Mon, Dec 03, 2018 at 06:39:18AM +0100, Maxime Villard wrote:

This is bad. GENERIC already has some KASLR enabled by default [1], and
with your change you're rendering that useless. Please revert.


Please describe in more details how this renders something useless,
if I read the change correctly it unbreaks things for the non-KASL part
w/o changing anything for kernels with KASL.

Martin


KASLR means randomizing the kernel memory.

Even though we have two distinct GENERIC and GENERIC_KASLR configurations,
the fact is that GENERIC already randomizes by default four of the five
randomizable kernel areas, as summed up in the table [1].

In other words, 80% of KASLR is enabled by default, regardless of #ifdef
KASLR. Therefore, it is wrong to add an ifdef, because in either case we
don't want unpriv to retrieve kernel addresses. And we don't want that,
for reasons that were already discussed more than two months ago.

[1] http://wiki.netbsd.org/security/kaslr/


Re: CVS commit: src/sys/kern

2018-12-03 Thread Christos Zoulas
In article <24368.1543847...@splode.eterna.com.au>,
matthew green   wrote:
>until all the broken kvm tools are fixed this change really
>must stay as-is.  if someone truly wants this level of 
>security they can choose it, but it's not OK to break basic
>features by default in the name of security.

Well, even if we want to break things to improve security, this
should be done in an expressive/organized fashion: It is not ok
for tools that worked before to now fail silently, or with unexpected
errors that don't communicate to the user what needs to be done to
fix them.

I.e. I would support changing the default again, if the tools
were made functional again, or they were modified to produce
a meaningful error if they failed because of the change.

I will work on both.

christos



Re: CVS commit: src/sys/kern

2018-12-03 Thread Steffen Nurpmeso
Manuel Bouyer wrote in <20181203183537.ga1...@antioche.eu.org>:
 |On Mon, Dec 03, 2018 at 12:54:26PM +0100, Maxime Villard wrote:
 |> In other words, 80% of KASLR is enabled by default, regardless of #ifdef
 |> KASLR. Therefore, it is wrong to add an ifdef, because in either case we
 |
 |So there's no way to completely disable KASLR now ?
 |Although I admit it's usefull to have it on by default, there should \
 |be a way
 |to turn it off for low-level debugging

As an idiot from user space only: why is layout randomization
still something desirable now that kernel and user address space
is totally, cleanly and completely separated, and caches etc. are
flushed upon context-switches and system calls?  It is like that,
right?

Also, i was always curious whether there were any runtime costs
implied due to the massive splitting of object files, i wondered
whether that negatively affects the cache "hotness" in any way.
As a programmer i always tried to keep things as compact as
possible.

--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter   he cheerfully and one by one
|einen nach dem anderen runter  wa.ks himself off
|(By Robert Gernhardt)


Re: CVS commit: src

2018-12-03 Thread Christos Zoulas
In article <27289.1543846...@splode.eterna.com.au>,
matthew green   wrote:
>"Maxime Villard" writes:
>> Module Name: src
>> Committed By:maxv
>> Date:Sun Dec  2 21:00:13 UTC 2018
>> 
>> Modified Files:
>>  src/share/mk: bsd.sys.mk
>>  src/sys/arch/amd64/conf: GENERIC
>>  src/sys/arch/amd64/include: param.h
>>  src/sys/conf: files ssp.mk
>>  src/sys/kern: files.kern subr_pool.c sys_syscall.c
>>  src/sys/sys: systm.h
>>  src/sys/uvm: uvm_km.c
>> Added Files:
>>  src/sys/arch/amd64/include: kleak.h
>>  src/sys/kern: subr_kleak.c
>>  src/usr.sbin/kleak: Makefile kleak.c
>> 
>> Log Message:
>> Introduce KLEAK, a new feature that can detect kernel information leaks.
>> 
>> It works by tainting memory sources with marker values, letting the data
>> travel through the kernel, and scanning the kernel<->user frontier for
>> these marker values. Combined with compiler instrumentation and rotation
>> of the markers, it is able to yield relevant results with little effort.
>> 
>> We taint the pools and the stack, and scan copyout/copyoutstr. KLEAK is
>> supported on amd64 only for now, but it is not complicated to add more
>> architectures (just a matter of having the address of .text, and a stack
>> unwinder).
>> 
>> A userland tool is provided, that allows to execute a command in rounds
>> and monitor the leaks generated all the while.
>> 
>> KLEAK already detected directly 12 kernel info leaks, and prompted changes
>> that in total fixed 25+ leaks.
>> 
>> Based on an idea developed jointly with Thomas Barabosch (of Fraunhofer
>> FKIE).
>
>extra thanks to Thomas and Max for this feature!
>
>this is great work.

Indeed, this is really nicely done!

christos



Re: CVS commit: src/sys/kern

2018-12-03 Thread Manuel Bouyer
On Mon, Dec 03, 2018 at 12:54:26PM +0100, Maxime Villard wrote:
> In other words, 80% of KASLR is enabled by default, regardless of #ifdef
> KASLR. Therefore, it is wrong to add an ifdef, because in either case we

So there's no way to completely disable KASLR now ?
Although I admit it's usefull to have it on by default, there should be a way
to turn it off for low-level debugging

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--


Re: CVS commit: src/sys/kern

2018-12-03 Thread Christos Zoulas
In article <20181203183537.ga1...@antioche.eu.org>,
Manuel Bouyer   wrote:
>On Mon, Dec 03, 2018 at 12:54:26PM +0100, Maxime Villard wrote:
>> In other words, 80% of KASLR is enabled by default, regardless of #ifdef
>> KASLR. Therefore, it is wrong to add an ifdef, because in either case we
>
>So there's no way to completely disable KASLR now ?
>Although I admit it's usefull to have it on by default, there should be a way
>to turn it off for low-level debugging

I don't think that the things that KASLR randomizes by default are useful
to debugging. I.e. you can't depend on two successive kernel crashes to
have identical PTE addresses; OTOH you can depend that the text addresses
are the same (which are for GENERIC and are not for GENERIC_KASLR).

christos



Re: CVS commit: src/sys/kern

2018-12-03 Thread Jason Thorpe



> On Dec 3, 2018, at 11:27 AM, Christos Zoulas  wrote:
> 
> I don't think that the things that KASLR randomizes by default are useful
> to debugging. I.e. you can't depend on two successive kernel crashes to
> have identical PTE addresses; OTOH you can depend that the text addresses
> are the same (which are for GENERIC and are not for GENERIC_KASLR).

Oh, that depends.  I would say if you are debugging a low-level VM problem, 
being able to have a predictable PTE address at the time of a crash could be 
VERY helpful.

-- thorpej



Re: CVS commit: src/sys/kern

2018-12-03 Thread Manuel Bouyer
On Mon, Dec 03, 2018 at 07:27:11PM +, Christos Zoulas wrote:
> In article <20181203183537.ga1...@antioche.eu.org>,
> Manuel Bouyer   wrote:
> >On Mon, Dec 03, 2018 at 12:54:26PM +0100, Maxime Villard wrote:
> >> In other words, 80% of KASLR is enabled by default, regardless of #ifdef
> >> KASLR. Therefore, it is wrong to add an ifdef, because in either case we
> >
> >So there's no way to completely disable KASLR now ?
> >Although I admit it's usefull to have it on by default, there should be a way
> >to turn it off for low-level debugging
> 
> I don't think that the things that KASLR randomizes by default are useful
> to debugging. I.e. you can't depend on two successive kernel crashes to
> have identical PTE addresses; OTOH you can depend that the text addresses
> are the same (which are for GENERIC and are not for GENERIC_KASLR).

It depends at which time it crashes; if early in boot (before things
start executing in parallel) I would expect 2 runs to produce the same
thing in memory. 

One enough is set up to have ddb functional, KASLR is less of an issue.

-- 
Manuel Bouyer 
 NetBSD: 26 ans d'experience feront toujours la difference
--


Re: CVS commit: src/sys/kern

2018-12-03 Thread Christos Zoulas
In article <20181203191043.zou-_%stef...@sdaoden.eu>,
Steffen Nurpmeso   wrote:
>Manuel Bouyer wrote in <20181203183537.ga1...@antioche.eu.org>:
> |On Mon, Dec 03, 2018 at 12:54:26PM +0100, Maxime Villard wrote:
> |> In other words, 80% of KASLR is enabled by default, regardless of #ifdef
> |> KASLR. Therefore, it is wrong to add an ifdef, because in either case we
> |
> |So there's no way to completely disable KASLR now ?
> |Although I admit it's usefull to have it on by default, there should \
> |be a way
> |to turn it off for low-level debugging
>
>As an idiot from user space only: why is layout randomization
>still something desirable now that kernel and user address space
>is totally, cleanly and completely separated, and caches etc. are
>flushed upon context-switches and system calls?  It is like that,
>right?

Because KVM reading or sysctl sometimes expose kernel addresses to
userland (some utilities still depend on that to function properly),
and that defeats KASLR (there is a way to find where the kernel was
loaded from userland -- to put it simplistically).

christos



Re: CVS commit: src/sys/kern

2018-12-03 Thread Christos Zoulas
In article <6c2b4529-2f69-4b94-a580-a7b85057c...@me.com>,
Jason Thorpe   wrote:
>
>
>> On Dec 3, 2018, at 11:27 AM, Christos Zoulas  wrote:
>> 
>> I don't think that the things that KASLR randomizes by default are useful
>> to debugging. I.e. you can't depend on two successive kernel crashes to
>> have identical PTE addresses; OTOH you can depend that the text addresses
>> are the same (which are for GENERIC and are not for GENERIC_KASLR).
>
>Oh, that depends.  I would say if you are debugging a low-level VM
>problem, being able to have a predictable PTE address at the time of a
>crash could be VERY helpful.

I should have added "in general" :-) Yes, there are corner cases
(where you could depend on things being identical on the same
machine) in which not having ASLR can be useful. But I think that
if you are doing this level of debugging you probably know how to
turn it off... If that proves to be a common case we can add an
ifdef to facilitate turning it off...

christos



re: CVS commit: src/sys/kern

2018-12-03 Thread matthew green
i just had an idea about a relatively simple hack to allow
kvm tools to work sanely in kaslr space, even if they're not
fully converted yet.

a secmodel overlay that has a way to allow a uid/gid combo
to retrieve the addresses, not just root, and then have that
combo set to */kvm.  then, kvm tools don't drop gid kvm until
after doing sysctl.

this would restrict the sysctls to gid kvm.

we still would have to audit the tools to ensure they do not
expose these addresses directly (ie, printf), but only use
them internally, but until functional parity is achieved it
would allow both security and usability today.

just an idea..


.mrg.


re: CVS commit: src/sys/kern

2018-12-03 Thread matthew green
Maxime Villard writes:
> > the broken kvm tools
> 
> which ones?

fstat for a start.

> > this level of security
> 
> None of Linux, Windows, MacOS and more accept to leak pointers; it is not
> a high level, it is a basic level.
> 
> And no, this change shouldn't stay as-is. It shouldn't be hard to open
> new sysctls that don't leak stuff. All of that was already discussed.

i don't care what other platforms do -- i care about netbsd not
breaking basic functionality.  you did that, and christos commited
my fix to unbreak it.

you're entirely welcome to fix this properly, but you are not welcome
to break every platform's.  fix the sysctls *THEN* enable the security.
you've broken my ability to debug problems on systems i am not the
admin on, and i've multiple times failed to diagnose a problem because
fstat did not work.


.mrg.