csv-lint & json-lint: Use printf %q to sanatize input and avoid command injection
#196
Conversation
| @@ -40,6 +40,7 @@ jobs: | |||
| run: | | |||
| cd csvlint | |||
| for file in ${{ steps.changed-files.outputs.all_changed_files }}; do | |||
There was a problem hiding this comment.
Is 42 subject to injection?
Did you asses env laundering as a potential mitigation? See the discussion in the "Remediation" section here:
https://securitylab.github.com/research/github-actions-untrusted-input/
env:
FILES: ${{ steps.changed-files.outputs.all_changed_files }}
run:
for file in $FILES; ...There was a problem hiding this comment.
Is 42 subject to injection?
I don't believe so. My understanding is that bash has a strict interpenetration of the input parameter for a for loop. Similar to how command injection should not be possible with FOO=${{ steps.changed-files.outputs.all_changed_files }}
Related since we are discussing it, I considered also setting the IFS to correctly handle filenames with spaces. But setting this value would actually make a theoretically command injection easier (though in theory not possible given the printf change).
Let me know if you have any opinion on if I should make that change.
Did you asses env laundering as a potential mitigation?
As I understand it the printf would still be necessary. Once the for loop logic breaks apart the input I think injection may still be possible. Do you think it's worth adding this protection given the above? I have no concerns, just trying to keep complexity to a minimum.
There was a problem hiding this comment.
@wadells friendly ping on this when you have a chance. Not high priority, but it would be nice to finish this NCC finding out.
There was a problem hiding this comment.
Sorry -- you hadn't requested re-review, so this wasn't showing up in my review queue:
The best way to make sure I (or most other folks) revisit something (especially if I've requested changes) is to re-request review. Otherwise, if I get pulled away from the initial @ notification (for whatever reason), it'll never show back up in my queue.
There was a problem hiding this comment.
Thank you, requested. To be clear I have not made any changes yet, I am just looking for feedback on if we want to additionally add the environment filter.
printf %qis a useful feature whenever we need to provide the input into a shell command. It will escape characters which could be used for injection.This PR fixes https://github.com/gravitational/security-findings/issues/52