From: William C. <wc...@re...> - 2011-06-02 14:31:44
|
Hi Maynard, Thanks for reviewing I have revised the patches in PR3303383 "Improve the security of opcontrol". I have revised: 0002-Do-additional-checks-on-user-supplied-arguments.patch There is a test to make sure there argument is non-empty for error_if_invalid_arg. Revised the code for --save to give a more understandable error messages. If the patch is okay I will commit it. -Will |
From: SourceForge.net <no...@so...> - 2011-05-17 15:24:23
|
Bugs item #3303383, was opened at 2011-05-17 11:24 Message generated for change (Tracker Item Submitted) 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 ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=116191&aid=3303383&group_id=16191 |
From: SourceForge.net <no...@so...> - 2011-05-18 13:17:52
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Comment added) made by maynardj 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: Maynard Johnson (maynardj) Date: 2011-05-18 08: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 |
From: SourceForge.net <no...@so...> - 2011-05-23 19:28:21
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Comment added) made by maynardj 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: Maynard Johnson (maynardj) Date: 2011-05-23 14: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 08: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 |
From: SourceForge.net <no...@so...> - 2011-05-23 20:08:59
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Comment added) made by maynardj 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: Maynard Johnson (maynardj) Date: 2011-05-23 15: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 14: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 08: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 |
From: SourceForge.net <no...@so...> - 2011-05-23 20:40:31
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Comment added) made by maynardj 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: Maynard Johnson (maynardj) Date: 2011-05-23 15: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 15: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 14: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 08: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 |
From: SourceForge.net <no...@so...> - 2011-05-23 21:00:48
|
Bugs item #3303383, was opened at 2011-05-17 11:24 Message generated for change (Comment added) made by psmith 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: 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 |
From: SourceForge.net <no...@so...> - 2011-05-23 21:05:13
|
Bugs item #3303383, was opened at 2011-05-17 11:24 Message generated for change (Comment added) made by psmith 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: 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 |
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 |
From: SourceForge.net <no...@so...> - 2011-05-23 22:00:09
|
Bugs item #3303383, was opened at 2011-05-17 11:24 Message generated for change (Comment added) made by psmith 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: Paul D. Smith (psmith) Date: 2011-05-23 18:00 Message: 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! ---------------------------------------------------------------------- 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 |
From: SourceForge.net <no...@so...> - 2011-05-31 20:13:21
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Comment added) made by maynardj 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: Maynard Johnson (maynardj) Date: 2011-05-31 15:13 Message: I reviewed 0001-Avoid-blindly-source-SETUP_FILE-with-.-PR3303383.patch and it looks fine. Will, you can go ahead and commit it, but could you please add comments for each case statement to indicate the meaning of the glob expressions (for easier maintenance). Thanks. ---------------------------------------------------------------------- Comment By: Paul D. Smith (psmith) Date: 2011-05-23 17:00 Message: 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! ---------------------------------------------------------------------- Comment By: William Cohen (wcohen) Date: 2011-05-23 16: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 16: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 16: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 15: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 15: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 14: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 08: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 |
From: SourceForge.net <no...@so...> - 2011-05-31 20:33:21
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Comment added) made by maynardj 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: Maynard Johnson (maynardj) Date: 2011-05-31 15:33 Message: Will, I forgot to mention . . . please indent the each case statement as is the style for other case statements in opcontrol. Thanks. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:13 Message: I reviewed 0001-Avoid-blindly-source-SETUP_FILE-with-.-PR3303383.patch and it looks fine. Will, you can go ahead and commit it, but could you please add comments for each case statement to indicate the meaning of the glob expressions (for easier maintenance). Thanks. ---------------------------------------------------------------------- Comment By: Paul D. Smith (psmith) Date: 2011-05-23 17:00 Message: 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! ---------------------------------------------------------------------- Comment By: William Cohen (wcohen) Date: 2011-05-23 16: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 16: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 16: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 15: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 15: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 14: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 08: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 |
From: SourceForge.net <no...@so...> - 2011-05-31 20:39:52
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Comment added) made by maynardj 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: Maynard Johnson (maynardj) Date: 2011-05-31 15:39 Message: Comments for 0002-Do-additional-checks-on-user-supplied-arguments.patch: - For the --save option, the error_if_not_basename function is called. If the test fails, the error message printed is not helpful at all. Since this error_if_not_basename function is only used by the --save option (at this time), perhaps it would be best to integrate into the --save case statement and print a more meaningful message. - There are a couple places where you have calls to error_if_invalid_arg, but have removed the call to error_if_empty. We still should fail if there is no value argument passed. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:33 Message: Will, I forgot to mention . . . please indent the each case statement as is the style for other case statements in opcontrol. Thanks. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:13 Message: I reviewed 0001-Avoid-blindly-source-SETUP_FILE-with-.-PR3303383.patch and it looks fine. Will, you can go ahead and commit it, but could you please add comments for each case statement to indicate the meaning of the glob expressions (for easier maintenance). Thanks. ---------------------------------------------------------------------- Comment By: Paul D. Smith (psmith) Date: 2011-05-23 17:00 Message: 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! ---------------------------------------------------------------------- Comment By: William Cohen (wcohen) Date: 2011-05-23 16: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 16: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 16: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 15: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 15: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 14: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 08: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 |
From: SourceForge.net <no...@so...> - 2011-05-31 20:40:33
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Comment added) made by maynardj 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: Maynard Johnson (maynardj) Date: 2011-05-31 15:40 Message: Will, 0003-Avoid-using-in-error_if_not_basename-to-improve-posi.patch looks fine. Please go ahead and commit it. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:39 Message: Comments for 0002-Do-additional-checks-on-user-supplied-arguments.patch: - For the --save option, the error_if_not_basename function is called. If the test fails, the error message printed is not helpful at all. Since this error_if_not_basename function is only used by the --save option (at this time), perhaps it would be best to integrate into the --save case statement and print a more meaningful message. - There are a couple places where you have calls to error_if_invalid_arg, but have removed the call to error_if_empty. We still should fail if there is no value argument passed. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:33 Message: Will, I forgot to mention . . . please indent the each case statement as is the style for other case statements in opcontrol. Thanks. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:13 Message: I reviewed 0001-Avoid-blindly-source-SETUP_FILE-with-.-PR3303383.patch and it looks fine. Will, you can go ahead and commit it, but could you please add comments for each case statement to indicate the meaning of the glob expressions (for easier maintenance). Thanks. ---------------------------------------------------------------------- Comment By: Paul D. Smith (psmith) Date: 2011-05-23 17:00 Message: 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! ---------------------------------------------------------------------- Comment By: William Cohen (wcohen) Date: 2011-05-23 16: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 16: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 16: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 15: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 15: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 14: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 08: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 |
From: Maynard J. <may...@us...> - 2011-06-02 17:06:21
|
William Cohen wrote: > Hi Maynard, > > Thanks for reviewing I have revised the patches in PR3303383 "Improve the security of opcontrol". I have revised: > > 0002-Do-additional-checks-on-user-supplied-arguments.patch > > There is a test to make sure there argument is non-empty for error_if_invalid_arg. > Revised the code for --save to give a more understandable error messages. > > If the patch is okay I will commit it. Looks fine. Go ahead and commit it. Thanks! -Maynard > > -Will |
From: SourceForge.net <no...@so...> - 2011-06-03 05:06:15
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Settings changed) made by ssuthiku 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: Fixed 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: Maynard Johnson (maynardj) Date: 2011-05-31 15:40 Message: Will, 0003-Avoid-using-in-error_if_not_basename-to-improve-posi.patch looks fine. Please go ahead and commit it. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:39 Message: Comments for 0002-Do-additional-checks-on-user-supplied-arguments.patch: - For the --save option, the error_if_not_basename function is called. If the test fails, the error message printed is not helpful at all. Since this error_if_not_basename function is only used by the --save option (at this time), perhaps it would be best to integrate into the --save case statement and print a more meaningful message. - There are a couple places where you have calls to error_if_invalid_arg, but have removed the call to error_if_empty. We still should fail if there is no value argument passed. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:33 Message: Will, I forgot to mention . . . please indent the each case statement as is the style for other case statements in opcontrol. Thanks. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:13 Message: I reviewed 0001-Avoid-blindly-source-SETUP_FILE-with-.-PR3303383.patch and it looks fine. Will, you can go ahead and commit it, but could you please add comments for each case statement to indicate the meaning of the glob expressions (for easier maintenance). Thanks. ---------------------------------------------------------------------- Comment By: Paul D. Smith (psmith) Date: 2011-05-23 17:00 Message: 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! ---------------------------------------------------------------------- Comment By: William Cohen (wcohen) Date: 2011-05-23 16: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 16: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 16: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 15: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 15: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 14: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 08: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 |
From: SourceForge.net <no...@so...> - 2011-08-12 14:05:34
|
Bugs item #3303383, was opened at 2011-05-17 10:24 Message generated for change (Settings changed) made by ssuthiku 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: Closed Resolution: Fixed 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: Maynard Johnson (maynardj) Date: 2011-05-31 15:40 Message: Will, 0003-Avoid-using-in-error_if_not_basename-to-improve-posi.patch looks fine. Please go ahead and commit it. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:39 Message: Comments for 0002-Do-additional-checks-on-user-supplied-arguments.patch: - For the --save option, the error_if_not_basename function is called. If the test fails, the error message printed is not helpful at all. Since this error_if_not_basename function is only used by the --save option (at this time), perhaps it would be best to integrate into the --save case statement and print a more meaningful message. - There are a couple places where you have calls to error_if_invalid_arg, but have removed the call to error_if_empty. We still should fail if there is no value argument passed. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:33 Message: Will, I forgot to mention . . . please indent the each case statement as is the style for other case statements in opcontrol. Thanks. ---------------------------------------------------------------------- Comment By: Maynard Johnson (maynardj) Date: 2011-05-31 15:13 Message: I reviewed 0001-Avoid-blindly-source-SETUP_FILE-with-.-PR3303383.patch and it looks fine. Will, you can go ahead and commit it, but could you please add comments for each case statement to indicate the meaning of the glob expressions (for easier maintenance). Thanks. ---------------------------------------------------------------------- Comment By: Paul D. Smith (psmith) Date: 2011-05-23 17:00 Message: 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! ---------------------------------------------------------------------- Comment By: William Cohen (wcohen) Date: 2011-05-23 16: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 16: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 16: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 15: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 15: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 14: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 08: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 |