nvlist_check_header() validated nvlh_size for overflow before
performing conversion. An mallicous user can set
NV_FLAG_BIG_ENDIAN in the header and craft nvlh_size so that
the orginall value passes the check, but after the conversion the
sizeof(nvlist_header) + size can overflow.
This can lead to a heap buffer overflow.
Details
Details
- Reviewers
markj - Commits
- rGf7f48005fbe2: libnv: fix heap overflow in nvlist_recv()
rG05b91c2a7106: libnv: fix heap overflow in nvlist_recv()
rG4f0992ce23b0: libnv: fix heap overflow in nvlist_recv()
rGaa15809f85de: libnv: fix heap overflow in nvlist_recv()
rG1cbd6e148249: libnv: fix heap overflow in nvlist_recv()
rGb345e07c8d71: libnv: fix heap overflow in nvlist_recv()
rG414e25d7d512: libnv: fix heap overflow in nvlist_recv()
rGe2219bbd634f: libnv: fix heap overflow in nvlist_recv()
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 72505 Build 69388: arc lint + arc unit
Event Timeline
Comment Actions
The commit log should include a "Fixes" tag. Presumably it also deserves an SA and a regression test?
| lib/libnv/tests/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #175905) | Why make it conditional on ASAN? |
| 8 ↗ | (On Diff #175905) | Why not just add this test to the existing send_recv_test file? |
| lib/libnv/tests/nvlist_header_overflow_test.c | ||
| 2 ↗ | (On Diff #175905) | Missing an SPDX license identifier. |
| 43 ↗ | (On Diff #175905) | We should test both the little-endian and big-endian cases. |
| lib/libnv/tests/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #175905) | Because without ASAN this test will not detect the bug as the heap override is to small. In such case this will give a false-positive that everything works. This is why I also moved this to separate file. |
| lib/libnv/tests/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #175905) | The other file has this NO_ASAN hack to accomplish the same goal, so I still don't see why it needs to be a separate file. I think our tests should be run unconditionally, and we should just run our tests with ASAN enabled by default. One step at a time. The disadvantage of disabling the test when ASAN is not configured is that you don't get any coverage at all. If I run the test suite with UBSAN enabled instead, this test doesn't run, even though it could find a new bug. |
| lib/libnv/tests/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #175905) | Oh, thats fair point. Somehow I didn't realize we have NO_ASAN already in the test. Thats fair point. I will rewrite this. |