[SECURITY-1446]

Co-Authored-By: Francisco Javier Fernandez Gonzalez <fjfernandez@cloudbees.com>

README.md

@@ -222,6 +222,8 @@ Currently, you can provide initial secrets to JCasC that all rely on <key,value>
 substitution of strings in the configuration. For example, `Jenkins: "${some_var}"`. Default variable substitution
 using the `:-` operator from `bash` is also available. For example, `key: "${VALUE:-defaultvalue}"` will evaluate to `defaultvalue` if `$VALUE` is unset. To escape a string from secret interpolation, put `^` in front of the value. For example, `Jenkins: "^${some_var}"` will produce the literal `Jenkins: "${some_var}"`.
 
+## Secret sources
+
 We can provide these initial secrets in the following ways:
 
 - Using environment variables.
@@ -246,7 +248,30 @@ can be used as:
       secret: ${filename}
 ```
 
-- Using Vault, see following section.
+- Using Vault, see below.
+
+### Security and compatibility considerations
+
+<!-- TODO(oleg_nenashev): Add a link to the advisory once ready -->
+
+Jenkins configurations might include property definitions,
+e.g. for Token Macro resolution in Mail Ext Plugin.
+Such properties are not supposed to be resolved when importing configurations,
+but the JCasC plugin has no way to determine which variables should be resolved when reading the configurations.
+
+In some cases non-admin users can contribute to JCasC exports if they have some permissions
+(e.g. agent/view configuration or credentials management),
+and they could potentially inject variable expressions in plain text fields like descriptions 
+and then see the resolved secrets in Jenkins Web UI if the Jenkins admin exports and imports the configuration without checking contents.
+It led to a security vulnerability which was addressed in JCasC `1.25` (SECURITY-1446).
+
+* When reading configuration YAMLs, JCasC plugin will try to resolve
+  **all** variables having the `${VARNAME}` format.
+* Starting from JCasC `1.25`, JCasC export escapes the internal variable expressions,
+  e.g. as `^${VARNAME}`, so newly exported and then imported configurations are 
+  are not subject for this risk
+* For previously exported configurations, Jenkins admins are expected to manually
+  resolve the issues by putting the escape symbol `^` in front of variables which should not be resolved 
 
 ### Vault
 

integrations/src/test/java/io/jenkins/plugins/casc/MailExtTest.java

@@ -1,10 +1,13 @@
 package io.jenkins.plugins.casc;
 
 import hudson.plugins.emailext.ExtendedEmailPublisher;
+import hudson.plugins.emailext.ExtendedEmailPublisherDescriptor;
 import io.jenkins.plugins.casc.misc.ConfiguredWithCode;
 import io.jenkins.plugins.casc.misc.JenkinsConfiguredWithCodeRule;
+import io.jenkins.plugins.casc.yaml.YamlSource;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import org.apache.tools.ant.filters.StringInputStream;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.RuleChain;
@@ -14,6 +17,7 @@
 import static io.jenkins.plugins.casc.misc.Util.assertLogContains;
 import static io.jenkins.plugins.casc.misc.Util.assertNotInLog;
 import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
@@ -43,4 +47,30 @@ public void shouldNotExportOrLogCredentials() throws Exception {
         assertThat("No entry was exported for SMTP credentials", exportedConfig, containsString("smtpPassword"));
         assertThat("There should be no SMTP password in the exported YAML", exportedConfig, not(containsString(SMTP_PASSWORD)));
     }
+
+    @Test
+    @Issue("SECURITY-1446")
+    public void shouldProperlyRoundTripTokenMacro() throws Exception {
+        final String defaultBody = "${PROJECT_NAME} - Build # ${BUILD_NUMBER} - ${BUILD_STATUS}:\n" +
+                "Check console output at $BUILD_URL to view the results.";
+        // This string contains extra escaping
+        final String defaultSubject = "^^^${PROJECT_NAME} - Build # ^^${BUILD_NUMBER} - ^${BUILD_STATUS}!";
+
+        ExtendedEmailPublisherDescriptor descriptor = ExtendedEmailPublisher.descriptor();
+        descriptor.setDefaultBody(defaultBody);
+        descriptor.setDefaultSubject(defaultSubject);
+
+        // Verify that the variables get exported properly
+        String exportedConfig = j.exportToString(false);
+        assertThat("PROJECT_NAME should be escaped", exportedConfig, containsString("^${PROJECT_NAME}"));
+        assertThat("BUILD_NUMBER should be escaped", exportedConfig, containsString("^${BUILD_NUMBER}"));
+        assertThat("BUILD_STATUS should be escaped", exportedConfig, containsString("^${BUILD_STATUS}"));
+
+        // Reimport the configuration
+        ConfigurationAsCode.get().configureWith(YamlSource.of(new StringInputStream(exportedConfig)));
+        assertLogContains(logging, "defaultBody =");
+        assertLogContains(logging, "defaultSubject =");
+        assertThat(ExtendedEmailPublisher.descriptor().getDefaultBody(), equalTo(defaultBody));
+        assertThat(ExtendedEmailPublisher.descriptor().getDefaultSubject(), equalTo(defaultSubject));
+    }
 }

integrations/src/test/java/io/jenkins/plugins/casc/Security1446Test.java

@@ -0,0 +1,83 @@
+package io.jenkins.plugins.casc;
+
+import com.cloudbees.plugins.credentials.CredentialsProvider;
+import com.cloudbees.plugins.credentials.CredentialsScope;
+import com.cloudbees.plugins.credentials.casc.CredentialsRootConfigurator;
+import com.cloudbees.plugins.credentials.common.StandardUsernamePasswordCredentials;
+import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl;
+import hudson.ExtensionList;
+import io.jenkins.plugins.casc.impl.configurators.DataBoundConfigurator;
+import io.jenkins.plugins.casc.misc.ConfiguredWithCode;
+import io.jenkins.plugins.casc.misc.JenkinsConfiguredWithCodeRule;
+import io.jenkins.plugins.casc.model.CNode;
+import io.jenkins.plugins.casc.snakeyaml.error.YAMLException;
+import io.jenkins.plugins.casc.snakeyaml.nodes.Node;
+import java.io.IOException;
+import java.io.StringWriter;
+import java.util.Collections;
+import java.util.List;
+import jenkins.model.Jenkins;
+import org.jenkinsci.plugins.plaincredentials.StringCredentials;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+
+public class Security1446Test {
+
+    @Rule
+    public JenkinsConfiguredWithCodeRule j = new JenkinsConfiguredWithCodeRule();
+
+    private static final String PATH_PATTERN = "path = \\$\\{PATH\\}";
+    private static final String JAVA_HOME_PATTERN = "java-home = \\$\\{JAVA_HOME\\}";
+
+    @ConfiguredWithCode("Security1446Test.yml")
+    @Test
+    @Issue("SECURITY-1446")
+    public void testImportWithEnvVar() {
+        List<StandardUsernamePasswordCredentials> userPasswCred = CredentialsProvider.lookupCredentials(StandardUsernamePasswordCredentials.class,Jenkins.getInstanceOrNull(), null, Collections.emptyList());
+        assertThat(userPasswCred.size(), is(1));
+        for (StandardUsernamePasswordCredentials cred : userPasswCred) {
+            assertTrue("The JAVA_HOME environment variable should not be resolved", cred.getUsername().matches(JAVA_HOME_PATTERN));
+            assertTrue("The PATH environment variable should not be resolved", cred.getDescription().matches(PATH_PATTERN));
+        }
+
+        List<StringCredentials> stringCred = CredentialsProvider.lookupCredentials(StringCredentials.class,Jenkins.getInstanceOrNull(), null, Collections.emptyList());
+        assertThat(stringCred.size(), is(1));
+        for (StringCredentials cred : stringCred) {
+            assertTrue("The PATH environment variable should not be resolved", cred.getDescription().matches(PATH_PATTERN));
+        }
+    }
+
+    @Test
+    @Issue("SECURITY-1446")
+    public void testExportWithEnvVar() throws Exception {
+        final String message = "Hello, world! PATH=${PATH} JAVA_HOME=^${JAVA_HOME}";
+        ConfiguratorRegistry registry = ConfiguratorRegistry.get();
+        ConfigurationContext context = new ConfigurationContext(registry);
+        CredentialsRootConfigurator root = ExtensionList.lookupSingleton(CredentialsRootConfigurator.class);
+
+        DataBoundConfigurator<UsernamePasswordCredentialsImpl> configurator = new DataBoundConfigurator<>(UsernamePasswordCredentialsImpl.class);
+        UsernamePasswordCredentialsImpl creds = new UsernamePasswordCredentialsImpl(CredentialsScope.GLOBAL, "test",
+                message, "foo", "bar");
+        final CNode config = configurator.describe(creds, context);
+        final Node valueNode = ConfigurationAsCode.get().toYaml(config);
+        final String exported;
+        try (StringWriter writer = new StringWriter()) {
+            ConfigurationAsCode.serializeYamlNode(valueNode, writer);
+            exported = writer.toString();
+        } catch (IOException e) {
+            throw new YAMLException(e);
+        }
+
+        assertThat("Message was not escaped", exported, not(containsString(message)));
+        assertThat("Improper masking for PATH", exported, containsString("^${PATH}"));
+        assertThat("Improper masking for JAVA_HOME", exported, containsString("^^${JAVA_HOME}"));
+    }
+
+}

integrations/src/test/resources/io/jenkins/plugins/casc/Security1446Test.yml

@@ -0,0 +1,15 @@
+credentials:
+  system:
+    domainCredentials:
+    - credentials:
+        - string:
+            description: "path = ^${PATH}"
+            id: "system secret escaped"
+            scope: SYSTEM
+            secret: "{AQAAABAAAAAQwDmo12BE6995SezdXe1Y9ewaoYR6KgzX46VjtoPDqE0=}"
+        - usernamePassword:
+            description: "path = ^${PATH}"
+            id: "global user-passw escaped"
+            password: "{AQAAABAAAAAQpNs2vtahkRGcR5vzanaIb4UJkCyeNGdP23/X3+Tl1Ic=}"
+            scope: GLOBAL
+            username: "java-home = ^${JAVA_HOME}"

plugin/pom.xml

@@ -17,6 +17,11 @@
   <description>Manage Jenkins master configuration as code</description>
   <url>https://wiki.jenkins.io/display/JENKINS/Configuration+as+Code+Plugin</url>
 
+  <properties>
+    <!-- SECURITY-1446 and SECURITY-1290 are considered as breaking changes. Others also contain a risk of regressions -->
+    <hpi.compatibleSinceVersion>1.25</hpi.compatibleSinceVersion>
+  </properties>
+
   <developers>
     <developer>
       <id>casz</id>
@@ -44,6 +49,7 @@
       <version>4.0.0</version>
     </dependency>
 
+    <!--TODO: Not longer used, remove it? -->
     <dependency>
       <groupId>org.bigtesting</groupId>
       <artifactId>interpolatd</artifactId>

plugin/src/main/java/io/jenkins/plugins/casc/SecretSourceResolver.java

@@ -6,36 +6,69 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.stream.Stream;
-import org.bigtesting.interpolatd.Interpolator;
+import javax.annotation.CheckForNull;
+import org.apache.commons.lang.text.StrLookup;
+import org.apache.commons.lang.text.StrSubstitutor;
 
 import static io.vavr.API.unchecked;
 
+/**
+ * Resolves secret variables and converts escaped internal variables.
+ */
 public class SecretSourceResolver {
     private static final String enclosedBy = "${";
     private static final String enclosedIn = "}";
-    private static final String escapedWith = "^";
+    private static final char escapedWith = '^';
     private static final String defaultDelimiter = ":-";
 
     private static final Logger LOGGER = Logger.getLogger(SecretSourceResolver.class.getName());
 
+    /**
+     * Encodes String so that it can be safely represented in the YAML after export.
+     * @param toEncode String to encode
+     * @return Encoded string
+     * @since TODO
+     */
+    public static String encode(@CheckForNull String toEncode) {
+        if (toEncode == null) {
+            return null;
+        }
+        return toEncode.replace("${", "^${");
+    }
+
     public static String resolve(ConfigurationContext context, String toInterpolate) {
-        return interpolator(context).interpolate(toInterpolate, "");
+        return substitutor(context).replace(toInterpolate);
+    }
+
+    private static StrSubstitutor substitutor(ConfigurationContext context) {
+        StrSubstitutor substitutor = new StrSubstitutor(new ConfigurationContextStrLookup(context));
+        substitutor.setEscapeChar(escapedWith);
+        substitutor.setVariablePrefix(enclosedBy);
+        substitutor.setVariableSuffix(enclosedIn);
+        return substitutor;
     }
 
-    private static Interpolator<String> interpolator(ConfigurationContext context) {
-        Interpolator<String> interpolator = new Interpolator<>();
-        interpolator.when().enclosedBy(enclosedBy).and(enclosedIn).handleWith((captured, argument) -> handle(context, captured));
-        interpolator.escapeWith(escapedWith);
-        return interpolator;
+    private static String handleJenkinsVariableDeclaration(ConfigurationContext context, String captured) {
+        return enclosedBy + captured + enclosedIn;
     }
 
-    private static String handle(ConfigurationContext context, String captured) {
-        String[] split = captured.split(defaultDelimiter, 2);
-        return Tuple.of(split[0], Try.of(() -> split[1]).toJavaOptional()).apply(
-            (toReveal, defaultValue) -> reveal(context, toReveal)
-                .map(Optional::of)
-                .orElse(defaultValue)
-                .orElseGet(() -> handleUndefinedVariable(captured)));
+    private static class ConfigurationContextStrLookup extends StrLookup {
+
+        private final ConfigurationContext context;
+
+        public ConfigurationContextStrLookup(ConfigurationContext context) {
+            this.context = context;
+        }
+
+        @Override
+        public String lookup(String key) {
+            String[] split = key.split(defaultDelimiter, 2);
+            return Tuple.of(split[0], Try.of(() -> split[1]).toJavaOptional()).apply(
+                    (toReveal, defaultValue) -> reveal(context, toReveal)
+                            .map(Optional::of)
+                            .orElse(defaultValue)
+                            .orElseGet(() -> handleUndefinedVariable(key)));
+        }
     }
 
     private static String handleUndefinedVariable(String captured) {

plugin/src/main/java/io/jenkins/plugins/casc/impl/configurators/PrimitiveConfigurator.java

@@ -69,7 +69,7 @@ public CNode describe(Object instance, ConfigurationContext context) {
             return new Scalar((Enum) instance);
         }
 
-        return new Scalar(String.valueOf(instance));
+        return new Scalar(SecretSourceResolver.encode(String.valueOf(instance)));
     }
 
     @NonNull

plugin/src/test/java/io/jenkins/plugins/casc/SecretSourceResolverTest.java

@@ -8,8 +8,10 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.contrib.java.lang.system.EnvironmentVariables;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.LoggerRule;
+import org.jvnet.hudson.test.WithoutJenkins;
 
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.junit.Assert.assertThat;
@@ -71,6 +73,11 @@ public void resolve_singleEntryEscaped() {
         assertThat(SecretSourceResolver.resolve(context, "^${FOO}"), equalTo("${FOO}"));
     }
 
+    @Test
+    public void resolve_singleEntryDoubleEscaped() {
+        assertThat(SecretSourceResolver.resolve(context, "^^${FOO}"), equalTo("^${FOO}"));
+    }
+
     @Test
     public void resolve_multipleEntries() {
         environment.set("FOO", "hello");
@@ -107,12 +114,12 @@ public void resolve_nothing() {
 
     @Test
     public void resolve_nothingSpace() {
-        assertThat(SecretSourceResolver.resolve(context, "${ }"), equalTo("${ }"));
+        assertThat(SecretSourceResolver.resolve(context, "${ }"), equalTo(""));
     }
 
     @Test
     public void resolve_nothingBrackets() {
-        assertThat(SecretSourceResolver.resolve(context, "${}"), equalTo("${}"));
+        assertThat(SecretSourceResolver.resolve(context, "${}"), equalTo(""));
     }
 
     @Test
@@ -171,4 +178,18 @@ public void resolve_mixedMultipleEntriesWithDefault() {
     public void resolve_mixedMultipleEntriesEscaped() {
         assertThat(SecretSourceResolver.resolve(context, "http://^${FOO}:^${BAR}"), equalTo("http://${FOO}:${BAR}"));
     }
+
+    @Test
+    @Issue("SECURITY-1446")
+    @WithoutJenkins
+    public void shouldEncodeInternalVarsProperly() {
+        assertVarEncoding("^${TEST}", "${TEST}");
+        assertVarEncoding("^^${TEST}", "^${TEST}");
+        assertVarEncoding("$TEST", "$TEST");
+    }
+
+    private static void assertVarEncoding(String expected, String toEncode) {
+        String encoded = SecretSourceResolver.encode(toEncode);
+        assertThat(encoded, equalTo(expected));
+    }
 }