On Thu, Oct 22, 2015 at 02:59:28AM +0000, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Tue, Oct 20, 2015 at 09:21:48PM +0200, Christian Brauner wrote:
> > > The mount_entry_overlay_dirs() and mount_entry_aufs_dirs() functions
> > > create
> > > workdirs and upperdirs for overlay and aufs lxc.mount.entry entries. They
> > > try
> > > to make sure that the workdirs and upperdirs can only be created under the
> > > containerdir (e.g. /path/to/the/container/CONTAINERNAME). In order to do
> > > this
> > > the right hand side of
> > >
> > > if ((strncmp(upperdir, lxcpath, dirlen) == 0) &&
> > > (strncmp(upperdir, rootfs->path, rootfslen) != 0))
> > >
> > > was thought to check if the rootfs->path is not present in the workdir and
> > > upperdir mount options. But the current check is bogus since it will be
> > > trivially true whenever the container is a block-dev or overlay or aufs
> > > backed
> > > since the rootfs->path will then have a form like e.g.
> > >
> > > overlayfs:/some/path:/some/other/path
> > >
> > > This patch adds the function ovl_get_rootfs_dir() which parses
> > > rootfs->path by
> > > searching backwards for the first occurrence of the delimiter pair ":/".
> > > We do
> > > not simply search for ":" since it might be used in path names. If ":/"
> > > is not
> > > found we assume the container is directory backed and simply return
> > > strdup(rootfs->path).
> > >
> > > Signed-off-by: Christian Brauner <christianvanbrau...@gmail.com>
> > > ---
> > > src/lxc/conf.c | 101
> > > +++++++++++++++++++++++++++++++++++++++++++--------------
> > > 1 file changed, 76 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/src/lxc/conf.c b/src/lxc/conf.c
> > > index 16a62f8..fc072f7 100644
> > > --- a/src/lxc/conf.c
> > > +++ b/src/lxc/conf.c
> > > @@ -1815,12 +1815,51 @@ static void cull_mntent_opt(struct mntent *mntent)
> > > }
> > > }
> > >
> > > +static char *ovl_get_rootfs_dir(const char *rootfs_path, size_t
> > > *rootfslen)
> > > +{
> > > + char *rootfsdir = NULL;
> > > + char *s1 = NULL;
> > > + char *s2 = NULL;
> > > + char *s3 = NULL;
> > > + size_t s3len = 0;
> > > +
> > > + if (!rootfs_path || !rootfslen)
> > > + return NULL;
> > > +
> > > + s1 = strdup(rootfs_path);
> > > + if (!s1)
> > > + return NULL;
> > > +
> > > + if ((s2 = strstr(s1, ":/"))) {
> > > + s2 = s2 + 1;
> > > + if ((s3 = strstr(s2, ":/"))) {
> > > + s3len = strlen(s3);
> > > + s2[strlen(s2) - s3len] = '\0';
> > > + }
> > > + rootfsdir = strdup(s2);
> > > + if (!rootfsdir) {
> > > + free(s1);
> > > + return NULL;
> > > + }
> > > + }
> > > +
> > > + if (!rootfsdir)
> > > + rootfsdir = s1;
> > > + else
> > > + free(s1);
> > > +
> > > + *rootfslen = strlen(rootfsdir);
> > > +
> > > + return rootfsdir;
> > > +}
> > > +
> > > static int mount_entry_create_overlay_dirs(const struct mntent *mntent,
> > > const struct lxc_rootfs *rootfs,
> > > const char *lxc_name,
> > > const char *lxc_path)
> > > {
> > > char lxcpath[MAXPATHLEN];
> > > + char *rootfsdir = NULL;
> > > char *upperdir = NULL;
> > > char *workdir = NULL;
> > > char **opts = NULL;
> > > @@ -1832,13 +1871,13 @@ static int mount_entry_create_overlay_dirs(const
> > > struct mntent *mntent,
> > > size_t rootfslen = 0;
> > >
> > > if (!rootfs->path || !lxc_name || !lxc_path)
> > > - return -1;
> > > + goto err;
> > >
> > > opts = lxc_string_split(mntent->mnt_opts, ',');
> > > if (opts)
> > > arrlen = lxc_array_len((void **)opts);
> > > else
> > > - return -1;
> > > + goto err;
> > >
> > > for (i = 0; i < arrlen; i++) {
> > > if (strstr(opts[i], "upperdir=") && (strlen(opts[i]) > (len =
> > > strlen("upperdir="))))
> > > @@ -1848,31 +1887,37 @@ static int mount_entry_create_overlay_dirs(const
> > > struct mntent *mntent,
> > > }
> > >
> > > ret = snprintf(lxcpath, MAXPATHLEN, "%s/%s", lxc_path, lxc_name);
> > > - if (ret < 0 || ret >= MAXPATHLEN) {
> > > - lxc_free_array((void **)opts, free);
> > > - return -1;
> > > - }
> > > + if (ret < 0 || ret >= MAXPATHLEN)
> > > + goto err;
> > > +
> > > + rootfsdir = ovl_get_rootfs_dir(rootfs->path, &rootfslen);
> > > + if (!rootfsdir)
> > > + goto err;
> > >
> > > dirlen = strlen(lxcpath);
> > > - rootfslen = strlen(rootfs->path);
> > >
> > > /* We neither allow users to create upperdirs and workdirs outside the
> > > * containerdir nor inside the rootfs. The latter might be debatable. */
> > > if (upperdir)
> > > - if ((strncmp(upperdir, lxcpath, dirlen) == 0) &&
> > > (strncmp(upperdir, rootfs->path, rootfslen) != 0))
> > > + if ((strncmp(upperdir, lxcpath, dirlen) == 0) &&
> > > (strncmp(upperdir, rootfsdir, rootfslen) != 0))
> > > if (mkdir_p(upperdir, 0755) < 0) {
> > > WARN("Failed to create upperdir");
> > > }
> >
> > We should also consider putting
> >
> > return -1;
> >
> > here when the check fails. Consider a container named AA which has an
> > lxc.mount.entry for an overlay mount:
> >
> > lxc.mount.entry = /lowerdir dest overlay
> > lowerdir=/lowerdir,upperdir=/path/to/BB/delta,workdir=/path/to/BB/workdir,create=dir
> >
> > whose upperdir and workdir refer to another container BB. Both, upperdir and
> > workdir already exist under BB's containerdir. If we do not return -1 when
> > the
> > check fails the mount will succeed and we will write changes to
> > workdir/upperdir
> > of BB. That should not happen. Once you let me know that you're fine with
> > the
> > reimplementation of ovl_get_rootfs_dir() I would like to change that too
> > before
> > you ack it.
>
> Sorry, I just don't know who uses this or how they use it. The example you
> show could be problematic, but enforcing that the upperdir be in the
> container's
> lxcdir could annoy some people.
>
> -serge
Hm, just to be clear, I didn't mean that we return error when no upperdir or
workdir is given, just when one is given but it fails the pre mkdir_p() check.
In code:
if (upperdir)
if ((strncmp(upperdir, lxcpath, dirlen) == 0) &&
(strncmp(upperdir, rootfsdir, rootfslen) != 0)) {
if (mkdir_p(upperdir, 0755) < 0) {
WARN("Failed to create upperdir");
}
} else {
return -1;
}
if (workdir)
if ((strncmp(workdir, lxcpath, dirlen) == 0) &&
(strncmp(workdir, rootfsdir, rootfslen) != 0)) {
if (mkdir_p(workdir, 0755) < 0) {
WARN("Failed to create workdir");
}
} else {
return -1;
}
Your point about that this might annoy some people still stands though. But I
think not returning -1 or at least some informative INFO() where the upperdir
actually is located in this case could be a security issue.
> (For workdir that does not apply, in fact I'm
> tempted to say we should just auto-choose a random workdir if none is given)
We can do this. I will need that function for lxc-copy anyway. What do you
think about something along the lines of:
char *generate_random_name(const char *name, const char *path, const char *sep)
{
char testpath[MAXPATHLEN];
char tmp[MAXPATHLEN];
char *randname = NULL;
int ret = 0;
unsigned int seed;
int suffix = 0;
do {
#ifndef HAVE_RAND_R
seed = randseed(true);
#endif
#ifdef HAVE_RAND_R
seed = randseed(false);
suffix = rand_r(&seed);
#else
suffix = rand();
#endif
if (name)
ret = snprintf(tmp, MAXPATHLEN, "%s%s%x", name, sep,
suffix);
else
ret = snprintf(tmp, MAXPATHLEN, "%s%x", sep, suffix);
if (ret < 0 || ret >= MAXPATHLEN) {
return NULL;
}
ret = snprintf(testpath, MAXPATHLEN, "%s/%s", path, tmp);
if (ret < 0 || ret >= MAXPATHLEN) {
return NULL;
}
} while (dir_exists(testpath));
randname = strdup(tmp);
if (!randname)
return NULL;
return randname;
}
signature.asc
Description: PGP signature
_______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel
