Skip to content

test(fuzz): add fuzzing for dpapi-pdu structures #412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 9, 2025

Conversation

TheBestTvarynka
Copy link
Collaborator

Hi,
I decided to add fuzzing to dpapi-pdu crate. Because we have a lot of decoding/encoding there.

It wasn't hard to implement it. It was much harder to fix bugs found by fuzzing.

@TheBestTvarynka TheBestTvarynka self-assigned this Apr 7, 2025
@TheBestTvarynka TheBestTvarynka force-pushed the feat/dpapi-pdu-fuzzing branch from 1cf50e2 to aa8b2c5 Compare April 7, 2025 14:53
@TheBestTvarynka TheBestTvarynka marked this pull request as ready for review April 7, 2025 15:15
@TheBestTvarynka TheBestTvarynka requested a review from CBenoit April 7, 2025 15:15
Comment on lines +27 to +30
"crates/dpapi-pdu",
"crates/dpapi-fuzzing",
"crates/dpapi-pdu",
"crates/dpapi-fuzzing",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes. I think I accidentally added it during conflicts resolving

Comment on lines +13 to +17
pub fn round_trip(any: AnyStruct) {
let mut buf = WriteBuf::new();

if let Ok(name) = any.encode(&mut buf) {
let round_tripped_struct =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Those are called "oracles". I think it would be slightly better to put all the oracles into a oracle module.

let floor_length = src.read_u16();
let tower = (0..floor_length)
.map(|_| Floor::decode_owned(src))
.collect::<DecodeResult<Vec<Floor>>>()?;

// invalid tower_length can lead to invalid padding and corrupted fields.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Proper sentence for comments:

Suggested change
// invalid tower_length can lead to invalid padding and corrupted fields.
// Invalid tower_length can lead to invalid padding and corrupted fields.

Comment on lines +12 to +14
dpapi-fuzzing = { path = "../crates/dpapi-fuzzing" }
libfuzzer-sys = "0.4"
sspi = { path = ".." }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note:

Suggested change
dpapi-fuzzing = { path = "../crates/dpapi-fuzzing" }
libfuzzer-sys = "0.4"
sspi = { path = ".." }
dpapi-fuzzing.path = "../crates/dpapi-fuzzing"
libfuzzer-sys = "0.4"
sspi.path = ".."

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Good job improving the robustness of the code!

Small note: it’s better for both reviewing and version tracking to open a separate PR for the bug fixes, but it’s not a big deal.

@CBenoit CBenoit changed the title feat(fuzz): add fuzzing for dpapi-pdu structures test(fuzz): add fuzzing for dpapi-pdu structures Apr 9, 2025
@CBenoit CBenoit merged commit 187d6f5 into feat/ws-tunneling-support Apr 9, 2025
42 checks passed
@CBenoit CBenoit deleted the feat/dpapi-pdu-fuzzing branch April 9, 2025 10:06
@CBenoit
Copy link
Member

CBenoit commented Apr 9, 2025

Argh, I didn’t notice it was against another PR branch…! If the intention was to merge this PR later, I think it’s better to mark it as draft to help avoiding this kind of mistakes 🙂

@TheBestTvarynka
Copy link
Collaborator Author

If the intention was to merge this PR later, I think it’s better to mark it as draft to help avoiding this kind of mistakes 🙂

Oh, okay. I will take it into account

TheBestTvarynka added a commit that referenced this pull request Apr 9, 2025
@TheBestTvarynka TheBestTvarynka restored the feat/dpapi-pdu-fuzzing branch April 14, 2025 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants