Re: secmodel_securelevel(9) and machdep.svs.enabled

[Alexander Nasonov]

26.04.2018, 13:03, "Thor Lancelot Simon" :If that is true it's a serious regression.  Are you talking about thepathological case where "options INSECURE" is set?ThorMy reasoning was based entirely upon reading secmodel_securelevel(9) and kmem(4) man pages. I don’t know whether it accurately reflects the reality.Alex

Re: secmodel_securelevel(9) and machdep.svs.enabled

[Thor Lancelot Simon]

On Wed, Apr 25, 2018 at 09:43:18PM +0100, Alexander Nasonov wrote:
> 
> Thinking a bit more about this, I don't think my patch will prevent
> data leakage from the kernel because /dev/mem and /dev/kmem are
> readable at all securelevels. It can only prevent leakage in some

If that is true it's a serious regression.  Are you talking about the
pathological case where "options INSECURE" is set?

Thor

Re: secmodel_securelevel(9) and machdep.svs.enabled

[Alexander Nasonov]

Alexander Nasonov wrote:
> Thinking a bit more about this, I don't think my patch will prevent
> data leakage from the kernel because /dev/mem and /dev/kmem are
> readable at all securelevels.

There is an important distrinction, though. Code in sys/dev/mm.c
can be changed to scramble sensitive pages (e.g. cgd(4) keys) while
meltdown is a wild beast and it's nearly impossible to control.

-- 
Alex

Re: secmodel_securelevel(9) and machdep.svs.enabled

[Alexander Nasonov]

Maxime Villard wrote:
> Yes, it's fine. I've never taken care of securelevel, but your change
> can't be incorrect. Perhaps I would use just KAUTH_MACHDEP_SVS instead
> of KAUTH_MACHDEP_SVS_DISABLE, in case another operation gets added in
> the future, but that doesn't matter.

I don't think securelevel should care about details of SVS. If you
want to introduce levels of SVS, KAUTH_MACHDEP_SVS_DISABLE can still
be used to prevent lowering (instead of disabling SVS completely).
Perhaps the name can be changed to KAUTH_MACHDEP_SVS_DEGRADE or
something similar but it's not that important.

Thinking a bit more about this, I don't think my patch will prevent
data leakage from the kernel because /dev/mem and /dev/kmem are
readable at all securelevels. It can only prevent leakage in some
situations. For example, when root is compromised inside chroot
and chroot directory is on a file system mounted with nodev.

-- 
Alex

re: secmodel_securelevel(9) and machdep.svs.enabled

[matthew green]

Maxime Villard writes:
> Le 25/04/2018 à 19:47, Alexander Nasonov a écrit :
> > Alexander Nasonov wrote:
> >> Alexander Nasonov wrote:
> >>> When securelevel is set, should be lock 1->0 change for
> >>> machdep.svs.enabled (and possibly for other sysctls related
> >>> to recent security mitigations)?
> >>
> >> Can I commit the attached patch? (doc update will follow)
> > 
> > If I don't hear any objections, I will commit the patch soon and
> > I will request a pullup to netbsd-8.

it's the right idea to me.

> > Alex
> 
> Yes, it's fine. I've never taken care of securelevel, but your change
> can't be incorrect. Perhaps I would use just KAUTH_MACHDEP_SVS instead
> of KAUTH_MACHDEP_SVS_DISABLE, in case another operation gets added in
> the future, but that doesn't matter.

i considered this idea -- plain SVS would have to not include
ENABLE, which doesn't seem right.  perhaps another generic
name that implies !enable would work.


.mrg.

Re: secmodel_securelevel(9) and machdep.svs.enabled

[Maxime Villard]

Le 25/04/2018 à 19:47, Alexander Nasonov a écrit :

Alexander Nasonov wrote:

Alexander Nasonov wrote:

When securelevel is set, should be lock 1->0 change for
machdep.svs.enabled (and possibly for other sysctls related
to recent security mitigations)?


Can I commit the attached patch? (doc update will follow)


If I don't hear any objections, I will commit the patch soon and
I will request a pullup to netbsd-8.

Alex


Yes, it's fine. I've never taken care of securelevel, but your change
can't be incorrect. Perhaps I would use just KAUTH_MACHDEP_SVS instead
of KAUTH_MACHDEP_SVS_DISABLE, in case another operation gets added in
the future, but that doesn't matter.

Maxime

Re: secmodel_securelevel(9) and machdep.svs.enabled

[Alexander Nasonov]

Alexander Nasonov wrote:
> When securelevel is set, should be lock 1->0 change for
> machdep.svs.enabled (and possibly for other sysctls related
> to recent security mitigations)?

Can I commit the attached patch? (doc update will follow)

-- 
Alex
Index: src/sys/sys/kauth.h
===
RCS file: /cvsroot/src/sys/sys/kauth.h,v
retrieving revision 1.75
diff -p -u -u -r1.75 kauth.h
--- src/sys/sys/kauth.h 28 Aug 2017 00:46:07 -  1.75
+++ src/sys/sys/kauth.h 24 Apr 2018 17:59:13 -
@@ -320,7 +320,8 @@ enum {
KAUTH_MACHDEP_NVRAM,
KAUTH_MACHDEP_UNMANAGEDMEM,
KAUTH_MACHDEP_PXG,
-   KAUTH_MACHDEP_X86PMC
+   KAUTH_MACHDEP_X86PMC,
+   KAUTH_MACHDEP_SVS_DISABLE
 };
 
 /*
Index: src/sys/secmodel/suser/secmodel_suser.c
===
RCS file: /cvsroot/src/sys/secmodel/suser/secmodel_suser.c,v
retrieving revision 1.43
diff -p -u -u -r1.43 secmodel_suser.c
--- src/sys/secmodel/suser/secmodel_suser.c 14 Jun 2017 17:48:41 -  
1.43
+++ src/sys/secmodel/suser/secmodel_suser.c 24 Apr 2018 17:59:13 -
@@ -854,6 +854,7 @@ secmodel_suser_machdep_cb(kauth_cred_t c
case KAUTH_MACHDEP_UNMANAGEDMEM:
case KAUTH_MACHDEP_PXG:
case KAUTH_MACHDEP_X86PMC:
+   case KAUTH_MACHDEP_SVS_DISABLE:
if (isroot)
result = KAUTH_RESULT_ALLOW;
break;
Index: src/sys/secmodel/securelevel/secmodel_securelevel.c
===
RCS file: /cvsroot/src/sys/secmodel/securelevel/secmodel_securelevel.c,v
retrieving revision 1.30
diff -p -u -u -r1.30 secmodel_securelevel.c
--- src/sys/secmodel/securelevel/secmodel_securelevel.c 25 Feb 2014 18:30:13 
-  1.30
+++ src/sys/secmodel/securelevel/secmodel_securelevel.c 24 Apr 2018 17:59:13 
-
@@ -494,6 +494,11 @@ secmodel_securelevel_machdep_cb(kauth_cr
result = KAUTH_RESULT_DENY;
break;
 
+   case KAUTH_MACHDEP_SVS_DISABLE:
+   if (securelevel > 0)
+   result = KAUTH_RESULT_DENY;
+   break;
+
case KAUTH_MACHDEP_CPU_UCODE_APPLY:
if (securelevel > 1)
result = KAUTH_RESULT_DENY;
Index: src/sys/arch/x86/x86/svs.c
===
RCS file: /cvsroot/src/sys/arch/x86/x86/svs.c,v
retrieving revision 1.17
diff -p -u -u -r1.17 svs.c
--- src/sys/arch/x86/x86/svs.c  30 Mar 2018 19:58:05 -  1.17
+++ src/sys/arch/x86/x86/svs.c  24 Apr 2018 17:59:11 -
@@ -38,6 +38,7 @@ __KERNEL_RCSID(0, "$NetBSD: svs.c,v 1.17
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -737,11 +738,13 @@ sysctl_machdep_svs_enabled(SYSCTLFN_ARGS
error = 0;
else
error = EOPNOTSUPP;
-   } else {
-   if (svs_enabled)
+   } else if (svs_enabled) {
+   error = kauth_authorize_machdep(kauth_cred_get(),
+   KAUTH_MACHDEP_SVS_DISABLE, NULL, NULL, NULL, NULL);
+   if (!error)
error = svs_disable();
-   else
-   error = 0;
+   } else {
+   error = 0;
}
 
return error;

Re: secmodel_securelevel(9) and machdep.svs.enabled

[Paul Goyette]

On Thu, 19 Apr 2018, Alexander Nasonov wrote:


When securelevel is set, should be lock 1->0 change for
machdep.svs.enabled (and possibly for other sysctls related
to recent security mitigations)?


Possibly.  At the very least, we should prevent _disabling" the
svs code if securelevel is set.  IMHO.


+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses:  |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+--+--++

secmodel_securelevel(9) and machdep.svs.enabled

[Alexander Nasonov]

When securelevel is set, should be lock 1->0 change for
machdep.svs.enabled (and possibly for other sysctls related
to recent security mitigations)?

-- 
Alex