libselinux: restorecon: pin file to avoid TOCTOU issues
Pin the file to operate on in restorecon_sb() to prevent symlink attacks
in between the label database lookup, the current context query and the
final context write. Also don't use the file information from
fts_read(3), which might also be out of sync.
Due to querying file information twice, one in fts_read(3) needed for
the cross device check and one on the pinned file descriptor for the
database lookup, there is a slight slowdown:
[current]
Time (mean ± σ): 14.456 s ± 0.306 s [User: 45.863 s, System: 4.463 s]
Range (min … max): 14.275 s … 15.294 s 10 runs
[changed]
Time (mean ± σ): 15.843 s ± 0.045 s [User: 46.274 s, System: 9.495 s]
Range (min … max): 15.787 s … 15.916 s 10 runs
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Acked-by: James Carter <jwcart2@gmail.com>
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 42ef30c..12b8510 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -621,13 +621,13 @@
return rc;
}
-static int restorecon_sb(const char *pathname, const struct stat *sb,
- struct rest_flags *flags, bool first)
+static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool first)
{
char *newcon = NULL;
char *curcon = NULL;
char *newtypecon = NULL;
- int rc;
+ int fd = -1, rc;
+ struct stat stat_buf;
bool updated = false;
const char *lookup_path = pathname;
float pc;
@@ -642,13 +642,21 @@
lookup_path += rootpathlen;
}
+ fd = open(pathname, O_PATH | O_NOFOLLOW | O_EXCL);
+ if (fd < 0)
+ goto err;
+
+ rc = fstat(fd, &stat_buf);
+ if (rc < 0)
+ goto err;
+
if (rootpath != NULL && lookup_path[0] == '\0')
/* this is actually the root dir of the alt root. */
rc = selabel_lookup_raw(fc_sehandle, &newcon, "/",
- sb->st_mode);
+ stat_buf.st_mode);
else
rc = selabel_lookup_raw(fc_sehandle, &newcon, lookup_path,
- sb->st_mode);
+ stat_buf.st_mode);
if (rc < 0) {
if (errno == ENOENT) {
@@ -657,10 +665,10 @@
"Warning no default label for %s\n",
lookup_path);
- return 0; /* no match, but not an error */
+ goto out; /* no match, but not an error */
}
- return -1;
+ goto err;
}
if (flags->progress) {
@@ -680,19 +688,17 @@
}
if (flags->add_assoc) {
- rc = filespec_add(sb->st_ino, newcon, pathname, flags);
+ rc = filespec_add(stat_buf.st_ino, newcon, pathname, flags);
if (rc < 0) {
selinux_log(SELINUX_ERROR,
"filespec_add error: %s\n", pathname);
- freecon(newcon);
- return -1;
+ goto out1;
}
if (rc > 0) {
/* Already an association and it took precedence. */
- freecon(newcon);
- return 0;
+ goto out;
}
}
@@ -700,7 +706,7 @@
selinux_log(SELINUX_INFO, "%s matched by %s\n",
pathname, newcon);
- if (lgetfilecon_raw(pathname, &curcon) < 0) {
+ if (fgetfilecon_raw(fd, &curcon) < 0) {
if (errno != ENODATA)
goto err;
@@ -733,7 +739,7 @@
}
if (!flags->nochange) {
- if (lsetfilecon(pathname, newcon) < 0)
+ if (fsetfilecon(fd, newcon) < 0)
goto err;
updated = true;
}
@@ -758,6 +764,8 @@
out:
rc = 0;
out1:
+ if (fd >= 0)
+ close(fd);
freecon(curcon);
freecon(newcon);
return rc;
@@ -855,7 +863,6 @@
FTSENT *ftsent;
int error;
char ent_path[PATH_MAX];
- struct stat ent_st;
bool first = false;
if (state->parallel)
@@ -953,11 +960,11 @@
/* fall through */
default:
strcpy(ent_path, ftsent->fts_path);
- ent_st = *ftsent->fts_statp;
+
if (state->parallel)
pthread_mutex_unlock(&state->mutex);
- error = restorecon_sb(ent_path, &ent_st, &state->flags,
+ error = restorecon_sb(ent_path, &state->flags,
first);
if (state->parallel) {
@@ -1153,7 +1160,7 @@
goto cleanup;
}
- error = restorecon_sb(pathname, &sb, &state.flags, true);
+ error = restorecon_sb(pathname, &state.flags, true);
goto cleanup;
}
