3 files changed, 46 insertions, 10 deletions

diff --git a/util/libreboot-utils/lib/file.c b/util/libreboot-utils/lib/file.c
index 4623748c..3620f425 100644
--- a/util/libreboot-utils/lib/file.c
+++ b/util/libreboot-utils/lib/file.c
@@ -691,6 +691,10 @@ rootfs(void)
 }
 
 /* filesystem sandboxing in userspace
+ * TODO:
+	missing length bound check.
+	potential CPU DoS on very long paths, spammed repeatedly.
+	perhaps cap at PATH_LEN?
  */
 int
 fs_resolve_at(int dirfd, const char *path, int flags)
@@ -754,6 +758,15 @@ err:
 	return -1;
 }
 
+/* NOTE:
+	rejects . and .. but not empty strings
+	after normalisation. edge case:
+	//////
+
+	normalised implicitly, but might be good
+	to add a defensive check regardless. code
+	probably not exploitable in current state.
+ */
 int
 fs_next_component(const char **p,
     char *name, size_t namesz)
diff --git a/util/libreboot-utils/lib/io.c b/util/libreboot-utils/lib/io.c
index a09e3992..eac6073e 100644
--- a/util/libreboot-utils/lib/io.c
+++ b/util/libreboot-utils/lib/io.c
@@ -4,6 +4,10 @@
  * I/O functions specific to nvmutil.
  */
 
+/* TODO: local tmpfiles not being deleted
+	when flags==O_RDONLY e.g. dump command
+ */
+
 #include <sys/types.h>
 #include <sys/stat.h>
 
diff --git a/util/libreboot-utils/lib/mkhtemp.c b/util/libreboot-utils/lib/mkhtemp.c
index b5b9aeeb..61479b25 100644
--- a/util/libreboot-utils/lib/mkhtemp.c
+++ b/util/libreboot-utils/lib/mkhtemp.c
@@ -206,8 +206,8 @@ env_tmpdir(int bypass_all_sticky_checks, char **tmpdir,
 	int allow_noworld_unsticky;
 	int saved_errno = errno;
 
-	char tmp[] = "/tmp";
-	char vartmp[] = "/var/tmp";
+	static const char tmp[] = "/tmp";
+	static const char vartmp[] = "/var/tmp";
 
 	/* tmpdir is a user override, if set */
 	if (override_tmpdir == NULL)
@@ -238,26 +238,26 @@ env_tmpdir(int bypass_all_sticky_checks, char **tmpdir,
 
 	allow_noworld_unsticky = 0;
 
-	if (world_writeable_and_sticky("/tmp",
+	if (world_writeable_and_sticky(tmp,
 	    allow_noworld_unsticky,
 	    bypass_all_sticky_checks)) {
 
 		if (tmpdir != NULL)
-			*tmpdir = tmp;
+			*tmpdir = (char *)tmp;
 
 		errno = saved_errno;
-		return "/tmp";
+		return (char *)tmp;
 	}
 
-	if (world_writeable_and_sticky("/var/tmp",
+	if (world_writeable_and_sticky(vartmp,
 	    allow_noworld_unsticky,
 	    bypass_all_sticky_checks)) {
 
 		if (tmpdir != NULL)
-			*tmpdir = vartmp;
+			*tmpdir = (char *)vartmp;
 
 		errno = saved_errno;
-		return "/var/tmp";
+		return (char *)vartmp;
 	}
 
 	return NULL;
@@ -327,11 +327,11 @@ same_dir(const char *a, const char *b)
 	if (rval_scmp == 0)
 		goto success_same_dir;
 
-	fd_a = fs_open(a, O_RDONLY | O_DIRECTORY);
+	fd_a = fs_open(a, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
 	if (fd_a < 0)
 		goto err_same_dir;
 
-	fd_b = fs_open(b, O_RDONLY | O_DIRECTORY);
+	fd_b = fs_open(b, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
 	if (fd_b < 0)
 		goto err_same_dir;
 
@@ -513,6 +513,17 @@ sticky_hell:
  * file ownership under hardened mode
  * (only lets you touch your own files/dirs)
  */
+/*
+ TODO:
+	some variables e.g. template vs suffix,
+	assumes they match.
+	we should test this explicitly,
+	but the way this is called is
+	currently safe - this would however
+	be nice for future library use
+	by outside projects.
+	this whole code needs to be reorganised
+*/
 int
 mkhtemp(int *fd,
     struct stat *st,
@@ -821,6 +832,12 @@ err:
 }
 #endif
 
+/* TODO: potential infinite loop under entropy failure.
+ * e.g. keeps returning low quality RNG, or atacker
+ * has control (DoS attack potential).
+ * possible solution: add a timeout (and abort if
+ * the timeout is reached)
+ */
 int
 mkhtemp_fill_random(char *p, size_t xc)
 {
@@ -900,6 +917,8 @@ int secure_file(int *fd,
 		if (lock_file(*fd, flags) == -1)
 			goto err_demons;
 
+		/* TODO: why would this be NULL? audit
+		 * to find out. we should always verify! */
 		if (expected != NULL)
 			if (fd_verify_identity(*fd, expected, &st_now) < 0)
 				goto err_demons;