#195 Improve the security of opcontrol

closed-fixed
William Cohen
None
5
2011-08-12
2011-05-17
William Cohen
No

There are a number of things to improve security in the opcontrol script:
-Do not allow blindly use eval on unverified arguments
-Avoid using "." to source variable from /root/.oprofile/daemonrc
-Limit "--save-session" to the samples directory

Discussion

1 2 > >> (Page 1 of 2)
  • Will, were you going to attach patches? Maybe got distracted and forgot? :-)

     
  • Reviewed 0001-Avoid-blindly-writing-to-SESSION_DIR-opd_pipe.patch. Patch looks good, so I committed it.

     
  • Reviewed 0002-Ensure-that-save-only-saves-things-in-SESSION_DIR.patch. Looks fine, so it's committed now.

     
  • Reviewing 0001-Do-additional-checks-on-user-supplied-arguments.patch . . .

    diff --git a/utils/opcontrol b/utils/opcontrol
    index b99757a..232052e 100644
    --- a/utils/opcontrol
    +++ b/utils/opcontrol
    @@ -68,7 +68,8 @@ guess_number_base()
    # check value is a valid number
    error_if_not_number()
    {
    - guess_number_base $2
    + error_if_empty "$1" "$2"
    + guess_number_base "$2"
    if test "$?" -eq 0 ; then
    echo "Argument for $1, $2, is not a valid number." >&2
    exit 1
    @@ -85,6 +86,16 @@ error_if_not_basename()
    fi
    }

    +error_if_invalid_arg()
    +{
    + error_if_empty "$1" "$2"
    + arg_re='^[0-9a-zA-Z_:\/,\.\-]*$'
    + if [[ ! "$2" =~ $arg_re ]] ; then

    Is the above compatible with busybox?

    + echo "Argument for $1, $2, is not valid argument." >&2
    + exit 1
    + fi
    +}
    +

     
  • Paul D. Smith
    Paul D. Smith
    2011-05-23

    The =~ match operator is a bash-ism and isn't available in any other shell that I'm aware of: certainly it's not available in basic POSIX shells like ash/hush/dash (busybox, ubuntu). FWIW, you can get the behavior you want in the patch below using a POSIX case statement and avoiding bash-specific match operators:

    case $2 in
    *[!-0-9a-zA-Z_:V,.]*) echo "Argument for $1, $2, is not valid argument." >&2; exit 1;;
    esac

    (not tested 100%) That basically matches any word which has at least one character that is not a member of that class.

     
  • Paul D. Smith
    Paul D. Smith
    2011-05-23

    Looks like a number of the other patches also make use of =~. They look trivial to convert to case/esac statements.

    The [[ ... ]] test operators are also not POSIX, IIRC. Please stick to using the standard [ ... ] versions. usually it doesn't make a difference.

     
  • William Cohen
    William Cohen
    2011-05-23

    The patches were tested with "busybox sh" on busybox 1.15.1 and that version of busybox appears to understand "[[ foo =~ bar ]]". So this code definitely fails to work on hush/dash shells?

     
  • Paul D. Smith
    Paul D. Smith
    2011-05-23

    dash doesn't support [[ ... ]]:

    $ dash
    $ if [[ /tmp =~ /tmp ]]; then echo oooh; fi
    dash: [[: not found

    And what busybox sh supports depends entirely on (a) whether your busybox builds ash or hush, and (b) what optional features you enable in the busybox config. For example on my embedded systems busybox sh (ash) groks [[ ... ]] but not =~:

    BusyBox v1.18.3 (2011-03-15 10:26:16 CDT) built-in shell (ash)
    Enter 'help' for a list of built-in commands.
    # if [[ /tmp =~ /tmp ]]; then echo oooh; fi
    [[: =~: unknown operand

    This doesn't even begin to address the horror that is, for example, the Solaris /bin/sh but hopefully we can be pretty confident that oprofile doesn't need to be portable to Solaris :-).

    Your best bet is to adhere to the standard by reading the text about things you're not sure of, and test with dash. I'm not aware of too many things that dash supports that are not supported by all other POSIX/Bourne-derived shells. I think redhat has a dash package available?

    Cheers!

     
1 2 > >> (Page 1 of 2)