Initial fuzzer integration by DavidKorczynski · Pull Request #961 · linkerd/linkerd2-proxy

DavidKorczynski · 2021-03-30T21:22:20Z

Adds a set of fuzzers for various of the linkerd crates.

The goal of this PR is to create an initial fuzzer infrastructure as well as fuzzers that can be integrated with OSS-Fuzz for continuous fuzzing.

The way I set it up was to place the fuzzers for a given crate within the crate folder itself. I then put in a fuzz_logic module in the source code that is to be fuzzed, thus following the low of how the existing tests are written. The majority of additions in this PR are independent of the linkerd2-proxy source code, but there are a few changes.

olix0r · 2021-03-31T15:35:58Z

Cargo.toml


 [patch.crates-io]
-webpki = { git = "https://github.com/linkerd/webpki", branch = "cert-dns-names-0.21", rev = "b2c3bb3" }
+webpki = { git = "https://github.com/linkerd/webpki", branch = "cert-dns-names-0.21" }


did the pinned revision cause problems?

Yes, I get the following issue:

linkerd2-proxy/linkerd/proxy/http$ cargo +nightly fuzz build error: failed to parse manifest at `/linkerd2-proxy-6/linkerd2-proxy/Cargo.toml` Caused by: dependency (webpki) specification is ambiguous. Only one of `branch`, `tag` or `rev` is allowed. Error: failed to build fuzz script: "cargo" "build" "--manifest-path" "linkerd2-proxy/linkerd/proxy/http/fuzz/Cargo.toml" "--target" "x86_64-unknown-linux-gnu" "--release" "--bins"```

hmm, maybe we should prefer the pinned revision over the pinned branch, since the rev is more specific? but i don't have a strong opinion on this one (and the branch is much more nicely human readable!)

out of curiosity, is there a reason we don't use the latest of the branch?

oh, huh, are we not? :)

nope: https://github.com/linkerd/webpki/commits/cert-dns-names-0.21 - just a single commit behind which bumps the version.

welp, maybe we should fix this...but it's a job for another PR :)

linkerd/transport-header/fuzz/fuzz_targets/fuzz_target_1.rs

hawkw

I had some largely procedural questions about how the fuzz tests are organized.

linkerd/addr/fuzz/.gitignore

linkerd/addr/fuzz/Cargo.toml

linkerd/proxy/http/src/detect.rs

hawkw

Gave this a closer read and I had some additional suggestions. Let me know if any of these don't or you have follow-up questions?

linkerd/dns/src/lib.rs

hawkw · 2021-04-01T19:10:31Z

linkerd/dns/src/lib.rs

+    pub fn fuzz_entry(fuzz_data: &str) {
+        if let Ok(_name) = Name::from_str(fuzz_data) {
+            let _fcon = FuzzConfig {};
+            let _resolver = Resolver::from_system_config_with(&_fcon).unwrap();
+            let a1 = async {
+                let _w = _resolver.resolve_a(&_name).await;
+            };
+            futures::executor::block_on(a1);
+            let a2 = async {
+                let _w2 = _resolver.resolve_srv(&_name).await;
+            };
+            futures::executor::block_on(a2);
+        }
+    }


Using futures::executor::block_on will block the current thread on the returned futures, not allowing any background tasks spawned by trust-dns etc to proceed. I don't know if this will correctly exercise the resolver code. Could we maybe change this so that fuzz_entry is an async fn that's passed to Runtime::block_on in fuzz_target_1, rather than using futures::executor::block_on? That would look something like this:

Suggested change

pub fn fuzz_entry(fuzz_data: &str) {

if let Ok(_name) = Name::from_str(fuzz_data) {

let _fcon = FuzzConfig {};

let _resolver = Resolver::from_system_config_with(&_fcon).unwrap();

let a1 = async {

let _w = _resolver.resolve_a(&_name).await;

};

futures::executor::block_on(a1);

let a2 = async {

let _w2 = _resolver.resolve_srv(&_name).await;

};

futures::executor::block_on(a2);

}

}

pub async fn fuzz_entry(fuzz_data: &str) {

if let Ok(_name) = Name::from_str(fuzz_data) {

let _fcon = FuzzConfig {};

let _resolver = Resolver::from_system_config_with(&_fcon).unwrap();

let _w = _resolver.resolve_a(&_name).await;

let _w2 = _resolver.resolve_srv(&_name).await;

}

}

I think this code may happen to work as-is if the Tokio runtime is configured in multithreaded mode (by another crate enabling the "rt-multi-thread" feature). But, I think we should remove the use of futures::executor::block_on regardless, since it's adding additional complexity that isn't actually part of the code we want to fuzz here, and it's never used in the actual proxy binary...

I think you're right entirely - it also seems the fuzzer is running much better with your suggestions. Please check the new set up!

hawkw · 2021-04-01T19:12:11Z

linkerd/dns/fuzz/Cargo.toml

+
+[dependencies]
+libfuzzer-sys = "0.4"
+tokio = { version = "1", features = ["rt", "sync", "time"] }


I think we may need to also enable tokio's "io" feature flag --- I believe constructing the resolver here

linkerd2-proxy/linkerd/dns/src/lib.rs

Lines 49 to 51 in 743c0f1

// This function is synchronous, but needs to be called within the Tokio

// 0.2 runtime context, since it gets a handle.

let dns = AsyncResolver::tokio(config, opts).expect("system DNS config must be valid");

will try to bind a UDP socket to the Tokio event loop, which will fail without the "io" feature flag enabled?

I'm surprised we need the "sync" feature, also --- AFAICT, the fuzz targets don't use tokio's synchronization primitives?

I don't think Tokio has an "io" flag? Do you perhaps mean io-util?

linkerd/proxy/http/src/detect.rs

linkerd/proxy/http/fuzz/fuzz_targets/fuzz_target_1.rs

linkerd/transport-header/src/lib.rs

linkerd/addr/src/lib.rs

We have to build fuzzers with Cargo nightly, which complains about having both the branch and rev tag. Only one can be used. Signed-off-by: davkor <[email protected]>

Creates a set up for fuzzing Addr. This fuzzer is minor but nice to have. Signed-off-by: davkor <[email protected]>

Integrates a fuzzer for DNS. Signed-off-by: davkor <[email protected]>

Signed-off-by: davkor <[email protected]>

DavidKorczynski · 2021-04-02T13:23:48Z

@hawkw please check my replies to your review. There are a few ones still ones I didn't resolves as they can perhaps be up for debate.

Signed-off-by: davkor <[email protected]>

This is an attempt to fix CI issue by removing cfg(fuzzing) feature. This simplifies things and if it works we should use the same fix elsewhere. Signed-off-by: davkor <[email protected]>

Signed-off-by: davkor <[email protected]>

hawkw

Thanks for making the changes we discussed, this looks great. I had a few more comments, but none of them are critical --- I think this is close to ready to merge!

hawkw · 2021-04-05T20:57:36Z

Cargo.toml


 [patch.crates-io]
-webpki = { git = "https://github.com/linkerd/webpki", branch = "cert-dns-names-0.21", rev = "b2c3bb3" }
+webpki = { git = "https://github.com/linkerd/webpki", branch = "cert-dns-names-0.21" }


welp, maybe we should fix this...but it's a job for another PR :)

linkerd/addr/src/lib.rs

hawkw · 2021-04-05T21:00:14Z

linkerd/transport-header/fuzz/fuzz_targets/fuzz_target_1.rs

+use libfuzzer_sys::arbitrary::Arbitrary;
+
+#[derive(Debug, Arbitrary)]
+pub struct AllocatorMethod {


I'm not sure if I understand the naming "AllocatorMethod"?

hawkw · 2021-04-05T21:01:10Z

linkerd/transport-header/fuzz/fuzz_targets/fuzz_target_1.rs

+pub struct AllocatorMethod {
+    pub data: Vec<u8>,
+    pub port: u16,
+    pub protocol: bool,
+}


nit, not important: I don't think any of this needs to be pub?

Suggested change

pub struct AllocatorMethod {

pub data: Vec<u8>,

pub port: u16,

pub protocol: bool,

}

struct AllocatorMethod {

data: Vec<u8>,

port: u16,

protocol: bool,

}

linkerd/transport-header/src/lib.rs

Signed-off-by: davkor <[email protected]>

hawkw

Thanks for adding the new fuzzer for invalid input to TransportHeader::read_prefaced! I think this PR looks great now --- it looks like cargo fmt needs to be run before it'll pass CI, but then I'm happy to merge it!

linkerd/transport-header/src/lib.rs

This release fixes a caching issue in the outbound proxy's "ingress mode" that could cause the incorrect client to be used for requests. This caching has been fixed so that clients cannot be incorrectly reused across logical destinations. --- * transport: introduce Bind trait and move orig dst to the type level (linkerd/linkerd2-proxy#957) * Initial fuzzer integration (linkerd/linkerd2-proxy#961) * Fix caching in ingress mode (linkerd/linkerd2-proxy#965)

This release fixes a caching issue in the outbound proxy's "ingress mode" that could cause the incorrect client to be used for requests. This caching has been fixed so that clients cannot be incorrectly reused across logical destinations. --- * transport: introduce Bind trait and move orig dst to the type level (linkerd/linkerd2-proxy#957) * Initial fuzzer integration (linkerd/linkerd2-proxy#961) * Fix caching in ingress mode (linkerd/linkerd2-proxy#965) Signed-off-by: Jijeesh <[email protected]>

This release fixes a caching issue in the outbound proxy's "ingress mode" that could cause the incorrect client to be used for requests. This caching has been fixed so that clients cannot be incorrectly reused across logical destinations. --- * transport: introduce Bind trait and move orig dst to the type level (linkerd/linkerd2-proxy#957) * Initial fuzzer integration (linkerd/linkerd2-proxy#961) * Fix caching in ingress mode (linkerd/linkerd2-proxy#965)

DavidKorczynski requested a review from a team March 30, 2021 21:22

olix0r reviewed Mar 31, 2021

View reviewed changes

olix0r requested a review from hawkw March 31, 2021 19:05

hawkw reviewed Mar 31, 2021

View reviewed changes

linkerd/addr/fuzz/.gitignore Show resolved Hide resolved

linkerd/addr/fuzz/Cargo.toml Show resolved Hide resolved

linkerd/proxy/http/src/detect.rs Outdated Show resolved Hide resolved

hawkw reviewed Apr 1, 2021

View reviewed changes

DavidKorczynski added 4 commits April 2, 2021 14:23

Root Cargo.toml: remove rev specification so cargo can build fuzzers.

4f91874

We have to build fuzzers with Cargo nightly, which complains about having both the branch and rev tag. Only one can be used. Signed-off-by: davkor <[email protected]>

Addr: initial fuzzer integration.

968115d

Creates a set up for fuzzing Addr. This fuzzer is minor but nice to have. Signed-off-by: davkor <[email protected]>

dns: inital fuzzer integration

698b2e5

Integrates a fuzzer for DNS. Signed-off-by: davkor <[email protected]>

tls: initial fuzzer integration.

804293f

Signed-off-by: davkor <[email protected]>

DavidKorczynski added 12 commits April 2, 2021 14:23

transport-header: initial fuzzer integration.

175d9a3

Signed-off-by: davkor <[email protected]>

proxy: initial fuzzer integration.

10987f3

Signed-off-by: davkor <[email protected]>

fuzzers: clean up formatting.

b9367ef

Signed-off-by: davkor <[email protected]>

fuzzers: remove Cargo.lock files.

adcbdcf

Signed-off-by: davkor <[email protected]>

fuzzer: fix CI issue.

b3259e2

This is an attempt to fix CI issue by removing cfg(fuzzing) feature. This simplifies things and if it works we should use the same fix elsewhere. Signed-off-by: davkor <[email protected]>

fuzzers: complete build setup so CI passes in all places.

055a7be

Signed-off-by: davkor <[email protected]>

fuzzers: proxy: http: remove unused macro.

7af4aa0

Signed-off-by: davkor <[email protected]>

Fixup dns fuzzer.

cf6e16b

Signed-off-by: davkor <[email protected]>

fix http proxy fuzzer.

982237a

Signed-off-by: davkor <[email protected]>

transport-header: fixup fuzzer.

142eb0f

Signed-off-by: davkor <[email protected]>

fuzzers: transport-header: setup structure-aware fuzzing

578fcb0

Signed-off-by: davkor <[email protected]>

fuzzers: transport-header: fix formatting.

fee2878

Signed-off-by: davkor <[email protected]>

hawkw reviewed Apr 5, 2021

View reviewed changes

DavidKorczynski added 2 commits April 5, 2021 23:11

fuzzers: transport-header: add additional fuzzer and refine code.

b5a936c

Signed-off-by: davkor <[email protected]>

fuzzers: transport-header: fix formatting.

c03d324

Signed-off-by: davkor <[email protected]>

hawkw approved these changes Apr 5, 2021

View reviewed changes

linkerd/transport-header/src/lib.rs Show resolved Hide resolved

hawkw requested a review from olix0r April 5, 2021 22:53

olix0r approved these changes Apr 5, 2021

View reviewed changes

olix0r merged commit b401746 into linkerd:main Apr 5, 2021

olix0r mentioned this pull request Apr 7, 2021

proxy: v2.141.0 linkerd/linkerd2#5998

Merged

hawkw mentioned this pull request Apr 21, 2021

Commit lock files for fuzzers #984

Merged

	// This function is synchronous, but needs to be called within the Tokio
	// 0.2 runtime context, since it gets a handle.
	let dns = AsyncResolver::tokio(config, opts).expect("system DNS config must be valid");

Conversation

DavidKorczynski commented Mar 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidKorczynski Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidKorczynski Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DavidKorczynski commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DavidKorczynski Mar 31, 2021 •

edited

Loading

hawkw Mar 31, 2021 •

edited

Loading

DavidKorczynski Apr 2, 2021 •

edited

Loading

DavidKorczynski commented Apr 2, 2021 •

edited

Loading