Skip to content
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

fix: FuzzParseDN causing OOMs in restricted environments #466

Merged
merged 1 commit into from
Sep 15, 2023
Merged

fix: FuzzParseDN causing OOMs in restricted environments #466

merged 1 commit into from
Sep 15, 2023

Conversation

cpuschma
Copy link
Member

See the discussion in #460. The fuzzing might crash in certain environments because of the high ber.MaxPacketLengthBytes size of 2147483647 bytes (2.1 GB).

This change limits the maximum ASN1 BER packet size to 65KB, which should be sufficient for the fuzzer. We'll look into providing custom encoders/decoders to allow setting a limit without breaking things globally, as the configuration is package-wide.

Additionally, the fuzz_test.go file was missing in the v3 directory. This slipped through in the initial PR

Parallel and large amount of fuzzing data can create large amounts of allocated data and cause restricted fuzzing environments to crash (see #460)
@cpuschma cpuschma added the bug label Sep 15, 2023
@0x34d
Copy link
Contributor

0x34d commented Sep 15, 2023

Bug : v3/fuzz_test.go in this you need to change the names.

To: FuzzParseDNv3, FuzzDecodeEscapedSymbolsv3, FuzzEscapeDNv3.

also in fuzz_test.go To: FuzzParseDNv0, FuzzDecodeEscapedSymbolsv0, FuzzEscapeDNv0.

And then in build.sh need to be updated.

@0x34d
Copy link
Contributor

0x34d commented Sep 15, 2023

google/oss-fuzz#10969

@cpuschma
Copy link
Member Author

cpuschma commented Sep 15, 2023

Oh you already opened the PR, alright. Thank you! I was about to remove the mirroring and separate this into a new PR, since the v3 and root directory are out of sync anyways.

@TomSellers
Copy link
Contributor

TomSellers commented Sep 15, 2023

Odd, so with the current code, the following works
go test -v -fuzz ^'FuzzParseDNv0$' .

But this returns testing: warning: no fuzz tests to fuzz
go test -v -fuzz '^FuzzParseDNv3$' .

Is this module related?

EDIT: Ah, so if I cd into v3 and run the fuzzer it works.

@TomSellers
Copy link
Contributor

TomSellers commented Sep 15, 2023

Note, after the changes the following had reasonable memory consumption (< 2 GB combined) when fuzzing across 10 cores.

go test -v -fuzz ^'FuzzParseDNv0$' .

(cd v3 && go test -v -fuzz '^FuzzParseDNv3$' .)

@cpuschma
Copy link
Member Author

v3 was created back when Go Modules weren't a thing. Since then, both the root and v3 directory have been mostly in sync to not break any backwards compatibility. Imo, this is obsolete, since the oldest version we support is Go 1.14 anyways, which already had Go Module integrated.

To get things moving: I'll remove the part where I mirrored the fuzz_test.go into v3 so that the CI runs successfully. As noted in another PR to John Weldon, both directories are out of sync and as mentioned, maybe it's time to remove this duplicate (version older than Go 1.14 won't compile anyway due to a missing dependency).

@0x34d
Copy link
Contributor

0x34d commented Sep 15, 2023

EDIT: Ah, so if I cd into v3 and run the fuzzer it works.

v3 is a different module, so you have to pushd and popd.

@cpuschma cpuschma merged commit 80095a3 into go-ldap:master Sep 15, 2023
20 checks passed
@cpuschma cpuschma deleted the fix_fuzzing_oom branch September 16, 2023 11:52
cpuschma added a commit to cpuschma/ldap that referenced this pull request Apr 1, 2024
cpuschma added a commit that referenced this pull request Apr 1, 2024
#500)

* Revert "fix: Limit maximum BER packet length in `FuzzParseDN` to 65536 bytes (#466)"

This reverts commit 80095a3

* Fix index out of range crash
gustavoluvizotto pushed a commit to gustavoluvizotto/ldap-fork that referenced this pull request Apr 11, 2024
…o-ldap#466)

Parallel and large amount of fuzzing data can create large amounts of allocated data and cause restricted fuzzing environments to crash (see go-ldap#460)
gustavoluvizotto pushed a commit to gustavoluvizotto/ldap-fork that referenced this pull request Apr 11, 2024
go-ldap#500)

* Revert "fix: Limit maximum BER packet length in `FuzzParseDN` to 65536 bytes (go-ldap#466)"

This reverts commit 80095a3

* Fix index out of range crash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants