- Move bzero() inside the while loop
- Add malloc() u_int overflow check
- Fix use-after-free by moving xdr_free() to cleanup block
- Fix warnings in xdr_free() by casting function pointers to (void *)
- Add missing headers and forward declarations
Details
Details
- Reviewers
kfv - Group Reviewers
Src Committers Contributor Reviews (src)
Compiled with -Weverything -Wno-unsafe-buffer-usage -Wno-sign-conversion
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 72588 Build 69471: arc lint + arc unit
Event Timeline
This comment was removed by kfv.
| lib/libc/yp/xdryp.c | ||
|---|---|---|
| 33–37 | Sort per style(9) | |
| 53–55 | It looks like xdr_datum is exported by yp_prot.h: rpcsvc/yp_prot.h 308:bool_t xdr_datum(XDR *, datum *); xdr_ypresp_all_seq doesn't look like it gets exported by another header, but should it be in yp_prot.h? NetBSD moved these definitions around ~30 years ago in https://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/yp/xdryp.c.diff?r1=1.11;r2=1.11.4.1;f=h . Worth investigating, maybe, but should not be handled in this review. Bottom line: rpcsvc/yp_prot.h should remove the need for the xdr_datum declaration. | |
| 74 | If this is being moved around, you can convert this from bzero to memset. | |
| 88 | I don't think the cast was needed here. | |
| 90 | ||
| 97 | (same as earlier comment) | |
| 100 | ||
| 113 | ||
| 120 | ||
| 124 | ||
| lib/libc/yp/xdryp.c | ||
|---|---|---|
| 88 |
It would also be a lot more legible using a pattern like this: key = NULL;
if (out.ypresp_all_u.val.key.keydat_len < UINT_MAX)
key = malloc(out.ypresp_all_u.val.key.keydat_len + 1)I'd doublecheck NetBSD to see if they fixed this issue in the last few decades. | |
Comment Actions
Besides my inline comments:
- Please drop the (void *) casts. libc is built with WARNS=2, and the function-pointer-to-void * conversion isn’t ISO C. Plain (xdrproc_t) is just fine and matches the rest of the tree.
- Please reword the proposed commit message, as this is not a UAF fix.
| lib/libc/yp/xdryp.c | ||
|---|---|---|
| 33 | Already in by <rpc/rpc.h>, so drop it please. | |
| 39–40 | Unrelated whitespace change. | |
| 53–55 | Drop these two. Declaring them right above their definitions doesn’t get them checked against anything, so they’re redundant. IMHO, a real check, if that's what we're after, would probably require putting the declarations in a header shared with the callers. I’d need to look into that more closely before saying for sure, though. In any case, that should be a separate change. | |
| 88 | This can’t trigger. xdr_bytes() already caps it at YPMAXRECORD on decode, so len + 1 can’t ever wrap. | |
| 97 | Same as above, this cannot trigger. | |
| 113 | Happy with the move, but there’s no UAF, to be honest. After xdr_free(), only the *_len fields are read, and they’re plain integers in the on-stack out, so freeing doesn’t touch them. | |