From: SourceForge.net <no...@so...> - 2011-05-23 21:44:21
|
Bugs item #3303383, was opened at 2011-05-17 11:24 Message generated for change (Comment added) made by wcohen You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=116191&aid=3303383&group_id=16191 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: William Cohen (wcohen) Assigned to: William Cohen (wcohen) Summary: Improve the security of opcontrol Initial Comment: 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 ---------------------------------------------------------------------- >Comment By: William Cohen (wcohen) Date: 2011-05-23 17:44 Message: 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? ---------------------------------------------------------------------- Comment By: Paul D. Smith (psmith) Date: 2011-05-23 17:05 Message: 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. ---------------------------------------------------------------------- Comment By: Paul D. Smith (psmith) Date: 2011-05-23 17:00 Message: 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. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-23 16:40 Message: 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 +} + ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-23 16:08 Message: Reviewed 0002-Ensure-that-save-only-saves-things-in-SESSION_DIR.patch. Looks fine, so it's committed now. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-23 15:28 Message: Reviewed 0001-Avoid-blindly-writing-to-SESSION_DIR-opd_pipe.patch. Patch looks good, so I committed it. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-18 09:17 Message: Will, were you going to attach patches? Maybe got distracted and forgot? :-) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=116191&aid=3303383&group_id=16191 |