Re: /dev/ksyms permissions

[Christos Zoulas]

In article ,
Maxime Villard   wrote:
>Le 18/01/2018 à 13:43, Tom Ivar Helbekkmo a écrit :
>> Maxime Villard  writes:
>> 
>>> Well, looking at the code, it seems to me that _kvm_open() should be
>>> changed to keep /dev/ksyms open, the same way it keeps /dev/kmem open.
>> 
>> Agreed.  This works fine for me, with and without /dev/ksyms present:
>> 
>> Index: lib/libkvm/kvm.c
>> [...]
>
>So, I guess I can commit it?

Go for it.

christos

Re: /dev/ksyms permissions

[Maxime Villard]

Le 18/01/2018 à 13:43, Tom Ivar Helbekkmo a écrit :

Maxime Villard  writes:


Well, looking at the code, it seems to me that _kvm_open() should be
changed to keep /dev/ksyms open, the same way it keeps /dev/kmem open.


Agreed.  This works fine for me, with and without /dev/ksyms present:

Index: lib/libkvm/kvm.c
[...]


So, I guess I can commit it?

Re: /dev/ksyms permissions

[Tom Ivar Helbekkmo]

Maxime Villard  writes:

>> Agreed.  This works fine for me, with and without /dev/ksyms present:
>>
>> Index: lib/libkvm/kvm.c
>> [...]
>
> So, I guess I can commit it?

I've been running with that modification (and /dev/ksyms mode 440 and
group kmem) on all my systems since then, with no ill effects observed.

-tih
-- 
Most people who graduate with CS degrees don't understand the significance
of Lisp.  Lisp is the most important idea in computer science.  --Alan Kay


signature.asc
Description: PGP signature

Re: /dev/ksyms permissions

[Tom Ivar Helbekkmo]

Maxime Villard  writes:

> Well, looking at the code, it seems to me that _kvm_open() should be
> changed to keep /dev/ksyms open, the same way it keeps /dev/kmem open.

Agreed.  This works fine for me, with and without /dev/ksyms present:

Index: lib/libkvm/kvm.c
===
RCS file: /cvsroot/src/lib/libkvm/kvm.c,v
retrieving revision 1.102
diff -u -u -r1.102 kvm.c
--- lib/libkvm/kvm.c29 Mar 2016 06:51:40 -  1.102
+++ lib/libkvm/kvm.c18 Jan 2018 12:41:56 -
@@ -322,15 +322,6 @@
strlcpy(kd->kernelname, uf, sizeof(kd->kernelname));
} else {
strlcpy(kd->kernelname, _PATH_KSYMS, sizeof(kd->kernelname));
-   /*
-* We're here because /dev/ksyms was opened
-* successfully.  However, we don't want to keep it
-* open, so we close it now.  Later, we will open
-* it again, since it will be the only case where
-* kd->nlfd is negative.
-*/
-   close(kd->nlfd);
-   kd->nlfd = -1;
}
 
if ((kd->pmfd = open(mf, flag | O_CLOEXEC, 0)) < 0) {
@@ -769,33 +760,16 @@
 int
 kvm_nlist(kvm_t *kd, struct nlist *nl)
 {
-   int rv, nlfd;
-
-   /*
-* kd->nlfd might be negative when we get here, and in that
-* case that means that we're using /dev/ksyms.
-* So open it again, just for the time we retrieve the list.
-*/
-   if (kd->nlfd < 0) {
-   nlfd = open(_PATH_KSYMS, O_RDONLY | O_CLOEXEC, 0);
-   if (nlfd < 0) {
-   _kvm_err(kd, 0, "failed to open %s", _PATH_KSYMS);
-   return (nlfd);
-   }
-   } else
-   nlfd = kd->nlfd;
+   int rv;
 
/*
 * Call the nlist(3) routines to retrieve the given namelist.
 */
-   rv = __fdnlist(nlfd, nl);
+   rv = __fdnlist(kd->nlfd, nl);
 
if (rv == -1)
_kvm_err(kd, 0, "bad namelist");
 
-   if (kd->nlfd < 0)
-   close(nlfd);
-
return (rv);
 }
 

-tih
-- 
Most people who graduate with CS degrees don't understand the significance
of Lisp.  Lisp is the most important idea in computer science.  --Alan Kay


signature.asc
Description: PGP signature

Re: /dev/ksyms permissions

[Maxime Villard]

Le 18/01/2018 à 11:03, Tom Ivar Helbekkmo a écrit :

Maxime Villard  writes:


So, making /dev/ksyms 440 root:kmem should not break anything.

If it does, then there's a bug in the offending tool in the first place.


Agreed.  systat is one of them.  It takes care to call kvm_openfiles()
while setgid kmem, but kvm_openfiles() doesn't open /dev/ksyms,
expecting that the other kvm functions can do that at need.  So when
e.g. 'systat vmstat' calls kvm_nlist() after privileges have been
dropped, it fails:

systat: nlist: can't find following symbols:
  _intrnames
  _eintrnames
  _intrcnt
  _eintrcnt
  _allevents


Well, looking at the code, it seems to me that _kvm_open() should be changed
to keep /dev/ksyms open, the same way it keeps /dev/kmem open.

Re: /dev/ksyms permissions

[Tom Ivar Helbekkmo]

Maxime Villard  writes:

> So, making /dev/ksyms 440 root:kmem should not break anything.
>
> If it does, then there's a bug in the offending tool in the first place.

Agreed.  systat is one of them.  It takes care to call kvm_openfiles()
while setgid kmem, but kvm_openfiles() doesn't open /dev/ksyms,
expecting that the other kvm functions can do that at need.  So when
e.g. 'systat vmstat' calls kvm_nlist() after privileges have been
dropped, it fails:

systat: nlist: can't find following symbols:
 _intrnames
 _eintrnames
 _intrcnt
 _eintrcnt
 _allevents

-tih
-- 
Most people who graduate with CS degrees don't understand the significance
of Lisp.  Lisp is the most important idea in computer science.  --Alan Kay


signature.asc
Description: PGP signature

Re: /dev/ksyms permissions

[Maxime Villard]

Le 17/01/2018 à 21:43, Anders Magnusson a écrit :

Den 2018-01-17 kl. 20:20, skrev Mouse:

Maybe group kmem read, but that might require more elevated
privileges in the programs that uses ksyms.

What program uses ksyms now that doesn't require at least group kmem?

You cannot give up kmem read privileges when calling ksyms read
routines.

I don't see why not - or, at least, I don't see the ksyms change as
being relevant.  Just read /dev/ksyms at startup (at the same time as
you open /dev/kmem, probably), before dropping group kmem.  Isn't that
all this change (making /dev/ksyms 440 root:kmem) requires?

You still have to call library functions with elevated privileges compared
with today.


Not really. libkvm uses ksyms(4) only when the file opened is the active
kernel. A tool that needs to see the symbols of the kernel is a tool that
will then use kmem(4); and since /dev/kmem is already 440 root:kmem, the
tool already needs to be in $g_kmem.

So, making /dev/ksyms 440 root:kmem should not break anything.

If it does, then there's a bug in the offending tool in the first place.

For the record: I haven't yet started closing all the paths that leak kernel
addresses, but basically ksyms(4) is one of them.

Maxime

Re: /dev/ksyms permissions

[maya]

On Wed, Jan 17, 2018 at 07:19:32PM +0100, Anders Magnusson wrote:
> libkvm uses it to get the kernel symbol namelist instead of reading /netbsd
> for it (originally kvmdb, which was retired when ksyms was added).
> Programs like ps, netstat etc... uses it to find in-kernel stuff, so you
> cannot change it to require root privs to be read.
> Maybe group kmem read, but that might require more elevated privileges in
> the programs that uses ksyms.

Thanks for the heads up. I was wondering if I missed an obvious non-root
user.

Re: /dev/ksyms permissions

[Anders Magnusson]

Den 2018-01-17 kl. 20:20, skrev Mouse:

Maybe group kmem read, but that might require more elevated
privileges in the programs that uses ksyms.

What program uses ksyms now that doesn't require at least group kmem?

You cannot give up kmem read privileges when calling ksyms read
routines.

I don't see why not - or, at least, I don't see the ksyms change as
being relevant.  Just read /dev/ksyms at startup (at the same time as
you open /dev/kmem, probably), before dropping group kmem.  Isn't that
all this change (making /dev/ksyms 440 root:kmem) requires?

You still have to call library functions with elevated privileges compared
with today.  May not be a big problem, but the code should be audited first
and this behaviour documented.

-- Ragge

Re: /dev/ksyms permissions

[Mouse]

>>> Maybe group kmem read, but that might require more elevated
>>> privileges in the programs that uses ksyms.
>> What program uses ksyms now that doesn't require at least group kmem?
> You cannot give up kmem read privileges when calling ksyms read
> routines.

I don't see why not - or, at least, I don't see the ksyms change as
being relevant.  Just read /dev/ksyms at startup (at the same time as
you open /dev/kmem, probably), before dropping group kmem.  Isn't that
all this change (making /dev/ksyms 440 root:kmem) requires?

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Re: /dev/ksyms permissions

[Anders Magnusson]

Den 2018-01-17 kl. 20:03, skrev Mouse:



Maybe group kmem read, but that might require more elevated
privileges in the programs that uses ksyms.

What program uses ksyms now that doesn't require at least group kmem?


You cannot give up kmem read privileges when calling ksyms read routines.
Think setegid().

-- Ragge

Re: /dev/ksyms permissions

[Mouse]

> libkvm uses it to get the kernel symbol namelist instead of reading
> /netbsd for it (originally kvmdb, which was retired when ksyms was
> added).  Programs like ps, netstat etc... uses it to find in-kernel
> stuff, so you cannot change it to require root privs to be read.

But the symbol values are useless except for reading kernel memory (and
kernel-side debugging, which latter I think we can assume can assume
root access for).  So I see no harm changing /dev/ksyms to be 440
root:kmem.  (I don't _like_ it, and would configure my own systems
otherwise, but that's for much the same reasons I dislike kaslr, which
are fairly specific to my use aptterns.)

> Maybe group kmem read, but that might require more elevated
> privileges in the programs that uses ksyms.

What program uses ksyms now that doesn't require at least group kmem?

/~\ The ASCII Mouse
\ / Ribbon Campaign
 X  Against HTMLmo...@rodents-montreal.org
/ \ Email!   7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Re: /dev/ksyms permissions

[Christos Zoulas]

In article <20180117152524.ga11...@sdf.org>,   wrote:
>-=-=-=-=-=-
>
>This leaks information that unprivileged user probably has no reason to
>own:
>
>> cat /dev/ksyms > ksyms
>> readelf -a ksyms |wc -l
>   47594
>
>Any strong reason not to apply the following?
>Presumably it will have benefits for GENERIC_KASLR, or people with
>Intel CPUs :-)
>
>-=-=-=-=-=-
>
>Index: MAKEDEV.tmpl
>===
>RCS file: /cvsroot/src/etc/MAKEDEV.tmpl,v
>retrieving revision 1.189
>diff -u -r1.189 MAKEDEV.tmpl
>--- MAKEDEV.tmpl   9 Jan 2018 03:31:14 -   1.189
>+++ MAKEDEV.tmpl   17 Jan 2018 15:19:04 -
>@@ -933,7 +933,7 @@
>   mkdev   fullc %mem_chr% 11  666
>   mkdev   zeroc %mem_chr% 12  666
>   mkdev   klogc %log_chr% 0   600
>-  mkdev   ksyms   c %ksyms_chr% 0 444
>+  mkdev   ksyms   c %ksyms_chr% 0 400
>   mkdev   random  c %rnd_chr% 0   444
>   mkdev   urandom c %rnd_chr% 1   644
>   if ! $fdesc_mounted; then

Perhaps 440 $g_kmem, if you don't want to break the world :-)

christos

Re: /dev/ksyms permissions

[Anders Magnusson]

libkvm uses it to get the kernel symbol namelist instead of reading 
/netbsd for it (originally kvmdb, which was retired when ksyms was added).
Programs like ps, netstat etc... uses it to find in-kernel stuff, so you 
cannot change it to require root privs to be read.
Maybe group kmem read, but that might require more elevated privileges 
in the programs that uses ksyms.


-- Ragge

Den 2018-01-17 kl. 16:25, skrev co...@sdf.org:

This leaks information that unprivileged user probably has no reason to
own:


cat /dev/ksyms > ksyms
readelf -a ksyms |wc -l

47594

Any strong reason not to apply the following?
Presumably it will have benefits for GENERIC_KASLR, or people with
Intel CPUs :-)

/dev/ksyms permissions

[coypu]

This leaks information that unprivileged user probably has no reason to
own:

> cat /dev/ksyms > ksyms
> readelf -a ksyms |wc -l
   47594

Any strong reason not to apply the following?
Presumably it will have benefits for GENERIC_KASLR, or people with
Intel CPUs :-)
Index: MAKEDEV.tmpl
===
RCS file: /cvsroot/src/etc/MAKEDEV.tmpl,v
retrieving revision 1.189
diff -u -r1.189 MAKEDEV.tmpl
--- MAKEDEV.tmpl9 Jan 2018 03:31:14 -   1.189
+++ MAKEDEV.tmpl17 Jan 2018 15:19:04 -
@@ -933,7 +933,7 @@
mkdev   fullc %mem_chr% 11  666
mkdev   zeroc %mem_chr% 12  666
mkdev   klogc %log_chr% 0   600
-   mkdev   ksyms   c %ksyms_chr% 0 444
+   mkdev   ksyms   c %ksyms_chr% 0 400
mkdev   random  c %rnd_chr% 0   444
mkdev   urandom c %rnd_chr% 1   644
if ! $fdesc_mounted; then