Skip to content

Commit

Permalink
fix: refactor to prevent concurrent modification exception
Browse files Browse the repository at this point in the history
  • Loading branch information
@jeremylong
jeremylong committed Oct 16, 2022
1 parent 1a04fa6 commit 4d1c843
Show file tree
Hide file tree
Showing 11 changed files with 203 additions and 250 deletions.
8 changes: 1 addition & 7 deletions ant/src/test/java/org/owasp/dependencycheck/taskdefs/DependencyCheckTaskIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import org.owasp.dependencycheck.BaseDBTestCase;

import static org.junit.Assert.assertTrue;
import org.owasp.dependencycheck.xml.suppression.SuppressionRules;

/**
*
Expand Down Expand Up @@ -145,8 +144,7 @@ public void testGetFailBuildOnCVSS() {
public void testSuppressingCVE() {
// GIVEN an ant task with a vulnerability
final String antTaskName = "suppression";
//as the suppression rules are now a singleton - we must reset the list to cause the new suppression rules to load
SuppressionRules.getInstance().list().clear();

// WHEN executing the ant task
buildFileRule.executeTarget(antTaskName);
if (buildFileRule.getError() != null && !buildFileRule.getError().isEmpty()) {
Expand All @@ -170,8 +168,6 @@ public void testSuppressingCVE() {
public void testSuppressingSingle() {
// GIVEN an ant task with a vulnerability using the legacy property
final String antTaskName = "suppression-single";
//as the suppression rules are now a singleton - we must reset the list to cause the new suppression rules to load
SuppressionRules.getInstance().list().clear();
// WHEN executing the ant task
buildFileRule.executeTarget(antTaskName);

Expand All @@ -188,8 +184,6 @@ public void testSuppressingSingle() {
public void testSuppressingMultiple() {
// GIVEN an ant task with a vulnerability using multiple was to configure the suppression file
final String antTaskName = "suppression-multiple";
//as the suppression rules are now a singleton - we must reset the list to cause the new suppression rules to load
SuppressionRules.getInstance().list().clear();
// WHEN executing the ant task
buildFileRule.executeTarget(antTaskName);

Expand Down
49 changes: 47 additions & 2 deletions core/src/main/java/org/owasp/dependencycheck/Engine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.EnumMap;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
Expand All @@ -67,6 +68,7 @@
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import org.owasp.dependencycheck.analyzer.AbstractSuppressionAnalyzer;

import static org.owasp.dependencycheck.analyzer.AnalysisPhase.FINAL;
import static org.owasp.dependencycheck.analyzer.AnalysisPhase.FINDING_ANALYSIS;
Expand All @@ -82,7 +84,6 @@
import static org.owasp.dependencycheck.analyzer.AnalysisPhase.PRE_IDENTIFIER_ANALYSIS;
import static org.owasp.dependencycheck.analyzer.AnalysisPhase.PRE_INFORMATION_COLLECTION;
import org.owasp.dependencycheck.analyzer.DependencyBundlingAnalyzer;
import org.owasp.dependencycheck.xml.suppression.SuppressionRules;

/**
* Scans files, directories, etc. for Dependencies. Analyzers are loaded and
Expand Down Expand Up @@ -125,6 +126,10 @@ public class Engine implements FileFilter, AutoCloseable {
* The configured settings.
*/
private final Settings settings;
/**
* A storage location to persist objects throughout the execution of ODC.
*/
private final Map<String, Object> objects = new HashMap<>();
/**
* The external view of the dependency list.
*/
Expand Down Expand Up @@ -660,7 +665,7 @@ public void analyzeDependencies() throws ExceptionCollection {
.map(analyzers::get)
.forEach((analyzerList) -> analyzerList.forEach(this::closeAnalyzer));

SuppressionRules.getInstance().logUnusedRules();
AbstractSuppressionAnalyzer.logUnusedRules(this);

LOGGER.debug("\n----------------------------------------------------\nEND ANALYSIS\n----------------------------------------------------");
final long analysisDurationSeconds = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - analysisStart);
Expand Down Expand Up @@ -1068,6 +1073,46 @@ public Settings getSettings() {
return settings;
}

/**
* Retrieve an object from the objects collection.
*
* @param key the key to retrieve the object
* @return the object
*/
public Object getObject(String key) {
return objects.get(key);
}

/**
* Put an object in the object collection.
*
* @param key the key to store the object
* @param object the object to store
*/
public void putObject(String key, Object object) {
objects.put(key, object);
}

/**
* Verifies if the object exists in the object store.
*
* @param key the key to retrieve the object
* @return <code>true</code> if the object exists; otherwise
* <code>false</code>
*/
public boolean hasObject(String key) {
return objects.containsKey(key);
}

/**
* Removes an object from the object store.
*
* @param key the key to the object
*/
public void removeObject(String key) {
objects.remove(key);
}

/**
* Returns the mode of the engine.
*
Expand Down
111 changes: 72 additions & 39 deletions core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.xml.sax.SAXException;
import org.owasp.dependencycheck.xml.suppression.SuppressionRules;

/**
* Abstract base suppression analyzer that contains methods for parsing the
Expand All @@ -63,31 +62,9 @@ public abstract class AbstractSuppressionAnalyzer extends AbstractAnalyzer {
*/
private static final String BASE_SUPPRESSION_FILE = "dependencycheck-base-suppression.xml";
/**
* Settings flag to indicate if the suppression rules have been previously loaded.
* The key used to store and retrieve the suppression files.
*/
private static final String SUPPRESSION_LOADED = "SUPPRESSION_LOADED";
/**
* The collection of suppression rules.
*/
private final SuppressionRules rules = SuppressionRules.getInstance();

/**
* Returns the suppression rules.
*
* @return the suppression rules
*/
protected SuppressionRules getSuppressionRules() {
return rules;
}

/**
* Get the number of suppression rules.
*
* @return the number of suppression rules
*/
protected int getRuleCount() {
return rules.size();
}
private static final String SUPPRESSION_OBJECT_KEY = "suppression.rules";

/**
* Returns a list of file EXTENSIONS supported by this analyzer.
Expand All @@ -107,41 +84,46 @@ public Set<String> getSupportedExtensions() {
*/
@Override
public synchronized void prepareAnalyzer(Engine engine) throws InitializationException {
//check if we have a brand new settings object - if we do the suppression rules could be different
final boolean loaded = getSettings().getBoolean(SUPPRESSION_LOADED, false);
if (!loaded) {
SuppressionRules.getInstance().list().clear();
if (engine.hasObject(SUPPRESSION_OBJECT_KEY)) {
return;
}
@SuppressWarnings("unchecked")
final List<SuppressionRule> rules = (List<SuppressionRule>) engine.getObject(SUPPRESSION_OBJECT_KEY);
if (rules.isEmpty()) {
try {
loadSuppressionBaseData();
loadSuppressionBaseData(engine);
} catch (SuppressionParseException ex) {
throw new InitializationException("Error initializing the suppression analyzer: " + ex.getLocalizedMessage(), ex, true);
}

try {
loadSuppressionData();
loadSuppressionData(engine);
} catch (SuppressionParseException ex) {
throw new InitializationException("Warn initializing the suppression analyzer: " + ex.getLocalizedMessage(), ex, false);
}
getSettings().setBoolean(SUPPRESSION_LOADED, true);
}
}

@Override
protected void analyzeDependency(Dependency dependency, Engine engine) throws AnalysisException {
protected synchronized void analyzeDependency(Dependency dependency, Engine engine) throws AnalysisException {
if (engine == null) {
return;
}
@SuppressWarnings("unchecked")
final List<SuppressionRule> rules = (List<SuppressionRule>) engine.getObject(SUPPRESSION_OBJECT_KEY);
if (rules.isEmpty()) {
return;
}
for (SuppressionRule rule : rules.list()) {
for (SuppressionRule rule : rules) {
if (filter(rule)) {
rule.process(dependency);
}
}
}

/**
* Determines whether a suppression rule should be retained when filtering a set of suppression rules for a concrete suppression analyzer.
* Determines whether a suppression rule should be retained when filtering a
* set of suppression rules for a concrete suppression analyzer.
*
* @param rule the suppression rule to evaluate
* @return <code>true</code> if the rule should be retained; otherwise
Expand All @@ -152,9 +134,10 @@ protected void analyzeDependency(Dependency dependency, Engine engine) throws An
/**
* Loads all the suppression rules files configured in the {@link Settings}.
*
* @param engine a reference to the ODC engine.
* @throws SuppressionParseException thrown if the XML cannot be parsed.
*/
private void loadSuppressionData() throws SuppressionParseException {
private void loadSuppressionData(Engine engine) throws SuppressionParseException {
final List<SuppressionRule> ruleList = new ArrayList<>();
final SuppressionParser parser = new SuppressionParser();
final String[] suppressionFilePaths = getSettings().getArray(Settings.KEYS.SUPPRESSION_FILE);
Expand All @@ -170,8 +153,17 @@ private void loadSuppressionData() throws SuppressionParseException {
}
}
}

LOGGER.debug("{} suppression rules were loaded.", ruleList.size());
rules.addAll(ruleList);
if (!ruleList.isEmpty()) {
if (engine.hasObject(SUPPRESSION_OBJECT_KEY)) {
@SuppressWarnings("unchecked")
final List<SuppressionRule> rules = (List<SuppressionRule>) engine.getObject(SUPPRESSION_OBJECT_KEY);
rules.addAll(ruleList);
} else {
engine.putObject(SUPPRESSION_OBJECT_KEY, ruleList);
}
}
if (!failedLoadingFiles.isEmpty()) {
LOGGER.debug("{} suppression files failed to load.", failedLoadingFiles.size());
final StringBuilder sb = new StringBuilder();
Expand All @@ -183,9 +175,10 @@ private void loadSuppressionData() throws SuppressionParseException {
/**
* Loads all the base suppression rules files.
*
* @param engine a reference to the ODC engine
* @throws SuppressionParseException thrown if the XML cannot be parsed.
*/
private void loadSuppressionBaseData() throws SuppressionParseException {
private void loadSuppressionBaseData(Engine engine) throws SuppressionParseException {
final SuppressionParser parser = new SuppressionParser();
final List<SuppressionRule> ruleList;
try (InputStream in = FileUtils.getResourceAsStream(BASE_SUPPRESSION_FILE)) {
Expand All @@ -196,7 +189,15 @@ private void loadSuppressionBaseData() throws SuppressionParseException {
} catch (SAXException | IOException ex) {
throw new SuppressionParseException("Unable to parse the base suppression data file", ex);
}
rules.addAll(ruleList);
if (!ruleList.isEmpty()) {
if (engine.hasObject(SUPPRESSION_OBJECT_KEY)) {
@SuppressWarnings("unchecked")
final List<SuppressionRule> rules = (List<SuppressionRule>) engine.getObject(SUPPRESSION_OBJECT_KEY);
rules.addAll(ruleList);
} else {
engine.putObject(SUPPRESSION_OBJECT_KEY, ruleList);
}
}
}

/**
Expand Down Expand Up @@ -304,4 +305,36 @@ private void throwSuppressionParseException(String message, Exception exception,
LOGGER.debug("", exception);
throw new SuppressionParseException(message, exception);
}

/**
* Returns the number of suppression rules currently loaded in the engine.
*
* @param engine a reference to the ODC engine
* @return the count of rules loaded
*/
public static int getRuleCount(Engine engine) {
if (engine.hasObject(SUPPRESSION_OBJECT_KEY)) {
@SuppressWarnings("unchecked")
final List<SuppressionRule> rules = (List<SuppressionRule>) engine.getObject(SUPPRESSION_OBJECT_KEY);
return rules.size();
}
return 0;
}

/**
* Logs unused suppression RULES.
*
* @param engine a reference to the ODC engine
*/
public static void logUnusedRules(Engine engine) {
if (engine.hasObject(SUPPRESSION_OBJECT_KEY)) {
@SuppressWarnings("unchecked")
final List<SuppressionRule> rules = (List<SuppressionRule>) engine.getObject(SUPPRESSION_OBJECT_KEY);
rules.forEach((rule) -> {
if (!rule.isMatched() && !rule.isBase()) {
LOGGER.info("Suppression Rule had zero matches: {}", rule.toString());
}
});
}
}
}
12 changes: 7 additions & 5 deletions core/src/main/java/org/owasp/dependencycheck/analyzer/CPEAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,13 @@ public void closeAnalyzer() {
* dependency passed in is updated with any identified CPE values.
*
* @param dependency the dependency to search for CPE entries on
* @param engine a reference to the ODC engine
* @throws CorruptIndexException is thrown when the Lucene index is corrupt
* @throws IOException is thrown when an IOException occurs
* @throws ParseException is thrown when the Lucene query cannot be parsed
* @throws AnalysisException thrown if the suppression rules failed
*/
protected void determineCPE(Dependency dependency) throws CorruptIndexException, IOException, ParseException, AnalysisException {
protected void determineCPE(Dependency dependency, Engine engine) throws CorruptIndexException, IOException, ParseException, AnalysisException {
boolean identifierAdded;

final Set<String> majorVersions = dependency.getSoftwareIdentifiers()
Expand Down Expand Up @@ -295,7 +296,7 @@ protected void determineCPE(Dependency dependency) throws CorruptIndexException,
final String vendor = e.getVendor();
final String product = e.getProduct();
LOGGER.debug("identified vendor/product: {}/{}", vendor, product);
identifierAdded |= determineIdentifiers(dependency, vendor, product, confidence);
identifierAdded |= determineIdentifiers(engine, dependency, vendor, product, confidence);
}
}
if (identifierAdded) {
Expand Down Expand Up @@ -776,7 +777,7 @@ protected void analyzeDependency(Dependency dependency, Engine engine) throws An
return;
}
try {
determineCPE(dependency);
determineCPE(dependency, engine);
} catch (CorruptIndexException ex) {
throw new AnalysisException("CPE Index is corrupt.", ex);
} catch (IOException ex) {
Expand All @@ -793,6 +794,7 @@ protected void analyzeDependency(Dependency dependency, Engine engine) throws An
* a best effort "guess" based on the vendor, product, and version
* information.
*
* @param engine a reference to the ODC engine
* @param dependency the Dependency being analyzed
* @param vendor the vendor for the CPE being analyzed
* @param product the product for the CPE being analyzed
Expand All @@ -804,7 +806,7 @@ protected void analyzeDependency(Dependency dependency, Engine engine) throws An
* @throws AnalysisException thrown if the suppression rules failed
*/
@SuppressWarnings("StringSplitter")
protected boolean determineIdentifiers(Dependency dependency, String vendor, String product,
protected boolean determineIdentifiers(Engine engine, Dependency dependency, String vendor, String product,
Confidence currentConfidence) throws UnsupportedEncodingException, AnalysisException {

final CpeBuilder cpeBuilder = new CpeBuilder();
Expand Down Expand Up @@ -957,7 +959,7 @@ protected boolean determineIdentifiers(Dependency dependency, String vendor, Str

//TODO - while this gets the job down it is slow; consider refactoring
dependency.addVulnerableSoftwareIdentifier(i);
suppression.analyze(dependency, null);
suppression.analyze(dependency, engine);
if (dependency.getVulnerableSoftwareIdentifiers().contains(i)) {
identifierAdded = true;
if (!addedNonGuess && bestIdentifierQuality != IdentifierConfidence.BEST_GUESS) {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/owasp/dependencycheck/analyzer/NpmCPEAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ protected void analyzeDependency(Dependency dependency, Engine engine) throws An
return;
}
try {
determineCPE(dependency);
determineCPE(dependency, engine);
} catch (CorruptIndexException ex) {
throw new AnalysisException("CPE Index is corrupt.", ex);
} catch (IOException ex) {
Expand Down
Loading

0 comments on commit 4d1c843

Please sign in to comment.