Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

petitboot: openpower rules for usb devices #2790

Closed
wants to merge 1 commit into from
Closed

Conversation

debmc
Copy link

@debmc debmc commented Apr 15, 2019

USB Device Objectives:
Disable USB devices to preserve secure booting, protecting the security
vulnerability surface for openpower machines.

The Petitboot environment is limited in scope of utilities available for
instrumentation of a solution (due to space constraints in flash image).

Implement udev rule which disables the authorization of USB devices.

To build with this set BR2_PETITBOOT_RESTRICT_USB=y in defconfig.

Provide the ability for authorized access to enable USB devices
if so desired.

Implementation:
Udev rule which sets the authorized_default attribute of USB devices
to not allow USB devices to be able to connect to the kernel.

Operational Characteristics:
Upon boot no USB devices will be authorized to become functional by
the disablement of the USB device.

Once booted, if desired, an authorized user (one being able to su as root)
can perform actions which will either temporarily or persistently
enable USB devices.

Method 1 - Set petitboot,usb-override=1
Setting the petitboot,usb-override=1 will persist the desired override
and will enable USB devices from boot to boot (setting requires
root privileges).

Method 2 - Set file system authorized_default to either enable or
disable the device as desired.
(echo 1 > /sys/bus/usb/devices/usb1/authorized_default).

Special Considerations:
If access to the openpower machine is desired to allow physically attached
USB devices (such as keyboard/mouse), authorized users can access the
BMC's SOL Console and set one of the two methods described earlier for
manual overrides.

Signed-off-by: Deb McLemore [email protected]

@sammj
Copy link
Contributor

sammj commented Apr 23, 2019

Is this something we could put under an option like BR2_PACKAGE_PETITBOOT_RETRICT_USB? This probably isn't something we want on by default on all systems.

# we want to leverage ancestry so use ATTRS first
# query to see if the petitboot,usb-override=1 exists (grep -c == 1)
# RESULT holds the shell command output, not the shell exit code
ACTION=="add", SUBSYSTEM=="usb", ATTRS{authorized_default}=="*", PROGRAM="/bin/sh -c '/usr/sbin/nvram --print-config | /bin/grep -c petitboot,usb-override=1 || true'", RESULT=="0", ATTR{authorized_default}="0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a script in the filesystem that is called by the udev rule, instead of implementing it inline.

/usr/sbin/usb-authorisation

#!/bin/sh

VALUE=$(/usr/sbin/nvram --print-config=petitboot,usb-override)

if [ "$VALUE" = 1 ]; then
   exit 1
fi

exit 0

Copy link
Author

@debmc debmc May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer

If I understand the limitations correctly, I'm not sure that udev rules allow the suggestion you make, my understanding is that the exit code from the script/program executed from the udev rule must run to success (exit code=0). The script/program can do any short running procedures and set output on standard out, but that is the extent. Let me know if you have a different understanding or how-to (and how to get the exit code returned via udev interface).

Just a sample to show what I think udev would allow, but I don't think this buys anything useful in functionality.


VALUE=$(/usr/sbin/nvram --print-config=petitboot,usb-override)

# udev rule openpower    
if [ "$VALUE" = 1 ]; then
   echo "1"
else          
   echo "0"
fi
                                                                   
# exit code must be zero for udev handling to account as successful
exit 0


@debmc debmc force-pushed the usb branch 3 times, most recently from 15376ee to 6e204c7 Compare May 7, 2019 01:14
@debmc
Copy link
Author

debmc commented May 8, 2019

Retest this please

1 similar comment
@debmc
Copy link
Author

debmc commented May 8, 2019

Retest this please

@oohal
Copy link
Contributor

oohal commented May 9, 2019

I don't think it's a great idea to disable USB devices by default without having something to enable them in petitboot. It's not reasonable to expect an average system admin to know about random sysfs hacks or platform specific petitboot nvram options.

@sammj
Copy link
Contributor

sammj commented May 9, 2019

I think the udev rule is default-allow but udev rules are weird so I'm doing a build to test.
@debmc I just noticed your commit accidentally bumps the Buildroot submodule commit, we'll want to remove that from the commit before merging.

@sammj sammj self-requested a review May 9, 2019 04:02
Copy link
Contributor

@sammj sammj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the default is usb-disabled if my now-keyboardless VM is anything to go by :) We should make this default-allow.
We'll also need to remove that buildroot bump.

@debmc
Copy link
Author

debmc commented May 9, 2019

The objective -> open-power/skiboot#211

Depending on the security approach, secure out-of-the-box.

Not sure how the overall disablement of security is to be approached or if the objective is to secure the box and then allow customizations to disable (which requires an explicit act).

The ability to override this security feature is easily done by normal linux methods (setting authorized_default), so that is not anything unknown to the typical linux administrator concerned with security lock down.

@sammj
Copy link
Contributor

sammj commented May 10, 2019

NVRAM configuration option to disable certain PCI devices (e.g. USB controllers)

I would read this as default-allow. If we suddenly disable USB on every machine with no obvious announcement, I have no doubt we'll be snowed under with bug reports. I disagree with making it default-disable for the same reason that the petitboot user changes don't set a default password; "breaking" changes need to be extremely well communicated if they're going to be made the default and as it stands we don't have that yet.

@debmc
Copy link
Author

debmc commented May 11, 2019

I disagree

It seems that we are discussing the policy of should this allow USB devices
or should this disallow USB devices (the implementation can do either way).

The security policy of the OpenPOWER box seems like it would have a policy
of either favoring security or favoring performance (as I've seen it referenced
in hostboot related risk level features).

I believe the default risk policy is normally set to risk level 0 (which I believe
is security favored) so it seems that we would want to align with that policy.

I suppose the prevalence of USB device usage can also be considered, I'm not sure
if its common practice that disallowing USB devices on an OpenPOWER box would be
disruptive. I would think most security officers would encourage default practice
to disable USB devices and that only a system administrators with root authority
can bring down the policy to allow temporary access and that we would want to then
not persist allowing USB devices (if the industry dictates).

So, maybe its more of a higher level discussion.

I would also think strategically it would be better to disallow USB devices as a
policy to start with, once a policy is in place in the field it is more challenging
to change the policy.

The BR2_PETITBOOT_RESTRICT_USB defconfig option is the trigger to have this policy
become built and subsequently active. It may be more of a question of where we
change the defconfig's to set this security policy to disable USB devices.

@sammj
Copy link
Contributor

sammj commented May 13, 2019

The BR2_PETITBOOT_RESTRICT_USB defconfig option

I must have been having a proper Friday because I just realised this PR doesn't enable this yet! In that case this is probably fine so that the rules are available.

That said I'll probably have some similar concerns about this when a PR comes around to put the option in defconfigs, depending on the machine. We can take that conversation up later though.
Also, I have no idea what this means:

I believe the default risk policy is normally set to risk level 0

Is there some policy documentation you can point me to that I can check that explains this?

@debmc
Copy link
Author

debmc commented May 13, 2019

documentation

Only minimal found:

hostboot/src/usr/secureboot/smf/smf_utils.C

    // SMF is enabled by default on Axone, so need to check the risk level
    // only on P9C/P9N.
    // WARNING: If more risk levels are added in the future that don't
    // support SMF, the below check needs to be altered accordingly.


hostboot/src/include/usr/targeting/targplatutil.H

    // WARNING: addition of risk levels that don't support SMF will have a
    // significant effect on the behavior of SMF code. Please ensure that
    // (at least) src/usr/secureboot/smf/smf_utils.C is updated
    // accordingly!
    typedef enum
    {
        P9N22_P9C12_RUGBY_FAVOR_SECURITY               = 0x00,
        P9N22_P9C12_RUGBY_FAVOR_PERFORMANCE            = 0x01,
        P9N22_NO_RUGBY_MITIGATIONS                     = 0x02,
        P9N22_P9N23_JAVA_PERF                          = 0x03,
        P9N23_P9C13_NATIVE_SMF_RUGBY_FAVOR_SECURITY    = 0x04,
        P9N23_P9C13_NATIVE_SMF_RUGBY_FAVOR_PERFORMANCE = 0x05,
    } Risk_level;

hostboot/src/usr/hdat/hdatiplparms.H

// Agreed strings and settings of different feature flags
// based on risk level and DD levels.

Copy link
Contributor

@sammj sammj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly forgot - @debmc you'll still need to remove that change to buildroot and push again.

@debmc
Copy link
Author

debmc commented May 14, 2019

remove that change

Can you elaborate on this ? I saw the thing, not sure what I did nor what I need to do to fix it . . .

@sammj
Copy link
Contributor

sammj commented May 14, 2019

Looks like your buildroot submodule was out of sync with your op-build and you've probably done a git commit -a ... and added that as a change. You should be able to remove it with

git reset -p HEAD^ -- buildroot
git commit --amend

@debmc
Copy link
Author

debmc commented May 14, 2019

buildroot submodule was out of sync

I've tried a few variations and it still exists. Any further tips ?

Apply this hunk to index [y,n,q,a,d,/,e,?]?

I think I got it, let me know if it is still a problem, I had to edit and then remove each of the lines in the hunk, it was not intuitive (at least for me), after I updated the submodule -> git submodule update --init --recursive

USB Device Objectives:
Disable USB devices to preserve secure booting, protecting the security
vulnerability surface for openpower machines.

The Petitboot environment is limited in scope of utilities available for
instrumentation of a solution (due to space constraints in flash image).

Implement udev rule which disables the authorization of USB devices.

To build with this set BR2_PETITBOOT_RESTRICT_USB=y in defconfig.

Provide the ability for authorized access to enable USB devices
if so desired.

Implementation:
Udev rule which sets the authorized_default attribute of USB devices
to not allow USB devices to be able to connect to the kernel.

Operational Characteristics:
Upon boot no USB devices will be authorized to become functional by
the disablement of the USB device.

Once booted, if desired, an authorized user (one being able to su as root)
can perform actions which will either temporarily or persistently
enable USB devices.

Method 1 - Set petitboot,usb-override=1
Setting the petitboot,usb-override=1 will persist the desired override
and will enable USB devices from boot to boot (setting requires
root privileges).

Method 2 - Set file system authorized_default to either enable or
disable the device as desired.
(echo 1 > /sys/bus/usb/devices/usb1/authorized_default).

Special Considerations:
If access to the openpower machine is desired to allow physically attached
USB devices (such as keyboard/mouse), authorized users can access the
BMC's SOL Console and set one of the two methods described earlier for
manual overrides.

Signed-off-by: Deb McLemore <[email protected]>
@sammj
Copy link
Contributor

sammj commented May 14, 2019

Yep, the latest version doesn't touch the buildroot submodule

@klauskiwi
Copy link

Apologies in advance for raising the dead here (I've taken the day to do that today)..

After some internal discussions, it appears that the act of disabling Petitboot access to USB devices is not sufficient from a Security point-of-view. We apparently need a way to completely disable USB for the system if the admin policy determines it as so.

So I think a udev rule is probably not sufficient here, we'd need something restricting Host OS access to it as well. @oohal mentioned that this could be made possible, I'm not sure how actually. Oliver, perhaps you could clarify?

@klauskiwi klauskiwi closed this Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants