Skip to content

Commit 16af238

Browse files
Christian Braunerstgraber
Christian Brauner
authored andcommitted
CVE-2017-5985: Ensure target netns is caller-owned
Before this commit, lxc-user-nic could potentially have been tricked into operating on a network namespace over which the caller did not hold privilege. This commit ensures that the caller is privileged over the network namespace by temporarily dropping privilege. Launchpad: https://bugs.launchpad.net/ubuntu/+source/lxc/+bug/1654676 Reported-by: Jann Horn <jannh@google.com> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
1 parent 7c58306 commit 16af238

File tree

1 file changed

+87
-32
lines changed

1 file changed

+87
-32
lines changed

Diff for: src/lxc/lxc_user_nic.c

+87-32
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,14 @@
5050
#include "utils.h"
5151
#include "network.h"
5252

53+
#define usernic_debug_stream(stream, format, ...) \
54+
do { \
55+
fprintf(stream, "%s: %d: %s: " format, __FILE__, __LINE__, \
56+
__func__, __VA_ARGS__); \
57+
} while (false)
58+
59+
#define usernic_error(format, ...) usernic_debug_stream(stderr, format, __VA_ARGS__)
60+
5361
static void usage(char *me, bool fail)
5462
{
5563
fprintf(stderr, "Usage: %s lxcpath name pid type bridge nicname\n", me);
@@ -670,68 +678,115 @@ static bool create_db_dir(char *fnam)
670678
}
671679

672680
#define VETH_DEF_NAME "eth%d"
673-
674681
static int rename_in_ns(int pid, char *oldname, char **newnamep)
675682
{
676-
int fd = -1, ofd = -1, ret, ifindex = -1;
683+
uid_t ruid, suid, euid;
684+
int fret = -1;
685+
int fd = -1, ifindex = -1, ofd = -1, ret;
677686
bool grab_newname = false;
678687

679688
ofd = lxc_preserve_ns(getpid(), "net");
680689
if (ofd < 0) {
681-
fprintf(stderr, "Failed opening network namespace path for '%d'.", getpid());
682-
return -1;
690+
usernic_error("Failed opening network namespace path for '%d'.", getpid());
691+
return fret;
683692
}
684693

685694
fd = lxc_preserve_ns(pid, "net");
686695
if (fd < 0) {
687-
fprintf(stderr, "Failed opening network namespace path for '%d'.", pid);
688-
return -1;
696+
usernic_error("Failed opening network namespace path for '%d'.", pid);
697+
goto do_partial_cleanup;
698+
}
699+
700+
ret = getresuid(&ruid, &euid, &suid);
701+
if (ret < 0) {
702+
usernic_error("Failed to retrieve real, effective, and saved "
703+
"user IDs: %s\n",
704+
strerror(errno));
705+
goto do_partial_cleanup;
706+
}
707+
708+
ret = setns(fd, CLONE_NEWNET);
709+
close(fd);
710+
fd = -1;
711+
if (ret < 0) {
712+
usernic_error("Failed to setns() to the network namespace of "
713+
"the container with PID %d: %s.\n",
714+
pid, strerror(errno));
715+
goto do_partial_cleanup;
689716
}
690717

691-
if (setns(fd, 0) < 0) {
692-
fprintf(stderr, "setns to container network namespace\n");
693-
goto out_err;
718+
ret = setresuid(ruid, ruid, 0);
719+
if (ret < 0) {
720+
usernic_error("Failed to drop privilege by setting effective "
721+
"user id and real user id to %d, and saved user "
722+
"ID to 0: %s.\n",
723+
ruid, strerror(errno));
724+
// COMMENT(brauner): It's ok to jump to do_full_cleanup here
725+
// since setresuid() will succeed when trying to set real,
726+
// effective, and saved to values they currently have.
727+
goto do_full_cleanup;
694728
}
695-
close(fd); fd = -1;
729+
696730
if (!*newnamep) {
697731
grab_newname = true;
698732
*newnamep = VETH_DEF_NAME;
699-
if (!(ifindex = if_nametoindex(oldname))) {
700-
fprintf(stderr, "failed to get netdev index\n");
701-
goto out_err;
733+
734+
ifindex = if_nametoindex(oldname);
735+
if (!ifindex) {
736+
usernic_error("Failed to get netdev index: %s.\n", strerror(errno));
737+
goto do_full_cleanup;
702738
}
703739
}
704-
if ((ret = lxc_netdev_rename_by_name(oldname, *newnamep)) < 0) {
705-
fprintf(stderr, "Error %d renaming netdev %s to %s in container\n", ret, oldname, *newnamep);
706-
goto out_err;
740+
741+
ret = lxc_netdev_rename_by_name(oldname, *newnamep);
742+
if (ret < 0) {
743+
usernic_error("Error %d renaming netdev %s to %s in container.\n", ret, oldname, *newnamep);
744+
goto do_full_cleanup;
707745
}
746+
708747
if (grab_newname) {
709-
char ifname[IFNAMSIZ], *namep = ifname;
748+
char ifname[IFNAMSIZ];
749+
char *namep = ifname;
750+
710751
if (!if_indextoname(ifindex, namep)) {
711-
fprintf(stderr, "Failed to get new netdev name\n");
712-
goto out_err;
752+
usernic_error("Failed to get new netdev name: %s.\n", strerror(errno));
753+
goto do_full_cleanup;
713754
}
755+
714756
*newnamep = strdup(namep);
715757
if (!*newnamep)
716-
goto out_err;
758+
goto do_full_cleanup;
717759
}
718-
if (setns(ofd, 0) < 0) {
719-
fprintf(stderr, "Error returning to original netns\n");
720-
close(ofd);
721-
return -1;
760+
761+
fret = 0;
762+
763+
do_full_cleanup:
764+
ret = setresuid(ruid, euid, suid);
765+
if (ret < 0) {
766+
usernic_error("Failed to restore privilege by setting effective "
767+
"user id to %d, real user id to %d, and saved user "
768+
"ID to %d: %s.\n",
769+
ruid, euid, suid, strerror(errno));
770+
fret = -1;
771+
// COMMENT(brauner): setns() should fail if setresuid() doesn't
772+
// succeed but there's no harm in falling through; keeps the
773+
// code cleaner.
722774
}
723-
close(ofd);
724775

725-
return 0;
776+
ret = setns(ofd, CLONE_NEWNET);
777+
if (ret < 0) {
778+
usernic_error("Failed to setns() to original network namespace "
779+
"of PID %d: %s.\n",
780+
ofd, strerror(errno));
781+
fret = -1;
782+
}
726783

727-
out_err:
728-
if (ofd >= 0)
729-
close(ofd);
730-
if (setns(ofd, 0) < 0)
731-
fprintf(stderr, "Error returning to original network namespace\n");
784+
do_partial_cleanup:
732785
if (fd >= 0)
733786
close(fd);
734-
return -1;
787+
close(ofd);
788+
789+
return fret;
735790
}
736791

737792
/*

0 commit comments

Comments
 (0)