re: [filemon] CVS commit: htdocs/support/security
> As far as I can tell, there are many races caused by autoloading. i have long advocated that we should turn off both module autoload and autounload, as they're security and reliability nightmares. *perhaps* autoload, for a specific list of known OK modules would be OK in the default for me, but the general & global defaults are, IMO, very bad. .mrg.
Re: [filemon] CVS commit: htdocs/support/security
On Tue, Dec 17, 2019 at 02:19:01PM +0100, Maxime Villard wrote: > Typically with a character device, the kmod can get unloaded while an ioctl > is being executed on it. When it comes to syscalls, I haven't looked > closely, but the issue is likely the same. > > You can use tricks to "narrow down" the races; eg in NVMM, I use a global > 'nmachines' variable, which prevents unloading in ~most cases. But I see > no way to fix these races, except using atomic refcounts and mutexes on > the ioctls and syscalls; obviously, this won't scale. It can be done; there was at one point an old thread called "kicking everybody out of the softc", and maybe others, but i don't remember what came of it. Also, passive serialization can be used to deal with this - block further access by atomically overwriting the pointer(s), wake up anyone sleeping in the driver, then wait for everybody to report back that they aren't in the driver any more, or never were. The last is expensive, but it doesn't happen often. > Putting stuff in kmods AND having the kernel load them automatically serves > no purpose; it just adds bugs, and sometimes creates the wrong feeling that > a driver is disabled while it isn't (like filemon). Indeed. -- David A. Holland dholl...@netbsd.org
Re: [filemon] CVS commit: htdocs/support/security
Le 17/12/2019 à 15:44, Andrew Doran a écrit :
Typically with a character device, the kmod can get unloaded while an ioctl
is being executed on it.
That's solvable. Conceptually I think the main stumbling block is that
there are two layers at play which need to reconciled: specfs and devsw. It
could also be an opportunity to lessen the distinction between block and
character devices, there's no real need for cached access from userspace,
that would simplify things too.
When it comes to syscalls, I haven't looked
closely, but the issue is likely the same.
It's atomic and side effect free if done correctly. We have pleasing
examples of this. This is hard to get right though, so naturally mistakes
are made.
Now that I'm looking at the syscall_{dis}establish() functions in detail,
I see that indeed the l_sysent checking is a good idea and does not require
refcounts; however the accesses on 'sy_call' should be atomic_relaxed. The
cost of these atomics is my concern.
Maxime
Re: [filemon] CVS commit: htdocs/support/security
On Tue, Dec 17, 2019 at 04:06:12PM +0100, Kamil Rytarowski wrote: > On 17.12.2019 15:44, Andrew Doran wrote: > Typically with a character device, the kmod can get unloaded while an > ioctl > is being executed on it. > > > > That's solvable. Conceptually I think the main stumbling block is that > > there are two layers at play which need to reconciled: specfs and devsw. It > > could also be an opportunity to lessen the distinction between block and > > character devices, there's no real need for cached access from userspace, > > that would simplify things too. > > > When it comes to syscalls, I haven't looked > closely, but the issue is likely the same. > > > > It's atomic and side effect free if done correctly. We have pleasing > > examples of this. This is hard to get right though, so naturally mistakes > > are made. > > > > It would be nice to have at least an example of doing it right. Sure, look at ksem_sysfini(). It tries to uninstall the syscall package. If any syscall is in use, fail. It then looks to see if it has created ksems. If any exist fail, and plug the syscalls back in. While that's happening new ksem syscalls are being gated by sys_nomodule() (this all happens under lock). The legitimate users of the ksem syscalls will never see a spurious failure due to an attempt to unload. ksem_sysinit() seems to have a bug though; ksem_max should be set before the syscall package is installed. Andrew
Re: [filemon] CVS commit: htdocs/support/security
Le 17/12/2019 à 12:34, Kamil Rytarowski a écrit : On 17.12.2019 09:16, Maxime Villard wrote: Module Name: htdocs Committed By: christos Date: Tue Dec 17 01:03:49 UTC 2019 Modified Files: htdocs/support/security: advisory.html index.html Log Message: new advisory To generate a diff of this commit: cvs rdiff -u -r1.140 -r1.141 htdocs/support/security/advisory.html cvs rdiff -u -r1.173 -r1.174 htdocs/support/security/index.html Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. There is something I don't understand here. Why keep this totally useless misfeature, when we already have many tracing facilities that can do just about the same job without having security issues? The recommendation in the advisory is literally to remove the kernel module from the fs. I don't see what could possibly be the use of such a misfeature as filemon; I would remove it completely from the kernel source tree. Maxime From: http://ftp.netbsd.org/pub/NetBSD/security/advisories/NetBSD-SA2019-006.txt.asc "Additionally the way filemon does filesystem interception is racy and can lead to random crashes if the system calls are in use while the module is unloaded." Is this issue fixable? Not speaking for filemon in particular, I find this ability to rewrite the syscall table on the fly as a feature. Keeping a functional module with this property (even if disabled by default) seems useful to me. As far as I can tell, there are many races caused by autoloading. Typically with a character device, the kmod can get unloaded while an ioctl is being executed on it. When it comes to syscalls, I haven't looked closely, but the issue is likely the same. You can use tricks to "narrow down" the races; eg in NVMM, I use a global 'nmachines' variable, which prevents unloading in ~most cases. But I see no way to fix these races, except using atomic refcounts and mutexes on the ioctls and syscalls; obviously, this won't scale. I find this autoloading stuff to be a misfeature, too. If we want something on by default, then we should put it in GENERIC; if we want it disabled but accessible, then we should put it in a kmod that requires a manual modload from root. Putting stuff in kmods AND having the kernel load them automatically serves no purpose; it just adds bugs, and sometimes creates the wrong feeling that a driver is disabled while it isn't (like filemon). Other than that, I don't really understand your point; you can still do syscall interception with a custom kmod if you want, regardless of filemon. Maxime
Re: [filemon] CVS commit: htdocs/support/security
Please note that we do have a way of installing new syscalls (via the
syscall_{,dis}establish() mechanism), but that only works for syscalls
that are not currently in use. We don't have a (clean) way to replace
an already-installed syscall (no way to retrieve the current function
pointer).
On Tue, 17 Dec 2019, Kamil Rytarowski wrote:
On 17.12.2019 15:44, Andrew Doran wrote:
Typically with a character device, the kmod can get unloaded while an ioctl
is being executed on it.
That's solvable. Conceptually I think the main stumbling block is that
there are two layers at play which need to reconciled: specfs and devsw. It
could also be an opportunity to lessen the distinction between block and
character devices, there's no real need for cached access from userspace,
that would simplify things too.
When it comes to syscalls, I haven't looked
closely, but the issue is likely the same.
It's atomic and side effect free if done correctly. We have pleasing
examples of this. This is hard to get right though, so naturally mistakes
are made.
It would be nice to have at least an example of doing it right.
Andrew
!DSPAM:5df8f070102259202610505!
++--+---+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org |
++--+---+
Re: [filemon] CVS commit: htdocs/support/security
On 17.12.2019 15:44, Andrew Doran wrote: Typically with a character device, the kmod can get unloaded while an ioctl is being executed on it. > > That's solvable. Conceptually I think the main stumbling block is that > there are two layers at play which need to reconciled: specfs and devsw. It > could also be an opportunity to lessen the distinction between block and > character devices, there's no real need for cached access from userspace, > that would simplify things too. > When it comes to syscalls, I haven't looked closely, but the issue is likely the same. > > It's atomic and side effect free if done correctly. We have pleasing > examples of this. This is hard to get right though, so naturally mistakes > are made. > It would be nice to have at least an example of doing it right. > Andrew >
Re: [filemon] CVS commit: htdocs/support/security
> > > Typically with a character device, the kmod can get unloaded while an > > > ioctl > > > is being executed on it. That's solvable. Conceptually I think the main stumbling block is that there are two layers at play which need to reconciled: specfs and devsw. It could also be an opportunity to lessen the distinction between block and character devices, there's no real need for cached access from userspace, that would simplify things too. > > > When it comes to syscalls, I haven't looked > > > closely, but the issue is likely the same. It's atomic and side effect free if done correctly. We have pleasing examples of this. This is hard to get right though, so naturally mistakes are made. Andrew
Re: [filemon] CVS commit: htdocs/support/security
Le 17/12/2019 à 14:32, Kamil Rytarowski a écrit :
On 17.12.2019 14:19, Maxime Villard wrote:
Le 17/12/2019 à 12:34, Kamil Rytarowski a écrit :
On 17.12.2019 09:16, Maxime Villard wrote:
Module Name: htdocs
Committed By: christos
Date: Tue Dec 17 01:03:49 UTC 2019
Modified Files:
htdocs/support/security: advisory.html index.html
Log Message:
new advisory
To generate a diff of this commit:
cvs rdiff -u -r1.140 -r1.141 htdocs/support/security/advisory.html
cvs rdiff -u -r1.173 -r1.174 htdocs/support/security/index.html
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
There is something I don't understand here. Why keep this totally
useless
misfeature, when we already have many tracing facilities that can do
just
about the same job without having security issues?
The recommendation in the advisory is literally to remove the kernel
module from the fs. I don't see what could possibly be the use of such a
misfeature as filemon; I would remove it completely from the kernel
source tree.
Maxime
From:
http://ftp.netbsd.org/pub/NetBSD/security/advisories/NetBSD-SA2019-006.txt.asc
"Additionally the way filemon does filesystem interception is racy
and can lead to random crashes if the system calls are in use
while the module is unloaded."
Is this issue fixable? Not speaking for filemon in particular, I find
this ability to rewrite the syscall table on the fly as a feature.
Keeping a functional module with this property (even if disabled by
default) seems useful to me.
As far as I can tell, there are many races caused by autoloading.
Typically with a character device, the kmod can get unloaded while an ioctl
is being executed on it. When it comes to syscalls, I haven't looked
closely, but the issue is likely the same.
You can use tricks to "narrow down" the races; eg in NVMM, I use a global
'nmachines' variable, which prevents unloading in ~most cases. But I see
no way to fix these races, except using atomic refcounts and mutexes on
the ioctls and syscalls; obviously, this won't scale.
I find this autoloading stuff to be a misfeature, too. If we want something
on by default, then we should put it in GENERIC; if we want it disabled but
accessible, then we should put it in a kmod that requires a manual modload
from root.
Putting stuff in kmods AND having the kernel load them automatically serves
no purpose; it just adds bugs, and sometimes creates the wrong feeling that
a driver is disabled while it isn't (like filemon).
Other than that, I don't really understand your point; you can still do
syscall interception with a custom kmod if you want, regardless of filemon.
Out of filemon, I am only interested in syscall_hijack
(filemon_wrapper_install() + filemon_wrapper_deinstall()). If that can
be reliable, I would keep it in src/sys/modules/example.
https://nxr.netbsd.org/xref/src/sys/dev/filemon/filemon_wrapper.c#90
This code is big cancer, and I'd rather not make it an example. I would prefer
an example with syscall_{dis}establish().
I have no personal interest on the rest of filemon. Switching this
make(1) feature to at least ptrace(2) should be straightforward.
Yes, and that would be a lot more useful and reliable than filemon. What's
clear nevertheless, is that now that we've stopped installing the filemon
kmod and told users to actually remove the file, make+filemon has no
remaining use.
I will prepare the removal of filemon.
Maxime
Re: [filemon] CVS commit: htdocs/support/security
On 17.12.2019 14:19, Maxime Villard wrote: > Le 17/12/2019 à 12:34, Kamil Rytarowski a écrit : >> On 17.12.2019 09:16, Maxime Villard wrote: Module Name: htdocs Committed By: christos Date: Tue Dec 17 01:03:49 UTC 2019 Modified Files: htdocs/support/security: advisory.html index.html Log Message: new advisory To generate a diff of this commit: cvs rdiff -u -r1.140 -r1.141 htdocs/support/security/advisory.html cvs rdiff -u -r1.173 -r1.174 htdocs/support/security/index.html Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. >>> >>> There is something I don't understand here. Why keep this totally >>> useless >>> misfeature, when we already have many tracing facilities that can do >>> just >>> about the same job without having security issues? >>> >>> The recommendation in the advisory is literally to remove the kernel >>> module from the fs. I don't see what could possibly be the use of such a >>> misfeature as filemon; I would remove it completely from the kernel >>> source tree. >>> >>> Maxime >> >> From: >> http://ftp.netbsd.org/pub/NetBSD/security/advisories/NetBSD-SA2019-006.txt.asc >> >> >> "Additionally the way filemon does filesystem interception is racy >> and can lead to random crashes if the system calls are in use >> while the module is unloaded." >> >> Is this issue fixable? Not speaking for filemon in particular, I find >> this ability to rewrite the syscall table on the fly as a feature. >> Keeping a functional module with this property (even if disabled by >> default) seems useful to me. > > As far as I can tell, there are many races caused by autoloading. > > Typically with a character device, the kmod can get unloaded while an ioctl > is being executed on it. When it comes to syscalls, I haven't looked > closely, but the issue is likely the same. > > You can use tricks to "narrow down" the races; eg in NVMM, I use a global > 'nmachines' variable, which prevents unloading in ~most cases. But I see > no way to fix these races, except using atomic refcounts and mutexes on > the ioctls and syscalls; obviously, this won't scale. > > I find this autoloading stuff to be a misfeature, too. If we want something > on by default, then we should put it in GENERIC; if we want it disabled but > accessible, then we should put it in a kmod that requires a manual modload > from root. > > Putting stuff in kmods AND having the kernel load them automatically serves > no purpose; it just adds bugs, and sometimes creates the wrong feeling that > a driver is disabled while it isn't (like filemon). > > Other than that, I don't really understand your point; you can still do > syscall interception with a custom kmod if you want, regardless of filemon. > Out of filemon, I am only interested in syscall_hijack (filemon_wrapper_install() + filemon_wrapper_deinstall()). If that can be reliable, I would keep it in src/sys/modules/example. https://nxr.netbsd.org/xref/src/sys/dev/filemon/filemon_wrapper.c#90 I have no personal interest on the rest of filemon. Switching this make(1) feature to at least ptrace(2) should be straightforward. > Maxime
Re: [filemon] CVS commit: htdocs/support/security
On 17.12.2019 09:16, Maxime Villard wrote: >> Module Name: htdocs >> Committed By: christos >> Date: Tue Dec 17 01:03:49 UTC 2019 >> >> Modified Files: >> htdocs/support/security: advisory.html index.html >> >> Log Message: >> new advisory >> >> >> To generate a diff of this commit: >> cvs rdiff -u -r1.140 -r1.141 htdocs/support/security/advisory.html >> cvs rdiff -u -r1.173 -r1.174 htdocs/support/security/index.html >> >> Please note that diffs are not public domain; they are subject to the >> copyright notices on the relevant files. > > There is something I don't understand here. Why keep this totally useless > misfeature, when we already have many tracing facilities that can do just > about the same job without having security issues? > > The recommendation in the advisory is literally to remove the kernel > module from the fs. I don't see what could possibly be the use of such a > misfeature as filemon; I would remove it completely from the kernel > source tree. > > Maxime From: http://ftp.netbsd.org/pub/NetBSD/security/advisories/NetBSD-SA2019-006.txt.asc "Additionally the way filemon does filesystem interception is racy and can lead to random crashes if the system calls are in use while the module is unloaded." Is this issue fixable? Not speaking for filemon in particular, I find this ability to rewrite the syscall table on the fly as a feature. Keeping a functional module with this property (even if disabled by default) seems useful to me.
