Maros Zatko
2015-Mar-16 18:09 UTC
[Libguestfs] [PATCH] RFE: Inspection should support systemd mount units
Adds support for systemd .mount files, uses Augeas to extract mount points. Fixes RHBZ#1113153. Maros Zatko (1): inspection: add support for systemd .mount files src/inspect-fs-unix.c | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 240 insertions(+) -- 1.9.3
Maros Zatko
2015-Mar-16 18:09 UTC
[Libguestfs] [PATCH] inspection: add support for systemd .mount files
Fixes RHBZ#1113153.
---
src/inspect-fs-unix.c | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 240 insertions(+)
diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c
index 2abbf24..6dfc299 100644
--- a/src/inspect-fs-unix.c
+++ b/src/inspect-fs-unix.c
@@ -96,6 +96,9 @@ static char *resolve_fstab_device (guestfs_h *g, const char
*spec,
Hash_table *md_map,
enum inspect_os_type os_type);
static int inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs, const char
**configfiles, int (*f) (guestfs_h *, struct inspect_fs *));
+static int inspect_with_augeas2 (guestfs_h *g, struct inspect_fs *fs, const
char **configfiles, int (*f) (guestfs_h *, struct inspect_fs *, const char *));
+static int check_systemd_mounts (guestfs_h *g, struct inspect_fs *fs);
+static int check_systemd_mnt (guestfs_h *g, struct inspect_fs *fs, const char
*path);
/* Hash structure for uuid->path lookups */
typedef struct md_uuid {
@@ -582,6 +585,9 @@ guestfs_int_check_linux_root (guestfs_h *g, struct
inspect_fs *fs)
if (check_hostname_unix (g, fs) == -1)
return -1;
+ if (check_systemd_mounts (g, fs) == -1)
+ return -1;
+
return 0;
}
@@ -981,6 +987,180 @@ check_hostname_freebsd (guestfs_h *g, struct inspect_fs
*fs)
}
static int
+check_systemd_mnt (guestfs_h *g, struct inspect_fs *fs, const char *fname)
+{
+ CLEANUP_FREE_STRING_LIST char **entries = NULL;
+ char **entry;
+ char augpath[256];
+ CLEANUP_HASH_FREE Hash_table *md_map = NULL;
+ bool is_bsd = (fs->type == OS_TYPE_FREEBSD ||
+ fs->type == OS_TYPE_NETBSD ||
+ fs->type == OS_TYPE_OPENBSD);
+
+ /* Generate a map of MD device paths listed in /etc/mdadm.conf to MD device
+ * paths in the guestfs appliance */
+ if (map_md_devices (g, &md_map) == -1) {
+ printf ("map_md_dev fail\n");
+ return -1;
+ }
+
+ char expr[512];
+ snprintf (expr, sizeof expr, "/files%s", fname);
+ entries = guestfs_aug_match (g, expr);
+ if (entries == NULL) {
+ return 0;
+ }
+
+ for (entry = entries; *entry != NULL; entry++) {
+ CLEANUP_FREE char *spec = NULL;
+ CLEANUP_FREE char *mp = NULL;
+ CLEANUP_FREE char *mountable = NULL;
+ CLEANUP_FREE char *vfstype = NULL;
+
+ snprintf (augpath, sizeof augpath, "%s/Mount/What/value",
*entry);
+ spec = guestfs_aug_get (g, augpath);
+ if (spec == NULL)
+ return -1;
+
+ /* Ignore /dev/fd (floppy disks) (RHBZ#642929) and CD-ROM drives.
+ *
+ * /dev/iso9660/FREEBSD_INSTALL can be found in FreeBSDs installation
+ * discs.
+ */
+ if ((STRPREFIX (spec, "/dev/fd") && c_isdigit (spec[7]))
||
+ STREQ (spec, "/dev/floppy") ||
+ STREQ (spec, "/dev/cdrom") ||
+ STRPREFIX (spec, "/dev/iso9660/"))
+ continue;
+
+ snprintf (augpath, sizeof augpath, "%s/Mount/Where/value",
*entry);
+ mp = guestfs_aug_get (g, augpath);
+ if (mp == NULL)
+ return -1;
+
+ /* Ignore certain mountpoints. */
+ if (STRPREFIX (mp, "/dev/") ||
+ STREQ (mp, "/dev") ||
+ STRPREFIX (mp, "/media/") ||
+ STRPREFIX (mp, "/proc/") ||
+ STREQ (mp, "/proc") ||
+ STRPREFIX (mp, "/selinux/") ||
+ STREQ (mp, "/selinux") ||
+ STRPREFIX (mp, "/sys/") ||
+ STREQ (mp, "/sys"))
+ continue;
+
+ /* Resolve UUID= and LABEL= to the actual device. */
+ if (STRPREFIX (spec, "UUID="))
+ mountable = guestfs_findfs_uuid (g, &spec[5]);
+ else if (STRPREFIX (spec, "LABEL="))
+ mountable = guestfs_findfs_label (g, &spec[6]);
+ /* Ignore "/.swap" (Pardus) and pseudo-devices like
"tmpfs". */
+ else if (STREQ (spec, "/dev/root") || (is_bsd && STREQ
(mp, "/")))
+ /* Resolve /dev/root to the current device.
+ * Do the same for the / partition of the *BSD systems, since the
+ * BSD -> Linux device translation is not straight forward.
+ */
+ mountable = safe_strdup (g, fs->mountable);
+ else if (STRPREFIX (spec, "/dev/"))
+ /* Resolve guest block device names. */
+ mountable = resolve_fstab_device (g, spec, md_map, fs->type);
+ else if (match (g, spec, re_openbsd_duid)) {
+ /* In OpenBSD's fstab you can specify partitions on a disk by
appending a
+ * period and a partition letter to a Disklable Unique Identifier. The
+ * DUID is a 16 hex digit field found in the OpenBSD's altered BSD
+ * disklabel. For more info see here:
+ * http://www.openbsd.org/faq/faq14.html#intro
+ */
+ char device[10]; /* /dev/sd[0-9][a-z] */
+ char part = spec[17];
+
+ /* We cannot peep into disklables, we can only assume that this is the
+ * first disk.
+ */
+ snprintf(device, 10, "%s%c", "/dev/sd0", part);
+ mountable = resolve_fstab_device (g, device, md_map, fs->type);
+ }
+
+ /* If we haven't resolved the device successfully by this point,
+ * we don't care, just ignore it.
+ */
+ if (mountable == NULL)
+ continue;
+
+ snprintf (augpath, sizeof augpath, "%s/Mount/Type/value",
*entry);
+ vfstype = guestfs_aug_get (g, augpath);
+ if (vfstype == NULL) return -1;
+
+ if (STREQ (vfstype, "btrfs")) {
+ char **opt;
+
+ snprintf (augpath, sizeof augpath, "%s/Mount/Options/value",
*entry);
+ CLEANUP_FREE_STRING_LIST char **opts = guestfs_aug_match (g, augpath);
+ if (opts == NULL) return -1;
+
+ for (opt = opts; *opt; opt++) {
+ CLEANUP_FREE char *optname = guestfs_aug_get (g, augpath);
+ if (optname == NULL) return -1;
+
+ if (STREQ (optname, "subvol")) {
+ CLEANUP_FREE char *subvol = NULL;
+ char *new;
+
+ snprintf (augpath, sizeof augpath, "%s/value", *opt);
+ subvol = guestfs_aug_get (g, augpath);
+ if (subvol == NULL) return -1;
+
+ new = safe_asprintf (g, "btrfsvol:%s/%s", mountable,
subvol);
+ free (mountable);
+ mountable = new;
+ }
+ }
+ }
+
+ if (add_fstab_entry (g, fs, mountable, mp) == -1) return -1;
+ }
+
+ return 0;
+}
+
+static int
+check_systemd_mounts (guestfs_h *g, struct inspect_fs *fs)
+{
+ const char *dirs[] = {"/etc/systemd/", "/lib/systemd/",
+ "/usr/lib/systemd/", NULL};
+ const char **dir;
+ for (dir = dirs; *dir != NULL; dir++) {
+ CLEANUP_FREE_STRING_LIST char **entries = NULL;
+ char **entry;
+
+ entries = guestfs_find (g, *dir);
+ size_t cnt = guestfs_int_count_strings (entries);
+
+ CLEANUP_FREE_STRING_LIST char **filenames + safe_malloc (g, (cnt + 1)
* sizeof (char *));
+
+ size_t idx = 0;
+ for (entry = entries; *entry != NULL; entry++) {
+ if (STRSUFFIX (*entry, ".mount")) {
+ size_t entry_len = strlen(*dir) + strlen(*entry) + 1;
+ filenames[idx] = safe_malloc (g, entry_len);
+ snprintf (filenames[idx], entry_len, "%s%s", *dir, *entry);
+ ++idx;
+ }
+ }
+ filenames[idx] = NULL;
+
+ if (inspect_with_augeas2 (g, fs,
+ (const char**) filenames, check_systemd_mnt) == -1) {
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+static int
check_fstab (guestfs_h *g, struct inspect_fs *fs)
{
CLEANUP_FREE_STRING_LIST char **entries = NULL;
@@ -1705,6 +1885,66 @@ static char *make_augeas_path_expression (guestfs_h *g,
const char **configfiles
* to 'f' the Augeas handle is closed.
*/
static int
+inspect_with_augeas2 (guestfs_h *g, struct inspect_fs *fs,
+ const char **configfiles,
+ int (*f) (guestfs_h *, struct inspect_fs *, const char *))
+{
+ size_t i;
+ int64_t size;
+ int r;
+ CLEANUP_FREE char *pathexpr = NULL;
+
+ /* Security: Refuse to do this if a config file is too large. */
+ for (i = 0; configfiles[i] != NULL; ++i) {
+ if (guestfs_is_file_opts (g, configfiles[i],
+ GUESTFS_IS_FILE_OPTS_FOLLOWSYMLINKS, 1, -1) == 0)
+ continue;
+
+ size = guestfs_filesize (g, configfiles[i]);
+ if (size == -1)
+ /* guestfs_filesize failed and has already set error in handle */
+ return -1;
+ if (size > MAX_AUGEAS_FILE_SIZE) {
+ error (g, _("size of %s is unreasonably large (%" PRIi64 "
bytes)"),
+ configfiles[i], size);
+ return -1;
+ }
+ }
+
+ if (guestfs_aug_init (g, "/", 16|32 /* AUG_SAVE_NOOP|AUG_NO_LOAD
*/) == -1)
+ return -1;
+
+ r = -1;
+
+ /* Tell Augeas to only load configfiles and no other files. This
+ * prevents a rogue guest from performing a denial of service attack
+ * by having large, over-complicated configuration files which are
+ * unrelated to the task at hand. (Thanks Dominic Cleal).
+ * Note this requires Augeas >= 1.0.0 because of RHBZ#975412.
+ */
+ pathexpr = make_augeas_path_expression (g, configfiles);
+ if (guestfs_aug_rm (g, pathexpr) == -1)
+ goto out;
+
+ if (guestfs_aug_load (g) == -1)
+ goto out;
+
+ for (i = 0; configfiles[i] != NULL; ++i) {
+ r = f (g, fs, configfiles[i]);
+ }
+
+ out:
+ guestfs_aug_close (g);
+
+ return r;
+}
+
+/* Call 'f' with Augeas opened and having parsed 'configfiles'
(these
+ * files must exist). As a security measure, this bails if any file
+ * is too large for a reasonable configuration file. After the call
+ * to 'f' the Augeas handle is closed.
+ */
+static int
inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs,
const char **configfiles,
int (*f) (guestfs_h *, struct inspect_fs *))
--
1.9.3
Richard W.M. Jones
2015-Mar-23 13:23 UTC
Re: [Libguestfs] [PATCH] inspection: add support for systemd .mount files
On Mon, Mar 16, 2015 at 07:09:51PM +0100, Maros Zatko wrote:> Fixes RHBZ#1113153. > --- > src/inspect-fs-unix.c | 240 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 240 insertions(+) > > diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c > index 2abbf24..6dfc299 100644 > --- a/src/inspect-fs-unix.c > +++ b/src/inspect-fs-unix.c > @@ -96,6 +96,9 @@ static char *resolve_fstab_device (guestfs_h *g, const char *spec, > Hash_table *md_map, > enum inspect_os_type os_type); > static int inspect_with_augeas (guestfs_h *g, struct inspect_fs *fs, const char **configfiles, int (*f) (guestfs_h *, struct inspect_fs *)); > +static int inspect_with_augeas2 (guestfs_h *g, struct inspect_fs *fs, const char **configfiles, int (*f) (guestfs_h *, struct inspect_fs *, const char *));I think there is a case for splitting out the commit which adds this new function. Also, do we need a new function? I don't see why it's needed at all - just modify the old inspect_with_augeas so it passes the extra parameter always, and update the call sites [in a separate commit].> +static int check_systemd_mounts (guestfs_h *g, struct inspect_fs *fs); > +static int check_systemd_mnt (guestfs_h *g, struct inspect_fs *fs, const char *path);You don't need to prototype check_systemd_mnt at all - just remove that line as it's confusing for people reading the code.> /* Hash structure for uuid->path lookups */ > typedef struct md_uuid { > @@ -582,6 +585,9 @@ guestfs_int_check_linux_root (guestfs_h *g, struct inspect_fs *fs) > if (check_hostname_unix (g, fs) == -1) > return -1; > > + if (check_systemd_mounts (g, fs) == -1) > + return -1; > + > return 0; > } > > @@ -981,6 +987,180 @@ check_hostname_freebsd (guestfs_h *g, struct inspect_fs *fs) > } > > static int > +check_systemd_mnt (guestfs_h *g, struct inspect_fs *fs, const char *fname) > +{ > + CLEANUP_FREE_STRING_LIST char **entries = NULL; > + char **entry; > + char augpath[256]; > + CLEANUP_HASH_FREE Hash_table *md_map = NULL; > + bool is_bsd = (fs->type == OS_TYPE_FREEBSD || > + fs->type == OS_TYPE_NETBSD || > + fs->type == OS_TYPE_OPENBSD); > + > + /* Generate a map of MD device paths listed in /etc/mdadm.conf to MD device > + * paths in the guestfs appliance */ > + if (map_md_devices (g, &md_map) == -1) { > + printf ("map_md_dev fail\n");This printf shouldn't be here.> + return -1; > + } > + > + char expr[512]; > + snprintf (expr, sizeof expr, "/files%s", fname);Yikes. Don't use fixed size buffers, especially where (as seems to be the case here) the path comes from user input. Use: CLEANUP_FREE char *expr = NULL; ... expr = safe_asprintf (g, "/files%s", fname);> + entries = guestfs_aug_match (g, expr); > + if (entries == NULL) { > + return 0; > + } > + > + for (entry = entries; *entry != NULL; entry++) { > + CLEANUP_FREE char *spec = NULL; > + CLEANUP_FREE char *mp = NULL; > + CLEANUP_FREE char *mountable = NULL; > + CLEANUP_FREE char *vfstype = NULL; > + > + snprintf (augpath, sizeof augpath, "%s/Mount/What/value", *entry); > + spec = guestfs_aug_get (g, augpath); > + if (spec == NULL) > + return -1; > + > + /* Ignore /dev/fd (floppy disks) (RHBZ#642929) and CD-ROM drives. > + * > + * /dev/iso9660/FREEBSD_INSTALL can be found in FreeBSDs installation > + * discs. > + */ > + if ((STRPREFIX (spec, "/dev/fd") && c_isdigit (spec[7])) || > + STREQ (spec, "/dev/floppy") || > + STREQ (spec, "/dev/cdrom") || > + STRPREFIX (spec, "/dev/iso9660/")) > + continue;[etc] There's a lot of common code with check_fstab here. It's not completely identical because the augeas queries are slightly different, but you can parameterize those easily enough. A nice thing would be a separate commit which first moves the common code out from check_fstab into its own function.> +static int > +check_systemd_mounts (guestfs_h *g, struct inspect_fs *fs) > +{ > + const char *dirs[] = {"/etc/systemd/", "/lib/systemd/", > + "/usr/lib/systemd/", NULL}; > + const char **dir; > + for (dir = dirs; *dir != NULL; dir++) { > + CLEANUP_FREE_STRING_LIST char **entries = NULL; > + char **entry; > + > + entries = guestfs_find (g, *dir);Error check? If the command fails it will immediately segfault in the next line, which is something that inspection code cannot be doing -- it's a DoS attack route.> + size_t cnt = guestfs_int_count_strings (entries); > + > + CLEANUP_FREE_STRING_LIST char **filenames > + safe_malloc (g, (cnt + 1) * sizeof (char *)); > + > + size_t idx = 0; > + for (entry = entries; *entry != NULL; entry++) { > + if (STRSUFFIX (*entry, ".mount")) { > + size_t entry_len = strlen(*dir) + strlen(*entry) + 1; > + filenames[idx] = safe_malloc (g, entry_len); > + snprintf (filenames[idx], entry_len, "%s%s", *dir, *entry); > + ++idx; > + } > + } > + filenames[idx] = NULL; > + > + if (inspect_with_augeas2 (g, fs, > + (const char**) filenames, check_systemd_mnt) == -1) { > + return -1; > + } > + } > + > + return 0; > +} > + > +static int > check_fstab (guestfs_h *g, struct inspect_fs *fs) > { > CLEANUP_FREE_STRING_LIST char **entries = NULL; > @@ -1705,6 +1885,66 @@ static char *make_augeas_path_expression (guestfs_h *g, const char **configfiles > * to 'f' the Augeas handle is closed. > */ > static int > +inspect_with_augeas2 (guestfs_h *g, struct inspect_fs *fs, > + const char **configfiles, > + int (*f) (guestfs_h *, struct inspect_fs *, const char *)) > +{As discussed above, I don't believe this function is necessary. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html