feat: even more precision for bash steps in github-env #208
Conversation
src/audit/github_env.rs
Outdated
| let bash = tree_sitter_bash::LANGUAGE; | ||
| let mut parser = Parser::new(); |
There was a problem hiding this comment.
Nit: creating the parser with each evaluation is probably slower than necessary; we could probably reuse parser by making it a property of GitHubEnv that gets loaded when the audit itself is loaded with GitHubEnv::new 🙂
There was a problem hiding this comment.
Done here, along with other changes!
| if let StepBody::Run { run, .. } = &step.deref().body { | ||
| if Self::uses_github_environment(run) { | ||
| if let StepBody::Run { run, shell, .. } = &step.deref().body { | ||
| let interpreter = shell.clone().unwrap_or("bash".into()); |
There was a problem hiding this comment.
This is unfortunately not guaranteed to be the right default, since Windows runners default to pwsh. I think what we need to do is look at the surrounding NormalJob::runs_on and maybe add some kind of RunsOn::default_shell() helper. I'll look into that a bit.
(Nonblocker, since this is still improving on the precision of the current approach 🙂)
There was a problem hiding this comment.
Actually, it's a bit more annoying than even that: we need to determine the step's shell via the following precedence:
- defaults.run.shell
- jobs.<job_id>.defaults.run.shell
- jobs.<job_id>.steps[*].shell
- If none of the above are set, use the default shell implied by the runner from
runs-on(which might be indeterminate in the self-hosted runner case)
There was a problem hiding this comment.
@woodruffw I totally missed pwsh on Windows as the default, I never used Windows runners and I though that bash was the default (via WLS). Another day, another learning I guess 🙂
There was a problem hiding this comment.
I forget about too 🙂 -- the changes in #213 should enable this, but I'll rebase off of your work so that it's not a blocker here.
| @@ -72,7 +111,7 @@ mod tests { | |||
| use crate::audit::github_env::GitHubEnv; | |||
|
|
|||
| #[test] | |||
| fn test_shell_patterns() { | |||
| fn test_exploitable_bash_patterns() { | |||
There was a problem hiding this comment.
I think we can fold these two test functions into one, with (case, expected: bool) as the parametrization 🙂
|
@woodruffw I lack the energy to work on anything else today, I'll resume the work on this PR tomorrow, addressing all your suggestions, and perhaps making the most of #213 🙂 Meanwhile, thanks again for the great review (as usual), it's a pleasure contributing to and learning from this project ❤️ |
|
Thank you too! And there's no rush or urgency at all, it's all supposed to be open source fun 🙂 |
ubiratansoares
force-pushed
the
ufs/patch-08
branch
from
79fd0d6 to
bc0e914
Compare
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
A follow-up for #197
Here we leverage tree-sitter-bash rather than regexes to match usages of
$GITHUB_ENVwithin a Bash script body.I took a simple approach, getting the full expansion for a
file_redirectorpipelineand matching inner contents. Perhaps I'm missing some corner cases, but at least we have a starting point to iterate over 🙂I also added some additional test cases (comments) and prepared the implementation to match against other types of shells in upcoming PRs.