New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 856866 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 28
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

shill: fuzzing support for netlink parsers

Project Member Reported by briannorris@chromium.org, Jun 27 2018

Issue description

See
https://chromium.googlesource.com/chromiumos/docs/+/master/fuzzing.md

When I got bored while investigating some shill bugs...I noticed that there's some reasonably large, stateless surface on our netlink parsing code. And it has some unit tests. This is a prime candidate for fuzzing. So I wrote a tiny ClusterFuzz test for the rtnl_handler. I believe it already caught a few small error handling bugs. Might as well merge it?

We could apply a similar method to netlink_packet too.
 
sure. any link for code review? link to the bugs the fuzzer caught/filed (isnt it supposed to be autofiling bugs)?

https://chromium-review.googlesource.com/c/aosp/platform/system/connectivity/shill/+/1116253 shill: RTNLHandler: add fuzzer        
https://chromium-review.googlesource.com/c/aosp/platform/system/connectivity/shill/+/1116254 shill: RTNL: fixup bounds checking in decoding  
https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1116255 shill: add fuzzer targets        

I don't think it will catch new bugs until we land it.

After fixing the first 2 bugs it caught, it ran for several hours on my workstation without hitting anything but a bunch of NOTREACHED() "unknown message", which is good I guess.
Cc: manojgupta@chromium.org infe...@chromium.org metzman@chromium.org llozano@chromium.org
Nice thing about fuzzing is many easy to reach bugs will be caught in local testing itself.
Clusterfuzz will of course find the more difficult ones once the fuzzer lands.

Comment 4 Deleted

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/6a5fa4f2417f75b7b9637c0b03b69e94c0308249

commit 6a5fa4f2417f75b7b9637c0b03b69e94c0308249
Author: Brian Norris <briannorris@chromium.org>
Date: Thu Jun 28 05:07:02 2018

shill: RTNLHandler: add fuzzer

ClusterFuzz makes this easy, so why not?

This already catches 2 small issues in error handling.

BUG= chromium:856866 
TEST=build for x86 with USE="asan fuzzer", then run
     /usr/libexec/fuzzers/rtnl_handler_fuzzer

Change-Id: I0e9fba8a582a23f7aca7d4ddf1cf8e99013fa594
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1116253
Reviewed-by: Kirtika Ruchandani <kirtika@chromium.org>

[add] https://crrev.com/6a5fa4f2417f75b7b9637c0b03b69e94c0308249/net/rtnl_handler_fuzzer.cc
[modify] https://crrev.com/6a5fa4f2417f75b7b9637c0b03b69e94c0308249/net/rtnl_handler.h
[modify] https://crrev.com/6a5fa4f2417f75b7b9637c0b03b69e94c0308249/shill.gyp

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/c07609a263be1a53b9296c31c086a015f868fc03

commit c07609a263be1a53b9296c31c086a015f868fc03
Author: Brian Norris <briannorris@chromium.org>
Date: Thu Jun 28 05:07:02 2018

shill: RTNL: fixup bounds checking in decoding

The fuzzing framework noticed two heap overruns in RTNL parsing.

For NLMSG_ERROR, we validate that the header fits in our buffer, but we
don't validate that the 'nlmsgerr' does. Fix that.

For DeocdeNdUserOption(), our bounds check isn't big enough; beyond the
first element, we're trusting that the headers contain correct length
values, and we only bounds check against them. Extend the initial bounds
checking to account for all the bits we're expecting to be packed into
this message.

BUG= chromium:856866 
TEST=build for x86 with USE="asan fuzzer", then run
     /usr/libexec/fuzzers/rtnl_handler_fuzzer
TEST=network_Ipv6SimpleNegotiation

Change-Id: I0bb289208f08d971355be10033389877fc600716
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1116254
Reviewed-by: Kirtika Ruchandani <kirtika@chromium.org>

[modify] https://crrev.com/c07609a263be1a53b9296c31c086a015f868fc03/net/rtnl_handler.cc
[modify] https://crrev.com/c07609a263be1a53b9296c31c086a015f868fc03/net/rtnl_message.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/ed5e30cd4c15c16c1ce571068d781c26364ec0b3

commit ed5e30cd4c15c16c1ce571068d781c26364ec0b3
Author: Brian Norris <briannorris@chromium.org>
Date: Thu Jun 28 05:07:07 2018

shill: add fuzzer targets

Only one for now (rtnl_handler_fuzzer), but we might add more.

CQ-DEPEND=CL:1116253
BUG= chromium:856866 
TEST=build for x86 with USE="asan fuzzer", then run
     /usr/libexec/fuzzers/rtnl_handler_fuzzer

Change-Id: Idcfeed20eeb232d2728d131b4afe36e7e893a77f
Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1116255
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/ed5e30cd4c15c16c1ce571068d781c26364ec0b3/virtual/chromium-os-fuzzers/chromium-os-fuzzers-1.ebuild
[modify] https://crrev.com/ed5e30cd4c15c16c1ce571068d781c26364ec0b3/chromeos-base/shill/shill-9999.ebuild
[rename] https://crrev.com/ed5e30cd4c15c16c1ce571068d781c26364ec0b3/virtual/chromium-os-fuzzers/chromium-os-fuzzers-1-r6.ebuild

Cc: grundler@chromium.org briannorris@chromium.org
Owner: ----
Status: Available (was: Started)
There's probably more low-hanging fruit here. I'm not planning to work on that immediately, and kirtika@ suggested she might. I might have also tickled grundler@'s ear :)
Cc: benchan@chromium.org
Owner: briannorris@chromium.org
Status: Fixed (was: Available)
I forgot this was still open. I added a few more fuzzers for this month's Fuzz-a-Thon. Feel free to do more, with or without this $BUG.

Sign in to add a comment