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

oscap-ssh: simplify to allow limited sudo rule #1881

Open
wants to merge 8 commits into
base: maint-1.3
Choose a base branch
from

Conversation

maage
Copy link
Contributor

@maage maage commented Aug 21, 2022

With this patch set I can use very limited sudo rule when running oscap xccdf eval.

Also address some shellcheck warnings.

Without this to try to limit how sudo needs to be configured, sudoers rule needs to have /usr/bin/sh permission, exec permission (sh -c), and run chown without known path. This is too broad.

chown is not needed as umask 022 + rm -rf dir is enough to remove files as directory is user owned.

My example rule:

  • Note: in Fedora binary is named oscap.
  • Only allow xccdf eval under sudo, example user is named oscap-eval-user.
  • Limit what arguments eval are accepted, tune for your solution
  • Sudo currently must run oscap as unconfined_t. This is default, but rule just ensures it works even if SELinux user (staff_u) are used.
  • Relax requiretty only for this user, so there is no issues, but not expose other users.
  • With NOEXEC, ensures oscap is not able to run external commands, I guess this needs to be removed if SCE is used.
Cmnd_Alias CMND_OSCAP = \
	/usr/bin/oscap xccdf eval *, \
	! /usr/bin/oscap * --fetch-remote-resources *, \
	! /usr/bin/oscap * --remediate *, \
	! /usr/bin/oscap * --skip-signature-validation *, \
	! /usr/bin/oscap * --skip-valid *, \
	! /usr/bin/oscap * --skip-validation *
Defaults:oscap-eval-user !requiretty
oscap-eval-user ALL = ( root ) ROLE=unconfined_r NOEXEC: NOPASSWD: CMND_OSCAP

@maage maage mentioned this pull request Sep 4, 2022
maage added 8 commits May 4, 2023 16:49
Handle options conversions up to 2nd last arg as last is input and is
handled next.

(( ..., 1 )) ensures return value is ok.
arr[-1] is last element

From shellcheck:

In utils/oscap-ssh line 217:
for i in $(seq 0 `expr $# - 1`); do
                 ^-----------^ SC2046: Quote this to prevent word splitting.
                 ^-----------^ SC2006: Use $(...) notation instead of legacy backticked `...`.
                  ^--^ SC2003: expr is antiquated. Consider rewriting this using $((..)), ${} or [[ ]].

Did you mean:
for i in $(seq 0 $(expr $# - 1)); do

In utils/oscap-ssh line 218:
    let j=i+1
    ^-------^ SC2219: Instead of 'let expr', prefer (( expr )) .

In utils/oscap-ssh line 267:
    LOCAL_CONTENT_PATH="${oscap_args[`expr $# - 1`]}"
                                     ^-----------^ SC2006: Use $(...) notation instead of legacy backticked `...`.
                                      ^--^ SC2003: expr is antiquated. Consider rewriting this using $((..)), ${} or [[ ]].

Did you mean:
    LOCAL_CONTENT_PATH="${oscap_args[$(expr $# - 1)]}"

In utils/oscap-ssh line 268:
    oscap_args[`expr $# - 1`]="$REMOTE_TEMP_DIR/input.xml"
               ^-----------^ SC2006: Use $(...) notation instead of legacy backticked `...`.
                ^--^ SC2003: expr is antiquated. Consider rewriting this using $((..)), ${} or [[ ]].

Did you mean:
    oscap_args[$(expr $# - 1)]="$REMOTE_TEMP_DIR/input.xml"
- use printf %q instead of home made implementation
- use $@
  - there is no point using fancy array arrayref and eval in this simple
    use case
  - printf just iterates parameters and "$@" works just fine
- changes usage:
  from: command_array_to_string arref
  to: command_array_to_string "${array[@]}"
Change OSCAP_SUDO as array and after this there is no need to test it.
Fail if can not cd into a directory. Shellcheck would warn about this.
This is needed sometimes when debugging.
@maage
Copy link
Contributor Author

maage commented May 5, 2023

Two test enva failed at Start 199: probes/fwupdsecattr/test_probes_fwupdsecattr_mock.sh. I don't see connection with this patchset and that tests.

@evgenyz
Copy link
Contributor

evgenyz commented Feb 9, 2024

Hey, @maage. Can you please rebase the PR?

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.

2 participants