[PATCH] liblkid: Add length check in probe_nilfs2 before crc32

[PATCH] liblkid: Add length check in probe_nilfs2 before crc32

[Torsten Hilbrich]

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(-)

diff --git a/libblkid/src/superblocks/nilfs.c b/libblkid/src/superblocks/nilfs.c
index d12472c..ff420cf 100644
--- a/libblkid/src/superblocks/nilfs.c
+++ b/libblkid/src/superblocks/nilfs.c
@@ -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));
 }
-- 
2.1.4

Re: [PATCHv2] liblkid: Add length check in probe_nilfs2 before crc32

[Torsten Hilbrich]

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(-)

diff --git a/libblkid/src/superblocks/nilfs.c b/libblkid/src/superblocks/nilfs.c
index d12472c..fbafb8d 100644
--- a/libblkid/src/superblocks/nilfs.c
+++ b/libblkid/src/superblocks/nilfs.c
@@ -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));
 }
-- 
2.1.4

Re: [PATCHv2] liblkid: Add length check in probe_nilfs2 before crc32

[Karel Zak]

On Mon, Jun 20, 2016 at 07:09:10AM +0200, Torsten Hilbrich wrote:
>  libblkid/src/superblocks/nilfs.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/libblkid/src/superblocks/nilfs.c b/libblkid/src/superblocks/nilfs.c
> index d12472c..fbafb8d 100644
> --- a/libblkid/src/superblocks/nilfs.c
> +++ b/libblkid/src/superblocks/nilfs.c
> @@ -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;

 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

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com