On Thu, Oct 22, 2015 at 02:01:56PM +0000, Serge Hallyn wrote:
> 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.
>
Will do. I sent an updated
version [Patch v5] which just includes the changes we discussed in the previous
mail (explained in the cover letter in case you forgot). Until I heard back from
#lxcontainers let's keep the part we've discussed right now as it is.
> > > (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?
Sure, we can use that if that is sufficient for our needs. Once the updated
version [Patch v5] is merged I'll extend with another patch.
>
> > 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
