Quoting Christian Brauner (christianvanbrau...@gmail.com):
> 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.
Ok, might be worth asking in #lxcontainers whether this would get in
anyone's way. If not, just do it and back it out if anyone complains.
> > (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:
Cursory glance it looks fine, though just using mkdtemp() should also
suffice, right?
> 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;
> }
>
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
