Skip to contentSkip to conversation

liblkid: Add length check in probe_nilfs2 before crc32

PATCH Applied Torsten Hilbrich 2 participants 3 messages Jun 17, 2016 – Jun 24, 2016 Util-Linux package

lore
Share & tools
lore

Torsten Hilbrich

author
#0 lore raw

PATCH 1/1 liblkid: Add length check in probe_nilfs2 before crc32

The bytes variable is read from the file system to probe and must be checked before used as length parameter in the crc32 call.

The following problems may occur here:

- bytes smaller than sumoff + 4: underflow in length calculation
- bytes larger than remaining space in sb: overflow of buffer

This fixes a problem where an encrypted volume had the correct magic values 0x3434 at offset 0x406 and the following uint16_t (which is read into the nilfs_super_block.s_bytes struct) was parsed as 1.

Then crc32 was called with the length value 18446744073709551597 causing a segmentation fault.

---
 libblkid/src/superblocks/nilfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
libblkid/src/superblocks/nilfs.c +8 -1
index d12472c..ff420cf 100644
@@ -72,6 +72,7 @@ static int nilfs_valid_sb(blkid_probe pr, struct nilfs_super_block *sb, int is_b
 	static unsigned char sum[4];
 	const int sumoff = offsetof(struct nilfs_super_block, s_sum);
 	size_t bytes;
+	size_t crc_offset = sumoff + 4;
 	uint32_t crc;
 
 	if (!sb || le16_to_cpu(sb->s_magic) != NILFS_SB_MAGIC)

@@ -82,9 +83,15 @@ static int nilfs_valid_sb(blkid_probe pr, struct nilfs_super_block *sb, int is_b
 		return 0;
 
 	bytes = le16_to_cpu(sb->s_bytes);
+	/* ensure that no underrun can happen in the length parameter
+	 * of the crc32 call or more data are processed than read into
+	 * sb */
+	if (bytes < crc_offset || bytes > sizeof(struct nilfs_super_block) - crc_offset)
+		return -1;
+
 	crc = crc32(le32_to_cpu(sb->s_crc_seed), (unsigned char *)sb, sumoff);
 	crc = crc32(crc, sum, 4);
-	crc = crc32(crc, (unsigned char *)sb + sumoff + 4, bytes - sumoff - 4);
+	crc = crc32(crc, (unsigned char *)sb + crc_offset, bytes - crc_offset);
 
 	return blkid_probe_verify_csum(pr, crc, le32_to_cpu(sb->s_sum));
 }
Signature · 1 line
2.1.4
author
#1 lore raw

I have feedback on my first version of the patch sent to the kernel maintainers, so
I'm sending you the result of this discussion as well.

The following changes were made:

- add const to the newly introduced variable
- renaming it to crc_start
- fix check for maximum size, bytes is the end of the area to crc32-check, not its length

Torsten

------PATCH starts here -------------------------------------------------------

The bytes variable is read from the file system to probe and must be checked before used as length parameter in the crc32 call.

The following problems may occur here:

- bytes smaller than sumoff + 4: underflow in length calculation
- bytes larger than remaining space in sb: overflow of buffer

This fixes a problem where an encrypted volume had the correct magic values 0x3434 at offset 0x406 and the following uint16_t (which is read into the nilfs_super_block.s_bytes struct) was parsed as 1.

Then crc32 was called with the length value 18446744073709551597 causing a segmentation fault.

---
 libblkid/src/superblocks/nilfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
libblkid/src/superblocks/nilfs.c +8 -1
index d12472c..fbafb8d 100644
@@ -72,6 +72,7 @@ static int nilfs_valid_sb(blkid_probe pr, struct nilfs_super_block *sb, int is_b
 	static unsigned char sum[4];
 	const int sumoff = offsetof(struct nilfs_super_block, s_sum);
 	size_t bytes;
+	const size_t crc_start = sumoff + 4;
 	uint32_t crc;
 
 	if (!sb || le16_to_cpu(sb->s_magic) != NILFS_SB_MAGIC)

@@ -82,9 +83,15 @@ static int nilfs_valid_sb(blkid_probe pr, struct nilfs_super_block *sb, int is_b
 		return 0;
 
 	bytes = le16_to_cpu(sb->s_bytes);
+	/* ensure that no underrun can happen in the length parameter
+	 * of the crc32 call or more data are processed than read into
+	 * sb */
+	if (bytes < crc_start || bytes > sizeof(struct nilfs_super_block))
+		return -1;
+
 	crc = crc32(le32_to_cpu(sb->s_crc_seed), (unsigned char *)sb, sumoff);
 	crc = crc32(crc, sum, 4);
-	crc = crc32(crc, (unsigned char *)sb + sumoff + 4, bytes - sumoff - 4);
+	crc = crc32(crc, (unsigned char *)sb + crc_start, bytes - crc_start);
 
 	return blkid_probe_verify_csum(pr, crc, le32_to_cpu(sb->s_sum));
 }
Signature · 1 line
2.1.4
maintainer Applied
#2 lore raw
Quoted 16 lines · Torsten Hilbrich
 libblkid/src/superblocks/nilfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
libblkid/src/superblocks/nilfs.c +1 -0
index d12472c..fbafb8d 100644
@@ -72,6 +72,7 @@ static int nilfs_valid_sb(blkid_probe pr, struct nilfs_super_block *sb, int is_b
 	static unsigned char sum[4];
 	const int sumoff = offsetof(struct nilfs_super_block, s_sum);
 	size_t bytes;
+	const size_t crc_start = sumoff + 4;
 	uint32_t crc;
 
 	if (!sb || le16_to_cpu(sb->s_magic) != NILFS_SB_MAGIC)

libblkid/src/superblocks/nilfs.c static int nilfs_valid_sb(blkid_probe pr, struct nilfs_super_block *sb, int is_b · line 90

Quoted 8 lines
libblkid/src/superblocks/nilfs.c +5 -0
@@ -82,9 +83,15 @@ static int nilfs_valid_sb(blkid_probe pr, struct nilfs_super_block *sb, int is_b
 		return 0;
 
 	bytes = le16_to_cpu(sb->s_bytes);
+	/* ensure that no underrun can happen in the length parameter
+	 * of the crc32 call or more data are processed than read into
+	 * sb */
+	if (bytes < crc_start || bytes > sizeof(struct nilfs_super_block))
+		return -1;

I guess that -1 is not expected return code there; nilfs_valid_sb() is used as boolean, see probe_nilfs2(). Fixed (replaced with zero).

Applied, thanks!

Karel

Signature · 2 lines
 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com