[PATCH] initramfs: avoid filename buffer overrun

[PATCH] initramfs: avoid filename buffer overrun

[David Disseldorp]

The initramfs filename field is defined in
Documentation/driver-api/early-userspace/buffer-format.rst as:

 37 cpio_file := ALGN(4) + cpio_header + filename + "\0" + ALGN(4) + data
...
 55 ============= ================== =========================
 56 Field name    Field size         Meaning
 57 ============= ================== =========================
...
 70 c_namesize    8 bytes            Length of filename, including final \0

When extracting an initramfs cpio archive, the kernel's do_name() path
handler assumes a zero-terminated path at @collected, passing it
directly to filp_open() / init_mkdir() / init_mknod().

If a specially crafted cpio entry carries a non-zero-terminated filename
and is followed by uninitialized memory, then a file may be created with
trailing characters that represent the uninitialized memory. Symlink
filename fields handled in do_symlink() won't overrun past the data
segment, due to the explicit zero-termination of the symlink target.

Append the output of the following bash script to an existing initramfs
and observe any created /initramfs_test_fname_overrunAA* path. E.g.
  ./reproducer.sh | gzip >> /myinitramfs

It's easiest to observe non-zero uninitialized memory when the output is
gzipped, as it'll overflow the heap allocated @out_buf in __gunzip(),
rather than the initrd_start+initrd_size block.

---- reproducer.sh ----
nilchar="A"	# change to "\0" to properly zero terminate / pad
magic="070701"
ino=1
mode=$(( 0100777 ))
uid=0
gid=0
nlink=1
mtime=1
filesize=0
devmajor=0
devminor=1
rdevmajor=0
rdevminor=0
csum=0
fname="initramfs_test_fname_overrun"
namelen=$(( ${#fname} + 1 ))	# plus one to account for terminator

printf "%s%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%s" \
	$magic $ino $mode $uid $gid $nlink $mtime $filesize \
	$devmajor $devminor $rdevmajor $rdevminor $namelen $csum $fname

termpadlen=$(( 1 + ((4 - ((110 + $namelen) & 3)) % 4) ))
printf "%.s${nilchar}" $(seq 1 $termpadlen)
---- reproducer.sh ----

Fix filename buffer overrun by skipping over any cpio entries where the
field doesn't carry a zero-terminator at the expected (name_len - 1)
offset.

Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index bc911e466d5bb..a44386bcbb566 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -360,6 +360,14 @@ static int __init do_name(void)
 {
 	state = SkipIt;
 	next_state = Reset;
+
+	/* name_len > 0 && name_len <= PATH_MAX checked in do_header */
+	if (collected[name_len - 1] != '\0') {
+		pr_err("Skipping name without nulterm: %.*s\n",
+		       (int)name_len, collected);
+		return 0;
+	}
+
 	if (strcmp(collected, "TRAILER!!!") == 0) {
 		free_hash();
 		return 0;
@@ -424,13 +432,19 @@ static int __init do_copy(void)
 
 static int __init do_symlink(void)
 {
+	state = SkipIt;
+	next_state = Reset;
+
+	if (collected[name_len - 1] != '\0') {
+		pr_err("Skipping symlink without nulterm: %.*s\n",
+		       (int)name_len, collected);
+		return 0;
+	}
 	collected[N_ALIGN(name_len) + body_len] = '\0';
 	clean_path(collected, 0);
 	init_symlink(collected + N_ALIGN(name_len), collected);
 	init_chown(collected, uid, gid, AT_SYMLINK_NOFOLLOW);
 	do_utime(collected, mtime);
-	state = SkipIt;
-	next_state = Reset;
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH] initramfs: avoid filename buffer overrun

[Al Viro]

On Tue, Oct 29, 2024 at 12:48:37PM +0000, David Disseldorp wrote:

> When extracting an initramfs cpio archive, the kernel's do_name() path
> handler assumes a zero-terminated path at @collected, passing it
> directly to filp_open() / init_mkdir() / init_mknod().
> 
> If a specially crafted cpio entry carries

... /linuxrc with whatever code they want to run as root before anything
else in userland, you are already FUBAR.

> a non-zero-terminated filename
> and is followed by uninitialized memory, then a file may be created with
> trailing characters that represent the uninitialized memory. Symlink
> filename fields handled in do_symlink() won't overrun past the data
> segment, due to the explicit zero-termination of the symlink target.

... or that.
 
> Fix filename buffer overrun by skipping over any cpio entries where the
> field doesn't carry a zero-terminator at the expected (name_len - 1)
> offset.
> 
> Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
> Signed-off-by: David Disseldorp <ddiss@suse.de>

[snip]

> +	if (collected[name_len - 1] != '\0') {
> +		pr_err("Skipping symlink without nulterm: %.*s\n",
> +		       (int)name_len, collected);

I'm not sure pr_err() and continue is a good approach here -
you'd been given a corrupted image, so there's no point trying
to do anything further with it.  Have it return 1, at least,
and preferably use error("buggered symlink") in addition or
instead of your pr_err().

FWIW, it's _not_ about trying to stop an attack - if you get there with
image contents controlled by attacker, you have already hopelessly lost;
no buffer overruns are needed.

It does catch corrupted images, which is the right thing to do, but it's
not a security issue.

Re: [PATCH] initramfs: avoid filename buffer overrun

[David Disseldorp]

On Tue, 29 Oct 2024 18:35:20 +0000, Al Viro wrote:

> On Tue, Oct 29, 2024 at 12:48:37PM +0000, David Disseldorp wrote:
...
> > +	if (collected[name_len - 1] != '\0') {
> > +		pr_err("Skipping symlink without nulterm: %.*s\n",
> > +		       (int)name_len, collected);  
> 
> I'm not sure pr_err() and continue is a good approach here -
> you'd been given a corrupted image, so there's no point trying
> to do anything further with it.  Have it return 1, at least,
> and preferably use error("buggered symlink") in addition or
> instead of your pr_err().

I was following the name_len > PATH_MAX handling, but failing
immediately makes more sense here. Will change in v2.

> FWIW, it's _not_ about trying to stop an attack - if you get there with
> image contents controlled by attacker, you have already hopelessly lost;
> no buffer overruns are needed.
> 
> It does catch corrupted images, which is the right thing to do, but it's
> not a security issue.

Agreed. I'll rework the commit message to more clearly state that
initramfs image write access is required, at which point all bets are
off.