[PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check

[PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check

[gerbert]

This patch tries to fix a CVE-2019-14196 fix

   In if-condition, where NFSV2_FLAG is checked, memcpy call is performed
to transfer a reply data of NFS_FHSIZE size. Since the data field in
struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
NFS_FHSIZE is only 32, it won't lead to out-of-bounds write (considering
the size of data array won't change in the future).

   What concerns if-condition for NFSV3_FLAG, since filefh3_length is
signed integer, it may carry negative values which may lead to memcpy
failure, so in this case we need to introduce not only boundary check
(filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
filefh3_length is not negative.

Signed-off-by: gerbert <gerbert@users.noreply.github.com>
---
  net/nfs.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/nfs.c b/net/nfs.c
index 9152ab742e..5186130ea9 100644
--- a/net/nfs.c
+++ b/net/nfs.c
@@ -566,13 +566,13 @@ static int nfs_lookup_reply(uchar *pkt, unsigned 
len)
      }

      if (supported_nfs_versions & NFSV2_FLAG) {
-        if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) 
+ NFS_FHSIZE) > len)
-            return -NFS_RPC_DROP;
          memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
      } else {  /* NFSV3_FLAG */
          filefh3_length = ntohl(rpc_pkt.u.reply.data[1]);
+        if (filefh3_length < 0)
+            return -NFS_RPC_DROP;
          if (filefh3_length > NFS3_FHSIZE)
-            filefh3_length  = NFS3_FHSIZE;
+            filefh3_length = NFS3_FHSIZE;
          memcpy(filefh, rpc_pkt.u.reply.data + 2, filefh3_length);
      }

-- 
2.32.0

Re: [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check

[Heinrich Schuchardt]

On 6/2/22 20:32, gerbert wrote:
> This patch tries to fix a CVE-2019-14196 fix
>
>    In if-condition, where NFSV2_FLAG is checked, memcpy call is performed
> to transfer a reply data of NFS_FHSIZE size. Since the data field in
> struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
> NFS_FHSIZE is only 32, it won't lead to out-of-bounds write (considering
> the size of data array won't change in the future).
>
>    What concerns if-condition for NFSV3_FLAG, since filefh3_length is
> signed integer, it may carry negative values which may lead to memcpy
> failure, so in this case we need to introduce not only boundary check
> (filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
> filefh3_length is not negative.
>
> Signed-off-by: gerbert <gerbert@users.noreply.github.com>
> ---
>   net/nfs.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/nfs.c b/net/nfs.c
> index 9152ab742e..5186130ea9 100644
> --- a/net/nfs.c
> +++ b/net/nfs.c
> @@ -566,13 +566,13 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len)
>       }
>
>       if (supported_nfs_versions & NFSV2_FLAG) {
> -        if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt)
> + NFS_FHSIZE) > len)
> -            return -NFS_RPC_DROP;
>           memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
>       } else {  /* NFSV3_FLAG */
>           filefh3_length = ntohl(rpc_pkt.u.reply.data[1]);
> +        if (filefh3_length < 0)

This is the definition:

static unsigned int filefh3_length
The value cannot be negative.

Cf.
bdbf7a05e26f3c5 ("net: nfs: Fix CVE-2022-30767 (old CVE-2019-14196)")


> +            return -NFS_RPC_DROP;
>           if (filefh3_length > NFS3_FHSIZE)
> -            filefh3_length  = NFS3_FHSIZE;
> +            filefh3_length = NFS3_FHSIZE;

This seems to be an unrelated change.

Best regards

Heinrich

>           memcpy(filefh, rpc_pkt.u.reply.data + 2, filefh3_length);
>       }
>

Re: [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check

[gerbert]

Heinrich Schuchardt писал 2022-06-04 20:44:
> On 6/2/22 20:32, gerbert wrote:
>> This patch tries to fix a CVE-2019-14196 fix
>> 
>>    In if-condition, where NFSV2_FLAG is checked, memcpy call is 
>> performed
>> to transfer a reply data of NFS_FHSIZE size. Since the data field in
>> struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
>> NFS_FHSIZE is only 32, it won't lead to out-of-bounds write 
>> (considering
>> the size of data array won't change in the future).
>> 
>>    What concerns if-condition for NFSV3_FLAG, since filefh3_length is
>> signed integer, it may carry negative values which may lead to memcpy
>> failure, so in this case we need to introduce not only boundary check
>> (filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
>> filefh3_length is not negative.
>> 
>> Signed-off-by: gerbert <gerbert@users.noreply.github.com>
>> ---
>>   net/nfs.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/net/nfs.c b/net/nfs.c
>> index 9152ab742e..5186130ea9 100644
>> --- a/net/nfs.c
>> +++ b/net/nfs.c
>> @@ -566,13 +566,13 @@ static int nfs_lookup_reply(uchar *pkt, unsigned 
>> len)
>>       }
>> 
>>       if (supported_nfs_versions & NFSV2_FLAG) {
>> -        if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar 
>> *)(&rpc_pkt)
>> + NFS_FHSIZE) > len)
>> -            return -NFS_RPC_DROP;
>>           memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
>>       } else {  /* NFSV3_FLAG */
>>           filefh3_length = ntohl(rpc_pkt.u.reply.data[1]);
>> +        if (filefh3_length < 0)
> 
> This is the definition:
> 
> static unsigned int filefh3_length
> The value cannot be negative.
That's right. It's unsigned now. Unfortunately I was looking into the 
version
I was using in the project, which is, 2021.01. A bit outdated and has 
this field
as signed integer. That's why I was thinking that this should work.

Meanwhile, I then think that this part can be removed:
>> -        if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar 
>> *)(&rpc_pkt)
>> + NFS_FHSIZE) > len)

as

>>           memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
won't leave the boundary of 'rpc_pkt.u.reply.data + 1' due to the size 
of NFS_FHSIZE.

> 
> Cf.
> bdbf7a05e26f3c5 ("net: nfs: Fix CVE-2022-30767 (old CVE-2019-14196)")
> 
> 
>> +            return -NFS_RPC_DROP;
>>           if (filefh3_length > NFS3_FHSIZE)
>> -            filefh3_length  = NFS3_FHSIZE;
>> +            filefh3_length = NFS3_FHSIZE;
> 
> This seems to be an unrelated change.
Thought about a little cosmetics here.

> 
> Best regards
> 
> Heinrich
> 
>>           memcpy(filefh, rpc_pkt.u.reply.data + 2, filefh3_length);
>>       }
>> 
Kind regards,
Gerbert

[PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check

[gerbert]

This patch tries to fix a CVE-2019-14196 fix

   In if-condition, where NFSV2_FLAG is checked, memcpy call is performed
to transfer a reply data of NFS_FHSIZE size. Since the data field in
struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
NFS_FHSIZE is only 32, it won't lead to out-of-bounds write (considering
the size of data array won't change in the future).

   What concerns if-condition for NFSV3_FLAG, since filefh3_length is
signed integer, it may carry negative values which may lead to memcpy
failure, so in this case we need to introduce not only boundary check
(filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
filefh3_length is not negative.

Signed-off-by: gerbert <gerbert@users.noreply.github.com>
---
  net/nfs.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/nfs.c b/net/nfs.c
index 9152ab742e..5186130ea9 100644
--- a/net/nfs.c
+++ b/net/nfs.c
@@ -566,13 +566,13 @@ static int nfs_lookup_reply(uchar *pkt, unsigned 
len)
  	}

  	if (supported_nfs_versions & NFSV2_FLAG) {
-		if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + 
NFS_FHSIZE) > len)
-			return -NFS_RPC_DROP;
  		memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
  	} else {  /* NFSV3_FLAG */
  		filefh3_length = ntohl(rpc_pkt.u.reply.data[1]);
+		if (filefh3_length < 0)
+			return -NFS_RPC_DROP;
  		if (filefh3_length > NFS3_FHSIZE)
-			filefh3_length  = NFS3_FHSIZE;
+			filefh3_length = NFS3_FHSIZE;
  		memcpy(filefh, rpc_pkt.u.reply.data + 2, filefh3_length);
  	}

-- 
2.32.0

Re: [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

On Thu, Jun 02, 2022 at 09:18:42PM +0300, gerbert wrote:

> This patch tries to fix a CVE-2019-14196 fix
> 
>   In if-condition, where NFSV2_FLAG is checked, memcpy call is performed
> to transfer a reply data of NFS_FHSIZE size. Since the data field in
> struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
> NFS_FHSIZE is only 32, it won't lead to out-of-bounds write (considering
> the size of data array won't change in the future).
> 
>   What concerns if-condition for NFSV3_FLAG, since filefh3_length is
> signed integer, it may carry negative values which may lead to memcpy
> failure, so in this case we need to introduce not only boundary check
> (filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
> filefh3_length is not negative.
> 
> Signed-off-by: gerbert <gerbert@users.noreply.github.com>

This has been addressed as:
https://patchwork.ozlabs.org/project/uboot/patch/20220518163103.372-1-zi0Black@protonmail.com/
and more clearly:
https://source.denx.de/u-boot/u-boot/-/commit/bdbf7a05e26f3c5fd437c99e2755ffde186ddc80
recently, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check

[gerbert]

Tom Rini писал 2022-06-06 17:43:
> On Thu, Jun 02, 2022 at 09:18:42PM +0300, gerbert wrote:
> 
>> This patch tries to fix a CVE-2019-14196 fix
>> 
>>   In if-condition, where NFSV2_FLAG is checked, memcpy call is 
>> performed
>> to transfer a reply data of NFS_FHSIZE size. Since the data field in
>> struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
>> NFS_FHSIZE is only 32, it won't lead to out-of-bounds write 
>> (considering
>> the size of data array won't change in the future).
>> 
>>   What concerns if-condition for NFSV3_FLAG, since filefh3_length is
>> signed integer, it may carry negative values which may lead to memcpy
>> failure, so in this case we need to introduce not only boundary check
>> (filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
>> filefh3_length is not negative.
>> 
>> Signed-off-by: gerbert <gerbert@users.noreply.github.com>
> 
> This has been addressed as:
> https://patchwork.ozlabs.org/project/uboot/patch/20220518163103.372-1-zi0Black@protonmail.com/
> and more clearly:
> https://source.denx.de/u-boot/u-boot/-/commit/bdbf7a05e26f3c5fd437c99e2755ffde186ddc80
> recently, thanks.
That's right. But as far as I can see if-condition for NFS v2 has the 
same leftover,
which was removed from v3 part. In this case I guess it can be removed.

Kind regards,
Gerbert