public inbox for linux-unionfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Vivek Goyal <vgoyal@redhat.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
	overlayfs <linux-unionfs@vger.kernel.org>
Subject: Re: [PATCH v9 00/15] overlayfs: Delayed copy up of data
Date: Wed, 10 Jan 2018 10:27:15 -0500	[thread overview]
Message-ID: <20180110152715.GC8999@redhat.com> (raw)
In-Reply-To: <CAOQ4uxjjr=FzHLvYpBQ4Uebp9-fT2Prnm-XUNkc=LRaT3QMFHQ@mail.gmail.com>

On Wed, Jan 10, 2018 at 05:10:22PM +0200, Amir Goldstein wrote:

[..]
> >> >> 1. Considering Miklos' commit
> >> >>     438c84c2f0c7 ovl: don't follow redirects if redirect_dir=off
> >> >>     It is probably not a good idea to allow lookup of metacopy unless
> >> >>     metacopy=on. Is that already the behavior in V9?
> >> >
> >> > Hi Amir,
> >> >
> >> > Hmm.., no, that's not the behavior in V9. Remember, we wanted to follow
> >> > metacopy origin even if metacopy=off. That way a user can mount a
> >> > overlayfs with metacopy=off (which was previously mounted as metacopy=on)
> >> > and not be broken.
> >> >
> >>
> >> User can also mount with redirect_dir=nofollow after previously mounting with
> >> redirect_dir=on. It's the exact same thing.
> >>
> >> > If we follow metacopy only if metacopy=on, then we really need some
> >> > mechanism which can atleast warn user that this overlay mount was
> >> > mounted with metacopy=on in the past and expect some unexpected results
> >> > if mounted with metacopy=off.
> >> >
> >> > Has there been any agreement on what mechanism to use to remember what
> >> > features have been turned on existing overlay mount.
> >> >
> >>
> >> There is no agreement, but there is code in upstream that "allows" the user
> >> to make the same with redirect_dir. The consequences of this configuration is
> >> -EPERM on lookup.
> >> You actually have to allow this configuration for security reasons, the only
> >> question is whether metacopy will have 3 modes (off/follow/on) or just on/off
> >> where off implies nofollow.
> >
> > Hi Miklos and Amir,
> >
> > Thinking more about security implications of this.
> >
> > Can a user hand craft ORIGIN xattr? I mean, if inode number of lower file
> > is known, can a user come up with file handle of lower and put in ORIGIN
> > XATTR?
> 
> Yes, its quite easy if you know the underlying fs.
> For example for ext4, you don't even need to guess the generation number,
> you can provide 0 generation and ext4 treats it as ANY.
> 
> >
> > If yes, this sounds like a security concern. Then I as a user can simply
> > hand craft an upper file and point to any file in lower and put associated
> > ORIGIN and METACOPY xattr on upper and next time mount is done with
> > metacopy=on, I can get access to any lower file?
> >
> > In fact, not just metacopy, if ORIGIN can be handcrafted, then we will have
> > to be very careful on when ORIGIN should be followed otherwise an
> > handcrafted upper can lead to unexpected security issues. (This is
> > assuming that we will use ORIGIN for more and more features).
> >
> > Am I overthinking this?
> >
> 
> It is exactly as you wrote. Not any less or any more of a security concern
> than a hand crafted redirect_dir. The only difference is that without
> metacopy=on and without redirect_dir=origin, the only implication of
> following an hand crafted origin would be to get a different st_dev/st_ino
> and for example, to fake that 2 files/dirs are the same while one is actually
> a rootkit/malware. So not that easy to exploit in current upstream.

Right. Currently we seem to be using origin only for st_dev/st_ino so
no big impact. "metadata only copyup" is first feature which will make
data of lower file available using ORIGIN. So anymore features we add
using ORIGIN, we will have to be extra careful. Atleast make it
conditional on a mount option and document that using this mount option
on untrusted layer source can lead to privilege escalation.

Vivek

  reply	other threads:[~2018-01-10 15:27 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-29 15:54 [PATCH v9 00/15] overlayfs: Delayed copy up of data Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 01/15] ovl: Do not look for OVL_XATTR_NLINK if index is not there Vivek Goyal
2017-11-29 17:04   ` Amir Goldstein
2017-11-29 15:54 ` [PATCH v9 02/15] ovl: disable redirect_dir and index when no xattr support Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 03/15] ovl: ovl_check_setxattr() get rid of redundant -EOPNOTSUPP check Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 04/15] ovl: Create origin xattr on copy up for all files Vivek Goyal
2018-01-08 10:16   ` Miklos Szeredi
2018-01-08 11:18     ` Amir Goldstein
2018-01-08 15:58       ` Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 05/15] ovl: Provide a mount option metacopy=on/off for metadata copyup Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 06/15] ovl: During copy up, first copy up metadata and then data Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 07/15] ovl: Move the copy up helpers to copy_up.c Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 08/15] ovl: Copy up only metadata during copy up where it makes sense Vivek Goyal
2018-01-08 10:35   ` Miklos Szeredi
2018-01-08 17:03     ` Vivek Goyal
2018-01-09 10:49       ` Miklos Szeredi
2018-01-09 13:26         ` Vivek Goyal
2018-01-09 13:33           ` Amir Goldstein
2018-01-09 20:34             ` Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 09/15] ovl: Add helper ovl_already_copied_up() Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 10/15] ovl: A new xattr OVL_XATTR_METACOPY for file on upper Vivek Goyal
2018-01-08 15:50   ` Miklos Szeredi
2018-01-08 16:17     ` Vivek Goyal
2018-01-08 16:21       ` Miklos Szeredi
2018-01-08 16:25         ` Miklos Szeredi
2017-11-29 15:54 ` [PATCH v9 11/15] ovl: Fix ovl_getattr() to get number of blocks from lower Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 12/15] ovl: Set OVL_UPPERDATA flag during ovl_lookup() Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 13/15] ovl: Do not expose metacopy only upper dentry from d_real() Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 14/15] ovl: Fix encryption/compression status of a metacopy only file Vivek Goyal
2018-01-18 14:24   ` Vivek Goyal
2018-01-18 14:32     ` Amir Goldstein
2018-01-18 14:36       ` Vivek Goyal
2017-11-29 15:54 ` [PATCH v9 15/15] ovl: Enable metadata only feature Vivek Goyal
2018-01-06  7:38 ` [PATCH v9 00/15] overlayfs: Delayed copy up of data Amir Goldstein
2018-01-08 14:13   ` Vivek Goyal
2018-01-08 14:42     ` Amir Goldstein
2018-01-08 15:44       ` Vivek Goyal
2018-01-10 14:56       ` Vivek Goyal
2018-01-10 15:08         ` Miklos Szeredi
2018-01-10 15:23           ` Vivek Goyal
2018-01-10 15:10         ` Amir Goldstein
2018-01-10 15:27           ` Vivek Goyal [this message]
2018-01-10 15:38             ` Miklos Szeredi
2018-01-10 15:47               ` Vivek Goyal
2018-01-10 15:54                 ` Amir Goldstein
2018-01-10 16:03                   ` Miklos Szeredi
2018-01-10 16:30                     ` Vivek Goyal
2018-01-10 17:05                       ` Amir Goldstein

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180110152715.GC8999@redhat.com \
    --to=vgoyal@redhat.com \
    --cc=amir73il@gmail.com \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox