Re: [filemon] CVS commit: htdocs/support/security

[Kamil Rytarowski <n54%gmx.com@localhost>]

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