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 18, 2022
1 parent 1a04fa6 commit edaba8f
Show file tree
Hide file tree
Showing 12 changed files with 290 additions and 265 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
48 changes: 45 additions & 3 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 Down Expand Up @@ -82,7 +83,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 +125,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,8 +664,6 @@ public void analyzeDependencies() throws ExceptionCollection {
.map(analyzers::get)
.forEach((analyzerList) -> analyzerList.forEach(this::closeAnalyzer));

SuppressionRules.getInstance().logUnusedRules();

LOGGER.debug("\n----------------------------------------------------\nEND ANALYSIS\n----------------------------------------------------");
final long analysisDurationSeconds = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis() - analysisStart);
LOGGER.info("Analysis Complete ({} seconds)", analysisDurationSeconds);
Expand Down Expand Up @@ -1068,6 +1070,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
106 changes: 59 additions & 47 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();
}
public static final String SUPPRESSION_OBJECT_KEY = "suppression.rules";

/**
* Returns a list of file EXTENSIONS supported by this analyzer.
Expand All @@ -107,41 +84,42 @@ 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;
}
try {
loadSuppressionBaseData(engine);
} catch (SuppressionParseException ex) {
throw new InitializationException("Error initializing the suppression analyzer: " + ex.getLocalizedMessage(), ex, true);
}
if (rules.isEmpty()) {
try {
loadSuppressionBaseData();
} catch (SuppressionParseException ex) {
throw new InitializationException("Error initializing the suppression analyzer: " + ex.getLocalizedMessage(), ex, true);
}

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

@Override
protected 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 +130,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 +149,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 +171,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 +185,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 +301,19 @@ 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;
}
}
37 changes: 21 additions & 16 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 @@ -136,6 +136,10 @@ public class CPEAnalyzer extends AbstractAnalyzer {
* The CVE Database.
*/
private CveDB cve;
/**
* A reference to the ODC engine.
*/
private Engine engine;
/**
* The list of ecosystems to skip during analysis. These are skipped because
* there is generally a more accurate vulnerability analyzer in the
Expand Down Expand Up @@ -185,6 +189,7 @@ public AnalysisPhase getAnalysisPhase() {
@Override
public void prepareAnalyzer(Engine engine) throws InitializationException {
super.prepareAnalyzer(engine);
this.engine = engine;
try {
this.open(engine.getDatabase());
} catch (IOException ex) {
Expand Down Expand Up @@ -375,24 +380,24 @@ private void addMajorVersionToTerms(Set<String> majorVersions, Map<String, Mutab
products.entrySet().stream()
.filter(term -> term.getKey() != null)
.forEach(term -> majorVersions.stream()
.filter(version -> version != null
&& (!term.getKey().endsWith(version)
&& !Character.isDigit(term.getKey().charAt(term.getKey().length() - 1))
&& !products.containsKey(term.getKey() + version)))
.forEach(version -> {
addTerm(temp, term.getKey() + version);
}));
.filter(version -> version != null
&& (!term.getKey().endsWith(version)
&& !Character.isDigit(term.getKey().charAt(term.getKey().length() - 1))
&& !products.containsKey(term.getKey() + version)))
.forEach(version -> {
addTerm(temp, term.getKey() + version);
}));
products.entrySet().stream()
.filter(term -> term.getKey() != null)
.forEach(term -> majorVersions.stream()
.filter(Objects::nonNull)
.map(version -> "v" + version)
.filter(version -> (!term.getKey().endsWith(version)
&& !Character.isDigit(term.getKey().charAt(term.getKey().length() - 1))
&& !products.containsKey(term.getKey() + version)))
.forEach(version -> {
addTerm(temp, term.getKey() + version);
}));
.filter(Objects::nonNull)
.map(version -> "v" + version)
.filter(version -> (!term.getKey().endsWith(version)
&& !Character.isDigit(term.getKey().charAt(term.getKey().length() - 1))
&& !products.containsKey(term.getKey() + version)))
.forEach(version -> {
addTerm(temp, term.getKey() + version);
}));
products.putAll(temp);
}

Expand Down Expand Up @@ -957,7 +962,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
Loading

0 comments on commit edaba8f

Please sign in to comment.