nvram: bound the variable parser to the nvram partition

The "name=value\0 ... \0\0" walk over the mmap()ed nvram used strchr()
and strlen() without a bound, so an nvram without the terminating
double NUL made it run past the end of the mapping.

Search with memchr() up to the length from the nvram header, or up to
the end of the mapping when that length does not fit, so a broken
header is still readable. A variable that is not terminated in this
area ends the parsing.

Map a regular file, like the /tmp staging file, only as far as it is
backed, rounded down to 4 bytes for the padding in nvram_commit(),
instead of the partition size which raised SIGBUS. Only search the
start of a mapping smaller than NVRAM_MIN_SPACE for the magic instead
of underflowing the loop boundary; bcm47xxpart creates such nvram
partitions on flashes with 4 KiB erase blocks.

"nvram info" clamps the header length before the CRC8, it read beyond
the mapping or underflowed into a 4 GB read before.

This is reachable with a crafted nvram partition or staging file, on
bcm47xx the nvram partition is also parsed while booting.

Reported by @tonoyx91 and @0xROI.

Link: https://github.com/openwrt/openwrt/security/advisories/GHSA-5m25-qmhm-crr4
Fixes: 400c1a7fbf ("Add unvram, a nvram manipulation tool suitable for brcm-2.4 and bcm47xx platforms")
Assisted-by: Claude:claude-opus-5
Link: https://github.com/openwrt/openwrt/pull/25240
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

package/utils/nvram/Makefile

@@ -8,7 +8,7 @@
 include $(TOPDIR)/rules.mk
 
 PKG_NAME:=nvram
-PKG_RELEASE:=14
+PKG_RELEASE:=15
 
 PKG_BUILD_DIR := $(BUILD_DIR)/$(PKG_NAME)
 

package/utils/nvram/src/cli.c

@@ -107,10 +107,21 @@ static int do_set(nvram_handle_t *nvram, const char *pair)
 static int do_info(nvram_handle_t *nvram)
 {
 	nvram_header_t *hdr = nvram_header(nvram);
+	uint32_t len = hdr->len;
+	uint8_t crc;
+
+	/*
+	 * The length is taken from the nvram, do not use it to read beyond
+	 * the mapped partition or to underflow the calculations below.
+	 */
+	if (len > nvram->length - nvram->offset)
+		len = nvram->length - nvram->offset;
+	if (len < NVRAM_CRC_START_POSITION)
+		len = NVRAM_CRC_START_POSITION;
 
 	/* CRC8 over the last 11 bytes of the header and data bytes */
-	uint8_t crc = hndcrc8((unsigned char *) &hdr[0] + NVRAM_CRC_START_POSITION,
-		hdr->len - NVRAM_CRC_START_POSITION, 0xff);
+	crc = hndcrc8((unsigned char *) &hdr[0] + NVRAM_CRC_START_POSITION,
+		len - NVRAM_CRC_START_POSITION, 0xff);
 
 	/* Show info */
 	printf("Magic:         0x%08X\n",   hdr->magic);
@@ -127,8 +138,8 @@ static int do_info(nvram_handle_t *nvram)
 	printf("NCDL values:   0x%08X\n\n", hdr->config_ncdl);
 
 	printf("%i bytes used / %i bytes available (%.2f%%)\n",
-		hdr->len, nvram->length - nvram->offset - hdr->len,
-		(100.00 / (double)(nvram->length - nvram->offset)) * (double)hdr->len);
+		len, nvram->length - nvram->offset - len,
+		(100.00 / (double)(nvram->length - nvram->offset)) * (double)len);
 
 	return 0;
 }

package/utils/nvram/src/nvram.c

@@ -101,7 +101,7 @@ static nvram_tuple_t * _nvram_realloc( nvram_handle_t *h, nvram_tuple_t *t,
 static int _nvram_rehash(nvram_handle_t *h)
 {
 	nvram_header_t *header = nvram_header(h);
-	char buf[] = "0xXXXXXXXX", *name, *value, *eq;
+	char buf[] = "0xXXXXXXXX", *name, *value, *eq, *nul, *end;
 
 	/* (Re)initialize hash table */
 	_nvram_free(h);
@@ -109,13 +109,33 @@ static int _nvram_rehash(nvram_handle_t *h)
 	/* Parse and set "name=value\0 ... \0\0" */
 	name = (char *) &header[1];
 
-	for (; *name; name = value + strlen(value) + 1) {
-		if (!(eq = strchr(name, '=')))
+	/*
+	 * Stop at the end of the used area, but never look beyond the mapped
+	 * partition. A length which does not fit the partition is ignored to
+	 * still read the variables of a broken nvram.
+	 */
+	end = h->mmap + h->length;
+	if (header->len >= sizeof(nvram_header_t) &&
+	    header->len <= h->length - h->offset)
+		end = (char *) header + header->len;
+
+	while (name < end && *name) {
+		eq = memchr(name, '=', end - name);
+		if (!eq)
 			break;
-		*eq = '\0';
+
 		value = eq + 1;
+
+		/* The value has to be terminated within this area */
+		nul = memchr(value, '\0', end - value);
+		if (!nul)
+			break;
+
+		*eq = '\0';
 		nvram_set(h, name, value);
 		*eq = '=';
+
+		name = nul + 1;
 	}
 
 	/* Set special SDRAM parameters */
@@ -286,13 +306,13 @@ int nvram_commit(nvram_handle_t *h)
 		header->config_ncdl = strtoul(ncdl, NULL, 0);
 	}
 
-	/* Clear data area */
+	/* Clear data area, only the mapped part belongs to this handle */
 	ptr = (char *) header + sizeof(nvram_header_t);
-	memset(ptr, 0xFF, nvram_part_size - h->offset - sizeof(nvram_header_t));
+	memset(ptr, 0xFF, h->length - h->offset - sizeof(nvram_header_t));
 	memset(&tmp, 0, sizeof(nvram_header_t));
 
 	/* Leave space for a double NUL at the end */
-	end = (char *) header + nvram_part_size - h->offset - 2;
+	end = (char *) header + h->length - h->offset - 2;
 
 	/* Write out all tuples */
 	for (i = 0; i < NVRAM_ARRAYSIZE(h->nvram_hash); i++) {
@@ -345,6 +365,8 @@ nvram_handle_t * nvram_open(const char *file, int rdonly)
 	char *mtd = NULL;
 	nvram_handle_t *h;
 	nvram_header_t *header;
+	struct stat s;
+	size_t length, last;
 	int offset = -1;
 
 	/* If erase size or file are undefined then try to define them */
@@ -360,8 +382,31 @@ nvram_handle_t * nvram_open(const char *file, int rdonly)
 
 	if( (fd = open(file ? file : mtd, O_RDWR)) > -1 )
 	{
-		char *mmap_area = (char *) mmap(
-			NULL, nvram_part_size, PROT_READ | PROT_WRITE,
+		char *mmap_area;
+
+		/*
+		 * A regular file, like the staging file, can be shorter than
+		 * the nvram partition. Accessing the pages behind its end
+		 * would raise SIGBUS, only map what the file provides. The
+		 * size of a block device is not reported here, use the
+		 * partition size for it. nvram_commit() pads the data to
+		 * 4 bytes, keep the length a multiple of 4 for it.
+		 */
+		length = nvram_part_size;
+		if( fstat(fd, &s) > -1 && S_ISREG(s.st_mode) &&
+		    s.st_size < (off_t) length )
+			length = (size_t) s.st_size & ~(size_t) 3;
+
+		/* The header and the double NUL have to fit in */
+		if( length < sizeof(nvram_header_t) + 4 )
+		{
+			free(mtd);
+			close(fd);
+			return NULL;
+		}
+
+		mmap_area = (char *) mmap(
+			NULL, length, PROT_READ | PROT_WRITE,
 			(( rdonly == NVRAM_RO ) ? MAP_PRIVATE : MAP_SHARED) | MAP_LOCKED, fd, 0);
 
 		if( mmap_area != MAP_FAILED )
@@ -369,9 +414,16 @@ nvram_handle_t * nvram_open(const char *file, int rdonly)
 			/*
 			 * Start looking for NVRAM_MAGIC at beginning of MTD
 			 * partition. Stop if there is less than NVRAM_MIN_SPACE
-			 * to check, that was the lowest used size.
+			 * to check, that was the lowest used size. A smaller
+			 * partition, like the one bcm47xxpart creates on a
+			 * flash with 4 KiB erase blocks, can only have the
+			 * NVRAM at its start.
 			 */
-			for( i = 0; i <= ((nvram_part_size - NVRAM_MIN_SPACE) / sizeof(uint32_t)); i++ )
+			last = 0;
+			if( length >= NVRAM_MIN_SPACE )
+				last = (length - NVRAM_MIN_SPACE) / sizeof(uint32_t);
+
+			for( i = 0; i <= last; i++ )
 			{
 				if( ((uint32_t *)mmap_area)[i] == NVRAM_MAGIC )
 				{
@@ -382,7 +434,7 @@ nvram_handle_t * nvram_open(const char *file, int rdonly)
 
 			if( offset < 0 )
 			{
-				munmap(mmap_area, nvram_part_size);
+				munmap(mmap_area, length);
 				free(mtd);
 				close(fd);
 				return NULL;
@@ -392,7 +444,7 @@ nvram_handle_t * nvram_open(const char *file, int rdonly)
 			{
 				h->fd     = fd;
 				h->mmap   = mmap_area;
-				h->length = nvram_part_size;
+				h->length = length;
 				h->offset = offset;
 
 				header = nvram_header(h);