jensdietrich / xshady

vulnerabilities found in shaded projects
Apache License 2.0
5 stars 2 forks source link

pov-project structure #11

Open jensdietrich opened 1 year ago

jensdietrich commented 1 year ago

Thanks @alexjordan for contributing the tooling. Some notes on the pov: there is still a typo in the vulnableVersions key -- thanks for @wtwhite for pointing this out.

Also, we need to be clear about what signal means here. At the moment, the signal entry for CVE-2018-1324 is

"testSignal": "success"

But the test signaling that the vulnerability is present fails. So IMO this should be

"testSignal": "fail"

here, but success for CVE-2021-44228 (as an example), this is one from our initial "handcrafted" set.

Sorry I should have figured this out earlier while looking at the PR.

alexjordan commented 1 year ago

Oops, totally missed that typo, I guess that's why sed exists.

For CVE-2018-1324, I accidentally committed the fixed instead of a vulnerable version number in pom.xml (fixed in #13), the way the PoV test is written it causes a failure if the timeout does not happen.

Btw, the schema mandates "failure", same wording as JUnit, not "fail":

testSignal: "success" | "failure"
wtwhite commented 1 year ago

The tooling looks really nice @alexjordan 👍 Haven't met Cue before.

Wondering if you guys would be OK with renaming the testSignal field to something like testOutcomeWhenVulnerable? The latter naming makes it immediately clear what, e.g., a test success means, whereas I think the current naming leaves some doubt: Does the stated outcome signal vulnerability or safety? (The answer is in Alex's README, but we can save that one lookup :) )

jensdietrich commented 1 year ago

I am ok with this, perhaps testSignalWhenVulnerable ? I need to update the shadedetector too if we decide to go ahead as we use databinding to map the JSON schema to an object that is being parsed.

alexjordan commented 1 year ago

I'm fine with renaming this. I don't believe you will find a completely self-explanatory name though, so I would go with @jensdietrich's proposal because it's shorter.

wtwhite commented 1 year ago

Great, I'll make a PR tomorrow.