Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add findsecbugs plugin to spotbugs. #4381

Merged
merged 7 commits into from Feb 15, 2020
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -23,6 +23,7 @@
*/
package hudson.cli;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.cli.client.Messages;
import java.io.DataInputStream;
import javax.net.ssl.HostnameVerifier;
@@ -175,6 +176,7 @@ public static int _main(String[] _args) throws Exception {
HttpsURLConnection.setDefaultSSLSocketFactory(context.getSocketFactory());
// bypass host name check, too.
HttpsURLConnection.setDefaultHostnameVerifier(new HostnameVerifier() {
@SuppressFBWarnings(value = "WEAK_HOSTNAME_VERIFIER", justification = "User set parameter to skip verifier.")
public boolean verify(String s, SSLSession sslSession) {
return true;
}
@@ -188,7 +190,7 @@ public boolean verify(String s, SSLSession sslSession) {
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;
@@ -290,7 +292,7 @@ public boolean verify(String s, SSLSession sslSession) {
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) {
@@ -304,6 +306,16 @@ public boolean verify(String s, SSLSession sslSession) {
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<String> args) {
return new File(args.get(1));
}

private static int webSocketConnection(String url, List<String> args, CLIConnectionFactory factory) throws Exception {
LOGGER.fine(() -> "Trying to connect to " + url + " via plain protocol over WebSocket");
class CLIEndpoint extends Endpoint {
@@ -1,5 +1,7 @@
package hudson.cli;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
@@ -60,7 +62,7 @@ public FullDuplexHttpStream(URL base, String relativeTarget, String authorizatio

// 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());
@@ -78,7 +80,7 @@ public FullDuplexHttpStream(URL base, String relativeTarget, String authorizatio

// 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);
@@ -92,10 +94,15 @@ public FullDuplexHttpStream(URL base, String relativeTarget, String authorizatio
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);
}
@@ -1,5 +1,7 @@
package hudson.cli;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import javax.net.ssl.X509TrustManager;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
@@ -8,12 +10,15 @@
* @author Kohsuke Kawaguchi
*/
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];
}
@@ -61,11 +61,10 @@
*/
class SSHCLI {

@SuppressFBWarnings(value = "RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE", justification = "Due to whatever reason FindBugs reports it fot try-with-resources")
static int sshConnection(String jenkinsUrl, String user, List<String> 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 @@ public boolean verifyServerKey(ClientSession clientSession, SocketAddress remote
}
}

@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() {}

}
@@ -431,6 +431,7 @@ private DependencyClassLoader findAncestorDependencyClassLoader(ClassLoader clas
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())
@@ -23,6 +23,7 @@
*/
package hudson;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.Launcher.ProcStarter;
import hudson.model.TaskListener;
import hudson.remoting.Channel;
@@ -215,6 +216,7 @@ public LocalProc(String[] cmd,String[] env,InputStream in,OutputStream out, File
* @param err
* null to redirect stderr to stdout.
*/
@SuppressFBWarnings(value = "COMMAND_INJECTION", justification = "Command injection is the point of this old, barely used class.")
public LocalProc(String[] cmd,String[] env,InputStream in,OutputStream out,OutputStream err,File workDir) throws IOException {
this( calcName(cmd),
stderr(environment(new ProcessBuilder(cmd),env).directory(workDir), err==null || err== SELFPUMP_OUTPUT),
@@ -589,6 +589,8 @@ public static String ensureEndsWith(@CheckForNull String subject, @CheckForNull
/**
* 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
@@ -598,7 +600,7 @@ public static String ensureEndsWith(@CheckForNull String subject, @CheckForNull
@Nonnull
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.
@@ -618,6 +620,14 @@ public static String getDigestOf(@Nonnull InputStream source) throws IOException
*/
}

// 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.")
@Deprecated
private static MessageDigest getMd5() throws NoSuchAlgorithmException {
This conversation was marked as resolved by jeffret-b

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jan 7, 2020

Member

Good time to deprecate this and all callers?

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Jan 7, 2020

Author Contributor

Deprecating this makes sense. I've wondered about deprecating Util.getDigestOf(). Would it make sense to change the implementation to SHA-2? The documentation specifies MD5, but it might make sense to change that along with the implementation. It wouldn't be the first JavaDoc in Jenkins that is out-of-date, wrong, or not helpful.

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Jan 7, 2020

Author Contributor

I've figured it made more sense to answer that question when engaged in an effort to remove MD5.

This comment has been minimized.

Copy link
@jvz

jvz Jan 7, 2020

Member

By the time we fully remove MD5, maybe SHA-4 will be a thing by then! We can defer implementation until then as you suggest. :)

return MessageDigest.getInstance("MD5");
}

@Nonnull
public static String getDigestOf(@Nonnull String text) {
try {
@@ -23,6 +23,7 @@
*/
package hudson.cli;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.remoting.ClassFilter;
import hudson.remoting.ObjectInputStreamEx;
import hudson.remoting.SocketChannelStream;
@@ -113,6 +114,8 @@ public void writeObject(Object o) throws IOException {
/**
* Receives an object sent by {@link #writeObject(Object)}
*/
// TODO JENKINS-60562 remove this class
@SuppressFBWarnings(value = "OBJECT_DESERIALIZATION", justification = "Not used. We should just remove it. Class is deprecated.")
public <T> T readObject() throws IOException, ClassNotFoundException {
ObjectInputStream ois = new ObjectInputStreamEx(in,
getClass().getClassLoader(), ClassFilter.DEFAULT);
@@ -194,16 +197,20 @@ public KeyAgreement diffieHellman(boolean side, int keySize) throws IOException,
*/
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.
@@ -25,6 +25,7 @@
*/
package hudson.console;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import jenkins.model.Jenkins;
import hudson.remoting.ObjectInputStreamEx;
import java.util.concurrent.TimeUnit;
@@ -128,7 +129,7 @@ protected void setContentType(StaplerResponse rsp) {
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);
}
@@ -140,6 +141,11 @@ protected void setContentType(StaplerResponse rsp) {
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 {
@@ -23,6 +23,7 @@
*/
package hudson.console;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.ExtensionPoint;
import hudson.Functions;
import hudson.MarkupText;
@@ -275,7 +276,7 @@ public static ConsoleNote readFrom(DataInputStream in) throws IOException, Class
try (ObjectInputStream ois = new ObjectInputStreamEx(new GZIPInputStream(new ByteArrayInputStream(buf)),
jenkins != null ? jenkins.pluginManager.uberClassLoader : ConsoleNote.class.getClassLoader(),
ClassFilter.DEFAULT)) {
return (ConsoleNote) ois.readObject();
return getConsoleNote(ois);
}
} catch (Error e) {
// for example, bogus 'sz' can result in OutOfMemoryError.
@@ -284,6 +285,11 @@ public static ConsoleNote readFrom(DataInputStream in) throws IOException, Class
}
}

@SuppressFBWarnings(value = "OBJECT_DESERIALIZATION", justification = "Deserialization is protected by logic.")
private static ConsoleNote getConsoleNote(ObjectInputStream ois) throws IOException, ClassNotFoundException {
return (ConsoleNote) ois.readObject();
}

/**
* Skips the encoded console note.
*/
@@ -23,6 +23,8 @@
*/
package hudson.scheduler;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
@@ -51,10 +53,10 @@
* Produces an integer in [0,n)
*/
public abstract int next(int n);

public static Hash from(String seed) {
try {
MessageDigest md5 = MessageDigest.getInstance("MD5");
MessageDigest md5 = getMd5();
md5.update(seed.getBytes(StandardCharsets.UTF_8));
byte[] digest = md5.digest();

@@ -77,6 +79,12 @@ public int next(int n) {
}
}

// TODO JENKINS-60563 remove MD5 from all usages in Jenkins
@SuppressFBWarnings(value = "WEAK_MESSAGE_DIGEST_MD5", justification = "Should not be used for security.")
private static MessageDigest getMd5() throws NoSuchAlgorithmException {
return MessageDigest.getInstance("MD5");
}

/**
* Creates a hash that always return 0.
*/
@@ -23,6 +23,7 @@
*/
package hudson.security;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.model.User;
import jenkins.model.Jenkins;
import hudson.util.Scrambler;
@@ -162,15 +163,20 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
path += '?'+q;

// prepare a redirect
rsp.setStatus(HttpServletResponse.SC_MOVED_TEMPORARILY);
rsp.setHeader("Location",path);
prepareRedirect(rsp, path);

// ... but first let the container authenticate this request
RequestDispatcher d = servletContext.getRequestDispatcher("/j_security_check?j_username="+
URLEncoder.encode(username,"UTF-8")+"&j_password="+URLEncoder.encode(password,"UTF-8"));
d.include(req,rsp);
}

@SuppressFBWarnings(value = "UNVALIDATED_REDIRECT", justification = "Redirect is validated as processed.")
This conversation was marked as resolved by jeffret-b

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jan 7, 2020

Member

I don't understand this comment.

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Jan 9, 2020

Author Contributor

I'll see if I can investigate and improve this when I get a chance.

private void prepareRedirect(HttpServletResponse rsp, String path) {
rsp.setStatus(HttpServletResponse.SC_MOVED_TEMPORARILY);
rsp.setHeader("Location",path);
}

//public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
// HttpServletRequest req = (HttpServletRequest) request;
// String authorization = req.getHeader("Authorization");
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.