lxc-user-nic allows unprivileged users to open arbitrary files

Current thought is that we could statvfs the resulting path to make sure that the filesystem is nsfs and not even attempt an open if it's not the case.

This will be handled as a medium priority security issue on the LXC side, so we will be issuing patches for affected releases (LXC 2.0 and LXC 3.0 appear affected) and will publish them in our stable branches and mailing-list when we reach the CRD (TBD at this point).

We will not be issuing emergency tarballs for this though, the fix will simply get rolled into the upcoming LXC 3.0.2 release (assuming the release happens after the CRD), distros should just apply the patch as part of a security update (easier to verify than a full bugfix release).

For Ubuntu specifically:
 - precise: unaffected (0.7.5 or 1.0.9)
 - trusty: unaffected (1.0.10 or 2.0.8)
 - xenial: backports only (2.0.8 or 3.0.1)
 - bionic: affected (3.0.1)
 - cosmic: affected (3.0.1)

The 2.0 branch is only affected starting with 2.0.8, as we never pushed that bugfix release to Ubuntu, we're not affected for the series that include this version.

Ideal CRD for us would be the 6th (Monday) of August.

I think I have a patch:
1. open() O_PATH fd which won't trigger an actual open()
2. fstatvfs() the O_PATH fd and verify that f_fsid == NSFS_MAGIC
3. build proc path to the O_PATH fd and reopen O_PATH fd with O_RDONLY | O_CLOEXEC
4. perform setns(netns_fd, CLONE_NEWNET) (which will fail on anything else than an actual netns fd

Doing the O_PATH open and then the /proc reopen trick let's us avoid TOCTOU.

It'd be great if we could get reviews on this patch by:

 - Serge Hallyn
 - Matthias Gerstner
 - Ubuntu security team

If all are happy with it, we should be able to move forward with it.

Matthias, does SUSE want to issue a CVE number for this or do you prefer we do it on our side?

Once we have a CVE number assigned to this, we'll let the Ubuntu security team communicate the patch and CRD to the other distros and I'll be preparing Ubuntu packages for release on the CRD.

Hello and thanks for the quick patch. I will review it today and report back.

Regarding the CVE number, the SUSE security team can request one if it helps you out. Since SUSE is not directly affected (because we don't ship the binary with setuid bit set at the moment) maybe it's better if you request it.

On Thu, Jul 26, 2018 at 10:17:58AM -0000, Matthias Gerstner wrote:
> Hello and thanks for the quick patch. I will review it today and report
> back.

Cool, thanks very much for that!

>
> Regarding the CVE number, the SUSE security team can request one if it
> helps you out. Since SUSE is not directly affected (because we don't
> ship the binary with setuid bit set at the moment) maybe it's better if
> you request it.

I'll leave that for Stéphane to decide! :)

Christian

>
> --
> You received this bug notification because you are a member of Ubuntu
> Container Security team, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1783591
>
> Title:
> lxc-user-nic allows unprivileged users to open arbitrary files
>
> Status in lxc package in Ubuntu:
> Triaged
> Status in lxc source package in Xenial:
> Triaged
> Status in lxc source package in Bionic:
> Triaged
> Status in lxc source package in Cosmic:
> Triaged
>
> Bug description:
> Matthias Gerstner from SUSE reported the following:
>
> ```
> Hello,
>
> following the lxc security reporting guidelines [1] I am reporting a
> finding in the lxc-user-nic setuid binary. I'm encrypting this mail as a
> best practice and because I found valid GPG keys for all of your
> adresses. Please find my public key attached to this mail.
>
> In the context of an openSUSE security audit of the lxc-user-nic setuid
> binary [2] (currently private bug) I came across an issue that should be
> adressed. In the "delete" case the program runs the following piece of
> code unconditionally with effective uid 0 (from lxc_user_nic.c):
>
> ```
> } else if (request == LXC_USERNIC_DELETE) {
> netns_fd = open(args.pid, O_RDONLY);
> if (netns_fd < 0) {
> usernic_error("Could not open \"%s\": %s\n", args.pid,
> strerror(errno));
> exit(EXIT_FAILURE);
> }
> }
> ```
>
> `args.pid` is a user controlled parameter and can be an arbitrary path
> at the moment. Nothing is done with this file descriptor later on in the
> program except an attempt at `setns(fd, CLONE_NEWNET)` in
> `is_privileged_over_netns()`. Still this allows the unprivileged caller
> of the setuid binary to achieve the following:
>
> - it can test for existence of files normally not accessible to the
> caller (information leak). Example:
> ```
> # this file is existing
> $ /usr/lib/lxc/lxc-user-nic delete path name /root/.bash_history type bridge nic
> lxc_user_nic.c: 1017: is_privileged_over_netns: Failed to setns() to network namespace Invalid argument
> lxc_user_nic.c: 1161: main: Process is not privileged over network namespace
>
> # this file is not existing
> $ /usr/lib/lxc/lxc-user-nic delete path name /root/.zsh_history type bridge nic
> lxc_user_nic.c: 1130: main: Could not open "/root/.zsh_history": No such file or directory
> ```
>
> - it allows to trigger code paths in the kernel that are normally not
> accessible to the caller. This can happen when opening special...

I have looked at the patch and security wise it looks very good to me. For completeness I would also add the O_CLOEXEC i.e.:

netns_fd = open(args.pid, O_PATH | O_CLOEXEC);

I think there is still a functional issue, however. You are using `fstatvfs()` and evaluate `f_fsid`. This is, as far as I understand, a unique identifier for file systems independent of their file system type. It returns zero for pseudo file systems.

To get the file system magic you need to use `fstatfs()` and evaluate `f_type` instead.

On Thu, Jul 26, 2018 at 11:51:27AM -0000, Matthias Gerstner wrote:
> I have looked at the patch and security wise it looks very good to me.
> For completeness I would also add the O_CLOEXEC i.e.:
>
> netns_fd = open(args.pid, O_PATH | O_CLOEXEC);

The manpage (man 2 open ) states that:

"When O_PATH is specified in flags, flag bits other than O_CLOEXEC,
 O_DIRECTORY, and O_NOFOLLOW are ignored."

that's why I left it out.

>
> I think there is still a functional issue, however. You are using
> `fstatvfs()` and evaluate `f_fsid`. This is, as far as I understand, a
> unique identifier for file systems independent of their file system
> type. It returns zero for pseudo file systems.

Ok, I'll check the kernel sources but I need to switch this to our
has_fs_type() helper anyway that does:

/* __typeof__ should be safe to use with all compilers. */
typedef __typeof__(((struct statfs *)NULL)->f_type) fs_type_magic;
extern bool has_fs_type(const char *path, fs_type_magic magic_val);
extern bool is_fs_type(const struct statfs *fs, fs_type_magic magic_val);

bool is_fs_type(const struct statfs *fs, fs_type_magic magic_val)
{
        return (fs->f_type == (fs_type_magic)magic_val);
}

bool has_fs_type(const char *path, fs_type_magic magic_val)
{
        bool has_type;
        int ret;
        struct statfs sb;

        ret = statfs(path, &sb);
        if (ret < 0)
                return false;

        has_type = is_fs_type(&sb, magic_val);
        if (!has_type && magic_val == RAMFS_MAGIC)
                WARN("When the ramfs it a tmpfs statfs() might report tmpfs");

        return has_type;
}

Thanks!
Christian

>
> To get the file system magic you need to use `fstatfs()` and evaluate
> `f_type` instead.
>
> --
> You received this bug notification because you are a member of Ubuntu
> Container Security team, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1783591
>
> Title:
> lxc-user-nic allows unprivileged users to open arbitrary files
>
> Status in lxc package in Ubuntu:
> Triaged
> Status in lxc source package in Xenial:
> Triaged
> Status in lxc source package in Bionic:
> Triaged
> Status in lxc source package in Cosmic:
> Triaged
>
> Bug description:
> Matthias Gerstner from SUSE reported the following:
>
> ```
> Hello,
>
> following the lxc security reporting guidelines [1] I am reporting a
> finding in the lxc-user-nic setuid binary. I'm encrypting this mail as a
> best practice and because I found valid GPG keys for all of your
> adresses. Please find my public key attached to this mail.
>
> In the context of an openSUSE security audit of the lxc-user-nic setuid
> binary [2] (currently private bug) I came across an issue that should be
> adressed. In the "delete" case the program runs the following piece of
> code unconditionally with effective uid 0 (from lxc_user_nic.c):
>
> ```
> } else if (request == LXC_USERNIC_DELETE) {
> netns_fd = open(args.pid, O_RDONLY);
> if (netns_fd < 0) {
> usernic_error("Could not open \"%s\": %s\n", args.pid,
> strerror(errno)...

Here's the updated version that switches from fstatvfs() to fstatfs()
to reuse infrastructure we already have and to correctly check for the fs type.
The logic stays the same:

1. open() O_PATH fd which won't trigger an actual open()
2. fstats() the O_PATH fd and verify that f_type == NSFS_MAGIC
3. build proc path to the O_PATH fd and reopen O_PATH fd with O_RDONLY | O_CLOEXEC
4. perform setns(netns_fd, CLONE_NEWNET) (which will fail on anything else than an actual netns fd

Doing the O_PATH open and then the /proc reopen trick let's us avoid TOCTOU.

Fwiw, I misread the open manpage.

Here's the correct patch including the missing O_CLOEXEC flag for the O_PATH fd.

The updated patch looks good to me. As it happens I also misread the open man page initially, so probably the wording is difficult to follow ;-).

Hi,

looks good, my only complaint would be that the WARN message in fhas_fs_type() doesn't seem clear. I'd offer an alternative but I don't know what you're trying to say :)

Do you think there's any point in enforcing that the filename be 'net' ? (i.e. to avoid playing any sorts of games with a userns) Probably not because is_privileged_over_netns() will trip over it in its setns(netns_fd, CLONE_NEWNET) in that case, right?

On Thu, Jul 26, 2018 at 11:52:30PM -0000, Serge Hallyn wrote:
> Hi,
>
> looks good, my only complaint would be that the WARN message in
> fhas_fs_type() doesn't seem clear. I'd offer an alternative but I don't

One way to detect whether we are on a ramfs or not is to stat the fs.
However, when the ramfs is a tmpfs then the kernel will report back
tmpfs. So if you statfs for ramfs and get tmpfs back you still need to
go on and parse mountinfo to find out whether you are on a ramfs or not.
I think we can remove this warning here.

> know what you're trying to say :)
>
> Do you think there's any point in enforcing that the filename be 'net' ?

We can't really. lxc-user-nic is not sending /proc/<pid>/ns/net but
rather sends us /proc/<pid>/fd/<nr> which refers to a network namespace
that was preserved at container startup time. If we wanted to enforce
that the filename is "net" we'd need to start making bind-mounts which
leads to a whole set of other problems.
One thing we could do is to do a readlink() after we've verified that
the O_PATH fd refers to a nsfs. But setns() will be called as
setns(<fd>, CLONE_NEWNET) anyway and thus will fail with EINVAL when the
fd does refers to a non-network namespace nsfd. So there's no benefit.

> (i.e. to avoid playing any sorts of games with a userns) Probably not
> because is_privileged_over_netns() will trip over it in its
> setns(netns_fd, CLONE_NEWNET) in that case, right?

Exactly. See the end of my comment right above. :)
I'll update the patch to remove the WARN(). The rest will stay
the same.

Thanks!
Christian

>
> --
> You received this bug notification because you are a member of Ubuntu
> Container Security team, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1783591
>
> Title:
> lxc-user-nic allows unprivileged users to open arbitrary files
>
> Status in lxc package in Ubuntu:
> Triaged
> Status in lxc source package in Xenial:
> Triaged
> Status in lxc source package in Bionic:
> Triaged
> Status in lxc source package in Cosmic:
> Triaged
>
> Bug description:
> Matthias Gerstner from SUSE reported the following:
>
> ```
> Hello,
>
> following the lxc security reporting guidelines [1] I am reporting a
> finding in the lxc-user-nic setuid binary. I'm encrypting this mail as a
> best practice and because I found valid GPG keys for all of your
> adresses. Please find my public key attached to this mail.
>
> In the context of an openSUSE security audit of the lxc-user-nic setuid
> binary [2] (currently private bug) I came across an issue that should be
> adressed. In the "delete" case the program runs the following piece of
> code unconditionally with effective uid 0 (from lxc_user_nic.c):
>
> ```
> } else if (request == LXC_USERNIC_DELETE) {
> netns_fd = open(args.pid, O_RDONLY);
> if (netns_fd < 0) {
> usernic_error("Could not open \"%s\": %s\n", args.pid,
> strerror(errno));
> exit(EXIT_FAILURE);
> }
> }
> ```
>
> `args.pid` is a user controlled parameter and ...

Here's the updated patch that removes the WARN() from fhas_fs_type().
This is a non-functional and the only change.

Matthias, Serge: can you review the updated patch?

I reviewed the patch once more. Since it's only a minor change it still looks good to me.

lgtm, thanks.

Cool, thanks everyone! :)

CVE paperwork:

Vulnerability type: Incorrect access control
Vendor: LXC
Products:
 - LXC 2.0
 - LXC 3.0
Vendor acknowledged: yes
Attack type: local
Impact: Information disclosure
Affected components: lxc-user-nic
Attack vectors: Manually passing a PID path to a non-namespace path
Description:
  lxc-user-nic (setuid) when asked to delete a network interface will
  unconditionally open a user provided path.

  This code path may be used by an unprivileged user to check for
  the existence of a path which they wouldn't otherwise be able to reach.

  It may also be used to trigger side effects by causing a (read-only) open
  of special kernel files (ptmx, proc, sys).
References:
 - https://bugs.launchpad.net/ubuntu/+source/lxc/+bug/1783591
 - https://bugzilla.suse.com/show_bug.cgi?id=988348
Credits: Matthias Gerstner from SUSE

CRD to be set for Monday 6th of August at 16:00 UTC.

Version of the patch for stable-2.0 branch.
It does not contain any changes. It only just adjusts the path
in the diff from src/lxc/cmd/lxc_user_nic.c to src/lxc/lxc_user_nic.c

Once the CVE numbers are assigned the patches should both
reference them. So before pushing them upstream, please add
a reference either in the title of the commit message or the
body of the commit message.

Updated CVE paperwork:

Vulnerability type: Incorrect access control
Vendor: LXC
Products:
 - LXC 2.0
 - LXC 3.0
Vendor acknowledged: yes
Attack type: local
Impact: Information disclosure
Affected components: lxc-user-nic
Attack vectors: Manually passing a path that does not refer to a network namespace
Description:
  lxc-user-nic (setuid) when asked to delete a network interface will
  unconditionally open a user provided path.

  This code path may be used by an unprivileged user to check for
  the existence of a path which they wouldn't otherwise be able to reach.

  It may also be used to trigger side effects by causing a (read-only) open
  of special kernel files (ptmx, proc, sys).
References:
 - https://bugs.launchpad.net/ubuntu/+source/lxc/+bug/1783591
 - https://bugzilla.suse.com/show_bug.cgi?id=988348
Credits: Matthias Gerstner from SUSE

master: CVE 2018-6556: verify netns fd in lxc-user-nic

stable-3.0: CVE 2018-6556: verify netns fd in lxc-user-nic

stable-2.0: CVE 2018-6556: verify netns fd in lxc-user-nic

@Security team: We're now ready with this one. Can you send notifications to the distros list about this issue, including the 3 patches above (master, 2.0 and 3.0) and communicate a CRD of Monday 6th of August at 16:00 UTC?

Thanks!

I have sent the following to the linux-distros list:

A security issue has been discovered in LXC.
We have assigned CVE-2018-6556 to the issue.

The CRD is 2018-08-06 16:00:00 UTC.

Please contact me in private if you are shipping LXC and wish to gain access
to the related bug and patches.

Packages ready in our private PPAs:

 - ppa:ubuntu-lxc-security/devirt-security1
   - 3.0.1-0ubuntu2 => cosmic
   - 3.0.1-0ubuntu1~18.04.2 => bionic-security
 - ppa:ubuntu-lxc-security/devirt-security-backports
   - 3.0.1-0ubuntu1~16.04.1 => xenial-backports

All built with nothing else in the respective PPAs, built for all arches with debug symbols.
So they're all ready for direct copies into the respective pockets when we hit the CRD.

Does that PPA build with -updates disabled?

Also, xenial has 2.0.8, isn't that affected also?

@Marc, our security PPAs build with only -security enabled, except for the backports one which builds with -backports enabled (obviously).

My comment above was actually wrong, 2.0.9 and higher of LXC 2.0.x is affected, since we haven't yet had time to push that bugfix release to Ubuntu users, they are unaffected.

@Christian: Something is weird with the stable-2.0 patch. It applies cleanly, but doesn't compile because it refers to LXC_PROC_PID_FD_LEN which doesn't exist and isn't added to the patch, so the code doesn't compile. I can fix the build by adding this line in utils.h:

#define LXC_PROC_PID_FD_LEN (6 + LXC_NUMSTRLEN64 + 4 + LXC_NUMSTRLEN64 + 1)

On Sun, Aug 05, 2018 at 03:19:45PM -0000, Thomas Deutschmann wrote:
> @Christian: Something is weird with the stable-2.0 patch. It applies
> cleanly, but doesn't compile because it refers to LXC_PROC_PID_FD_LEN
> which doesn't exist and isn't added to the patch, so the code doesn't
> compile. I can fix the build by adding this line in utils.h:
>
> #define LXC_PROC_PID_FD_LEN (6 + LXC_NUMSTRLEN64 + 4 + LXC_NUMSTRLEN64 +
> 1)

Yeah, this was in my backport queue. I just cherry picked the patch to
support LXC_PROC_PID_FD_LEN to stable-2.0 now:

https://github.com/lxc/lxc/commit/f96f5f3c1341e73ee51c8b49bef4ba571c562d8c

So it should work fine now.

Thanks for the report!
Christian

>
> --
> You received this bug notification because you are a member of Ubuntu
> Container Security team, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1783591
>
> Title:
> lxc-user-nic allows unprivileged users to open arbitrary files
>
> Status in lxc package in Ubuntu:
> Triaged
> Status in lxc source package in Xenial:
> Triaged
> Status in lxc source package in Bionic:
> Triaged
> Status in lxc source package in Cosmic:
> Triaged
>
> Bug description:
> Matthias Gerstner from SUSE reported the following:
>
> ```
> Hello,
>
> following the lxc security reporting guidelines [1] I am reporting a
> finding in the lxc-user-nic setuid binary. I'm encrypting this mail as a
> best practice and because I found valid GPG keys for all of your
> adresses. Please find my public key attached to this mail.
>
> In the context of an openSUSE security audit of the lxc-user-nic setuid
> binary [2] (currently private bug) I came across an issue that should be
> adressed. In the "delete" case the program runs the following piece of
> code unconditionally with effective uid 0 (from lxc_user_nic.c):
>
> ```
> } else if (request == LXC_USERNIC_DELETE) {
> netns_fd = open(args.pid, O_RDONLY);
> if (netns_fd < 0) {
> usernic_error("Could not open \"%s\": %s\n", args.pid,
> strerror(errno));
> exit(EXIT_FAILURE);
> }
> }
> ```
>
> `args.pid` is a user controlled parameter and can be an arbitrary path
> at the moment. Nothing is done with this file descriptor later on in the
> program except an attempt at `setns(fd, CLONE_NEWNET)` in
> `is_privileged_over_netns()`. Still this allows the unprivileged caller
> of the setuid binary to achieve the following:
>
> - it can test for existence of files normally not accessible to the
> caller (information leak). Example:
> ```
> # this file is existing
> $ /usr/lib/lxc/lxc-user-nic delete path name /root/.bash_history type bridge nic
> lxc_user_nic.c: 1017: is_privileged_over_netns: Failed to setns() to network namespace Invalid argument
> lxc_user_nic.c: 1161: main: Process is not privileged over network namespace
>
> # this file is not existing
> $ /usr/lib/lxc/lxc-user-nic delete path name /root/.zsh_history type bridge nic
> lxc_user_nic.c: 1130: main: Could not...

@Thomas, in case you want to cherry pick this into another distro I'm appending you the patch for stable-2.0 here. :)

Christian

Thanks, we (Gentoo Linux) are now prepared and are waiting for the end of the embargo.

This bug was fixed in the package lxc - 3.0.1-0ubuntu1~18.04.2

---------------
lxc (3.0.1-0ubuntu1~18.04.2) bionic-security; urgency=medium

  * SECURITY UPDATE: lxc-user-nic allows for open() of arbitrary paths
    (LP: #1783591)
    - Ensure that the provided path is a netns reference
    - CVE-2018-6556

 -- Stéphane Graber <email address hidden> Wed, 01 Aug 2018 00:03:10 -0400

This bug was fixed in the package lxc - 3.0.1-0ubuntu2

---------------
lxc (3.0.1-0ubuntu2) cosmic; urgency=medium

  * SECURITY UPDATE: lxc-user-nic allows for open() of arbitrary paths
    (LP: #1783591)
    - Ensure that the provided path is a netns reference
    - CVE-2018-6556

 -- Stéphane Graber <email address hidden> Wed, 01 Aug 2018 00:03:10 -0400

Also released to xenial-backports.

New version to apply cleanly to master.

All supported upstream branches have been updated with the fix.

Upstream communication: https://lists.linuxcontainers.org/pipermail/lxc-devel/2018-August/018336.html

One can still test existence of files with those patches, but I guess this was explicitly not part of the fixes?

On Thu, Aug 30, 2018 at 08:02:56PM -0000, Salvatore Bonaccorso wrote:
> One can still test existence of files with those patches, but I guess
> this was explicitly not part of the fixes?

Is there a reproducer?
Yes, the open() can fail and we will report back to the user that the
open() failed but the user has no way of knowing why it failed since we
don't report the errno and stracing will strip the suid bit so you can't
get it from the strace and you also need to be root to strace this.

If you think that you have found an actual security bug please file it as a new one to follow best security practices.