Skip to content

Commit 19d25dd

Browse files
guyharrisinfrastation
authored andcommitted
CVE-2017-12898/NFS: Fix bounds checking.
Fix the bounds checking for the NFSv3 WRITE procedure to check whether the length of the opaque data being written is present in the captured data, not just whether the byte count is present in the captured data. furthest forward in the packet, not the item before it. (This also lets us eliminate the check for the "stable" argument being present in the captured data; rewrite the code to print that to make it a bit clearer.) Check that the entire ar_stat field is present in the capture. Note that parse_wcc_attr() is called after we've already checked whether the wcc_data is present. Check before fetching the "access" part of the NFSv3 ACCESS results. This fixes a buffer over-read discovered by Kamil Frankowicz. Include a test for the "check before fetching the "access" part..." fix, using the capture supplied by the reporter(s).
1 parent 1dcd10a commit 19d25dd

File tree

4 files changed

+313
-6
lines changed

4 files changed

+313
-6
lines changed

Diff for: print-nfs.c

+7-6
Original file line numberDiff line numberDiff line change
@@ -628,17 +628,15 @@ nfsreq_print_noaddr(netdissect_options *ndo,
628628
if ((dp = parsereq(ndo, rp, length)) != NULL &&
629629
(dp = parsefh(ndo, dp, v3)) != NULL) {
630630
if (v3) {
631-
ND_TCHECK(dp[2]);
631+
ND_TCHECK(dp[4]);
632632
ND_PRINT((ndo, " %u (%u) bytes @ %" PRIu64,
633633
EXTRACT_32BITS(&dp[4]),
634634
EXTRACT_32BITS(&dp[2]),
635635
EXTRACT_64BITS(&dp[0])));
636636
if (ndo->ndo_vflag) {
637-
dp += 3;
638-
ND_TCHECK(dp[0]);
639637
ND_PRINT((ndo, " <%s>",
640638
tok2str(nfsv3_writemodes,
641-
NULL, EXTRACT_32BITS(dp))));
639+
NULL, EXTRACT_32BITS(&dp[3]))));
642640
}
643641
} else {
644642
ND_TCHECK(dp[3]);
@@ -1002,11 +1000,11 @@ parserep(netdissect_options *ndo,
10021000
* skip past the ar_verf credentials.
10031001
*/
10041002
dp += (len + (2*sizeof(uint32_t) + 3)) / sizeof(uint32_t);
1005-
ND_TCHECK2(dp[0], 0);
10061003

10071004
/*
10081005
* now we can check the ar_stat field
10091006
*/
1007+
ND_TCHECK(dp[0]);
10101008
astat = (enum sunrpc_accept_stat) EXTRACT_32BITS(dp);
10111009
if (astat != SUNRPC_SUCCESS) {
10121010
ND_PRINT((ndo, " %s", tok2str(sunrpc_str, "ar_stat %d", astat)));
@@ -1243,6 +1241,7 @@ static const uint32_t *
12431241
parse_wcc_attr(netdissect_options *ndo,
12441242
const uint32_t *dp)
12451243
{
1244+
/* Our caller has already checked this */
12461245
ND_PRINT((ndo, " sz %" PRIu64, EXTRACT_64BITS(&dp[0])));
12471246
ND_PRINT((ndo, " mtime %u.%06u ctime %u.%06u",
12481247
EXTRACT_32BITS(&dp[2]), EXTRACT_32BITS(&dp[3]),
@@ -1511,8 +1510,10 @@ interp_reply(netdissect_options *ndo,
15111510
ND_PRINT((ndo, " attr:"));
15121511
if (!(dp = parse_post_op_attr(ndo, dp, ndo->ndo_vflag)))
15131512
break;
1514-
if (!er)
1513+
if (!er) {
1514+
ND_TCHECK(dp[0]);
15151515
ND_PRINT((ndo, " c %04x", EXTRACT_32BITS(&dp[0])));
1516+
}
15161517
return;
15171518

15181519
case NFSPROC_READLINK:

Diff for: tests/TESTLIST

+1
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,7 @@ hoobr_pimv1 hoobr_pimv1.pcap hoobr_pimv1.out
443443
hoobr_safeputs hoobr_safeputs.pcap hoobr_safeputs.out
444444
isakmp-rfc3948-oobr isakmp-rfc3948-oobr.pcap isakmp-rfc3948-oobr.out
445445
isoclns-oobr isoclns-oobr.pcap isoclns-oobr.out
446+
nfs-attr-oobr nfs-attr-oobr.pcap nfs-attr-oobr.out
446447

447448
# bad packets from Wilfried Kirsch
448449
slip-bad-direction slip-bad-direction.pcap slip-bad-direction.out -ve

0 commit comments

Comments
 (0)