Skip to content

Commit

Permalink
[SECURITY-3314]
Browse files Browse the repository at this point in the history
  • Loading branch information
Kevin-CB authored and jenkinsci-cert-ci committed Jan 16, 2024
1 parent de45096 commit 554f037
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 2 deletions.
16 changes: 15 additions & 1 deletion core/src/main/java/hudson/cli/CLICommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.AbortException;
import hudson.Extension;
import hudson.ExtensionList;
Expand All @@ -51,6 +52,7 @@
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.model.Jenkins;
import jenkins.util.SystemProperties;
import org.apache.commons.discovery.ResourceClassIterator;
import org.apache.commons.discovery.ResourceNameIterator;
import org.apache.commons.discovery.resource.ClassLoaders;
Expand All @@ -62,6 +64,7 @@
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.CmdLineParser;
import org.kohsuke.args4j.ParserProperties;
import org.kohsuke.args4j.spi.OptionHandler;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.authentication.BadCredentialsException;
Expand Down Expand Up @@ -107,6 +110,16 @@
*/
@LegacyInstancesAreScopedToHudson
public abstract class CLICommand implements ExtensionPoint, Cloneable {

/**
* Boolean values to either allow or disallow parsing of @-prefixes.
* If a command line value starts with @, it is interpreted as being a file, loaded,
* and interpreted as if the file content would have been passed to the command line
*/
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts")
@Restricted(NoExternalUse.class)
public static boolean ALLOW_AT_SYNTAX = SystemProperties.getBoolean(CLICommand.class.getName() + ".allowAtSyntax");

/**
* Connected to stdout and stderr of the CLI agent that initiated the session.
* IOW, if you write to these streams, the person who launched the CLI command
Expand Down Expand Up @@ -307,7 +320,8 @@ private void logAndPrintError(Throwable e, String errorMessage, String logMessag
* @since 1.538
*/
protected CmdLineParser getCmdLineParser() {
return new CmdLineParser(this);
ParserProperties properties = ParserProperties.defaults().withAtSyntax(ALLOW_AT_SYNTAX);
return new CmdLineParser(this, properties);
}

/**
Expand Down
4 changes: 3 additions & 1 deletion core/src/main/java/hudson/cli/declarative/CLIRegisterer.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import org.jvnet.localizer.ResourceBundleHolder;
import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.CmdLineParser;
import org.kohsuke.args4j.ParserProperties;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.authentication.BadCredentialsException;
import org.springframework.security.core.Authentication;
Expand Down Expand Up @@ -131,7 +132,8 @@ protected CmdLineParser getCmdLineParser() {
private CmdLineParser bindMethod(List<MethodBinder> binders) {

registerOptionHandlers();
CmdLineParser parser = new CmdLineParser(null);
ParserProperties properties = ParserProperties.defaults().withAtSyntax(ALLOW_AT_SYNTAX);
CmdLineParser parser = new CmdLineParser(null, properties);

// build up the call sequence
Stack<Method> chains = new Stack<>();
Expand Down
55 changes: 55 additions & 0 deletions test/src/test/java/jenkins/security/Security3314Test.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package jenkins.security;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.not;

import hudson.cli.CLICommandInvoker;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;
import jenkins.model.Jenkins;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.jvnet.hudson.test.JenkinsRule;

@RunWith(Parameterized.class)
public class Security3314Test {
private String commandName;

@Rule
public final JenkinsRule j = new JenkinsRule();

/**
* connect-node to test the CLICommand behavior
* disable-job to test the CLIRegisterer behavior (@CLIMethod)
*/
@Parameterized.Parameters
public static List<String> commands() {
return Arrays.asList("connect-node", "disable-job");
}

public Security3314Test(String commandName) {
this.commandName = commandName;
}

@Test
public void commandShouldNotParseAt() throws Exception {
CLICommandInvoker command = new CLICommandInvoker(j, commandName);

Path tempPath = Files.createTempFile("tempFile", ".txt");
tempPath.toFile().deleteOnExit();
String content = "AtGotParsed";
Files.write(tempPath, content.getBytes());

final CLICommandInvoker.Result result = command
.authorizedTo(Jenkins.READ)
.invokeWithArgs("@" + tempPath);

assertThat(result.stderr(), containsString("@" + tempPath));
assertThat(result.stderr(), not(containsString("AtGotParsed")));
}
}

0 comments on commit 554f037

Please sign in to comment.