Skip to content

Commit

Permalink
CVE-2019-14196: nfs: fix unbounded memcpy with a failed length check …
Browse files Browse the repository at this point in the history 
…at nfs_lookup_reply

This patch adds a check to rpc_pkt.u.reply.data at nfs_lookup_reply.

Signed-off-by: Cheng Liu <liucheng32@huawei.com>
Reported-by: Fermín Serna <fermin@semmle.com>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>
  • Loading branch information
@ChengLiuPHX @jhershbe
ChengLiuPHX authored and jhershbe committed Sep 4, 2019
1 parent cf3a4f1 commit 5d14ee4
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions net/nfs.c
View file
Expand Up @@ -566,11 +566,15 @@ 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 > NFS3_FHSIZE)
filefh3_length = NFS3_FHSIZE;
if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + filefh3_length) > len)
return -NFS_RPC_DROP;
memcpy(filefh, rpc_pkt.u.reply.data + 2, filefh3_length);
}

Expand Down

2 comments on commit 5d14ee4

@zi0Black
Copy link

@zi0Black zi0Black commented on 5d14ee4 May 10, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
The following fix (lines 576-577) is an ineffective mitigation for the CVE-2019-14196:

if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + filefh3_length) > len)
    return -NFS_RPC_DROP;

The result of the following expression is always 24, ((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) . Then we can modify the code as follows for demonstration purposes (some comments in line).

filefh3_length = ntohl(rpc_pkt.u.reply.data[1]); //attacker controlled data, we know this from CVE-2019-14196

if (filefh3_length > NFS3_FHSIZE)
    filefh3_length  = NFS3_FHSIZE;

if ( (24 + filefh3_length) > len) //filefh3_length is declared as int so it can represent negative values, If an attacker supplies a negative number big enought in filefh3_length it could bypass this "security" check
    return -NFS_RPC_DROP;

Assuming an attacker has bypassed the current sanity check, his negative number will land as the third argument of the function memcpy (line 578). It will be cast to an unsigned int leading to a buffer overflow if the data copied is more than the size of the destination buffer filefh. The destination buffer filefh is a global one and can hold up to 64 bytes.

The proper fix for this vulnerability is to declare filefh3_length as an unsigned int and not as an int so that it cannot represent negative numbers.

Xeno Kovah (@XenoKovah) found this bypass and is presented as a case study in the course "Vulnerabilities 1001 - OST2". After dynamically verifying the bypass, I am merely reporting the discovery (tested with some dirty code that I hope would never see the sun's light).

@carnil
Copy link

@carnil carnil commented on 5d14ee4 May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, The following fix (lines 576-577) is an ineffective mitigation for the CVE-2019-14196:

if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) + filefh3_length) > len)
    return -NFS_RPC_DROP;

The result of the following expression is always 24, ((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) . Then we can modify the code as follows for demonstration purposes (some comments in line).

filefh3_length = ntohl(rpc_pkt.u.reply.data[1]); //attacker controlled data, we know this from CVE-2019-14196

if (filefh3_length > NFS3_FHSIZE)
    filefh3_length  = NFS3_FHSIZE;

if ( (24 + filefh3_length) > len) //filefh3_length is declared as int so it can represent negative values, If an attacker supplies a negative number big enought in filefh3_length it could bypass this "security" check
    return -NFS_RPC_DROP;

Assuming an attacker has bypassed the current sanity check, his negative number will land as the third argument of the function memcpy (line 578). It will be cast to an unsigned int leading to a buffer overflow if the data copied is more than the size of the destination buffer filefh. The destination buffer filefh is a global one and can hold up to 64 bytes.

The proper fix for this vulnerability is to declare filefh3_length as an unsigned int and not as an int so that it cannot represent negative numbers.

Xeno Kovah (@XenoKovah) found this bypass and is presented as a case study in the course "Vulnerabilities 1001 - OST2". After dynamically verifying the bypass, I am merely reporting the discovery (tested with some dirty code that I hope would never see the sun's light).

CVE-2022-30767 is associated with this incorrect fix for CVE-2019-14196.

Please sign in to comment.