[PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check
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); }
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); }
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
participants (2)
-
gerbert -
Heinrich Schuchardt