Filter environment variables when creating child processes

cli/src/funTest/kotlin/OrtMainFunTest.kt

@@ -37,6 +37,7 @@ import org.ossreviewtoolkit.model.config.OrtConfiguration
 import org.ossreviewtoolkit.model.config.OrtConfigurationWrapper
 import org.ossreviewtoolkit.model.readValue
 import org.ossreviewtoolkit.model.writeValue
+import org.ossreviewtoolkit.utils.common.EnvironmentVariableFilter
 import org.ossreviewtoolkit.utils.common.redirectStdout
 import org.ossreviewtoolkit.utils.ort.normalizeVcsUrl
 import org.ossreviewtoolkit.utils.test.createSpecTempFile
@@ -202,6 +203,18 @@ class OrtMainFunTest : StringSpec() {
                 )
             }
         }
+
+        "EnvironmentVariableFilter is correctly initialized" {
+            val referenceConfigFile = File("../model/src/main/resources/reference.yml").absolutePath
+            runMain(
+                "-c",
+                referenceConfigFile,
+                "config"
+            )
+
+            EnvironmentVariableFilter.isAllowed("PASSPORT") shouldBe true
+            EnvironmentVariableFilter.isAllowed("DB_PASS") shouldBe false
+        }
     }
 
     private fun runMain(vararg args: String) =

cli/src/main/kotlin/OrtMain.kt

@@ -45,6 +45,7 @@ import org.ossreviewtoolkit.cli.commands.*
 import org.ossreviewtoolkit.cli.utils.logger
 import org.ossreviewtoolkit.model.config.LicenseFilenamePatterns
 import org.ossreviewtoolkit.model.config.OrtConfiguration
+import org.ossreviewtoolkit.utils.common.EnvironmentVariableFilter
 import org.ossreviewtoolkit.utils.common.Os
 import org.ossreviewtoolkit.utils.common.expandTilde
 import org.ossreviewtoolkit.utils.ort.Environment
@@ -187,6 +188,11 @@ class OrtMain : CliktCommand(name = ORT_NAME, invokeWithoutSubcommand = true) {
         currentContext.findOrSetObject { GlobalOptions(ortConfiguration, forceOverwrite) }
         LicenseFilenamePatterns.configure(ortConfiguration.licenseFilePatterns)
 
+        EnvironmentVariableFilter.reset(
+            ortConfiguration.deniedProcessEnvironmentVariablesSubstrings,
+            ortConfiguration.allowedProcessEnvironmentVariableNames
+        )
+
         if (helpAll) {
             registeredSubcommands().forEach {
                 println(it.getFormattedHelp())

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

@@ -29,6 +29,7 @@ import java.io.File
 import org.apache.logging.log4j.kotlin.Logging
 
 import org.ossreviewtoolkit.model.Severity
+import org.ossreviewtoolkit.utils.common.EnvironmentVariableFilter
 
 /**
  * The configuration model for all ORT components.
@@ -68,6 +69,19 @@ data class OrtConfiguration(
      */
     val enableRepositoryPackageConfigurations: Boolean = false,
 
+    /**
+     * A set with substrings to filter out environment variables before creating child processes to prevent that those
+     * processes can access sensitive information. See [EnvironmentVariableFilter] for further details.
+     */
+    val deniedProcessEnvironmentVariablesSubstrings: Set<String> = EnvironmentVariableFilter.DEFAULT_DENY_SUBSTRINGS,
+
+    /**
+     * A set with the names of environment variables that are explicitly allowed to be passed to child processes,
+     * even if they are matched by one of the [deniedProcessEnvironmentVariablesSubstrings].
+     * See [EnvironmentVariableFilter] for further details.
+     */
+    val allowedProcessEnvironmentVariableNames: Set<String> = EnvironmentVariableFilter.DEFAULT_ALLOW_NAMES,
+
     /**
      * The configuration of the analyzer.
      */

model/src/main/resources/reference.yml

@@ -30,6 +30,15 @@ ort:
   enableRepositoryPackageCurations: true
   enableRepositoryPackageConfigurations: true
 
+  deniedProcessEnvironmentVariablesSubstrings:
+    - PASS
+    - SECRET
+    - TOKEN
+    - USER
+  allowedProcessEnvironmentVariableNames:
+    - PASSPORT
+    - USER_HOME
+
   analyzer:
     allowDynamicVersions: true
 

model/src/test/kotlin/config/OrtConfigurationTest.kt

@@ -24,6 +24,7 @@ import io.kotest.core.TestConfiguration
 import io.kotest.core.spec.style.WordSpec
 import io.kotest.extensions.system.withEnvironment
 import io.kotest.matchers.collections.containExactly
+import io.kotest.matchers.collections.containExactlyInAnyOrder
 import io.kotest.matchers.collections.shouldContainExactly
 import io.kotest.matchers.collections.shouldContainExactlyInAnyOrder
 import io.kotest.matchers.maps.containExactly as containExactlyEntries
@@ -37,6 +38,7 @@ import java.io.File
 import java.lang.IllegalArgumentException
 
 import org.ossreviewtoolkit.model.SourceCodeOrigin
+import org.ossreviewtoolkit.utils.common.EnvironmentVariableFilter
 import org.ossreviewtoolkit.utils.test.createTestTempFile
 import org.ossreviewtoolkit.utils.test.shouldNotBeNull
 
@@ -46,6 +48,11 @@ class OrtConfigurationTest : WordSpec({
             val refConfig = File("src/main/resources/$REFERENCE_CONFIG_FILENAME")
             val ortConfig = OrtConfiguration.load(file = refConfig)
 
+            ortConfig.deniedProcessEnvironmentVariablesSubstrings should containExactlyInAnyOrder(
+                "PASS", "SECRET", "TOKEN", "USER"
+            )
+            ortConfig.allowedProcessEnvironmentVariableNames should containExactlyInAnyOrder("PASSPORT", "USER_HOME")
+
             with(ortConfig.analyzer) {
                 allowDynamicVersions shouldBe true
 
@@ -358,6 +365,15 @@ class OrtConfigurationTest : WordSpec({
                 }
             }
         }
+
+        "use defaults for propagating environment variables to child processes" {
+            val config = OrtConfiguration()
+
+            with(config) {
+                deniedProcessEnvironmentVariablesSubstrings shouldBe EnvironmentVariableFilter.DEFAULT_DENY_SUBSTRINGS
+                allowedProcessEnvironmentVariableNames shouldBe EnvironmentVariableFilter.DEFAULT_ALLOW_NAMES
+            }
+        }
     }
 })
 

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

@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) 2022 The ORT Project Authors (see <https://github.com/oss-review-toolkit/ort/blob/main/NOTICE>)
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ * License-Filename: LICENSE
+ */
+
+package org.ossreviewtoolkit.utils.common
+
+/**
+ * An object providing functionality to filter environments that are passed to newly created processes.
+ *
+ * For many tasks, ORT spawns new processes using the [ProcessCapture] class. When creating a new process, the child
+ * process by default inherits all environment variables from the parent. This could impose a security risk, for
+ * instance if logic in build scripts could theoretically access sensitive information stored in environment
+ * variables, such as database or service credentials.
+ *
+ * To reduce this risk, this object filters the environment variables passed to child processes based on the
+ * following criteria:
+ * - Substrings for variable names can be defined to determine variables with sensitive information. The object provides
+ *   some default strings to match variable names like "PASS", "USER", "TOKEN", etc.
+ * - There is an allow list to include variables even if they contain one of these substrings.
+ *
+ * So in order to determine whether a specific variable "E" can be passed to a child process, this filter applies the
+ * following steps:
+ * - If E is contained in the allow list, it is included.
+ * - Otherwise, E is included if and only if its name does not contain one of the exclusion substrings (ignoring case).
+ *
+ * TODO: Find an alternative mechanism to initialize this object from the ORT configuration (maybe using dependency
+ *       injection) which does not require this object to be public.
+ */
+object EnvironmentVariableFilter {
+    /**
+     * A set with substrings contained in 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_SUBSTRINGS = setOf(
+        "key",
+        "pass",
+        "pwd",
+        "token",
+        "user"
+    )
+
+    /** A set of known variable names that are allowed despite being matched by deny substrings. */
+    val DEFAULT_ALLOW_NAMES = setOf(
+        "CARGO_HTTP_USER_AGENT",
+        "COMPOSER_ALLOW_SUPERUSER",
+        "CONAN_LOGIN_ENCRYPTION_KEY",
+        "CONAN_LOGIN_USERNAME",
+        "CONAN_PASSWORD",
+        "CONAN_USERNAME",
+        "CONAN_USER_HOME",
+        "CONAN_USER_HOME_SHORT",
+        "DOTNET_CLI_CONTEXT_ANSI_PASS_THRU",
+        "GIT_ASKPASS",
+        "GIT_HTTP_USER_AGENT",
+        "GRADLE_USER_HOME",
+        "HACKAGE_USERNAME",
+        "HACKAGE_PASSWORD",
+        "HACKAGE_KEY",
+        "PWD",
+        "USER",
+        "USERPROFILE"
+    )
+
+    /** Stores the current deny substrings used by this filter. */
+    private var denySubstrings = DEFAULT_DENY_SUBSTRINGS
+
+    /** Stores the current allow names used by this filter. */
+    private var allowNames = DEFAULT_ALLOW_NAMES
+
+    /**
+     * Reset this filter to use the given [denySubstrings] and [allowNames].
+     */
+    fun reset(
+        denySubstrings: Collection<String> = DEFAULT_DENY_SUBSTRINGS,
+        allowNames: Collection<String> = DEFAULT_ALLOW_NAMES
+    ) {
+        this.denySubstrings = denySubstrings.toSet()
+        this.allowNames = allowNames.toSet()
+    }
+
+    /**
+     * Test whether the variable with the given [name] can be passed to a child process according to the criteria
+     * described in the header comment.
+     */
+    fun isAllowed(name: String): Boolean {
+        return name in allowNames || denySubstrings.none { name.contains(it, ignoreCase = true) }
+    }
+
+    /**
+     * Remove all keys from [environment] that do not pass this filter.
+     */
+    fun filter(environment: MutableMap<String, String>): MutableMap<String, String> {
+        val keysToRemove = environment.keys.filterNot(EnvironmentVariableFilter::isAllowed)
+
+        @Suppress("ConvertArgumentToSet") // The list cannot contain duplicates.
+        environment -= keysToRemove
+
+        return environment
+    }
+}

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

@@ -84,7 +84,9 @@ class ProcessCapture(
         .redirectOutput(stdoutFile)
         .redirectError(stderrFile)
         .apply {
-            environment().putAll(environment)
+            // Apply the environment filter to the map with inherited variables, but not to the variables added
+            // explicitly via the environment parameter, since these are expected to be always relevant for the command.
+            EnvironmentVariableFilter.filter(environment()).putAll(environment)
         }
 
     val commandLine = command.joinToString(" ")

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

@@ -0,0 +1,83 @@
+/*
+ * Copyright (C) 2022 The ORT Project Authors (see <https://github.com/oss-review-toolkit/ort/blob/main/NOTICE>)
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ * License-Filename: LICENSE
+ */
+
+package org.ossreviewtoolkit.utils.common
+
+import io.kotest.core.spec.style.StringSpec
+import io.kotest.inspectors.forAll
+import io.kotest.matchers.maps.containExactly
+import io.kotest.matchers.should
+import io.kotest.matchers.shouldBe
+import io.kotest.matchers.types.beTheSameInstanceAs
+
+class EnvironmentVariableFilterTest : StringSpec({
+    afterAny {
+        // Reset default values after each test.
+        EnvironmentVariableFilter.reset()
+    }
+
+    "Uncritical variables should be accepted" {
+        listOf("FOO", "bar", "boring", "uncritical", "PATH").forAll { variable ->
+            EnvironmentVariableFilter.isAllowed(variable) shouldBe true
+        }
+    }
+
+    "Variables should be filtered based on default deny substrings" {
+        listOf("DATABASE_PASSWORD", "repositoryToken", "service-user", "APIKEY").forAll { variable ->
+            EnvironmentVariableFilter.isAllowed(variable) shouldBe false
+        }
+    }
+
+    "Variables should be accepted if they are on the allowed list" {
+        listOf("USER", "USERPROFILE", "GRADLE_USER_HOME", "GIT_HTTP_USER_AGENT").forAll { variable ->
+            EnvironmentVariableFilter.isAllowed(variable) shouldBe true
+        }
+    }
+
+    "Denied substrings can be changed" {
+        EnvironmentVariableFilter.reset(listOf("foo"))
+
+        EnvironmentVariableFilter.isAllowed("ENV_FOO") shouldBe false
+        EnvironmentVariableFilter.isAllowed("MY_PASSWORD") shouldBe true
+    }
+
+    "Allowed names can be changed" {
+        val allowedSecret = "MAVEN_REPO_PASSWORD"
+
+        EnvironmentVariableFilter.reset(allowNames = setOf(allowedSecret))
+
+        EnvironmentVariableFilter.isAllowed(allowedSecret) shouldBe true
+    }
+
+    "The environment map can be filtered" {
+        val environment = mutableMapOf(
+            "USER" to "scott",
+            "password" to "tiger",
+            "foo" to "bar"
+        )
+        val filteredEnvironment = mapOf(
+            "USER" to "scott",
+            "foo" to "bar"
+        )
+
+        EnvironmentVariableFilter.filter(environment) should beTheSameInstanceAs(environment)
+
+        environment should containExactly(filteredEnvironment)
+    }
+})

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

@@ -20,6 +20,7 @@
 package org.ossreviewtoolkit.utils.common
 
 import io.kotest.core.spec.style.StringSpec
+import io.kotest.extensions.system.withEnvironment
 import io.kotest.matchers.shouldBe
 import io.kotest.matchers.string.shouldContain
 
@@ -36,6 +37,21 @@ class ProcessCaptureTest : StringSpec({
         proc.stdout.trimEnd() shouldBe "This is some path: /foo/bar"
     }
 
+    "Only allowed environment variables should be passed" {
+        val env = mapOf("DB_USER" to "scott", "DB_PASSWORD" to "tiger", "DB_CONN" to "my-db.example.org")
+
+        withEnvironment(env) {
+            val proc = if (Os.isWindows) {
+                ProcessCapture("cmd.exe", "/c", "echo %DB_USER%:%DB_PASSWORD%.%DB_CONN%")
+            } else {
+                ProcessCapture("sh", "-c", "echo \$DB_USER:\$DB_PASSWORD.\$DB_CONN")
+            }
+
+            proc.exitValue shouldBe 0
+            proc.stdout.trimEnd() shouldBe ":.my-db.example.org"
+        }
+    }
+
     "Masked strings should be processed correctly" {
         val masked = MaskedString("echo unmasked")
         val proc = if (Os.isWindows) {