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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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);
>       }
>