Filter environment variables when creating child processes #5984

oheger-bosch · 2022-10-21T13:54:06Z

This PR addresses #5973. Refer to the single commits for further details.

codecov · 2022-10-21T14:06:49Z

Codecov Report

Base: 57.79% // Head: 57.83% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (23d5657) compared to base (5788347).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5984      +/-   ##
============================================
+ Coverage     57.79%   57.83%   +0.04%     
- Complexity     2013     2019       +6     
============================================
  Files           324      324              
  Lines         18761    18761              
  Branches       3876     3881       +5     
============================================
+ Hits          10842    10850       +8     
+ Misses         6862     6852      -10     
- Partials       1057     1059       +2

Flag	Coverage Δ
funTest-non-analyzer	`51.01% <0.00%> (+0.06%)`	⬆️
test	`27.53% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
model/src/main/kotlin/config/OrtConfiguration.kt	`97.43% <100.00%> (+0.13%)`	⬆️
.../src/main/kotlin/config/LicenseFilenamePatterns.kt
...odel/src/main/kotlin/config/LicenseFilePatterns.kt	`86.95% <0.00%> (ø)`
analyzer/src/main/kotlin/managers/Bundler.kt	`48.07% <0.00%> (+0.19%)`	⬆️
cli/src/main/kotlin/commands/AnalyzerCommand.kt	`87.19% <0.00%> (+0.60%)`	⬆️
cli/src/main/kotlin/commands/ConfigCommand.kt	`59.37% <0.00%> (+3.12%)`	⬆️
downloader/src/main/kotlin/vcs/Git.kt	`74.33% <0.00%> (+5.16%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

model/src/main/kotlin/config/OrtConfiguration.kt

sschuberth · 2022-10-21T17:18:29Z

model/src/main/resources/defaults.yml

+# A file with default configuration options used by ORT. These options can be overridden in custom configuration files.
+
+ort:
+  variablesToPropagate:


Phew. Looking at the length of this, I really wonder whether the "allow"-approach is the right one.

In either case we should probably consider to support wildcards or regexes, so one could allow / deny e.g. CARGO_*.

Yes, I was already sceptical when the decision was taken. It will be very difficult to come up with an exhaustive list. In addition, this list will have to be maintained when there are new versions of tools.

Using of wildcards will simplify the configuration, but again increase the risk that an unwanted variable is passed through.

I wonder whether we should come up with something completely different... a "crazy" approach could be to look at the entropy of the value of an environment variable to "guess" whether it's likely a password or so, and only then do not pass it on to the spawned process.

Or, instead of a global list, create tool-specific lists that are used in CommandLineTool objects. I.e. we should wrap each tool we call in an object instead of implementing the interface in the main class itself (see MercurialCommand for an example), and each of these objects gets its own (hard-coded) list of allowed environment variables. That should at least ease maintainability a bit.

I fear the entropy approach is too clever to work in practice.

If the allowed variables were configured at the command line tools, this could indeed make the maintenance a bit easier. But how could these configurations then be extended? For our use case, we will need to pass additional variables with credentials to package managers.

model/src/main/kotlin/config/OrtConfiguration.kt

utils/common/src/main/kotlin/ProcessCapture.kt

sschuberth · 2022-10-21T17:26:14Z

utils/common/src/main/kotlin/ProcessCapture.kt

@@ -70,12 +79,21 @@ class ProcessCapture(
         * Filter out the variables that are not white-listed to be propagated from [environment].
         */
        private fun filterEnvironmentVariables(environment: MutableMap<String, String>): MutableMap<String, String> {
-            val keysToRemove = environment.keys.filterNot { it in variablesToPropagate }
+            val keys = environment.keys.handleCase()


Alternatively to this, apply .toSortedMap(String.CASE_INSENSITIVE_ORDER) to environment on Windows before looking up the keys.

nnobelis · 2022-10-24T05:27:30Z

model/src/main/resources/defaults.yml

+    - NODE_PATH
+    - NODE_EXTRA_CA_CERTS
+    - PREFIX
+    - TERM


We should also propagate PATH and PWD.

PATH really seems to be required. Lots of tests were failing because tools could not be found.

oheger-bosch · 2022-10-26T13:19:12Z

I have implemented an alternative approach as described in my last comment on #5973. This seems to be significantly easier by (hopefully) offering a comparable level of security.

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

model/src/main/kotlin/config/OrtConfiguration.kt

utils/common/src/main/kotlin/ProcessCapture.kt

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

sschuberth · 2022-11-04T13:18:56Z

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

+     * A set with patterns for variable names that are denied by default. All variables containing one of these
+     * strings (ignoring case) are not propagated to child processes.
+     */
+    val DEFAULT_DENY_PATTERNS = setOf(


Thinking about it, the term "pattern" in the variable name (and also in the docs above) probably is a bit too auspicious as these neither are glob nor regex patterns (I was looking for docs by which principle patterns are matched). So, as this simply is a case-insensitive sub-string search, maybe just use "term"?

Or "substrings", also see my comment about not using a regexes below.

Now using the term "substrings" (hopefully) everywhere.

sschuberth · 2022-11-04T13:22:18Z

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

+    )
+
+    /** Stores the current deny patterns used by this filter. */
+    private var denyPatterns = DEFAULT_DENY_PATTERNS.toPatterns()


BTW, this line confirms me that "patters" is the wrong word for the variable, because if they already were patters, there would be no point in converting them toPatterns().

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

model/src/main/kotlin/config/OrtConfiguration.kt

sschuberth · 2022-11-07T19:57:23Z

BTW @oheger-bosch, did you forget to push?

oheger-bosch · 2022-11-08T06:19:21Z

BTW @oheger-bosch, did you forget to push?

I did not manage to finish this yesterday. Sorry for the confusion. Now all changes should be available.

sschuberth

Just some (hopefully) final nits.

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

utils/common/src/test/kotlin/EnvironmentVariableFilterTest.kt

sschuberth · 2022-11-08T07:30:04Z

utils/common/src/test/kotlin/ProcessCaptureTest.kt

+
+        withEnvironment(env) {
+            val proc = if (Os.isWindows) {
+                ProcessCapture("cmd.exe", "/c", "echo %DB_USER%:%DB_PASSWORD%.%DB_CONN%")


Just wondering, why are you using . as a separator before the DB_CONN? For a more realistic example, shouldn't it be @?

Yes, but the "@" character could be problematic when it is interpreted by the shell. I did not want to mess around with escaping syntax, since for the test it is only relevant that some of the values are replaced while others are not.

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

nnobelis · 2022-11-08T09:19:01Z

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

+    /**
+     * Remove all keys from [environment] that do not pass this filter.
+     */
+    fun filter(environment: MutableMap<String, String>): MutableMap<String, String> {


Having a function that modifies it parameters AND returns the modified version is a bit of codesmell.

I would rather have either an extension function MutableMap<String, String>.filter or no return value.

I like the idea of an extension function that only filters in-place.

The parameter is returned to have a single expression in ProcessCapture where the filter is applied. I am not sure whether I understand the proposal for an extension function. How would the signature of such an extension function look like and how would it help in this context?

As written MutableMap<String, String>.filter() and applied like that:

with(EnvironmentVariableFilter) { environment().filter().putAll(environment) }

but it is more verbose.

Or remove the return value, you don't use it in ProcessCapture.

Hm, the extension function would suffer from the same codesmell problem, wouldn't it? If it returns a value (this in this case), it is not obvious that filtering was done in-place.

I am fine with removing the return value. It is used in ProcessCapture for the method chaining, but if this becomes more verbose, this should not be a big issue.

You're right: since environment() is a MutableMap and there is no 'cleaner' way of changing it, I am find to leave the code as it is.

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

cli/src/funTest/kotlin/OrtMainFunTest.kt

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt

This object contains functionality to filter out specific variables from the environment when creating child processes. It is going to be used by ProcessCapture to prevent that sensitive information is accidentally passed to external programs called by ORT. Signed-off-by: Oliver Heger <oliver.heger@bosch.io>

These properties allow the configuration of the EnvironmentVariableFilter class introduced in the previous commit. Signed-off-by: Oliver Heger <oliver.heger@bosch.io>

Before spawning a new process, apply EnvironmentVariableFilter to the environment map to make sure that only uncritical variables are passed. Signed-off-by: Oliver Heger <oliver.heger@bosch.io>

After loading the configuration, pass the properties related to filtering environment variables to EnvironmentVariableFilter, so that they are taken into account when creating new child processes. Signed-off-by: Oliver Heger <oliver.heger@bosch.io>

oheger-bosch added the breaking change label Oct 21, 2022

oheger-bosch mentioned this pull request Oct 21, 2022

Filter environment variables passed to ProcessCapture to increase security #5973

Closed

sschuberth requested changes Oct 21, 2022

View reviewed changes

nnobelis reviewed Oct 24, 2022

View reviewed changes

oheger-bosch force-pushed the oheger-bosch/utils/filter_env_variables branch from 5487735 to fbde14e Compare October 26, 2022 13:13

oheger-bosch requested review from nnobelis and sschuberth October 26, 2022 13:19

oheger-bosch marked this pull request as ready for review October 27, 2022 06:23

oheger-bosch requested a review from a team as a code owner October 27, 2022 06:23

oheger-bosch force-pushed the oheger-bosch/utils/filter_env_variables branch from fbde14e to d95f17c Compare November 2, 2022 09:02

sschuberth requested changes Nov 2, 2022

View reviewed changes

oheger-bosch force-pushed the oheger-bosch/utils/filter_env_variables branch 2 times, most recently from 65e5231 to 8703f03 Compare November 4, 2022 11:11

oheger-bosch requested a review from sschuberth November 4, 2022 11:19

oheger-bosch force-pushed the oheger-bosch/utils/filter_env_variables branch from 8703f03 to da9c584 Compare November 4, 2022 12:18

sschuberth requested changes Nov 4, 2022

View reviewed changes

oheger-bosch force-pushed the oheger-bosch/utils/filter_env_variables branch from da9c584 to 6e712ec Compare November 8, 2022 06:17

oheger-bosch requested a review from sschuberth November 8, 2022 06:19

sschuberth requested changes Nov 8, 2022

View reviewed changes

oheger-bosch force-pushed the oheger-bosch/utils/filter_env_variables branch from 6e712ec to 78058c0 Compare November 8, 2022 08:34

sschuberth requested a review from a team November 8, 2022 09:27

sschuberth previously approved these changes Nov 8, 2022

View reviewed changes

nnobelis requested changes Nov 8, 2022

View reviewed changes

oheger-bosch dismissed sschuberth’s stale review via bf094ed November 8, 2022 12:16

oheger-bosch force-pushed the oheger-bosch/utils/filter_env_variables branch from 78058c0 to bf094ed Compare November 8, 2022 12:16

nnobelis previously approved these changes Nov 8, 2022

View reviewed changes

sschuberth requested changes Nov 8, 2022

View reviewed changes

utils/common/src/main/kotlin/EnvironmentVariableFilter.kt Outdated Show resolved Hide resolved

oheger-bosch added 4 commits November 8, 2022 14:36

OrtConfiguration: Add properties for variables to pass to processes

8e2e43b

These properties allow the configuration of the EnvironmentVariableFilter class introduced in the previous commit. Signed-off-by: Oliver Heger <oliver.heger@bosch.io>

ProcessCapture: Only propagate allowed variables to child processes

8c7aa2a

Before spawning a new process, apply EnvironmentVariableFilter to the environment map to make sure that only uncritical variables are passed. Signed-off-by: Oliver Heger <oliver.heger@bosch.io>

oheger-bosch dismissed nnobelis’s stale review via 23d5657 November 8, 2022 13:36

oheger-bosch force-pushed the oheger-bosch/utils/filter_env_variables branch from bf094ed to 23d5657 Compare November 8, 2022 13:36

oheger-bosch requested review from nnobelis and sschuberth November 8, 2022 13:37

sschuberth approved these changes Nov 8, 2022

View reviewed changes

oheger-bosch merged commit 3d009a0 into oss-review-toolkit:main Nov 8, 2022

oheger-bosch deleted the oheger-bosch/utils/filter_env_variables branch November 8, 2022 14:21

sschuberth added the release notes label Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter environment variables when creating child processes #5984

Filter environment variables when creating child processes #5984

oheger-bosch commented Oct 21, 2022

codecov bot commented Oct 21, 2022 •

edited

Loading

sschuberth Oct 21, 2022

oheger-bosch Oct 24, 2022

sschuberth Oct 24, 2022

oheger-bosch Oct 24, 2022

sschuberth Oct 21, 2022

nnobelis Oct 24, 2022

oheger-bosch Oct 24, 2022

oheger-bosch commented Oct 26, 2022

sschuberth Nov 4, 2022

sschuberth Nov 4, 2022

oheger-bosch Nov 7, 2022

sschuberth Nov 4, 2022

sschuberth commented Nov 7, 2022

oheger-bosch commented Nov 8, 2022

sschuberth left a comment

sschuberth Nov 8, 2022

oheger-bosch Nov 8, 2022

nnobelis Nov 8, 2022

sschuberth Nov 8, 2022 •

edited

Loading

oheger-bosch Nov 8, 2022

nnobelis Nov 8, 2022

nnobelis Nov 8, 2022

oheger-bosch Nov 8, 2022

nnobelis Nov 8, 2022

Filter environment variables when creating child processes #5984

Filter environment variables when creating child processes #5984

Conversation

oheger-bosch commented Oct 21, 2022

codecov bot commented Oct 21, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oheger-bosch commented Oct 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth commented Nov 7, 2022

oheger-bosch commented Nov 8, 2022

sschuberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 21, 2022 •

edited

Loading

sschuberth Nov 8, 2022 •

edited

Loading