From 348a26f77eec159261d5567b5228300ff140996d Mon Sep 17 00:00:00 2001 From: Jeff Thompson Date: Fri, 20 Dec 2019 15:01:47 -0700 Subject: [PATCH] Narrow the suppression. And add TODO connected with JENKINS tickets for relevant issues. --- cli/src/main/java/hudson/cli/CLI.java | 14 +++++++++-- .../java/hudson/cli/FullDuplexHttpStream.java | 13 ++++++---- .../java/hudson/cli/NoCheckTrustManager.java | 4 +++- cli/src/main/java/hudson/cli/SSHCLI.java | 8 +++++-- .../java/hudson/ClassicPluginStrategy.java | 2 +- core/src/main/java/hudson/Util.java | 13 +++++++--- core/src/main/java/hudson/cli/Connection.java | 12 ++++++---- .../hudson/console/AnnotatedLargeText.java | 8 +++++-- .../main/java/hudson/console/ConsoleNote.java | 8 +++++-- core/src/main/java/hudson/scheduler/Hash.java | 9 +++++-- .../security/BasicAuthenticationFilter.java | 10 +++++--- .../HudsonAuthenticationEntryPoint.java | 24 +++++++++++-------- .../main/java/hudson/util/ConsistentHash.java | 10 ++++++-- .../java/hudson/util/DOSToUnixPathHelper.java | 8 ++++--- .../java/hudson/util/FormFieldValidator.java | 9 ++++--- .../java/jenkins/model/RunIdMigrator.java | 9 +++++-- .../model/identity/IdentityRootAction.java | 8 ++++++- .../slaves/JnlpSlaveAgentProtocol4.java | 10 ++++---- 18 files changed, 127 insertions(+), 52 deletions(-) diff --git a/cli/src/main/java/hudson/cli/CLI.java b/cli/src/main/java/hudson/cli/CLI.java index 4cf98e85c1..8cbc3882de 100644 --- a/cli/src/main/java/hudson/cli/CLI.java +++ b/cli/src/main/java/hudson/cli/CLI.java @@ -191,7 +191,7 @@ public class CLI { continue; } if(head.equals("-i") && args.size()>=2) { - File f = new File(args.get(1)); + File f = getFileFromArguments(args); if (!f.exists()) { printUsage(Messages.CLI_NoSuchFileExists(f)); return -1; @@ -293,7 +293,7 @@ public class CLI { if (userInfo != null) { factory = factory.basicAuth(userInfo); } else if (auth != null) { - factory = factory.basicAuth(auth.startsWith("@") ? FileUtils.readFileToString(new File(auth.substring(1)), Charset.defaultCharset()).trim() : auth); + factory = factory.basicAuth(auth.startsWith("@") ? readAuthFromFile(auth).trim() : auth); } if (mode == Mode.HTTP) { @@ -307,6 +307,16 @@ public class CLI { throw new AssertionError(); } + @SuppressFBWarnings(value = {"PATH_TRAVERSAL_IN", "URLCONNECTION_SSRF_FD"}, justification = "User provided values for running the program.") + private static String readAuthFromFile(String auth) throws IOException { + return FileUtils.readFileToString(new File(auth.substring(1)), Charset.defaultCharset()); + } + + @SuppressFBWarnings(value = {"PATH_TRAVERSAL_IN", "URLCONNECTION_SSRF_FD"}, justification = "User provided values for running the program.") + private static File getFileFromArguments(List args) { + return new File(args.get(1)); + } + private static int webSocketConnection(String url, List args, CLIConnectionFactory factory) throws Exception { LOGGER.fine(() -> "Trying to connect to " + url + " via plain protocol over WebSocket"); class CLIEndpoint extends Endpoint { diff --git a/cli/src/main/java/hudson/cli/FullDuplexHttpStream.java b/cli/src/main/java/hudson/cli/FullDuplexHttpStream.java index 1f066a620d..fe8db45116 100644 --- a/cli/src/main/java/hudson/cli/FullDuplexHttpStream.java +++ b/cli/src/main/java/hudson/cli/FullDuplexHttpStream.java @@ -46,7 +46,6 @@ public class FullDuplexHttpStream { * @param authorization * The value of the authorization header, if non-null. */ - @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Client-side code doesn't involve SSRF.") public FullDuplexHttpStream(URL base, String relativeTarget, String authorization) throws IOException { if (!base.toString().endsWith("/")) { throw new IllegalArgumentException(base.toString()); @@ -63,7 +62,7 @@ public class FullDuplexHttpStream { // server->client LOGGER.fine("establishing download side"); - HttpURLConnection con = (HttpURLConnection) target.openConnection(); + HttpURLConnection con = openHttpConnection(target); con.setDoOutput(true); // request POST to avoid caching con.setRequestMethod("POST"); con.addRequestProperty("Session", uuid.toString()); @@ -81,7 +80,7 @@ public class FullDuplexHttpStream { // client->server uses chunked encoded POST for unlimited capacity. LOGGER.fine("establishing upload side"); - con = (HttpURLConnection) target.openConnection(); + con = openHttpConnection(target); con.setDoOutput(true); // request POST con.setRequestMethod("POST"); con.setChunkedStreamingMode(0); @@ -95,11 +94,15 @@ public class FullDuplexHttpStream { LOGGER.fine("established upload side"); } - // As this transport mode is using POST, it is necessary to resolve possible redirections using GET first. @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Client-side code doesn't involve SSRF.") + private HttpURLConnection openHttpConnection(URL target) throws IOException { + return (HttpURLConnection) target.openConnection(); + } + + // As this transport mode is using POST, it is necessary to resolve possible redirections using GET first. @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Client-side code doesn't involve SSRF.") private URL tryToResolveRedirects(URL base, String authorization) { try { - HttpURLConnection con = (HttpURLConnection) base.openConnection(); + HttpURLConnection con = openHttpConnection(base); if (authorization != null) { con.addRequestProperty("Authorization", authorization); } diff --git a/cli/src/main/java/hudson/cli/NoCheckTrustManager.java b/cli/src/main/java/hudson/cli/NoCheckTrustManager.java index 5a8346cb6e..cceacf2b21 100644 --- a/cli/src/main/java/hudson/cli/NoCheckTrustManager.java +++ b/cli/src/main/java/hudson/cli/NoCheckTrustManager.java @@ -9,14 +9,16 @@ import java.security.cert.X509Certificate; /** * @author Kohsuke Kawaguchi */ -@SuppressFBWarnings(value = "WEAK_TRUST_MANAGER", justification = "User set parameter to skip verifier.") public class NoCheckTrustManager implements X509TrustManager { + @SuppressFBWarnings(value = "WEAK_TRUST_MANAGER", justification = "User set parameter to skip verifier.") public void checkClientTrusted(X509Certificate[] x509Certificates, String s) throws CertificateException { } + @SuppressFBWarnings(value = "WEAK_TRUST_MANAGER", justification = "User set parameter to skip verifier.") public void checkServerTrusted(X509Certificate[] x509Certificates, String s) throws CertificateException { } + @SuppressFBWarnings(value = "WEAK_TRUST_MANAGER", justification = "User set parameter to skip verifier.") public X509Certificate[] getAcceptedIssuers() { return new X509Certificate[0]; } diff --git a/cli/src/main/java/hudson/cli/SSHCLI.java b/cli/src/main/java/hudson/cli/SSHCLI.java index cbfeea16b8..5321536c5c 100644 --- a/cli/src/main/java/hudson/cli/SSHCLI.java +++ b/cli/src/main/java/hudson/cli/SSHCLI.java @@ -61,11 +61,10 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; */ class SSHCLI { - @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Client-side code doesn't involve SSRF.") static int sshConnection(String jenkinsUrl, String user, List args, PrivateKeyProvider provider, final boolean strictHostKey) throws IOException { Logger.getLogger(SecurityUtils.class.getName()).setLevel(Level.WARNING); // suppress: BouncyCastle not registered, using the default JCE provider URL url = new URL(jenkinsUrl + "login"); - URLConnection conn = url.openConnection(); + URLConnection conn = openConnection(url); CLI.verifyJenkinsConnection(conn); String endpointDescription = conn.getHeaderField("X-SSH-Endpoint"); @@ -131,6 +130,11 @@ class SSHCLI { } } + @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Client-side code doesn't involve SSRF.") + private static URLConnection openConnection(URL url) throws IOException { + return url.openConnection(); + } + private SSHCLI() {} } diff --git a/core/src/main/java/hudson/ClassicPluginStrategy.java b/core/src/main/java/hudson/ClassicPluginStrategy.java index a3903fbb1b..e3af9fc07c 100644 --- a/core/src/main/java/hudson/ClassicPluginStrategy.java +++ b/core/src/main/java/hudson/ClassicPluginStrategy.java @@ -78,7 +78,6 @@ import java.util.logging.Logger; import static org.apache.commons.io.FilenameUtils.getBaseName; -@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "Administrator action installing a plugin, which could do far worse.") public class ClassicPluginStrategy implements PluginStrategy { private static final Logger LOGGER = Logger.getLogger(ClassicPluginStrategy.class.getName()); @@ -432,6 +431,7 @@ public class ClassicPluginStrategy implements PluginStrategy { return null; } + @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "Administrator action installing a plugin, which could do far worse.") private static File resolve(File base, String relative) { File rel = new File(relative); if(rel.isAbsolute()) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index de29e9a013..7c9f7c5fe7 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -589,6 +589,8 @@ public class Util { /** * Computes MD5 digest of the given input stream. * + * This method should only be used for non-security applications where the MD5 weakness is not a problem. + * * @param source * The stream will be closed by this method at the end of this method. * @return @@ -596,11 +598,9 @@ public class Util { * @see DigestUtils#md5Hex(InputStream) */ @Nonnull - @SuppressFBWarnings(value = "WEAK_MESSAGE_DIGEST_MD5", justification = - "This method should only be used for non-security applications where their weakness is not a problem.") public static String getDigestOf(@Nonnull InputStream source) throws IOException { try { - MessageDigest md5 = MessageDigest.getInstance("MD5"); + MessageDigest md5 = getMd5(); DigestInputStream in = new DigestInputStream(source, md5); // Note: IOUtils.copy() buffers the input internally, so there is no // need to use a BufferedInputStream. @@ -620,6 +620,13 @@ public class Util { */ } + // TODO JENKINS-60563 remove MD5 from all usages in Jenkins + @SuppressFBWarnings(value = "WEAK_MESSAGE_DIGEST_MD5", justification = + "This method should only be used for non-security applications where the MD5 weakness is not a problem.") + private static MessageDigest getMd5() throws NoSuchAlgorithmException { + return MessageDigest.getInstance("MD5"); + } + @Nonnull public static String getDigestOf(@Nonnull String text) { try { diff --git a/core/src/main/java/hudson/cli/Connection.java b/core/src/main/java/hudson/cli/Connection.java index a181c5d208..bdeb192fd3 100644 --- a/core/src/main/java/hudson/cli/Connection.java +++ b/core/src/main/java/hudson/cli/Connection.java @@ -114,7 +114,8 @@ public class Connection { /** * Receives an object sent by {@link #writeObject(Object)} */ - @SuppressFBWarnings(value = "OBJECT_DESERIALIZATION", justification = "Not used. We should just remove it.") + // TODO JENKINS-60562 remove this class + @SuppressFBWarnings(value = "OBJECT_DESERIALIZATION", justification = "Not used. We should just remove it. Class is deprecated.") public T readObject() throws IOException, ClassNotFoundException { ObjectInputStream ois = new ObjectInputStreamEx(in, getClass().getClassLoader(), ClassFilter.DEFAULT); @@ -194,19 +195,22 @@ public class Connection { * @return * A new {@link Connection} object that includes the transport encryption. */ - @SuppressFBWarnings(value = "STATIC_IV", justification = "Not used. We should just remove it.") public Connection encryptConnection(SecretKey sessionKey, String algorithm) throws IOException, GeneralSecurityException { Cipher cout = Cipher.getInstance(algorithm); - cout.init(Cipher.ENCRYPT_MODE, sessionKey, new IvParameterSpec(sessionKey.getEncoded())); + cout.init(Cipher.ENCRYPT_MODE, sessionKey, createIv(sessionKey)); CipherOutputStream o = new CipherOutputStream(out, cout); Cipher cin = Cipher.getInstance(algorithm); - cin.init(Cipher.DECRYPT_MODE, sessionKey, new IvParameterSpec(sessionKey.getEncoded())); + cin.init(Cipher.DECRYPT_MODE, sessionKey, createIv(sessionKey)); CipherInputStream i = new CipherInputStream(in, cin); return new Connection(i,o); } + private IvParameterSpec createIv(SecretKey sessionKey) { + return new IvParameterSpec(sessionKey.getEncoded()); + } + /** * Given a byte array that contains arbitrary number of bytes, digests or expands those bits into the specified * number of bytes without loss of entropy. diff --git a/core/src/main/java/hudson/console/AnnotatedLargeText.java b/core/src/main/java/hudson/console/AnnotatedLargeText.java index 9625a8609f..36d2664e92 100644 --- a/core/src/main/java/hudson/console/AnnotatedLargeText.java +++ b/core/src/main/java/hudson/console/AnnotatedLargeText.java @@ -117,7 +117,6 @@ public class AnnotatedLargeText extends LargeText { rsp.setContentType(isHtml() ? "text/html;charset=UTF-8" : "text/plain;charset=UTF-8"); } - @SuppressFBWarnings(value = "OBJECT_DESERIALIZATION", justification = "Deserialization is protected by logic.") private ConsoleAnnotator createAnnotator(StaplerRequest req) throws IOException { try { String base64 = req!=null ? req.getHeader("X-ConsoleAnnotator") : null; @@ -130,7 +129,7 @@ public class AnnotatedLargeText extends LargeText { long timestamp = ois.readLong(); if (TimeUnit.HOURS.toMillis(1) > abs(System.currentTimeMillis()-timestamp)) // don't deserialize something too old to prevent a replay attack - return (ConsoleAnnotator) ois.readObject(); + return getConsoleAnnotator(ois); } catch (RuntimeException ex) { throw new IOException("Could not decode input", ex); } @@ -142,6 +141,11 @@ public class AnnotatedLargeText extends LargeText { return ConsoleAnnotator.initial(context); } + @SuppressFBWarnings(value = "OBJECT_DESERIALIZATION", justification = "Deserialization is protected by logic.") + private ConsoleAnnotator getConsoleAnnotator(ObjectInputStream ois) throws IOException, ClassNotFoundException { + return (ConsoleAnnotator) ois.readObject(); + } + @CheckReturnValue @Override public long writeLogTo(long start, Writer w) throws IOException { diff --git a/core/src/main/java/hudson/console/ConsoleNote.java b/core/src/main/java/hudson/console/ConsoleNote.java index 82ca8365e7..e55f7c2629 100644 --- a/core/src/main/java/hudson/console/ConsoleNote.java +++ b/core/src/main/java/hudson/console/ConsoleNote.java @@ -233,7 +233,6 @@ public abstract class ConsoleNote implements Serializable, Describable implements Serializable, Describable implements Serializable, Describable" + - "" + - "" + - "" + - "%n" + - "%n%n"+ - "Authentication required%n"+ - "