ecryptfs: validate encoded packet length extent

[PATCH] ecryptfs: validate encoded packet length extent

Posted by Pengpeng Hou 1 week, 3 days ago

ecryptfs_miscdev_write() accepts a six-byte write as a minimally sized
message.  It then copies two bytes from the packet-length field at byte
offset five, although such an input supplies only the first byte.

Copy only the bytes covered by the current write.  When the first byte
selects the two-byte encoding, reject the message unless it also supplies
the second byte before calling ecryptfs_parse_packet_length().

Fixes: 8bf2debd5f7b ("eCryptfs: introduce device handle for userspace daemon communications")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 fs/ecryptfs/miscdev.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/ecryptfs/miscdev.c b/fs/ecryptfs/miscdev.c
index 5a7d08149922..a99e16db8243 100644
--- a/fs/ecryptfs/miscdev.c
+++ b/fs/ecryptfs/miscdev.c
@@ -360,7 +360,7 @@ ecryptfs_miscdev_write(struct file *file, const char __user *buf,
 	u32 seq;
 	size_t packet_size, packet_size_length;
 	char *data;
-	unsigned char packet_size_peek[ECRYPTFS_MAX_PKT_LEN_SIZE];
+	unsigned char packet_size_peek[ECRYPTFS_MAX_PKT_LEN_SIZE] = { 0 };
 	ssize_t rc;
 
 	if (count == 0) {
@@ -376,11 +376,17 @@ ecryptfs_miscdev_write(struct file *file, const char __user *buf,
 	}
 
 	if (copy_from_user(packet_size_peek, &buf[PKT_LEN_OFFSET],
-			   sizeof(packet_size_peek))) {
+			   min_t(size_t, count - PKT_LEN_OFFSET,
+				 sizeof(packet_size_peek)))) {
 		printk(KERN_WARNING "%s: Error while inspecting packet size\n",
 		       __func__);
 		return -EFAULT;
 	}
+	if (packet_size_peek[0] >= 192 && packet_size_peek[0] < 224 &&
+	    count - PKT_LEN_OFFSET < ECRYPTFS_MAX_PKT_LEN_SIZE) {
+		ecryptfs_printk(KERN_WARNING, "Truncated packet length\n");
+		return -EINVAL;
+	}
 
 	rc = ecryptfs_parse_packet_length(packet_size_peek, &packet_size,
 					  &packet_size_length);
-- 
2.43.0

Re: [PATCH] ecryptfs: validate encoded packet length extent

Posted by Tyler Hicks 1 week, 2 days ago

On 2026-07-15 16:37:02, Pengpeng Hou wrote:
> ecryptfs_miscdev_write() accepts a six-byte write as a minimally sized
> message.  It then copies two bytes from the packet-length field at byte
> offset five, although such an input supplies only the first byte.
> 
> Copy only the bytes covered by the current write.  When the first byte
> selects the two-byte encoding, reject the message unless it also supplies
> the second byte before calling ecryptfs_parse_packet_length().
> 
> Fixes: 8bf2debd5f7b ("eCryptfs: introduce device handle for userspace daemon communications")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

Hello and thank you for the fix! Some comments below...

> ---
>  fs/ecryptfs/miscdev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ecryptfs/miscdev.c b/fs/ecryptfs/miscdev.c
> index 5a7d08149922..a99e16db8243 100644
> --- a/fs/ecryptfs/miscdev.c
> +++ b/fs/ecryptfs/miscdev.c
> @@ -360,7 +360,7 @@ ecryptfs_miscdev_write(struct file *file, const char __user *buf,
>  	u32 seq;
>  	size_t packet_size, packet_size_length;
>  	char *data;
> -	unsigned char packet_size_peek[ECRYPTFS_MAX_PKT_LEN_SIZE];
> +	unsigned char packet_size_peek[ECRYPTFS_MAX_PKT_LEN_SIZE] = { 0 };
>  	ssize_t rc;
>  
>  	if (count == 0) {
> @@ -376,11 +376,17 @@ ecryptfs_miscdev_write(struct file *file, const char __user *buf,
>  	}
>  
>  	if (copy_from_user(packet_size_peek, &buf[PKT_LEN_OFFSET],
> -			   sizeof(packet_size_peek))) {
> +			   min_t(size_t, count - PKT_LEN_OFFSET,
> +				 sizeof(packet_size_peek)))) {

This is a good change and fixes a potential one-byte over read.

>  		printk(KERN_WARNING "%s: Error while inspecting packet size\n",
>  		       __func__);
>  		return -EFAULT;
>  	}
> +	if (packet_size_peek[0] >= 192 && packet_size_peek[0] < 224 &&
> +	    count - PKT_LEN_OFFSET < ECRYPTFS_MAX_PKT_LEN_SIZE) {
> +		ecryptfs_printk(KERN_WARNING, "Truncated packet length\n");
> +		return -EINVAL;
> +	}

I don't think that this new check is necessary. The call to
ecryptfs_parse_packet_length(), below, will already return a
packet_size_length of 2 if packet_size_peek[0] is >= 192.

The existing check, right after the call to
ecryptfs_parse_packet_length() is error checked, ensures that the value
of count is equal to 1 + 4 + 2 + packet_size. This already protects
against the truncated packet length issue that the new conditional is
checking. Do you agree?

Tyler

>  
>  	rc = ecryptfs_parse_packet_length(packet_size_peek, &packet_size,
>  					  &packet_size_length);
> -- 
> 2.43.0
>