Buffer overflow when script file is set to /dev/stdin

Open

Jacoblightningopened 3mo ago

Version

irecovery 1.3.1-5-g04d04f7 (readline)

Crashing command

irecovery -e /dev/stdin

Expected behavior

reads a script from stdin

Actual behavior

buffer overflows

Log

sudo irecovery -e /dev/stdin
*** buffer overflow detected ***: terminated
Aborted                    sudo irecovery -e /dev/stdin

Did I miss anything?

Activity

Jacoblightning2mo ago

I've looked into this more and the issue occurs in buffer_read_from_filename (irecovery.c) when the input "file" is a named pipe (or unnamed and the input is /dev/stdin).

This happens because the output value of these are not checked and both fseek and ftell fail on /dev/stdin with an ILLEGAL_SEEK.

When ftell fails, it returns -1 and, since this is not checked, it is interpreted as a valid file size (an unsigned 64-bit int) (so 0xffffffffffffffff).

This is obviously not 0 so the check on line 127 is false and the program continues to the malloc.

Since malloc is called with size+1 (to allocate an extra byte for the null byte), the size overflows and wraps to 0. This (on glibc) results in "a unique pointer value that can later be successfully passed to free()". Presumably a 0-size buffer but it doesn't really matter.

The fread on the next line will then try to read 0xffffffffffffffff bytes from the named pipe. This could theoretically result in writing arbitrary data to the heap but this was not occurring in my tests where the fread function just returned 0 and the stream error bit was set.

The buffer overflow ends up occurring after the function returns: buffer_length is set to 0xffffffffffffffff and used to index into buffer which has a size of 0.

Jacoblightning2mo ago

I was able to fix this issue using the following patch which adds checks to the functions in buffer_read_from_filename. I can make a pull request if you want.

diff --git a/tools/irecovery.c b/tools/irecovery.c
index b293324..c721df7 100644
--- a/tools/irecovery.c
+++ b/tools/irecovery.c
@@ -120,17 +120,33 @@ static void buffer_read_from_filename(const char *filename, char **buffer, uint6
 		return;
 	}
 
-	fseek(f, 0, SEEK_END);
+	if (fseek(f, 0, SEEK_END) == -1) {
+		fclose(f);
+		return;
+	}
 	size = ftell(f);
+	if (size == (uint64_t)-1) {
+		fclose(f);
+		return;
+	}
 	rewind(f);
 
-	if (size == 0) {
+	if (size == 0 || sizeof(char)*(size+1) == 0) {
 		fclose(f);
 		return;
 	}
 
 	*buffer = (char*)malloc(sizeof(char)*(size+1));
-	fread(*buffer, sizeof(char), size, f);
+	if (*buffer == NULL) {
+		fclose(f);
+		return;
+	}
+	if (fread(*buffer, sizeof(char), size, f) < size && ferror(f)) {
+		*buffer = NULL;
+		fclose(f);
+		return;
+	}
+
 	fclose(f);
 
 	*length = size;