reconquest / atlassian-external-hooks

External Hooks plugin for Atlassian Bitbucket
https://external-hooks.reconquest.io
Other
45 stars 37 forks source link

Positional parameter command injection question #126

Closed brianlam-sfdc closed 2 years ago

brianlam-sfdc commented 2 years ago

Hi team!

How do you guys prevent command injection via. Positional Parameters? We tried to chain some commands e.g. $PARAMS | echo "hello world" > /tmp/test.txt which didn’t work but would like to confirm if there’s any sort of input validation performed here?

Thank you in advance!

kovetskiy commented 2 years ago

Hi! Could you elaborate on command injection & positional parameters? Currently, we treat positional parameters as something that we don't need to filter because it's a trusted user's input. What is your use case where you want to filter out something?

brianlam-sfdc commented 2 years ago

Hey @kovetskiy thanks for the reply mate!

I'm doing a security review of this tool for my team, so just thinking of all the potential attack vectors. From my understanding, only Bitbucket admins can configure the parameters. But if an admin is compromised, an attacker could exploit this tool to gain shell access into the CI/CD server and if the service user is running as root or a high-privileged account, the system could be compromised entirely.

Does this sound like a reasonable attack scenario? I know there are some mitigations in place such as safe dir but this could be disabled in the same scenario described above.

kovetskiy commented 2 years ago

Hi!

I see what you are looking for now, thanks. Let's split the question into two parts:

Script parameter validations

The add-on does provide a way to specify literally any number of positional parameters with no validation at all. But the script does escape all the parameters in order to avoid an unexpected evaluation since we are dealing with shell.

So, the goal of the add-on is to provide input without any side effects to the target script.

We use a Java package https://guava.dev/releases/19.0/api/docs/com/google/common/escape/package-summary.html to escape ' and " symbols and add ' around variables and script paths.

You can also take a look at the resulting hook script files in the bitbucket-home-dir/shared/config/hook-scripts/ directory.

Here is the escape procedure: https://github.com/reconquest/atlassian-external-hooks/blob/master/src/main/java/com/ngs/stash/externalhooks/hook/ExternalHookScript.java#L415

Safe Mode

We enforce Safe Mode for Bitbucket Data Center, which means that users can't specify an arbitrary path as an executable. The script should belong to bitbucket-home-dir/shared directory which means it should be uploaded by a user that already has full access to the entire Bitbucket server (or an entire cluster in case of DC edition).

We also do check that the file really exists before saving the user's input, here is the checking procedure: https://github.com/reconquest/atlassian-external-hooks/blob/master/src/main/java/com/ngs/stash/externalhooks/hook/ExternalHookScript.java#L157

Conclusion

The add-on doesn't provide a way to evaluate an arbitrary user input if two things are considered:

Let me know if you have questions!

brianlam-sfdc commented 2 years ago

That's very helpful information, you have more than answered my questions.

Thank you and much appreciated!