Date: Fri, 15 Aug 2014 02:55:12 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: freebsd-arch@freebsd.org Cc: Robert Watson <rwatson@FreeBSD.org>, Johan Schuijt <johan@transip.nl>, Konstantin Belousov <kib@FreeBSD.org> Subject: [PATCH 2/2] Use sequence counters to make sure fget_unlocked gets a file pointers in sync with its capabilities. Message-ID: <1408064112-573-3-git-send-email-mjguzik@gmail.com> In-Reply-To: <1408064112-573-1-git-send-email-mjguzik@gmail.com> References: <1408064112-573-1-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Otherwise there are exploitable races (e.g. with dup2) which can be used
to circumvent protection.
---
sys/kern/kern_descrip.c | 55 ++++++++++++++++++++++++++++++++++++++++++-------
sys/sys/filedesc.h | 16 ++++++++++++++
2 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 4aeb380..1033b2f 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -309,11 +309,18 @@ _fdfree(struct filedesc *fdp, int fd, int last)
struct filedescent *fde;
fde = &fdp->fd_ofiles[fd];
+#ifdef CAPABILITIES
+ if (!last)
+ seq_write_begin(&fde->fde_seq);
+#endif
filecaps_free(&fde->fde_caps);
if (last)
return;
- bzero(fde, sizeof(*fde));
+ bzero(fde_change(fde), fde_change_size);
fdunused(fdp, fd);
+#ifdef CAPABILITIES
+ seq_write_end(&fde->fde_seq);
+#endif
}
static inline void
@@ -902,13 +909,19 @@ do_dup(struct thread *td, int flags, int old, int new,
/*
* Duplicate the source descriptor.
*/
+#ifdef CAPABILITIES
+ seq_write_begin(&newfde->fde_seq);
+#endif
filecaps_free(&newfde->fde_caps);
- *newfde = *oldfde;
+ memcpy(fde_change(newfde), fde_change(oldfde), fde_change_size);
filecaps_copy(&oldfde->fde_caps, &newfde->fde_caps);
if ((flags & DUP_CLOEXEC) != 0)
newfde->fde_flags = oldfde->fde_flags | UF_EXCLOSE;
else
newfde->fde_flags = oldfde->fde_flags & ~UF_EXCLOSE;
+#ifdef CAPABILITIES
+ seq_write_end(&newfde->fde_seq);
+#endif
*retval = new;
if (delfp != NULL) {
@@ -1776,6 +1789,9 @@ finstall(struct thread *td, struct file *fp, int *fd, int flags,
}
fhold(fp);
fde = &fdp->fd_ofiles[*fd];
+#ifdef CAPABILITIES
+ seq_write_begin(&fde->fde_seq);
+#endif
fde->fde_file = fp;
if ((flags & O_CLOEXEC) != 0)
fde->fde_flags |= UF_EXCLOSE;
@@ -1783,6 +1799,9 @@ finstall(struct thread *td, struct file *fp, int *fd, int flags,
filecaps_move(fcaps, &fde->fde_caps);
else
filecaps_fill(&fde->fde_caps);
+#ifdef CAPABILITIES
+ seq_write_end(&fde->fde_seq);
+#endif
FILEDESC_XUNLOCK(fdp);
return (0);
}
@@ -2315,6 +2334,7 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
struct file *fp;
u_int count;
#ifdef CAPABILITIES
+ seq_t seq;
cap_rights_t haverights;
int error;
#endif
@@ -2338,7 +2358,12 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
*/
for (;;) {
#ifdef CAPABILITIES
+ seq = seq_read(fd_seq(fdp, fd));
fde = fdt->fdt_ofiles[fd];
+ if (!seq_consistent(fd_seq(fdp, fd), seq)) {
+ cpu_spinwait();
+ continue;
+ }
fp = fde.fde_file;
#else
fp = fdt->fdt_ofiles[fd].fde_file;
@@ -2724,6 +2749,7 @@ int
dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode,
int openerror, int *indxp)
{
+ struct filedescent *newfde, *oldfde;
struct file *fp;
int error, indx;
@@ -2767,17 +2793,32 @@ dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode,
return (EACCES);
}
fhold(fp);
- fdp->fd_ofiles[indx] = fdp->fd_ofiles[dfd];
- filecaps_copy(&fdp->fd_ofiles[dfd].fde_caps,
- &fdp->fd_ofiles[indx].fde_caps);
+ newfde = &fdp->fd_ofiles[indx];
+ oldfde = &fdp->fd_ofiles[dfd];
+#ifdef CAPABILITIES
+ seq_write_begin(&newfde->fde_seq);
+#endif
+ memcpy(fde_change(newfde), fde_change(oldfde), fde_change_size);
+ filecaps_copy(&oldfde->fde_caps, &newfde->fde_caps);
+#ifdef CAPABILITIES
+ seq_write_end(&newfde->fde_seq);
+#endif
break;
case ENXIO:
/*
* Steal away the file pointer from dfd and stuff it into indx.
*/
- fdp->fd_ofiles[indx] = fdp->fd_ofiles[dfd];
- bzero(&fdp->fd_ofiles[dfd], sizeof(fdp->fd_ofiles[dfd]));
+ newfde = &fdp->fd_ofiles[indx];
+ oldfde = &fdp->fd_ofiles[dfd];
+#ifdef CAPABILITIES
+ seq_write_begin(&newfde->fde_seq);
+#endif
+ memcpy(fde_change(newfde), fde_change(oldfde), fde_change_size);
+ bzero(fde_change(oldfde), fde_change_size);
fdunused(fdp, dfd);
+#ifdef CAPABILITIES
+ seq_write_end(&newfde->fde_seq);
+#endif
break;
}
FILEDESC_XUNLOCK(fdp);
diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
index 29cb6ae..a7f7737 100644
--- a/sys/sys/filedesc.h
+++ b/sys/sys/filedesc.h
@@ -33,11 +33,14 @@
#ifndef _SYS_FILEDESC_H_
#define _SYS_FILEDESC_H_
+#include "opt_capsicum.h"
+
#include <sys/caprights.h>
#include <sys/queue.h>
#include <sys/event.h>
#include <sys/lock.h>
#include <sys/priority.h>
+#include <sys/seq.h>
#include <sys/sx.h>
#include <machine/_limits.h>
@@ -50,6 +53,9 @@ struct filecaps {
};
struct filedescent {
+#ifdef CAPABILITIES
+ seq_t fde_seq; /* if you need fde_file and fde_caps in sync */
+#endif
struct file *fde_file; /* file structure for open file */
struct filecaps fde_caps; /* per-descriptor rights */
uint8_t fde_flags; /* per-process open file flags */
@@ -58,6 +64,13 @@ struct filedescent {
#define fde_fcntls fde_caps.fc_fcntls
#define fde_ioctls fde_caps.fc_ioctls
#define fde_nioctls fde_caps.fc_nioctls
+#ifdef CAPABILITIES
+#define fde_change(fde) ((char *)(fde) + sizeof(seq_t))
+#define fde_change_size (sizeof(struct filedescent) - sizeof(seq_t))
+#else
+#define fde_change(fde) ((fde))
+#define fde_change_size (sizeof(struct filedescent))
+#endif
struct fdescenttbl {
int fdt_nfiles; /* number of open files allocated */
@@ -88,6 +101,9 @@ struct filedesc {
};
#define fd_nfiles fd_files->fdt_nfiles
#define fd_ofiles fd_files->fdt_ofiles
+#ifdef CAPABILITIES
+#define fd_seq(fdp, fd) (&(fdp)->fd_ofiles[(fd)].fde_seq)
+#endif
/*
* Structure to keep track of (process leader, struct fildedesc) tuples.
--
2.0.2
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1408064112-573-3-git-send-email-mjguzik>