Bug 260546
| Summary: | nfsv4_loadattr() can pass huge sizes to malloc() | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Robert Morris <rtm> | ||||
| Component: | kern | Assignee: | Rick Macklem <rmacklem> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Some People | CC: | emaste, pi, rmacklem | ||||
| Priority: | --- | Flags: | rmacklem:
mfc-stable13+
rmacklem: mfc-stable12+ |
||||
| Version: | CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
Created attachment 233181 [details]
add a sanity check for a large owner/Owner_group string
There is no upper limit for the length of an
Owner/Owner_group string specified in the RFCs.
As such, this patch uses a large (10K) sanity
limit. I will post on a FreeBSD mailing list
to try and get a better upper bound for a
user/group name.
However, any reasonable sanity limit should
fix this problem. I did Owner as well as
Owner_group, since they were both affected the
same way.
If the reporter has a way to test this, maybe
they can report back if the patch fixes the
problem for them?
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=ef4edb70c909fc2b1de867601c2230597d07daa0 commit ef4edb70c909fc2b1de867601c2230597d07daa0 Author: Rick Macklem <rmacklem@FreeBSD.org> AuthorDate: 2022-05-04 20:58:22 +0000 Commit: Rick Macklem <rmacklem@FreeBSD.org> CommitDate: 2022-05-04 20:58:22 +0000 nfsd: Add a sanity check for Owner/OwnerGroup string length Robert Morris reported that, if a client sends an absurdly large Owner/OwnerGroup string, the kernel malloc() for the large size string can block forever. This patch adds a sanity limit for Owner/OwnerGroup string length. Since the RFCs do not specify any limit and FreeBSD can handle a group name greater than 1Kbyte, the limit is set at a generous 10Kbytes. Reported by: rtm@lcs.mit.edu PR: 260546 MFC after: 2 weeks sys/fs/nfs/nfs.h | 7 +++++++ sys/fs/nfs/nfs_commonsubs.c | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) A patch similar to the attachment has been committed to main and will be MFC'd. A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=f01978f464cea1cffeb1a66a18f92d98380dd7f2 commit f01978f464cea1cffeb1a66a18f92d98380dd7f2 Author: Rick Macklem <rmacklem@FreeBSD.org> AuthorDate: 2022-05-04 20:58:22 +0000 Commit: Rick Macklem <rmacklem@FreeBSD.org> CommitDate: 2022-05-20 00:36:22 +0000 nfsd: Add a sanity check for Owner/OwnerGroup string length Robert Morris reported that, if a client sends an absurdly large Owner/OwnerGroup string, the kernel malloc() for the large size string can block forever. This patch adds a sanity limit for Owner/OwnerGroup string length. Since the RFCs do not specify any limit and FreeBSD can handle a group name greater than 1Kbyte, the limit is set at a generous 10Kbytes. PR: 260546 (cherry picked from commit ef4edb70c909fc2b1de867601c2230597d07daa0) sys/fs/nfs/nfs.h | 7 +++++++ sys/fs/nfs/nfs_commonsubs.c | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=c0ea059da22f0f1f20ee43db536a74032f140429 commit c0ea059da22f0f1f20ee43db536a74032f140429 Author: Rick Macklem <rmacklem@FreeBSD.org> AuthorDate: 2022-05-04 20:58:22 +0000 Commit: Rick Macklem <rmacklem@FreeBSD.org> CommitDate: 2022-05-20 00:43:22 +0000 nfsd: Add a sanity check for Owner/OwnerGroup string length Robert Morris reported that, if a client sends an absurdly large Owner/OwnerGroup string, the kernel malloc() for the large size string can block forever. This patch adds a sanity limit for Owner/OwnerGroup string length. Since the RFCs do not specify any limit and FreeBSD can handle a group name greater than 1Kbyte, the limit is set at a generous 10Kbytes. PR: 260546 (cherry picked from commit ef4edb70c909fc2b1de867601c2230597d07daa0) sys/fs/nfs/nfs.h | 7 +++++++ sys/fs/nfs/nfs_commonsubs.c | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) Patch has been committed and MFC'd. |
In the NFSATTRBIT_OWNERGROUP code in nfsv4_loadattr(): NFSM_DISSECT(tl, u_int32_t *, NFSX_UNSIGNED); j = fxdr_unsigned(int, *tl); if (j < 0) { error = NFSERR_BADXDR; goto nfsmout; } attrsum += (NFSX_UNSIGNED + NFSM_RNDUP(j)); if (j > NFSV4_SMALLSTR){ cp = malloc(j + 1, M_NFSSTRING, M_WAITOK); First, a big j can cause this code to allocate a few GB of memory, enough to cause my modest test machine to run out. Second, the "if (j < 0)" along with the "j + 1" means that if the RPC message contains j = 0x7fffffff, malloc will be called with (size_t)(j+1) = (size_t)(0x80000000) which is 0xffffffff80000000 if size_t is unsigned and 64 bits. This seems to cause malloc() to never return on my machine.