Re: dell-smm-hwmon: security problems

[Pali RohÃr]

On Wednesday 08 June 2016 19:54:35 Austin S. Hemmelgarn wrote:
> On 2016-06-08 13:37, Guenter Roeck wrote:
> > On Wed, Jun 08, 2016 at 03:55:48PM +0200, Pali RohÃr wrote:
> >> On Wednesday 08 June 2016 15:24:10 Guenter Roeck wrote:
> >>> On 06/08/2016 02:57 AM, Pali RohÃr wrote:
> >>>> Hello!
> >>>> 
> >>>> Mario wrote me about two I think security problems in
> >>>> dell-smm-hwmon driver and I would like to ask you, how to fix
> >>>> them.
> >>>> 
> >>>> 1) File /proc/i8k (exists only when kernel is compiled with
> >>>> CONFIG_I8K) exports DMI_PRODUCT_SERIAL and it can be read by
> >>>> ordinary user, without root permission. Normally
> >>>> DMI_PRODUCT_SERIAL can be read from sysfs file
> >>>> /sys/class/dmi/id/product_serial but only by root user.
> >>>> 
> >>>> 2) Via /proc/i8k ordinary user can set fan speed. This is
> >>>> because how "restricted" parameter and variable works. Setting
> >>>> fan speed by normal non-root user can be dangerous, e.g.
> >>>> malicious application under user "nobody" could take control of
> >>>> fans.
> >>>> 
> >>>> Do you have idea how to fix these problems? Just to note that
> >>>> /proc/i8k has stable kernel ABI and changing it will break all
> >>>> existing i8k* applications. But /proc/i8k is there only for old
> >>>> legacy laptops (year 2000).
> >>>> 
> >>>> There is module parameter "restricted" with default value false
> >>>> and description: "Allow fan control if SYS_ADMIN capability
> >>>> set".
> >>>> 
> >>>> Current code do:
> >>>> 	case I8K_SET_FAN:
> >>>> 		if (restricted && !capable(CAP_SYS_ADMIN))
> >>>> 		
> >>>> 			return -EPERM;
> >>>> 
> >>>> For me description is a bit ambiguous. What about setting
> >>>> "restricted" by default to true and updating description to
> >>>> something like this?
> >>>> 
> >>>> "Disallow fan control when SYS_ADMIN capability is not set
> >>>> (default: 1)"
> >>> 
> >>> Sure. I am sure that someone will complain (we learned just
> >>> recently that people still use the old commands, after all), but
> >>> then the old behavior can be restored by setting the flag to 0.
> >> 
> >> Either setting that flag to 0 or running that tool under root or
> >> with capability CAP_SYS_ADMIN.
> >> 
> >>> I would not use a double negative to describe it. Why not just
> >>> something like "Allow fan control only if SYS_ADMIN capability
> >>> set (default 1)" ?
> >> 
> >> I was thinking about that description too, but there is problem
> >> with meaning too...
> >> 
> >> 0 means fan control is allowed for any user
> >> 1 means fan control is allowed only for CAP_SYS_ADMIN
> >> 
> >> Description should be unambiguous for situation when flag is set
> >> to 0.
> > 
> > Sorry, I don't understand how a double negation "disallow ... if
> > not set" would make things less ambiguous than "allow ... only if
> > set".
> 
> Double negatives become ambiguous when you start to deal with the
> possibility of translation or working with people who are not native
> speakers of the language in question.  In English they're
> traditionally considered bad grammar, while in most other languages
> they are used for emphasis and nothing else, and thus are considered
> by some people to be bad form in technical documentation.
> 
> Given this particular case, it would probably be the least ambiguous
> to say: Restrict fan control to CAP_SYS_ADMIN

Thank you, this is really better!

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.