Libguestfs - May 2014 - Re: [PATCH] daemon: scrub-file: resolve the path before calling scrub (RHBZ#1099490).

Resolve the given path within the chroot, so scrub can be invoked
outside the chroot on an already-resolved path.
Given that realpath is used, its availability is checked manually,
since scrub-file already depends on the "scrub" feature. Slightly
ugly,
but on the other hand realpath is generally available nowadays, so the
check should not be failing.

Add few tests in scrub-file for this and other similar issues.
---
 daemon/scrub.c       | 24 +++++++++++++++++++++++-
 generator/actions.ml | 13 ++++++++++++-
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/daemon/scrub.c b/daemon/scrub.c
index cd880b9..f500a08 100644
--- a/daemon/scrub.c
+++ b/daemon/scrub.c
@@ -54,12 +54,34 @@ do_scrub_device (const char *device)
 int
 do_scrub_file (const char *file)
 {
+  CLEANUP_FREE char *rp = NULL;
   CLEANUP_FREE char *buf = NULL;
   CLEANUP_FREE char *err = NULL;
   int r;
 
+  if (! optgroup_realpath_available ()) {
+    reply_with_error_errno (ENOTSUP,
+       "feature '%s' is not available in this\n"
+       "build of libguestfs.  Read 'AVAILABILITY' in the
guestfs(3) man page for\n"
+       "how to check for the availability of features.",
+       "realpath");
+    return -1;
+  }
+
+  /* Resolve the path to the file.  If it fails, then the file
+   * most probably does not exist or "file" is a symlink pointing
+   * outside the chroot.
+   */
+  CHROOT_IN;
+  rp = realpath (file, NULL);
+  CHROOT_OUT;
+  if (rp == NULL) {
+    reply_with_perror ("realpath: %s", file);
+    return -1;
+  }
+
   /* Make the path relative to /sysroot. */
-  buf = sysroot_path (file);
+  buf = sysroot_path (rp);
   if (!buf) {
     reply_with_perror ("malloc");
     return -1;
diff --git a/generator/actions.ml b/generator/actions.ml
index 0826137..01f6ab5 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -5719,7 +5719,18 @@ manual page for more details." };
     tests = [
       InitScratchFS, Always, TestRun (
         [["write"; "/scrub_file"; "content"];
-         ["scrub_file"; "/scrub_file"]]), []
+         ["scrub_file"; "/scrub_file"]]), [];
+      InitScratchFS, Always, TestRun (
+        [["write"; "/scrub_file_2"; "content"];
+         ["ln_s"; "/scrub_file_2";
"/scrub_file_2_link"];
+         ["scrub_file"; "/scrub_file_2_link"]]), [];
+      InitScratchFS, Always, TestLastFail (
+        [["ln_s"; "/scrub_file_3_notexisting";
"/scrub_file_3_link"];
+         ["scrub_file"; "/scrub_file_3_link"]]), [];
+      InitScratchFS, Always, TestLastFail (
+        [["write"; "/scrub_file_4"; "content"];
+         ["ln_s"; "../sysroot/scrub_file_4";
"/scrub_file_4_link"];
+         ["scrub_file"; "/scrub_file_4_link"]]), [];
     ];
     shortdesc = "scrub (securely wipe) a file";
     longdesc = "\
-- 
1.9.0

On Tue, May 20, 2014 at 03:33:31PM +0200, Pino Toscano
wrote:
> Resolve the given path within the chroot, so scrub can be invoked
> outside the chroot on an already-resolved path.
> Given that realpath is used, its availability is checked manually,
> since scrub-file already depends on the "scrub" feature. Slightly
ugly,
> but on the other hand realpath is generally available nowadays, so the
> check should not be failing.
> 
> Add few tests in scrub-file for this and other similar issues.
The realpath test was added (commit a86eb0e0d2c67e2) at a time when we
thought it would be a good idea to have the daemon run on Windows.
Since no one thinks that's a good idea any longer, I think you could
prepend this commit with one which removes tests for realpath /
HAVE_REALPATH and hard-codes optgroup_realpath_available() == 1.

On to this patch ...

Is it possible to make this change more generic so in future it could
be applied to other functions?

- Would it make sense to have a 'sysroot_realpath' library function to
  replace 'sysroot_path' in certain functions?

- Would it even make sense to modify 'sysroot_path' so it just calls
  realpath?

(I suspect not the second one because there are likely to be calls
like 'lstat' that want to see the symlink, not the final path.)

The concept behind the patch is good, I'd just like to see if we can
apply this in more places in future.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Tuesday 20 May 2014 15:56:16 Richard W.M. Jones
wrote:
> On Tue, May 20, 2014 at 03:33:31PM +0200, Pino Toscano wrote:
> > Resolve the given path within the chroot, so scrub can be invoked
> > outside the chroot on an already-resolved path.
> > Given that realpath is used, its availability is checked manually,
> > since scrub-file already depends on the "scrub" feature.
Slightly
> > ugly, but on the other hand realpath is generally available
> > nowadays, so the check should not be failing.
> > 
> > Add few tests in scrub-file for this and other similar issues.
> 
> The realpath test was added (commit a86eb0e0d2c67e2) at a time when we
> thought it would be a good idea to have the daemon run on Windows.
> Since no one thinks that's a good idea any longer, I think you could
> prepend this commit with one which removes tests for realpath /
> HAVE_REALPATH and hard-codes optgroup_realpath_available() == 1.
I didn't know the story behind this, and went for the conservative 
route.
Interesting enough, realpath has been unconditionally used already, so I 
will pull the plug to the conditional usage in some other parts, then.
> Is it possible to make this change more generic so in future it could
> be applied to other functions?
> 
> - Would it make sense to have a 'sysroot_realpath' library function
to
> replace 'sysroot_path' in certain functions?
Good idea. Do you have also an idea which daemon functions, other than 
scrub-files and realpath itself, might need such handling?

-- 
Pino Toscano