Fix potential command injection vulnerability in jarjar_library rule
- Dominant language
- Starlark
- Stars
- 91
- Forks
- 45
- Avg merge
- 14m
- Merged PRs (30d)
- 7
Description
Currently, `jarjar.bzl` builds jar paths through direct string merging. Switching now to `ctx.actions.args()` improves how arguments are passed. Proper escaping of file names becomes possible this way. Unintended execution risks drop when special characters appear in paths. Following established Bazel patterns, the change strengthens reliability. Security gains come from avoiding raw text assembly methods. This update adjusts one component to match recommended standards. Handling inputs safely improves robustness even in the presence of unusual or malformed artifact paths. The method shift supports cleaner command formation behind the scenes. Overall structure remains unchanged despite internal adjustments.
### Steps to Reproduce
1. **Create a malicious target** in any `BUILD` file using a `genrule` to craft a filename with shell metacharacters:
```python
genrule(
name = "malicious_jar",
outs = ["lib';touch /tmp/pwned;'.jar"],
cmd = "touch $@",
)
```
2. **Use this target** as an input for `jarjar_library`:
```python
load("//tools/jarjar:jarjar.bzl", "jarjar_library")
jarjar_library(
name = "exploit_test",
jars = [":malicious_jar"],
rules = "rules.txt",
)
```
3. **Run the build** with standalone strategy:
`bazel build //:exploit_test --spawn_strategy=standalone`
4. **Verify the injection**:
Check if the file was created outside the build's expected scope: `ls /tmp/pwned`
### Summary of Findings (For your PR/Issue)
* **Root Cause:** Unquoted string concatenation at `jarjar.bzl:41`.
* **Impact:** Unexpected command execution during build-time when artifact paths contain shell metacharacters, reducing build safety and reliability.
* **Fix:** Use `ctx.actions.args()` to ensure all paths are safely escaped by Bazel's internal shell runner.
PoC video (unlisted, for maintainers only) :- https://youtu.be/wDeHLdHYXPs
Contributor guide
Assessment
This issue has not been assessed yet.