Skip to content

Commit

Permalink
[SECURITY-2455]
Browse files Browse the repository at this point in the history
  • Loading branch information
daniel-beck authored and jenkinsci-cert-ci committed Oct 25, 2021
1 parent 87a500a commit 63cde2d
Show file tree
Hide file tree
Showing 7 changed files with 991 additions and 132 deletions.
291 changes: 168 additions & 123 deletions core/src/main/java/hudson/FilePath.java

Large diffs are not rendered by default.

38 changes: 34 additions & 4 deletions core/src/main/java/jenkins/SoloFilePathFilter.java
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
package jenkins;

import edu.umd.cs.findbugs.annotations.Nullable;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.FilePath;
import jenkins.util.SystemProperties;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import java.io.File;
import java.util.UUID;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
* Variant of {@link FilePathFilter} that assumes it is the sole actor.
Expand All @@ -13,6 +21,13 @@
* @author Kohsuke Kawaguchi
*/
public final class SoloFilePathFilter extends FilePathFilter {

private static final Logger LOGGER = Logger.getLogger(SoloFilePathFilter.class.getName());

@SuppressFBWarnings("MS_SHOULD_BE_FINAL")
@Restricted(NoExternalUse.class)
public static /* non-final for Groovy */ boolean REDACT_ERRORS = SystemProperties.getBoolean(SoloFilePathFilter.class.getName() + ".redactErrors", true);

private final FilePathFilter base;

private SoloFilePathFilter(FilePathFilter base) {
Expand All @@ -28,8 +43,17 @@ private SoloFilePathFilter(FilePathFilter base) {
}

private boolean noFalse(String op, File f, boolean b) {
if (!b)
throw new SecurityException("agent may not " + op + " " + f+"\nSee https://www.jenkins.io/redirect/security-144 for more details");
if (!b) {
final String detailedMessage = "Agent may not '" + op + "' at '" + f + "'. See https://www.jenkins.io/redirect/security-144 for more information.";
if (REDACT_ERRORS) {
// We may end up trying to access file paths indirectly, e.g. FilePath#listFiles starts in an allowed dir but follows symlinks outside, so do not disclose paths in error message
UUID uuid = UUID.randomUUID();
LOGGER.log(Level.WARNING, () -> uuid + ": " + detailedMessage);
throw new SecurityException("Agent may not access a file path. See the system log for more details about the error ID '" + uuid + "' and https://www.jenkins.io/redirect/security-144 for more information.");
} else {
throw new SecurityException(detailedMessage);
}
}
return true;
}

Expand All @@ -49,12 +73,18 @@ public boolean write(File f) throws SecurityException {

@Override
public boolean symlink(File f) throws SecurityException {
return noFalse("symlink",f,base.write(normalize(f)));
return noFalse("symlink",f,base.symlink(normalize(f)));
}

@Override
public boolean mkdirs(File f) throws SecurityException {
return noFalse("mkdirs",f,base.mkdirs(normalize(f)));
// mkdirs is special because it could operate on parents of the specified path
File reference = normalize(f);
while (reference != null && !reference.exists()) {
noFalse("mkdirs", f, base.mkdirs(reference)); // Pass f as reference into the error to be vague
reference = reference.getParentFile();
}
return true;
}

@Override
Expand Down
24 changes: 20 additions & 4 deletions core/src/main/java/jenkins/security/s2m/FilePathRuleConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@
import hudson.Functions;
import hudson.model.Failure;
import java.io.File;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import jenkins.model.Jenkins;
Expand All @@ -21,6 +25,9 @@
* @author Kohsuke Kawaguchi
*/
class FilePathRuleConfig extends ConfigDirectory<FilePathRule,List<FilePathRule>> {

private static final Logger LOGGER = Logger.getLogger(FilePathRuleConfig.class.getName());

FilePathRuleConfig(File file) {
super(file);
}
Expand All @@ -40,10 +47,17 @@ protected FilePathRule parse(String line) {
line = line.trim();
if (line.isEmpty()) return null;

// TODO This does not support custom build dir configuration (Jenkins#getRawBuildsDir() etc.)
line = line.replace("<BUILDDIR>","<JOBDIR>/builds/<BUILDID>");
line = line.replace("<BUILDID>","(?:[0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]_[0-9][0-9]-[0-9][0-9]-[0-9][0-9]|[0-9]+)");
line = line.replace("<JOBDIR>","<JENKINS_HOME>/jobs/.+");
line = line.replace("<JENKINS_HOME>","\\Q"+Jenkins.get().getRootDir().getPath()+"\\E");
final File jenkinsHome = Jenkins.get().getRootDir();
try {
line = line.replace("<JENKINS_HOME>","\\Q" + jenkinsHome.getCanonicalPath() + "\\E");
} catch (IOException e) {
LOGGER.log(Level.WARNING, e, () -> "Failed to determine canonical path to Jenkins home directory, falling back to configured value: " + jenkinsHome.getPath());
line = line.replace("<JENKINS_HOME>","\\Q" + jenkinsHome.getPath() + "\\E");
}

// config file is always /-separated even on Windows, so bring it back to \-separation.
// This is done in the context of regex, so it has to be \\, which means in the source code it is \\\\
Expand Down Expand Up @@ -77,9 +91,11 @@ public boolean checkFileAccess(String op, File path) throws SecurityException {
for (FilePathRule rule : get()) {
if (rule.op.matches(op)) {
if (pathStr==null) {
// do not canonicalize, so that JENKINS_HOME that spans across
// multiple volumes via symlinks can look logically like one unit.
pathStr = path.getPath();
try {
pathStr = path.getCanonicalPath();
} catch (IOException ex) {
throw new UncheckedIOException(ex);
}
if (isWindows()) // Windows accepts '/' as separator, but for rule matching we want to normalize for consistent comparison
pathStr = pathStr.replace('/','\\');
}
Expand Down
Loading

0 comments on commit 63cde2d

Please sign in to comment.