Re: CVS commit: src/sys/kern
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
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
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
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
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.
Re: CVS commit: src/sys/kern
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
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
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
> 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
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
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/sys/kern
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
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
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
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
"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
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
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/sys/kern
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/bin/sh
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
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
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/
