Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter environment variables passed to ProcessCapture to increase security #5973

Closed
oheger-bosch opened this issue Oct 20, 2022 · 4 comments
Closed
Labels
configuration About configuration topics

Comments

@oheger-bosch
Copy link
Member

This is an idea how to reduce a security risk when running ORT, for which I would like to get some feedback:

Especially when running the Analyzer, external logic in build scripts is executed which could do potentially harmful thing. One attack vector could be reading sensitive information from environment variables, such as database credentials or credentials to other services provided to the ORT process.

At least when external processes are created via the ProcessCapture class, this risk could be mitigated by filtering the map with environment variables passed to the new process. Since the variables are specific to a concrete runtime environment, probably a configurable filter mechanism is required. For instance, users could define inclusion and/or exclusion filters in the ORT configuration file that are applied when constructing the environment. This would support use cases such as including only variables starting with a specific prefix (which are explicitly intended to be used by external tools) or removing variables with "postgres" in their name which are specific for the database.

This is of course not bullet-proof, but would be an improvement in this area.

@sschuberth sschuberth added enhancement configuration About configuration topics labels Oct 20, 2022
@oheger-bosch
Copy link
Member Author

The topic has been discussed in the ORT Community Meeting on 2022-10-20 with the following outcome:

  • There is general agreement that a solution addressing this issue would be useful.
  • The most secure solution - that should be followed here - is a whitelist approach. This means:
    • In the ORT configuration, users have to explicitly list the environment variables which should be passed through to external processes. All others are removed from the environment when starting an external process.
    • ORT provides a default for this setting that includes a minimum set of variables that are known to be required by the tools used, such as GRADLE_HOME, MAVEN_HOME, M2_REPO, etc.
  • For the beginning, a global whitelist is sufficient. Later on, tool-specific whitelists may be supported.

@oheger-bosch
Copy link
Member Author

#5984 proposes a solution for this problem. I would like to get some feedback about the default list of allowed variables. There is no guarantee that it is complete. But I think the main issue here is that users have to update their configuration to allow the variables they need, for instance credentials for package managers.

@oheger-bosch
Copy link
Member Author

The approach of listing all allowed variables used by the various tools is hardly practicable. In a first (and incomplete) attempt to compile such a list, I came up with more than 300 variables. This list would also have to be maintained and adapted to changes in the supported tools. So, while this may be the most secure approach, I doubt that it is feasible.

Therefore, I would suggest an alternative approach: ORT could define some patterns for variables it excludes by default, e.g. ".PASS.", ".USER.", ".*_KEY". The user could then configure a list of variables that match a pattern, but should be nevertheless included.

@nnobelis
Copy link
Member

This is a good idea. .*TOKEN.* too.

oheger-bosch added a commit to boschglobal/oss-review-toolkit that referenced this issue Nov 10, 2022
oheger-bosch added a commit to boschglobal/oss-review-toolkit that referenced this issue Nov 10, 2022
oheger-bosch added a commit to boschglobal/oss-review-toolkit that referenced this issue Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration About configuration topics
Projects
None yet
Development

No branches or pull requests

3 participants