[Pkg-privacy-commits] [libgsecuredelete] 43/168: Use absolute paths rather than searching the PATH for sub-commands.
Ulrike Uhlig
u-guest at moszumanska.debian.org
Thu Jul 7 20:06:36 UTC 2016
This is an automated email from the git hooks/post-receive script.
u-guest pushed a commit to branch master
in repository libgsecuredelete.
commit 7ff6a7fa2bf6cf739af485e7e9dbcf6e605cbf76
Author: Colomban Wendling <ban at herbesfolles.org>
Date: Fri Dec 18 00:18:51 2009 +0100
Use absolute paths rather than searching the PATH for sub-commands.
Fix the security hole of using the search path to find the commands.
Now an absolute path is used, that defaults to a configurable build-time
search, but that may be changed explicitly by the caller by setting the
AsyncOperation::path property.
---
TODO | 21 ------------------
configure.ac | 36 ++++++++++++++++++++++++++++++
gsecuredelete/Makefile.am | 4 +++-
gsecuredelete/async-operation.vala | 37 +++++++++++++++++++++++++++++--
gsecuredelete/config.vapi | 12 ++++++++++
gsecuredelete/delete-operation.vala | 6 ++++-
gsecuredelete/fill-operation.vala | 6 ++++-
gsecuredelete/mem-operation.vala | 6 ++++-
gsecuredelete/securedelete-operation.vala | 9 ++------
gsecuredelete/swap-operation.vala | 6 ++++-
10 files changed, 108 insertions(+), 35 deletions(-)
diff --git a/TODO b/TODO
index 8da478d..b9d16ca 100644
--- a/TODO
+++ b/TODO
@@ -10,27 +10,6 @@ See result of `rgrep FIXME gsecuredelete` and `rgrep TODO gsecuredelete` too.
FIXMEs:
-* Do NOT use search path for launching commands
- (gsecuredelete/securedelete-operation.vala:24).
- This is a SECURITY VULNERABILITY if the PATH (or the system default
- search path, or whatever used to find a command) used by a program with
- great rights using this library was tuned by a malicious user (or if the
- owner of the great rights is stupid, but for this we can't do anything).
- The vulnerability occurs if a user may modify the PATH used to search
- for the spawned commands of another user (and maybe root): this malicious
- user may then create its own command that gets launched in place of the
- expected one, with the launcher's rights.
- Of course, it should not happens in a perfect environment with prefect
- users, but this library is not only meant for Heaven.
- .
- An easy fix for this is to hardcode the command's path to something like
- /bin/<command> or another highly standard path, but this is not flexible.
- A better fix would be to use a user-specified command (and prehaps fall
- back to a hardcoded default if none found) through a configuration file.
- Even more better would be to have a system-wide configuration file
- (something like a configuration file in /etc or so) and per-user ones,
- allowing each user to use its own tools if needed/wanted.
-
+ Get the pass number reported by the program rather than guessing it.
(gsecuredelete/securedelete-operation.vala:88).
Not sure it is useful or really cleanly doable.
diff --git a/configure.ac b/configure.ac
index f5a0552..2b15dd3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -15,6 +15,42 @@ m4_ifdef([AM_SILENT_RULES],[AM_SILENT_RULES([yes])])
AM_PROG_VALAC
AC_PROG_LIBTOOL
+
+# AX_PATH_PROG(VAR, prog, [default])
+# --------------------------------
+# Checks for a program like AC_PATH_PROG but supports overwriting with a
+# --with option. With argument yes (or check, te default), search for the
+# program with AC_PATH_PROG; with argument no (or default), use the default
+# value, and with any other value, use this value as the prog's path.
+AC_DEFUN([AX_PATH_PROG],
+ [AC_ARG_WITH([$2],
+ [AS_HELP_STRING([--with-$2=PATH],
+ [Specify the path to the $2 program])],
+ [],
+ [with_$2=check])
+ case "$with_$2" in
+ "check"|"yes") AC_PATH_PROG([$1], [$2], [$3]);;
+ *)
+ AC_MSG_CHECKING([for $2])
+ case "$with_$2" in
+ "no"|"default") $1="$3";;
+ *) $1="$with_$2";;
+ esac
+ AC_MSG_RESULT([$1])
+ ;;
+ esac])
+
+AX_PATH_PROG([SRM], [srm], [/usr/bin/srm])
+AX_PATH_PROG([SFILL], [sfill], [/usr/bin/sfill])
+AX_PATH_PROG([SMEM], [smem], [/usr/bin/smem])
+AX_PATH_PROG([SSWAP], [sswap], [/usr/bin/sswap])
+
+AC_DEFINE_UNQUOTED([SRM_PATH], ["$SRM"], [Path to the srm binary])
+AC_DEFINE_UNQUOTED([SFILL_PATH], ["$SFILL"], [Path to the sfill binary])
+AC_DEFINE_UNQUOTED([SMEM_PATH], ["$SMEM"], [Path to the smem binary])
+AC_DEFINE_UNQUOTED([SSWAP_PATH], ["$SSWAP"], [Path to the sswap binary])
+
+
# Checks for libraries.
VALA_PACKAGES="glib-2.0 posix"
AC_SUBST([VALA_PACKAGES])
diff --git a/gsecuredelete/Makefile.am b/gsecuredelete/Makefile.am
index 8e8c2f4..484254e 100644
--- a/gsecuredelete/Makefile.am
+++ b/gsecuredelete/Makefile.am
@@ -7,7 +7,9 @@ AM_LDFLAGS = $(LIBGSECUREDELETE_LIBS)
libgsecuredelete_la_includedir=$(includedir)/gsecuredelete
-libgsecuredelete_la_VALAFLAGS = $(AM_VALAFLAGS) --library=gsecuredelete -H gsecuredelete.h
+libgsecuredelete_la_VALAFLAGS = $(AM_VALAFLAGS) \
+ --library=gsecuredelete -H gsecuredelete.h \
+ --vapidir=. --pkg=config
libgsecuredelete_la_SOURCES = async-operation.vala \
securedelete-operation.vala \
zeroable-operation.vala \
diff --git a/gsecuredelete/async-operation.vala b/gsecuredelete/async-operation.vala
index 5f3b5d5..f724b0e 100644
--- a/gsecuredelete/async-operation.vala
+++ b/gsecuredelete/async-operation.vala
@@ -48,7 +48,11 @@ namespace Gsd
* as less efforts as possible.
*
* To subclass this class, the only thing you need to implement is the
- * argument builder, that gives the command to be spawned, and its arguments.
+ * argument builder, that gives the arguments of the spawned command; but you
+ * need to set the AsyncOperation::path property to the command to spawn in
+ * your constructor too (hence it is not required, it is slightly better to
+ * set it since it must be set before calling run() or run_sync() - or getting
+ * it).
* This said, you may want to override get_max_progress() and get_progress()
* to add actual progress support; build_environ() to provide a specific
* environment to your command.
@@ -119,6 +123,8 @@ namespace Gsd
protected int fd_out;
protected int fd_err;
private bool _busy = false;
+ private string _path;
+ private string _path_default = null;
/* signals */
/** AsyncOperation::finished:
@@ -137,6 +143,32 @@ namespace Gsd
public signal void progress (double fraction);
/* properties */
+ /** path:
+ * The path to the command to spawn.
+ * Setting it to %null resets it to its default value.
+ */
+ public string path {
+ get {
+ if (this._path == null) {
+ critical ("Property AsyncOperation::path not set");
+ }
+ return this._path;
+ }
+ /* We fake the default value overriding by getting the first set as it.
+ * Then, subclasses should set this property to their default value at
+ * construction time. */
+ set {
+ if (value != null) {
+ if (this._path_default == null) {
+ this._path_default = value;
+ }
+ this._path = value;
+ } else {
+ this._path = this._path_default;
+ }
+ }
+ default = null;
+ }
/** AsyncOperation: busy:
* Whether the operation object is busy. A busy operation cannot be reused
* until it gets ready again. An operation is busy when it is currently
@@ -161,8 +193,9 @@ namespace Gsd
size_t i;
args = this.build_args ();
- array_args = new string[args.length () + 1];
+ array_args = new string[args.length () + 2];
i = 0;
+ array_args[i++] = this.path;
foreach (unowned string arg in args) {
array_args[i++] = arg;
}
diff --git a/gsecuredelete/config.vapi b/gsecuredelete/config.vapi
new file mode 100644
index 0000000..36a0348
--- /dev/null
+++ b/gsecuredelete/config.vapi
@@ -0,0 +1,12 @@
+
+[CCode (cprefix = "", lower_case_cprefix = "", cheader_filename = "config.h")]
+namespace Config
+{
+ public const string PACKAGE;
+ public const string VERSION;
+ /* program paths */
+ public const string SRM_PATH;
+ public const string SFILL_PATH;
+ public const string SMEM_PATH;
+ public const string SSWAP_PATH;
+}
diff --git a/gsecuredelete/delete-operation.vala b/gsecuredelete/delete-operation.vala
index 419efc7..0f414f7 100644
--- a/gsecuredelete/delete-operation.vala
+++ b/gsecuredelete/delete-operation.vala
@@ -38,6 +38,11 @@ namespace Gsd
}
}
+ /* constructor */
+ construct {
+ this.path = Config.SRM_PATH;
+ }
+
/** add_path:
* @path: a path to securely remove.
*
@@ -82,7 +87,6 @@ namespace Gsd
{
List<string> args = null;
- args.append ("srm");
args.append ("-rv");
this.add_mode_argument (ref args);
this.add_zeroise_argument (ref args);
diff --git a/gsecuredelete/fill-operation.vala b/gsecuredelete/fill-operation.vala
index d0ba21c..3a71fc4 100644
--- a/gsecuredelete/fill-operation.vala
+++ b/gsecuredelete/fill-operation.vala
@@ -59,6 +59,11 @@ namespace Gsd
default = null;
}
+ /* constructor */
+ construct {
+ this.path = Config.SFILL_PATH;
+ }
+
/* returns the argument needed for a wipe mode */
private unowned string? get_argument_for_wipe_mode (WipeMode wipe_mode)
{
@@ -90,7 +95,6 @@ namespace Gsd
throw new AsyncOperationError.ARGS_ERROR ("Missing directory to fill");
}
- args.append ("sfill");
args.append ("-v");
this.add_mode_argument (ref args);
this.add_wipe_mode_argument (ref args);
diff --git a/gsecuredelete/mem-operation.vala b/gsecuredelete/mem-operation.vala
index 979b19f..d982687 100644
--- a/gsecuredelete/mem-operation.vala
+++ b/gsecuredelete/mem-operation.vala
@@ -30,12 +30,16 @@ namespace Gsd
*/
public class MemOperation : SecureDeleteOperation
{
+ /* constructor */
+ construct {
+ this.path = Config.SMEM_PATH;
+ }
+
/* builds the argument vector */
private override List<string> build_args ()
{
List<string> args = null;
- args.append ("smem");
args.append ("-v");
this.add_mode_argument (ref args);
diff --git a/gsecuredelete/securedelete-operation.vala b/gsecuredelete/securedelete-operation.vala
index efccf87..58b38b9 100644
--- a/gsecuredelete/securedelete-operation.vala
+++ b/gsecuredelete/securedelete-operation.vala
@@ -21,10 +21,6 @@ using GLib;
namespace Gsd
{
- /* FIXME: do NOT use SEACH_PATH but rather give an absolute path, because
- * using the search path is everything but secure: if the process calling
- * this have privileges, and a malicious user tweaked the PATH to make another
- * program being called, this may lead to GRAVE problems. */
/**
* SecureDeleteOperation:
*
@@ -118,7 +114,7 @@ namespace Gsd
public new bool run (uint watch_interval = 100)
throws SpawnError, AsyncOperationError
{
- return base.run (null, SpawnFlags.SEARCH_PATH, watch_interval);
+ return base.run (null, 0, watch_interval);
}
/** run_sync:
@@ -129,8 +125,7 @@ namespace Gsd
public new bool run_sync ()
throws SpawnError, AsyncOperationError
{
- return base.run_sync (null, SpawnFlags.SEARCH_PATH |
- SpawnFlags.STDOUT_TO_DEV_NULL, null);
+ return base.run_sync (null, SpawnFlags.STDOUT_TO_DEV_NULL, null);
}
}
}
diff --git a/gsecuredelete/swap-operation.vala b/gsecuredelete/swap-operation.vala
index 80f1d9a..d4a9c28 100644
--- a/gsecuredelete/swap-operation.vala
+++ b/gsecuredelete/swap-operation.vala
@@ -45,6 +45,11 @@ namespace Gsd
default = true;
}
+ /* constructor */
+ construct {
+ this.path = Config.SSWAP_PATH;
+ }
+
/* check whether a swap device is currently in use by the system or not.
* TODO: For now we parse /proc/swaps to know it.. is this portable and/or
* the correct way to proceed? */
@@ -94,7 +99,6 @@ namespace Gsd
this.device);
}
- args.append ("sswap");
args.append ("-v");
this.add_mode_argument (ref args);
this.add_zeroise_argument (ref args);
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-privacy/packages/libgsecuredelete.git
More information about the Pkg-privacy-commits
mailing list