pledge: wl_paths: resolvpath() logic error

prepend chroot value *after* canonization and not before.

1 files changed, 73 insertions, 64 deletions

diff --git a/sys/kern/kern_pledge.c b/sys/kern/kern_pledge.c
index 1906c9f77b6..9d20c7f1784 100644
--- a/sys/kern/kern_pledge.c
+++ b/sys/kern/kern_pledge.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_pledge.c,v 1.156 2016/03/15 15:05:23 semarie Exp $	*/
+/*	$OpenBSD: kern_pledge.c,v 1.157 2016/03/15 15:10:09 semarie Exp $	*/
 
 /*
  * Copyright (c) 2015 Nicholas Marriott <nicm@openbsd.org>
@@ -1627,43 +1627,12 @@ resolvpath(struct proc *p,
     char **resolved, size_t *resolvedlen)
 {
 	int error;
-	char *fullpath = NULL, *rawcanopath = NULL, *canopath = NULL;
-	size_t fullpathlen, rawcanopathlen, canopathlen;
+	char *abspath = NULL, *canopath = NULL, *fullpath = NULL;
+	size_t abspathlen, canopathlen, fullpathlen, canopathlen_exact;
 
-	/* 1. get rdir if chrooted */
-	if (p->p_fd->fd_rdir != NULL) {
-		if (*rdir == NULL) {
-			char *rawrdir, *bp, *bpend;
-			size_t rawrdirlen = MAXPATHLEN * 4;
-
-			rawrdir = malloc(rawrdirlen, M_TEMP, M_WAITOK);
-			bp = &rawrdir[rawrdirlen];
-			bpend = bp;
-			*(--bp) = '\0';
-
-			error = vfs_getcwd_common(p->p_fd->fd_rdir,
-			    rootvnode, &bp, rawrdir, rawrdirlen/2,
-			    GETCWD_CHECK_ACCESS, p);
-			if (error) {
-				free(rawrdir, M_TEMP, rawrdirlen);
-				goto out;
-			}
-
-			/* NUL is included */
-			*rdirlen = (bpend - bp);
-			*rdir = malloc(*rdirlen, M_TEMP, M_WAITOK);
-			memcpy(*rdir, bp, *rdirlen);
-
-			free(rawrdir, M_TEMP, rawrdirlen);
-		}
-	} else {
-		if (*rdir == NULL)
-			*rdirlen = 0;	/* ensure rdirlen value is initialized */
-	}
-
-	/* 2. resolv: path -> fullpath */
+	/* 1. get an absolute path (inside any chroot) : path -> abspath */
 	if (path[0] != '/') {
-		/* path is relative: prepend cwd (rootvnode based) */
+		/* path is relative: prepend cwd */
 
 		/* get cwd first (if needed) */
 		if (*cwd == NULL) {
@@ -1676,7 +1645,7 @@ resolvpath(struct proc *p,
 			*(--bp) = '\0';
 
 			error = vfs_getcwd_common(p->p_fd->fd_cdir,
-			    rootvnode, &bp, rawcwd, rawcwdlen/2,
+			    NULL, &bp, rawcwd, rawcwdlen/2,
 			    GETCWD_CHECK_ACCESS, p);
 			if (error) {
 				free(rawcwd, M_TEMP, rawcwdlen);
@@ -1692,44 +1661,84 @@ resolvpath(struct proc *p,
 		}
 
 		/* NUL included in *cwdlen and pathlen */
-		fullpathlen = *cwdlen + pathlen;
-		fullpath = malloc(fullpathlen, M_TEMP, M_WAITOK);
-		snprintf(fullpath, fullpathlen, "%s/%s", *cwd, path);
-
-	} else if (p->p_fd->fd_rdir) {
-		/* path is absolute and we are chrooted : prepend *rdir */
-
-		/* NUL included in *rdirlen and pathlen (and no '/' between them) */
-		fullpathlen = *rdirlen + pathlen - 1;
-		fullpath = malloc(fullpathlen, M_TEMP, M_WAITOK);
-		snprintf(fullpath, fullpathlen, "%s%s", *rdir, path);
+		abspathlen = *cwdlen + pathlen;
+		abspath = malloc(abspathlen, M_TEMP, M_WAITOK);
+		snprintf(abspath, abspathlen, "%s/%s", *cwd, path);
 
 	} else {
 		/* path is absolute */
-		fullpathlen = pathlen;
-		fullpath = malloc(fullpathlen, M_TEMP, M_WAITOK);
-		memcpy(fullpath, path, pathlen);
+		abspathlen = pathlen;
+		abspath = malloc(abspathlen, M_TEMP, M_WAITOK);
+		memcpy(abspath, path, pathlen);
 	}
 
-	/* 3. canonization: fullpath -> rawcanopath */
-	rawcanopathlen = fullpathlen;
-	rawcanopath = malloc(rawcanopathlen, M_TEMP, M_WAITOK);
-	error = canonpath(fullpath, rawcanopath, rawcanopathlen);
+	/* 2. canonization: abspath -> canopath */
+	canopathlen = abspathlen;
+	canopath = malloc(canopathlen, M_TEMP, M_WAITOK);
+	error = canonpath(abspath, canopath, canopathlen);
+
+	/* free abspath now as we don't need it after */
+	free(abspath, M_TEMP, abspathlen);
+
+	/* error in canonpath() call (should not happen, but keep safe) */
 	if (error != 0)
 		goto out;
 
-	/* 4. output a fresh allocated path: rawcanopath -> canopath */
-	canopathlen = strlen(rawcanopath) + 1; /* NUL */
-	canopath = malloc(canopathlen, M_TEMP, M_WAITOK);
-	memcpy(canopath, rawcanopath, canopathlen);
+	/* check the canopath size */
+	canopathlen_exact = strlen(canopath) + 1;
+	if (canopathlen_exact > MAXPATHLEN) {
+		error = ENAMETOOLONG;
+		goto out;
+	}
+
+	/* 3. preprend *rdir if chrooted : canonpath -> fullpath */
+	if (p->p_fd->fd_rdir != NULL) {
+		if (*rdir == NULL) {
+			char *rawrdir, *bp, *bpend;
+			size_t rawrdirlen = MAXPATHLEN * 4;
+
+			rawrdir = malloc(rawrdirlen, M_TEMP, M_WAITOK);
+			bp = &rawrdir[rawrdirlen];
+			bpend = bp;
+			*(--bp) = '\0';
+
+			error = vfs_getcwd_common(p->p_fd->fd_rdir,
+			    rootvnode, &bp, rawrdir, rawrdirlen/2,
+			    GETCWD_CHECK_ACCESS, p);
+			if (error) {
+				free(rawrdir, M_TEMP, rawrdirlen);
+				goto out;
+			}
+
+			/* NUL is included */
+			*rdirlen = (bpend - bp);
+			*rdir = malloc(*rdirlen, M_TEMP, M_WAITOK);
+			memcpy(*rdir, bp, *rdirlen);
+
+			free(rawrdir, M_TEMP, rawrdirlen);
+		}
+
+		/*
+		 * NUL is included in *rdirlen and canopathlen_exact.
+		 * doesn't add "/" between them, as canopath is absolute.
+		 */
+		fullpathlen = *rdirlen + canopathlen_exact - 1;
+		fullpath = malloc(fullpathlen, M_TEMP, M_WAITOK);
+		snprintf(fullpath, fullpathlen, "%s%s", *rdir, canopath);
+
+	} else {
+		/* not chrooted: only reduce canopath to exact length */
+		fullpathlen = canopathlen_exact;
+		fullpath = malloc(fullpathlen, M_TEMP, M_WAITOK);
+		memcpy(fullpath, canopath, fullpathlen);
+	}
 
-	*resolvedlen = canopathlen;
-	*resolved = canopath;
+	*resolvedlen = fullpathlen;
+	*resolved = fullpath;
 
 out:
-	free(rawcanopath, M_TEMP, rawcanopathlen);
-	free(fullpath, M_TEMP, fullpathlen);
+	free(canopath, M_TEMP, canopathlen);
 	if (error != 0)
-		free(canopath, M_TEMP, canopathlen);
+		free(fullpath, M_TEMP, fullpathlen);
 	return error;
 }