Add sec-bugs plugin. Closes #618 (#628)

* Created build profile for sec-bugs * Replaced Math.Random with SecureRandom * Sanitize user input in log messages to prevent potential CRLF injection * Use SHA-256 instead of MD5 or SHA-1 * Remove unused method in CryptoUtils * Replace vulnerable regex in Monitor param validation
apache · Sep 6, 2018 · bf8ec89 · bf8ec89
1 parent 6caeeea
commit bf8ec89
Show file tree

Hide file tree

Showing 103 changed files with 580 additions and 185 deletions.
diff --git a/...t/mapreduce/src/main/java/org/apache/accumulo/core/client/mapred/AbstractInputFormat.java b/...t/mapreduce/src/main/java/org/apache/accumulo/core/client/mapred/AbstractInputFormat.java
@@ -20,6 +20,7 @@
 
 import java.io.IOException;
 import java.net.InetAddress;
+import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -646,7 +647,7 @@ public InputSplit[] getSplits(JobConf job, int numSplits) throws IOException {
     log.setLevel(logLevel);
     validateOptions(job);
 
-    Random random = new Random();
+    Random random = new SecureRandom();
     LinkedList<InputSplit> splits = new LinkedList<>();
     Map<String,InputTableConfig> tableConfigs = getInputTableConfigs(job);
     for (Map.Entry<String,InputTableConfig> tableConfigEntry : tableConfigs.entrySet()) {

diff --git a/...apreduce/src/main/java/org/apache/accumulo/core/client/mapreduce/AbstractInputFormat.java b/...apreduce/src/main/java/org/apache/accumulo/core/client/mapreduce/AbstractInputFormat.java
@@ -20,6 +20,7 @@
 
 import java.io.IOException;
 import java.net.InetAddress;
+import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -671,7 +672,7 @@ public List<InputSplit> getSplits(JobContext context) throws IOException {
     Level logLevel = getLogLevel(context);
     log.setLevel(logLevel);
     validateOptions(context);
-    Random random = new Random();
+    Random random = new SecureRandom();
     LinkedList<InputSplit> splits = new LinkedList<>();
     Map<String,InputTableConfig> tableConfigs = getInputTableConfigs(context);
     for (Map.Entry<String,InputTableConfig> tableConfigEntry : tableConfigs.entrySet()) {

diff --git a/client/mapreduce/src/main/spotbugs/exclude-filter.xml b/client/mapreduce/src/main/spotbugs/exclude-filter.xml
@@ -26,4 +26,10 @@
       <Bug code="NM" pattern="NM_SAME_SIMPLE_NAME_AS_INTERFACE" />
     </Or>
   </Match>
+  <Match>
+    <!-- Calling new File on input can be dangerous, but OK here -->
+    <Class name="org.apache.accumulo.core.client.mapreduce.lib.partition.RangePartitioner" />
+    <Method name="getCutPoints" />
+    <Bug code="PATH" pattern="PATH_TRAVERSAL_IN" />
+  </Match>
 </FindBugsFilter>
diff --git a/core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java b/core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
@@ -29,6 +29,7 @@
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.nio.ByteBuffer;
+import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -1077,7 +1078,7 @@ public Set<Range> splitRangeByTablets(String tableName, Range range, int maxSpli
     if (maxSplits == 1)
       return Collections.singleton(range);
 
-    Random random = new Random();
+    Random random = new SecureRandom();
     Map<String,Map<KeyExtent,List<Range>>> binnedRanges = new HashMap<>();
     Table.ID tableId = Tables.getTableId(context, tableName);
     TabletLocator tl = TabletLocator.getLocator(context, tableId);
@@ -1511,7 +1512,7 @@ public void importTable(String tableName, String importDir)
             && !entry.getValue().contains(Constants.CORE_PACKAGE_NAME)) {
           LoggerFactory.getLogger(this.getClass()).info(
               "Imported table sets '{}' to '{}'.  Ensure this class is on Accumulo classpath.",
-              entry.getKey(), entry.getValue());
+              sanitize(entry.getKey()), sanitize(entry.getValue()));
         }
       }
 
@@ -1536,6 +1537,14 @@ public void importTable(String tableName, String importDir)
 
   }
 
+  /**
+   * Prevent potential CRLF injection into logs from read in user data See
+   * https://find-sec-bugs.github.io/bugs.htm#CRLF_INJECTION_LOGS
+   */
+  private String sanitize(String msg) {
+    return msg.replaceAll("[\r\n]", "");
+  }
+
   @Override
   public void exportTable(String tableName, String exportDir)
       throws TableNotFoundException, AccumuloException, AccumuloSecurityException {

diff --git a/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftScanner.java b/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftScanner.java
@@ -17,12 +17,14 @@
 package org.apache.accumulo.core.client.impl;
 
 import java.io.IOException;
+import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.EnumMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Random;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.SortedSet;
@@ -76,6 +78,7 @@ public class ThriftScanner {
 
   public static final Map<TabletType,Set<String>> serversWaitedForWrites = new EnumMap<>(
       TabletType.class);
+  private static Random secureRandom = new SecureRandom();
 
   static {
     for (TabletType ttype : TabletType.values()) {
@@ -225,7 +228,7 @@ public static class ScanTimedOutException extends IOException {
   static long pause(long millis, long maxSleep) throws InterruptedException {
     Thread.sleep(millis);
     // wait 2 * last time, with +-10% random jitter
-    return (long) (Math.min(millis * 2, maxSleep) * (.9 + Math.random() / 5));
+    return (long) (Math.min(millis * 2, maxSleep) * (.9 + secureRandom.nextDouble() / 5));
   }
 
   public static List<KeyValue> scan(ClientContext context, ScanState scanState, int timeOut)

diff --git a/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java b/core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java
@@ -16,6 +16,7 @@
  */
 package org.apache.accumulo.core.client.impl;
 
+import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
@@ -25,7 +26,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
-import java.util.Random;
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -45,7 +45,7 @@
 
 public class ThriftTransportPool {
 
-  private static final Random random = new Random();
+  private static final SecureRandom random = new SecureRandom();
   private long killTime = 1000 * 3;
 
   private static class CachedConnections {

diff --git a/core/src/main/java/org/apache/accumulo/core/file/BloomFilterLayer.java b/core/src/main/java/org/apache/accumulo/core/file/BloomFilterLayer.java
@@ -24,12 +24,12 @@
 import java.io.DataOutputStream;
 import java.io.IOException;
 import java.io.PrintStream;
+import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Map;
-import java.util.Random;
 import java.util.Set;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.ExecutorService;
@@ -264,10 +264,10 @@ static class BloomFilterLoader {
 
           bloomFilter = null;
         } catch (ClassNotFoundException e) {
-          LOG.error("Failed to find KeyFunctor in config: " + ClassName, e);
+          LOG.error("Failed to find KeyFunctor in config: " + sanitize(ClassName), e);
           bloomFilter = null;
         } catch (InstantiationException e) {
-          LOG.error("Could not instantiate KeyFunctor: " + ClassName, e);
+          LOG.error("Could not instantiate KeyFunctor: " + sanitize(ClassName), e);
           bloomFilter = null;
         } catch (IllegalAccessException e) {
           LOG.error("Illegal acess exception", e);
@@ -292,6 +292,14 @@ static class BloomFilterLoader {
 
     }
 
+    /**
+     * Prevent potential CRLF injection into logs from read in user data See
+     * https://find-sec-bugs.github.io/bugs.htm#CRLF_INJECTION_LOGS
+     */
+    private String sanitize(String msg) {
+      return msg.replaceAll("[\r\n]", "");
+    }
+
     private synchronized void initiateLoad(int maxLoadThreads) {
       // ensure only one thread initiates loading of bloom filter by
       // only taking action when loadTask != null
@@ -445,7 +453,7 @@ public FileSKVIterator getSample(SamplerConfigurationImpl sampleConfig) {
   public static void main(String[] args) throws IOException {
     PrintStream out = System.out;
 
-    Random r = new Random();
+    SecureRandom r = new SecureRandom();
 
     HashSet<Integer> valsSet = new HashSet<>();
 

diff --git a/core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java b/core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java
@@ -26,6 +26,7 @@
 import java.util.Map;
 import java.util.Map.Entry;
 
+import org.apache.accumulo.core.Constants;
 import org.apache.accumulo.core.data.ArrayByteSequence;
 import org.apache.accumulo.core.data.ByteSequence;
 import org.apache.accumulo.core.data.Key;
@@ -131,15 +132,16 @@ public void printMetrics(boolean hash, String metricWord, PrintStream out) {
           + "Percent of blocks");
       for (Entry<String,Long> entry : metric.get(lGName).asMap().entrySet()) {
         if (hash) {
-          String md5String = "";
+          String encodedKey = "";
           try {
-            byte[] md5Bytes = MessageDigest.getInstance("MD5")
+            byte[] encodedBytes = MessageDigest.getInstance(Constants.PW_HASH_ALGORITHM)
                 .digest(entry.getKey().getBytes(UTF_8));
-            md5String = new String(md5Bytes, UTF_8);
+            encodedKey = new String(encodedBytes, UTF_8);
           } catch (NoSuchAlgorithmException e) {
-            out.println("Failed to convert key to MD5 hash: " + e.getMessage());
+            out.println("Failed to convert key to " + Constants.PW_HASH_ALGORITHM + " hash: "
+                + e.getMessage());
           }
-          out.printf("%-20s", md5String.substring(0, 8));
+          out.printf("%-20s", encodedKey.substring(0, 8));
         } else
           out.printf("%-20s", entry.getKey());
         out.print("\t\t" + entry.getValue() + "\t\t\t");

diff --git a/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java b/core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java
@@ -20,6 +20,7 @@
 import java.io.IOException;
 import java.net.InetAddress;
 import java.security.KeyStore;
+import java.security.SecureRandom;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Random;
@@ -64,7 +65,7 @@ public class ThriftUtil {
 
   public static final String GSSAPI = "GSSAPI", DIGEST_MD5 = "DIGEST-MD5";
 
-  private static final Random SASL_BACKOFF_RAND = new Random();
+  private static final Random SASL_BACKOFF_RAND = new SecureRandom();
   private static final int RELOGIN_MAX_BACKOFF = 5000;
 
   /**

diff --git a/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoUtils.java b/core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoUtils.java
@@ -24,10 +24,6 @@
 import java.security.SecureRandom;
 import java.util.Objects;
 
-import javax.crypto.Cipher;
-import javax.crypto.NoSuchPaddingException;
-import javax.crypto.NullCipher;
-
 import org.apache.accumulo.core.spi.crypto.CryptoService.CryptoException;
 import org.apache.commons.io.IOUtils;
 import org.slf4j.Logger;
@@ -55,38 +51,6 @@ private static SecureRandom newSecureRandom(String secureRNG, String secureRNGPr
     return secureRandom;
   }
 
-  public static Cipher getCipher(String cipherSuite, String securityProvider) {
-    Cipher cipher = null;
-
-    if (cipherSuite.equals("NullCipher")) {
-      cipher = new NullCipher();
-    } else {
-      try {
-        if (securityProvider == null || securityProvider.equals("")) {
-          cipher = Cipher.getInstance(cipherSuite);
-        } else {
-          cipher = Cipher.getInstance(cipherSuite, securityProvider);
-        }
-      } catch (NoSuchAlgorithmException e) {
-        log.error(String.format("Accumulo configuration file contained a cipher"
-            + " suite \"%s\" that was not recognized by any providers", cipherSuite));
-        throw new CryptoException(e);
-      } catch (NoSuchPaddingException e) {
-        log.error(String.format(
-            "Accumulo configuration file contained a"
-                + " cipher, \"%s\" with a padding that was not recognized by any" + " providers",
-            cipherSuite));
-        throw new CryptoException(e);
-      } catch (NoSuchProviderException e) {
-        log.error(String.format(
-            "Accumulo configuration file contained a provider, \"%s\" an unrecognized provider",
-            securityProvider));
-        throw new CryptoException(e);
-      }
-    }
-    return cipher;
-  }
-
   /**
    * Read the decryption parameters from the DataInputStream
    */

diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooCache.java
@@ -18,6 +18,7 @@
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 
+import java.security.SecureRandom;
 import java.util.Collections;
 import java.util.ConcurrentModificationException;
 import java.util.HashMap;
@@ -59,6 +60,7 @@ public class ZooCache {
   private final HashMap<String,List<String>> childrenCache;
 
   private final ZooReader zReader;
+  private final SecureRandom secureRandom = new SecureRandom();
 
   public static class ZcStat {
     private long ephemeralOwner;
@@ -287,7 +289,7 @@ public T retry() {
         }
         LockSupport.parkNanos(sleepTime);
         if (sleepTime < 10_000) {
-          sleepTime = (int) (sleepTime + sleepTime * Math.random());
+          sleepTime = (int) (sleepTime + sleepTime * secureRandom.nextDouble());
         }
       }
     }

diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
@@ -20,6 +20,7 @@
 
 import java.io.IOException;
 import java.net.UnknownHostException;
+import java.security.SecureRandom;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -53,6 +54,8 @@ public ZooSessionInfo(ZooKeeper zooKeeper, ZooWatcher watcher) {
 
   private static Map<String,ZooSessionInfo> sessions = new HashMap<>();
 
+  private static final SecureRandom secureRandom = new SecureRandom();
+
   private static String sessionKey(String keepers, int timeout, String scheme, byte[] auth) {
     return keepers + ":" + timeout + ":" + (scheme == null ? "" : scheme) + ":"
         + (auth == null ? "" : new String(auth, UTF_8));
@@ -138,7 +141,7 @@ public static ZooKeeper connect(String host, int timeout, String scheme, byte[]
         }
         UtilWaitThread.sleep(sleepTime);
         if (sleepTime < 10000)
-          sleepTime = sleepTime + (long) (sleepTime * Math.random());
+          sleepTime = sleepTime + (long) (sleepTime * secureRandom.nextDouble());
       }
     }