[PATCH] power: disable hibernation if module loading is disabled

[PATCH] power: disable hibernation if module loading is disabled

[Vasiliy Kulikov]

If /proc/sys/kernel/modules_disabled is set to 1, then nobody (even full
root) may not read/write arbitrary kernel memory.  In spite of it,
hibernation allows anyone with an access to either /dev/snapshot or
/sys/power/ make the full snapshot of the system.  This snapshot may be
freely changed and uploaded back.

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 kernel/power/hibernate.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 1832bd2..1ac9eee 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -328,6 +328,9 @@ int hibernation_snapshot(int platform_mode)
 {
 	int error;
 
+	if (modules_disabled)
+		return -EPERM;
+
 	error = platform_begin(platform_mode);
 	if (error)
 		goto Close;
@@ -385,6 +388,9 @@ static int resume_target_kernel(bool platform_mode)
 {
 	int error;
 
+	if (modules_disabled)
+		return -EPERM;
+
 	error = dpm_suspend_noirq(PMSG_QUIESCE);
 	if (error) {
 		printk(KERN_ERR "PM: Some devices failed to power down, "
-- 
1.7.0.4

Re: [PATCH] power: disable hibernation if module loading is disabled

[Kees Cook]

On Fri, Mar 04, 2011 at 07:11:24PM +0300, Vasiliy Kulikov wrote:
> If /proc/sys/kernel/modules_disabled is set to 1, then nobody (even full
> root) may not read/write arbitrary kernel memory.  In spite of it,
> hibernation allows anyone with an access to either /dev/snapshot or
> /sys/power/ make the full snapshot of the system.  This snapshot may be
> freely changed and uploaded back.

Ah, yes please. I'd like to try to have ways to close all the
"intentional" arbitrary memory writing interfaces. Hooking it to
modules_disable seems as good as any other toggle. Still waiting to hear
anything on this: http://article.gmane.org/gmane.linux.acpi.devel/49471

Acked-by: Kees Cook <kees.cook@canonical.com>

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] power: disable hibernation if module loading is disabled

[Rafael J. Wysocki]

On Friday, March 04, 2011, Kees Cook wrote:
> On Fri, Mar 04, 2011 at 07:11:24PM +0300, Vasiliy Kulikov wrote:
> > If /proc/sys/kernel/modules_disabled is set to 1, then nobody (even full
> > root) may not read/write arbitrary kernel memory.  In spite of it,
> > hibernation allows anyone with an access to either /dev/snapshot or
> > /sys/power/ make the full snapshot of the system.  This snapshot may be
> > freely changed and uploaded back.
> 
> Ah, yes please. I'd like to try to have ways to close all the
> "intentional" arbitrary memory writing interfaces.

They are not exactly arbitrary and I'd like to see a plausible attack
scenario using the hibernation interface as is (assuming you're not a
full root at least).

> Hooking it to modules_disable seems as good as any other toggle.

You're kidding, aren't you?

> Still waiting to hear
> anything on this: http://article.gmane.org/gmane.linux.acpi.devel/49471

I personally don't think it's a good idea, but I'm not the maintainer of that
code.

Thanks,
Rafael

Re: [PATCH] power: disable hibernation if module loading is disabled

[Pavel Machek]

On Fri 2011-03-04 19:11:24, Vasiliy Kulikov wrote:
> If /proc/sys/kernel/modules_disabled is set to 1, then nobody (even full
> root) may not read/write arbitrary kernel memory.  In spite of it,
> hibernation allows anyone with an access to either /dev/snapshot or
> /sys/power/ make the full snapshot of the system.  This snapshot may be
> freely changed and uploaded back.

module loading has nothing to do with hibernation, and hibernation
already checks for CAP_ADMIN or something similary strong. I don't see
why this new check is needed.

In fact, you are probably breaking someone's setup right now.

> +	if (modules_disabled)
> +		return -EPERM;
> +
>  	error = platform_begin(platform_mode);

Hmm. How is this supposed to work with CONFIG_MODULES off?

NAK.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] power: disable hibernation if module loading is disabled

[Vasiliy Kulikov]

On Fri, Mar 04, 2011 at 21:32 +0100, Pavel Machek wrote:
> > +	if (modules_disabled)
> > +		return -EPERM;
> > +
> >  	error = platform_begin(platform_mode);
> 
> Hmm. How is this supposed to work with CONFIG_MODULES off?

Good catch, thank you.  I'll fix this.

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] power: disable hibernation if module loading is disabled

[Rafael J. Wysocki]

On Friday, March 04, 2011, Vasiliy Kulikov wrote:
> If /proc/sys/kernel/modules_disabled is set to 1, then nobody (even full
> root) may not read/write arbitrary kernel memory.  In spite of it,
> hibernation allows anyone with an access to either /dev/snapshot or
> /sys/power/ make the full snapshot of the system.  This snapshot may be
> freely changed and uploaded back.
> 
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>

That "everyone" is actually the "full root" (in the case of /sys/power/state)
or someone having CAP_SYS_ADMIN in the /dev/snapshot case, right?

So the changelog is misleading and please fix it.

Second, there's _zero_ relationship between /proc/sys/kernel/modules_disabled
and the hibernation interface, so please find a different way to solve the
problem (if there is any).

Thanks,
Rafael


> ---
>  kernel/power/hibernate.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 1832bd2..1ac9eee 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -328,6 +328,9 @@ int hibernation_snapshot(int platform_mode)
>  {
>  	int error;
>  
> +	if (modules_disabled)
> +		return -EPERM;
> +
>  	error = platform_begin(platform_mode);
>  	if (error)
>  		goto Close;
> @@ -385,6 +388,9 @@ static int resume_target_kernel(bool platform_mode)
>  {
>  	int error;
>  
> +	if (modules_disabled)
> +		return -EPERM;
> +
>  	error = dpm_suspend_noirq(PMSG_QUIESCE);
>  	if (error) {
>  		printk(KERN_ERR "PM: Some devices failed to power down, "
> 

Re: [PATCH] power: disable hibernation if module loading is disabled

[Vasiliy Kulikov]

On Fri, Mar 04, 2011 at 21:42 +0100, Rafael J. Wysocki wrote:
> That "everyone" is actually the "full root" (in the case of /sys/power/state)
> or someone having CAP_SYS_ADMIN in the /dev/snapshot case, right?

Yes, sorry for my bad english.  "Everyone" is indeed misleading :-D

> Second, there's _zero_ relationship between /proc/sys/kernel/modules_disabled
> and the hibernation interface, so please find a different way to solve the
> problem (if there is any).

If modules_disabled is set to 1, then nobody, even full root may not write
to the kernel, right?  So, if something permits to indirectly pass
modules_disabled restriction, this is a bug.  Otherwise,
modules_disabled is confusing as it gives false sense of security.

-OR-

modules_disabled's documentation should be changed to note that it
doesn't prevent rootkit uploading, but only forbids modprob'ing modules
via the "official" init_module(2) gate, disallowing e.g. module autoloading.


Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] power: disable hibernation if module loading is disabled

[Rafael J. Wysocki]

On Friday, March 04, 2011, Vasiliy Kulikov wrote:
> On Fri, Mar 04, 2011 at 21:42 +0100, Rafael J. Wysocki wrote:
> > That "everyone" is actually the "full root" (in the case of /sys/power/state)
> > or someone having CAP_SYS_ADMIN in the /dev/snapshot case, right?
> 
> Yes, sorry for my bad english.  "Everyone" is indeed misleading :-D
> 
> > Second, there's _zero_ relationship between /proc/sys/kernel/modules_disabled
> > and the hibernation interface, so please find a different way to solve the
> > problem (if there is any).
> 
> If modules_disabled is set to 1, then nobody, even full root may not write
> to the kernel, right?  So, if something permits to indirectly pass
> modules_disabled restriction, this is a bug.  Otherwise,
> modules_disabled is confusing as it gives false sense of security.
> 
> -OR-
> 
> modules_disabled's documentation should be changed to note that it
> doesn't prevent rootkit uploading, but only forbids modprob'ing modules
> via the "official" init_module(2) gate, disallowing e.g. module autoloading.

Why not to change that documentation, then?

Also, please note that in order to "write" into memory using the hibernation
interface you need to have write access to swap, which you can use to corrupt
memory regardless of the modules_disabled setting AFAICS.

Thanks,
Rafael

Re: [PATCH] power: disable hibernation if module loading is disabled

[Vasiliy Kulikov]

On Fri, Mar 04, 2011 at 22:21 +0100, Rafael J. Wysocki wrote:
> On Friday, March 04, 2011, Vasiliy Kulikov wrote:
> > If modules_disabled is set to 1, then nobody, even full root may not write
> > to the kernel, right?  So, if something permits to indirectly pass
> > modules_disabled restriction, this is a bug.  Otherwise,
> > modules_disabled is confusing as it gives false sense of security.
> > 
> > -OR-
> > 
> > modules_disabled's documentation should be changed to note that it
> > doesn't prevent rootkit uploading, but only forbids modprob'ing modules
> > via the "official" init_module(2) gate, disallowing e.g. module autoloading.
> 
> Why not to change that documentation, then?

Because it's better to fix something (if it is possible, of course) than
simply documenting the bug.

> Also, please note that in order to "write" into memory using the hibernation
> interface you need to have write access to swap,

No, you may just "write the kernel" via write() /dev/snapshot, this is
the way uswsusp works.  I didn't check whether it really needs
temporary file to change the kernel memory or it may be done entirely
without disk iteraction.  This is irrelevant to modules_disabled policy
violation, though.

> which you can use to corrupt
> memory regardless of the modules_disabled setting AFAICS.

Please correct me if I'm wrong, but kernel memory is not swappable at
all and only userspace memory is written to the swap.  Root with
CAP_SYS_ADMIN already may do everything with all processes, so this is
not a threat.

If one may change kernel memory via swap then it is another problem with
modules_disabled.

Thanks,

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] power: disable hibernation if module loading is disabled

[Rafael J. Wysocki]

On Friday, March 04, 2011, Vasiliy Kulikov wrote:
> On Fri, Mar 04, 2011 at 22:21 +0100, Rafael J. Wysocki wrote:
> > On Friday, March 04, 2011, Vasiliy Kulikov wrote:
> > > If modules_disabled is set to 1, then nobody, even full root may not write
> > > to the kernel, right?  So, if something permits to indirectly pass
> > > modules_disabled restriction, this is a bug.  Otherwise,
> > > modules_disabled is confusing as it gives false sense of security.
> > > 
> > > -OR-
> > > 
> > > modules_disabled's documentation should be changed to note that it
> > > doesn't prevent rootkit uploading, but only forbids modprob'ing modules
> > > via the "official" init_module(2) gate, disallowing e.g. module autoloading.
> > 
> > Why not to change that documentation, then?
> 
> Because it's better to fix something (if it is possible, of course) than
> simply documenting the bug.

modules_disabled surely is not the right interface to disable hibernation
and I don't really think there's a bug because it doesn't work as you'd like
it to.  In fact, there would be a bug if it did work that way.

> > Also, please note that in order to "write" into memory using the hibernation
> > interface you need to have write access to swap,
> 
> No, you may just "write the kernel" via write() /dev/snapshot, this is
> the way uswsusp works.  I didn't check whether it really needs
> temporary file to change the kernel memory or it may be done entirely
> without disk iteraction.  This is irrelevant to modules_disabled policy
> violation, though.

Sorry, but who defined the "modules_disabled policy" and when did that happen
and how come that I'm not aware of it?

> > which you can use to corrupt
> > memory regardless of the modules_disabled setting AFAICS.
> 
> Please correct me if I'm wrong, but kernel memory is not swappable at
> all and only userspace memory is written to the swap.  Root with
> CAP_SYS_ADMIN already may do everything with all processes, so this is
> not a threat.

OK

> If one may change kernel memory via swap then it is another problem with
> modules_disabled.

If you want an interface to disable _any_ kind of writes into the kernel
memory by any means, then please add it and don't call it modules_disabled,
because it's a hell of a confusing name and no amount of documentation
can help that.

Thanks,
Rafael

Re: [PATCH] power: disable hibernation if module loading is disabled

[Pavel Machek]

> > If one may change kernel memory via swap then it is another problem with
> > modules_disabled.
> 
> If you want an interface to disable _any_ kind of writes into the kernel
> memory by any means, then please add it and don't call it modules_disabled,
> because it's a hell of a confusing name and no amount of documentation
> can help that.

...and that interface should be CAP_SYS_something, I'd say...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] power: disable hibernation if module loading is disabled

[Vasiliy Kulikov]

On Fri, Mar 04, 2011 at 23:30 +0100, Rafael J. Wysocki wrote:
> modules_disabled surely is not the right interface to disable hibernation
> and I don't really think there's a bug because it doesn't work as you'd like
> it to.  In fact, there would be a bug if it did work that way.

What do you mean here?  Do you agree that you may read kernel image,
slightly change it (including e.g. possible checksums, I didn't bother
to check how much one should change), and write it back?

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] power: disable hibernation if module loading is disabled

[Rafael J. Wysocki]

On Saturday, March 05, 2011, Vasiliy Kulikov wrote:
> On Fri, Mar 04, 2011 at 23:30 +0100, Rafael J. Wysocki wrote:
> > modules_disabled surely is not the right interface to disable hibernation
> > and I don't really think there's a bug because it doesn't work as you'd like
> > it to.  In fact, there would be a bug if it did work that way.
> 
> What do you mean here?  Do you agree that you may read kernel image,
> slightly change it (including e.g. possible checksums, I didn't bother
> to check how much one should change), and write it back?

Yes, you can, but that's not the point.  The point is that calling an interface
that disables all possible functionality modifying kernel memory
"modules_disabled" is completely dumb.  Sorry, but that's how it goes.

As I said before, if you want to have such an interface, call it properly
and introduce it along with documentation instead of changing an existing
one in a backwards-incopmatible fashion that in addition is totally
confusing.

Moreover, how are you going to protect your "protect kernel memory from
modification" interface itself from root access?

Re: [PATCH] power: disable hibernation if module loading is disabled

[Pavel Machek]

Hi!


> > > modules_disabled surely is not the right interface to disable hibernation
> > > and I don't really think there's a bug because it doesn't work as you'd like
> > > it to.  In fact, there would be a bug if it did work that way.
> > 
> > What do you mean here?  Do you agree that you may read kernel image,
> > slightly change it (including e.g. possible checksums, I didn't bother
> > to check how much one should change), and write it back?
> 
> Yes, you can, but that's not the point.  The point is that calling an interface
> that disables all possible functionality modifying kernel memory
> "modules_disabled" is completely dumb.  Sorry, but that's how it goes.

Fully agreed.

If you want a subset of cap_sys_admin than can't install
rootkit... just do it like that. Create cap_small_admin with such
subset and migrate people that don't need full admin to it.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Security] [PATCH] power: disable hibernation if module loading is disabled

[Greg KH]

On Fri, Mar 04, 2011 at 07:11:24PM +0300, Vasiliy Kulikov wrote:
> If /proc/sys/kernel/modules_disabled is set to 1, then nobody (even full
> root) may not read/write arbitrary kernel memory.  In spite of it,
> hibernation allows anyone with an access to either /dev/snapshot or
> /sys/power/ make the full snapshot of the system.  This snapshot may be
> freely changed and uploaded back.

This sounds like a very unintentional change to the "don't load any
modules" option, right?  If so, you should really document this
somewhere, otherwise people are going to get very confused when their
system suspends suddenly stop working for no obvious reason.

thanks,

greg k-h

Re: [Security] [PATCH] power: disable hibernation if module loading is disabled

[Vasiliy Kulikov]

On Fri, Mar 04, 2011 at 13:27 -0800, Greg KH wrote:
> On Fri, Mar 04, 2011 at 07:11:24PM +0300, Vasiliy Kulikov wrote:
> > If /proc/sys/kernel/modules_disabled is set to 1, then nobody (even full
> > root) may not read/write arbitrary kernel memory.  In spite of it,
> > hibernation allows anyone with an access to either /dev/snapshot or
> > /sys/power/ make the full snapshot of the system.  This snapshot may be
> > freely changed and uploaded back.
> 
> This sounds like a very unintentional change to the "don't load any
> modules" option, right?  If so, you should really document this
> somewhere, otherwise people are going to get very confused when their
> system suspends suddenly stop working for no obvious reason.

Agreed, thank you.  Is Documentation/sysctl/kernel.txt an appropriate
place?

-- 
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Re: [PATCH] power: disable hibernation if module loading is disabled

[david]

On Fri, 4 Mar 2011, Vasiliy Kulikov wrote:

> If /proc/sys/kernel/modules_disabled is set to 1, then nobody (even full
> root) may not read/write arbitrary kernel memory.  In spite of it,
> hibernation allows anyone with an access to either /dev/snapshot or
> /sys/power/ make the full snapshot of the system.  This snapshot may be
> freely changed and uploaded back.

given that the user can boot a different OS entirely and modify the stored 
image, I don't see how this can work, even conceptually.

and tieing anything modules related to hibernation is just wrong, you are 
mixing completely different concepts (even if the implementation happens 
to be similar)

David Lang