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

oss-fuzz integration of kryo #829

Closed
0roman opened this issue May 6, 2021 · 7 comments
Closed

oss-fuzz integration of kryo #829

0roman opened this issue May 6, 2021 · 7 comments

Comments

@0roman
Copy link
Contributor

0roman commented May 6, 2021

Hi all,

I prepared the integration CodeIntelligenceTesting/oss-fuzz@e770af6 of kryo into google oss-fuzz. This will enable continuous fuzzing of this project, which will be conducted by google. Bugs that will be found by fuzzing will be reported to you.

The integration makes only sense if someone will deal with the bug reports submitted by oss-fuzz. Are you interested in the integration? If yes, I would submit a pull request and I would provide your contact information for bug reports. I would need some contact information (email) of someone who would handle the bug reports.

For the Fuzzing of Java applications Jazzer is used. Jazzer is a coverage-guided, in-process fuzzer for the JVM platform developed by Code Intelligence. It is based on libFuzzer and brings many of its instrumentation-powered mutation features to the JVM.
Jazzer has already found a lot of critical bugs in JVM applications.

If there are any questions regarding fuzzing or the oss-fuzz integration, it would be a pleasure for me to help.

@theigl
Copy link
Collaborator

theigl commented May 7, 2021

That looks very interesting @0roman!

The integration makes only sense if someone will deal with the bug reports submitted by oss-fuzz. Are you interested in the integration? If yes, I would submit a pull request and I would provide your contact information for bug reports. I would need some contact information (email) of someone who would handle the bug reports.

Sure, I can deal with the bug reports. Contact info would be thomas at umschalt dot com.

If there are any questions regarding fuzzing or the oss-fuzz integration, it would be a pleasure for me to help.

I took a brief look at your current fuzzer implementations. What I don't understand is how the fuzzer manages to generate inputs that Kryo can actually parse, without the use of a dictionary. Does it generate random inputs and observe the coverage?

Most users of Kryo don't use the out-of-the box configuration options. The most common settings are probably setReferences(true) and the use of another default serializer such as CompatibleFieldSerializer. Would it make sense to add these options like it is done in the Jackson implementation?

@0roman
Copy link
Contributor Author

0roman commented May 10, 2021

@theigl okay, that is awesome.

I took a brief look at your current fuzzer implementations. What I don't understand is how the fuzzer manages to generate inputs that Kryo can actually parse, without the use of a dictionary. Does it generate random inputs and observe the coverage?

That is correct. Based on the feedback the input is mutated until new program paths are triggered. Input that triggers new paths is again mutated. Just generating and mutating bytecode that will be deserialized seems to be a good starting point for me.

Most users of Kryo don't use the out-of-the box configuration options. The most common settings are probably setReferences(true) and the use of another default serializer such as CompatibleFieldSerializer. Would it make sense to add these options like it is done in the Jackson implementation?

Sure, i can add them. Does it makes sense to have both configurations (one out of the box and the other one with CompatibleFieldSerializer and setReferences) tested?

@theigl
Copy link
Collaborator

theigl commented May 11, 2021

That is correct. Based on the feedback the input is mutated until new program paths are triggered. Input that triggers new paths is again mutated. Just generating and mutating bytecode that will be deserialized seems to be a good starting point for me.

OK thanks for clarifying!

How does that fuzzer then distinguish between "expected" exceptions (i.e. malformed input) and potential bugs? Do we need to account for KryoExceptions that indicate a problem with the input?

Sure, i can add them. Does it makes sense to have both configurations (one out of the box and the other one with CompatibleFieldSerializer and setReferences) tested?

It would make sense to test all combinations:

Default, Default+References, Default+CompatibleSerializer, Default+References+CompatibleSerializer, ...

But we can also start with your current implementation and see how it goes and add additional configuration options later.

@theigl
Copy link
Collaborator

theigl commented May 11, 2021

How does that fuzzer then distinguish between "expected" exceptions (i.e. malformed input) and potential bugs? Do we need to account for KryoExceptions that indicate a problem with the input?

My bad, I just saw that you already handle KryoException. I think we are good to go with your current implementation as a first version.

theigl added a commit that referenced this issue May 23, 2021
@theigl
Copy link
Collaborator

theigl commented May 23, 2021

@0roman: I got the first report from the fuzzer yesterday. An exception other than KryoException was thrown when deserializing an invalid reference. I have just fixed that and will wait for the next check. I hope the reference=true case doesn't cause too many false positives because of unspecific exceptions.

@0roman
Copy link
Contributor Author

0roman commented May 24, 2021

@theigl: I am really happy to hear that. It should be fine. By the current configuration we have covered all preferred combinations.

theigl added a commit that referenced this issue May 26, 2021
theigl added a commit that referenced this issue May 26, 2021
theigl added a commit that referenced this issue May 26, 2021
@theigl
Copy link
Collaborator

theigl commented Jun 3, 2021

Thanks @0roman! The initial implementation looks good now that coverage builds are working.

@theigl theigl closed this as completed Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants