slsa-framework / slsa-verifier

Verify provenance from SLSA compliant builders
Apache License 2.0
228 stars 49 forks source link

[feature] Harden maven verifier plugin against command injections #665

Open AdamKorcz opened 1 year ago

AdamKorcz commented 1 year ago

Is your feature request related to a problem? Please describe. The Maven slsa verifier plugin (https://github.com/slsa-framework/slsa-github-generator/pull/2380) has a potential command injection when invoking the verifier.

Describe the solution you'd like The plugin should be audited and hardened against command injections.

laurentsimon commented 1 year ago

Transferring this issue to the slsa-verifier repo

ianlewis commented 1 year ago

It seems like the command arguments are split in a semi-sane way which is not immediately obvious to me that it's vulnerable to an attack though it would be good if we could use an alternative way to execute the command that allows you to specify the command args as a list of strings.

https://github.com/sonatype/plexus-utils/blob/5ba6cfcca911200b5b9d2b313bb939e6d7cbbac6/src/main/java/org/codehaus/plexus/util/cli/CommandLineUtils.java#L302-L383

Unfortunately it feels like we would basically need to have our own implementation of mojo-executor if we actually want to harden this and not rely on the argument splitting logic in plexus-utils.

AdamKorcz commented 1 year ago

Unfortunately it feels like we would basically need to have our own implementation of mojo-executor if we actually want to harden this and not rely on the argument splitting logic in plexus-utils.

This looks doable given its size. Alternatively we can send PRs upstream to harden it.

For now we can also limit the characters allowed in the filename passed to the verifier, but this could prevent verification of valid files that the user wants to verify - although I have never seen java dependencies with non-base64 names.

It this a blocker for the rest of the release, or can we label this verifier experimental?

laurentsimon commented 1 year ago

not. blocker for the release. We can iterate on this one.