Simplify check_cmd_access() function

To avoid confusion between check_file_access() and check_cmd_access() in the future, remove unneeded arguments from check_cmd_access()

As a command will always be a file, it should always check for CHKACC_FILE and nothing else.  And as the commands always will need X_OK, check only for that.

One change from earlier behaviour is that R_OK is not checked for.  The reason is that only scripts require R_OK to work.  However, a system might be installed with binaries with only X_OK set.  If a script is missing R_OK, then the execution will fail due to lacking permissions.

Signed-off-by David Sommerseth <[email protected]...>

---
 src/openvpn/options.c |   48 ++++++++++++++++++++++++++----------------------

 1 files changed, 26 insertions(+), 22 deletions(-)

src/openvpn/options.c +0 -0

index 5da2eb6..7769625 100644
@@ -2690,7 +2690,7 @@ check_file_access(const int type, const char *file, const int mode, const char *
  * check_file_access() arguments.
  */

 static bool

src/openvpn/options.c +1 -1

-check_cmd_access(const int type, const char *command, const int mode, const char *opt)
+check_cmd_access(const char *command, const char *opt)

 {
  struct argv argv;
  bool return_code;

src/openvpn/options.c +5 -1

@@ -2705,7 +2705,11 @@ check_cmd_access(const int type, const char *command, const int mode, const char
 
   /* if an executable is specified then check it; otherwise, complain */
   if (argv.argv[0])
-      return_code = check_file_access(type, argv.argv[0], mode, opt);
+    /* Scripts requires R_OK as well, but that might fail on binaries which
+     * only requires X_OK to function on Unix - a scenario not unlikely to
+     * be seen on suid binaries.
+     */
+    return_code = check_file_access(CHKACC_FILE, argv.argv[0], X_OK, opt);

  else
    {
      msg (M_NOPREFIX|M_OPTERR, "%s fails with '%s': No path to executable.",

src/openvpn/options.c +20 -21

@@ -2797,26 +2801,26 @@ options_postprocess_filechecks (struct options *options)
 
   /* ** Script hooks that accept an optionally quoted and/or escaped executable path, ** */
   /* ** optionally followed by arguments ** */
-  errs |= check_cmd_access (CHKACC_FILE, options->auth_user_pass_verify_script,
-                            R_OK|X_OK, "--auth-user-pass-verify script");
-  errs |= check_cmd_access (CHKACC_FILE, options->client_connect_script,
-                            R_OK|X_OK, "--client-connect script");
-  errs |= check_cmd_access (CHKACC_FILE, options->client_disconnect_script,
-                            R_OK|X_OK, "--client-disconnect script");
-  errs |= check_cmd_access (CHKACC_FILE, options->tls_verify,
-                            R_OK|X_OK, "--tls-verify script");
-  errs |= check_cmd_access (CHKACC_FILE, options->up_script,
-                            R_OK|X_OK, "--up script");
-  errs |= check_cmd_access (CHKACC_FILE, options->down_script,
-                            R_OK|X_OK, "--down script");
-  errs |= check_cmd_access (CHKACC_FILE, options->ipchange,
-                            R_OK|X_OK, "--ipchange script");
-  errs |= check_cmd_access (CHKACC_FILE, options->route_script,
-                            R_OK|X_OK, "--route-up script");
-  errs |= check_cmd_access (CHKACC_FILE, options->route_predown_script,
-                            R_OK|X_OK, "--route-pre-down script");
-  errs |= check_cmd_access (CHKACC_FILE, options->learn_address_script,
-                            R_OK|X_OK, "--learn-address script");
+  errs |= check_cmd_access (options->auth_user_pass_verify_script,
+                            "--auth-user-pass-verify script");
+  errs |= check_cmd_access (options->client_connect_script,
+                            "--client-connect script");
+  errs |= check_cmd_access (options->client_disconnect_script,
+                            "--client-disconnect script");
+  errs |= check_cmd_access (options->tls_verify,
+                            "--tls-verify script");
+  errs |= check_cmd_access (options->up_script,
+                            "--up script");
+  errs |= check_cmd_access (options->down_script,
+                            "--down script");
+  errs |= check_cmd_access (options->ipchange,
+                            "--ipchange script");
+  errs |= check_cmd_access (options->route_script,
+                            "--route-up script");
+  errs |= check_cmd_access (options->route_predown_script,
+                            "--route-pre-down script");
+  errs |= check_cmd_access (options->learn_address_script,
+                            "--learn-address script");
  #endif /* P2MP_SERVER */
 
   if (errs)
--

1.7.4.4

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Looks good.
If you testing this one, please ensure symbolic links also works properly... :)

Looks good. If you testing this one, please ensure symbolic links also works properly... :)

Quoted 79 lines · David Sommerseth

To avoid confusion between check_file_access() and check_cmd_access() in the future, remove unneeded arguments from check_cmd_access()

As a command will always be a file, it should always check for CHKACC_FILE and nothing else. And as the commands always will need X_OK, check only for that.

One change from earlier behaviour is that R_OK is not checked for. The reason is that only scripts require R_OK to work. However, a system might be installed with binaries with only X_OK set. If a script is missing R_OK, then the execution will fail due to lacking permissions.

Signed-off-by David Sommerseth <[email protected]...> ---

src/openvpn/options.c | 48
++++++++++++++++++++++++++---------------------- 1 files changed, 26 insertions(+), 22 deletions(-)

src/openvpn/options.c index renamed from src/openvpn/options.c +0 -0

5da2eb6..7769625 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -2690,7 +2690,7 @@ check_file_access(const int type, const char *file, const int mode, const char * * check_file_access() arguments. */ static bool -check_cmd_access(const int type, const char *command, const int mode, const char *opt) +check_cmd_access(const char *command, const char *opt) { struct argv argv; bool return_code;

src/openvpn/options.c index renamed from src/openvpn/options.c +0 -0

@@ -2705,7 +2705,11 @@ check_cmd_access(const int type, const

char *command, const int mode, const char

/* if an executable is specified then check it; otherwise,
complain */ if (argv.argv[0]) - return_code =
check_file_access(type, argv.argv[0], mode, opt); + /*
Scripts requires R_OK as well, but that might fail on binaries which + * only requires X_OK to function on Unix - a scenario
not unlikely to + * be seen on suid binaries. + */ +
return_code = check_file_access(CHKACC_FILE, argv.argv[0], X_OK, opt); else { msg (M_NOPREFIX|M_OPTERR, "%s fails with '%s': No path to executable.", @@ -2797,26 +2801,26 @@ options_postprocess_filechecks (struct options *options)

/* ** Script hooks that accept an optionally quoted and/or escaped executable path, ** */ /* ** optionally followed by arguments ** */ - errs |= check_cmd_access (CHKACC_FILE, options->auth_user_pass_verify_script, - R_OK|X_OK, "--auth-user-pass-verify script"); - errs |= check_cmd_access (CHKACC_FILE, options->client_connect_script, - R_OK|X_OK, "--client-connect script"); - errs |= check_cmd_access (CHKACC_FILE, options->client_disconnect_script, - R_OK|X_OK, "--client-disconnect script"); - errs |= check_cmd_access (CHKACC_FILE, options->tls_verify, - R_OK|X_OK, "--tls-verify script"); - errs |= check_cmd_access (CHKACC_FILE, options->up_script, - R_OK|X_OK, "--up script"); - errs |= check_cmd_access (CHKACC_FILE, options->down_script, - R_OK|X_OK, "--down script"); - errs |= check_cmd_access (CHKACC_FILE, options->ipchange, - R_OK|X_OK, "--ipchange script"); - errs |= check_cmd_access (CHKACC_FILE, options->route_script, - R_OK|X_OK, "--route-up script"); - errs |= check_cmd_access (CHKACC_FILE, options->route_predown_script, - R_OK|X_OK, "--route-pre-down script"); - errs |= check_cmd_access (CHKACC_FILE, options->learn_address_script, - R_OK|X_OK, "--learn-address script"); + errs |= check_cmd_access (options->auth_user_pass_verify_script, + "--auth-user-pass-verify script"); + errs |= check_cmd_access (options->client_connect_script, + "--client-connect script"); + errs |= check_cmd_access (options->client_disconnect_script, + "--client-disconnect script"); + errs |= check_cmd_access
(options->tls_verify, + "--tls-verify
script"); + errs |= check_cmd_access (options->up_script, + "--up script"); + errs |= check_cmd_access
(options->down_script, + "--down
script"); + errs |= check_cmd_access (options->ipchange, + "--ipchange script"); + errs |= check_cmd_access
(options->route_script, + "--route-up
script"); + errs |= check_cmd_access (options->route_predown_script, + "--route-pre-down script"); + errs |= check_cmd_access (options->learn_address_script, + "--learn-address script"); #endif /* P2MP_SERVER */

Quoted 7 lines

if (errs) -- 1.7.4.4

------------------------------------------------------------------------------

Quoted 8 lines

Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats.
http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________ Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

------------------------------------------------------------------------------

Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats.
http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________ Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

To avoid confusion between check_file_access() and check_cmd_access() in the future, remove unneeded arguments from check_cmd_access()

As a command will always be a file, it should always check for CHKACC_FILE and nothing else. And as the commands always will need X_OK, check only for that.

One change from earlier behaviour is that R_OK is not checked for. The reason is that only scripts require R_OK to work. However, a system might be installed with binaries with only X_OK set. If a script is missing R_OK, then the execution will fail due to lacking permissions.

Signed-off-by David Sommerseth <[email protected]...> ---

src/openvpn/options.c | 48
++++++++++++++++++++++++++---------------------- 1 files changed, 26 insertions(+), 22 deletions(-)