wooorm / markdown-rs

CommonMark compliant markdown parser in Rust with ASTs and extensions
https://docs.rs/markdown/1.0.0-alpha.21/markdown/
MIT License
934 stars 52 forks source link

Integrate into Google OSS Fuzz #149

Open LeoDog896 opened 2 weeks ago

LeoDog896 commented 2 weeks ago

This project uses libfuzzer, which is compatible with Google OSS Fuzz. Integration with this would allow markdown-rs to be constantly fuzzed.

I would be open to working on this issue.

wooorm commented 2 weeks ago

Interesting. What would that mean?

I’m sure @ChristianMurphy has thoughts on this

LeoDog896 commented 2 weeks ago

It doesn't mean much for this repository itself - unless CIFuzz is integrated. Plus, if either you or @ChristianMurphy also wants to do this, OSS Fuzz also offers monetary rewards for projects integrated into the platform.

ChristianMurphy commented 2 weeks ago

I'm all for fuzzing, we already have an initial fuzz integration https://github.com/wooorm/markdown-rs/tree/main/fuzz I also think with some of the new features we could make the fuzzer more advanced https://github.com/wooorm/markdown-rs/pull/127#issuecomment-2395573742

I do have some mild concerns over https://google.github.io/oss-fuzz/advanced-topics/bug-fixing-guidance#should-all-reported-issues-be-solved

In the event that a bug is reported by OSS-Fuzz that is not relevant to security or reliability of the application then there may still be a point to fixing the bug. For example, if the issue is often run into by the fuzzer then the fuzzer may have difficulty exploring further code in the target, and thus fixing the bug will allow the fuzzer to explore further code. In this case some suggested examples of resolving the issue could be:

Perform a hot-patch that is only applied during fuzzer executions and does not overcomplicate the project’s code. Patch the code of the fuzzer to avoid the timeout. For example, some fuzzers restrict the size of the input to avoid certain deep recursions or time-intensive loops. Patch the code in the target despite complicating things.

I read this as OSS Fuzz expects every thing that is found to either be fixed, or for the fuzzer to be patched to avoid it. Either done promptly. We run at a lower level of urgency for issues that users don't run into. Like:

OSS Fuzz also offers monetary rewards for projects integrated into the platform

Sort of, they only pay in certain circumstances

Projects are accepted by the OSS-Fuzz team based on their criticality, e.g. >=0.7 criticality score or if they are used as part of critical infrastructure and/or have a large user base.

https://bughunters.google.com/about/rules/open-source/5097259337383936/oss-fuzz-reward-program-rules markdown-rs has a score of 0.43774 to Google.

LeoDog896 commented 2 weeks ago

I read this as OSS Fuzz expects every thing that is found to either be fixed, or for the fuzzer to be patched to avoid it. Either done promptly. We run at a lower level of urgency for issues that users don't run into. Like: markdown-rs has a score of 0.43774 to Google.

I see—those points especially make sense. I am especially surprised by markdown-rs's low criticality score. Is that without dependents?

ChristianMurphy commented 2 weeks ago

That is a question for ossf. This is what they extracted, it seems low to me, I don't know what range of version(s) they consider in the dependency counter.

[{
  "collection_date": "2024-09-18 12:15:07.243799 UTC",
  "default_score": "0.43774",
  "depsdev": {
    "dependent_count": "22"
  },
  "legacy": {
    "closed_issues_count": "20",
    "commit_frequency": "0.56",
    "contributor_count": "16",
    "created_since": "27",
    "github_mention_count": "86",
    "issue_comment_frequency": "4.0",
    "org_count": "7",
    "recent_release_count": "6",
    "updated_issues_count": "27",
    "updated_since": "0"
  },
  "repo": {
    "url": "https://github.com/wooorm/markdown-rs",
    "created_at": "2022-06-08 13:52:16.000000 UTC",
    "updated_at": "2024-09-19 09:23:50.000000 UTC",
    "star_count": "905",
    "license": "MIT License",
    "language": "Rust"
  },
  "worker_commit_id": "e8e782083145e40b4f285717048378ef9b1a079c"
}]

They throw this info through the formula:

formula

ChristianMurphy commented 2 weeks ago

https://crates.io/crates/markdown/reverse_dependencies reports 84+ just from public projects. I suspect the actual count is much higher.

LeoDog896 commented 2 weeks ago

Interesting - I'm opening an issue for comrak integration into OSS Fuzz: https://github.com/google/oss-fuzz/pull/12583, and markdown-rs fills in a lot of cases that comrak doesn't - it seems like both are good candidates for fuzzing.

comrak itself has a criticality score of ~0.6, but lower criticality projects are occasionally added to the fuzzer if they:

LeoDog896 commented 1 week ago

I read this as OSS Fuzz expects every thing that is found to either be fixed, or for the fuzzer to be patched to avoid it. Either done promptly. We run at a lower level of urgency for issues that users don't run into.

OSS Fuzz keeps a backlog of bugs - it doesn't expect maintainers to fix them, but they exist - some projects choose to simply ignore them until they have the time. They do hold a single expectation that vulnerabilities are fixed; though, I will close this issue as the criticality score is much too low compared to other markdown/commonmark rust libraries.

ChristianMurphy commented 1 week ago

OSS Fuzz keeps a backlog of bugs - it doesn't expect maintainers to fix them, but they exist - some projects choose to simply ignore them until they have the time.

That could work!

will close this issue as the criticality score is much too low compared to other markdown/commonmark rust libraries.

If they'll accept a PR, I think we'd be happy to have the additional fuzzing resources. My comments on this were more setting expectations if there was a hope of a payout, there probably isn't.

LeoDog896 commented 1 week ago

I'll open a PR then and link it to this issue!