From c2a5c8b8f89dde47be41155c0494a07def635ffe Mon Sep 17 00:00:00 2001 From: Johno Crawford Date: Thu, 30 Jan 2025 10:56:52 +0100 Subject: [PATCH] Improve thread safety issue #7338 --- .../analyzer/NvdCveAnalyzer.java | 6 +- .../analyzer/OssIndexAnalyzer.java | 2 +- .../dependency/Vulnerability.java | 57 ++++++++++++++----- .../processing/BundlerAuditProcessor.java | 2 +- 4 files changed, 49 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/owasp/dependencycheck/analyzer/NvdCveAnalyzer.java b/core/src/main/java/org/owasp/dependencycheck/analyzer/NvdCveAnalyzer.java index 1ba5986dd07..54d5a3e896b 100644 --- a/core/src/main/java/org/owasp/dependencycheck/analyzer/NvdCveAnalyzer.java +++ b/core/src/main/java/org/owasp/dependencycheck/analyzer/NvdCveAnalyzer.java @@ -18,7 +18,9 @@ package org.owasp.dependencycheck.analyzer; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; import javax.annotation.concurrent.ThreadSafe; import org.owasp.dependencycheck.Engine; @@ -150,7 +152,7 @@ private synchronized List filterEcosystem(String ecosystem, List< final List remove = new ArrayList<>(); vulnerabilities.forEach((v) -> { boolean found = false; - final List removeSoftare = new ArrayList<>(); + final Set removeSoftare = new HashSet<>(); for (VulnerableSoftware s : v.getVulnerableSoftware()) { if (ecosystemMatchesTargetSoftware(ecosystem, s.getTargetSw())) { found = true; @@ -160,7 +162,7 @@ private synchronized List filterEcosystem(String ecosystem, List< } if (found) { if (!removeSoftare.isEmpty()) { - removeSoftare.forEach(v.getVulnerableSoftware()::remove); + v.removeVulnerableSoftware(removeSoftare); } } else { remove.add(v); diff --git a/core/src/main/java/org/owasp/dependencycheck/analyzer/OssIndexAnalyzer.java b/core/src/main/java/org/owasp/dependencycheck/analyzer/OssIndexAnalyzer.java index a8566dcaeaa..f469e40a780 100644 --- a/core/src/main/java/org/owasp/dependencycheck/analyzer/OssIndexAnalyzer.java +++ b/core/src/main/java/org/owasp/dependencycheck/analyzer/OssIndexAnalyzer.java @@ -271,7 +271,7 @@ void enrich(final Dependency dependency) { .orElse(null); if (existing != null) { //TODO - can we enhance anything other than the references? - existing.getReferences().addAll(v.getReferences()); + existing.addReferences(v.getReferences()); } else { dependency.addVulnerability(v); } diff --git a/core/src/main/java/org/owasp/dependencycheck/dependency/Vulnerability.java b/core/src/main/java/org/owasp/dependencycheck/dependency/Vulnerability.java index 93aa9cf9a2e..3156a8cf9fd 100644 --- a/core/src/main/java/org/owasp/dependencycheck/dependency/Vulnerability.java +++ b/core/src/main/java/org/owasp/dependencycheck/dependency/Vulnerability.java @@ -92,7 +92,7 @@ public enum Source { /** * References for this vulnerability. */ - private final Set references = Collections.synchronizedSet(new HashSet<>()); + private final Set references = new HashSet<>(); /** * A set of vulnerable software. */ @@ -196,7 +196,9 @@ public void setDescription(String description) { * @return the value of references */ public Set getReferences() { - return references; + synchronized (references) { + return Collections.unmodifiableSet(new HashSet<>(references)); + } } /** @@ -207,7 +209,10 @@ public Set getReferences() { * @return the list of references */ public List getReferences(boolean sorted) { - final List sortedRefs = new ArrayList<>(this.references); + final List sortedRefs; + synchronized (references) { + sortedRefs = new ArrayList<>(references); + } if (sorted) { Collections.sort(sortedRefs); } @@ -220,7 +225,9 @@ public List getReferences(boolean sorted) { * @param references a collection of references to add */ public void addReferences(Set references) { - this.references.addAll(references); + synchronized (this.references) { + this.references.addAll(references); + } } /** @@ -229,7 +236,9 @@ public void addReferences(Set references) { * @param ref a reference for the vulnerability */ public void addReference(Reference ref) { - this.references.add(ref); + synchronized (this.references) { + this.references.add(ref); + } } /** @@ -244,7 +253,9 @@ public void addReference(String referenceSource, String referenceName, String re ref.setSource(referenceSource); ref.setName(referenceName); ref.setUrl(referenceUrl); - this.references.add(ref); + synchronized (this.references) { + this.references.add(ref); + } } /** @@ -271,7 +282,9 @@ public org.owasp.dependencycheck.data.knownexploited.json.Vulnerability getKnown * @return the value of vulnerableSoftware */ public Set getVulnerableSoftware() { - return vulnerableSoftware; + synchronized (vulnerableSoftware) { + return Collections.unmodifiableSet(new HashSet<>(vulnerableSoftware)); + } } /** @@ -283,12 +296,24 @@ public Set getVulnerableSoftware() { */ @SuppressWarnings("unchecked") public List getVulnerableSoftware(boolean sorted) { + final List sortedVulnerableSoftware; synchronized (vulnerableSoftware) { - final List sortedVulnerableSoftware = new ArrayList<>(this.vulnerableSoftware); - if (sorted) { - Collections.sort(sortedVulnerableSoftware); - } - return sortedVulnerableSoftware; + sortedVulnerableSoftware = new ArrayList<>(vulnerableSoftware); + } + if (sorted) { + Collections.sort(sortedVulnerableSoftware); + } + return sortedVulnerableSoftware; + } + + /** + * Removes the specified vulnerableSoftware from the collection. + * + * @param vulnerableSoftware a collection of vulnerable software to be removed + */ + public void removeVulnerableSoftware(Set vulnerableSoftware) { + synchronized (this.vulnerableSoftware) { + this.vulnerableSoftware.removeAll(vulnerableSoftware); } } @@ -298,7 +323,9 @@ public List getVulnerableSoftware(boolean sorted) { * @param vulnerableSoftware a collection of vulnerable software */ public void addVulnerableSoftware(Set vulnerableSoftware) { - this.vulnerableSoftware.addAll(vulnerableSoftware); + synchronized (this.vulnerableSoftware) { + this.vulnerableSoftware.addAll(vulnerableSoftware); + } } /** @@ -307,7 +334,9 @@ public void addVulnerableSoftware(Set vulnerableSoftware) { * @param software the vulnerable software reference to add */ public void addVulnerableSoftware(VulnerableSoftware software) { - vulnerableSoftware.add(software); + synchronized (this.vulnerableSoftware) { + vulnerableSoftware.add(software); + } } /** diff --git a/core/src/main/java/org/owasp/dependencycheck/processing/BundlerAuditProcessor.java b/core/src/main/java/org/owasp/dependencycheck/processing/BundlerAuditProcessor.java index 091224ab5a9..747540042e9 100644 --- a/core/src/main/java/org/owasp/dependencycheck/processing/BundlerAuditProcessor.java +++ b/core/src/main/java/org/owasp/dependencycheck/processing/BundlerAuditProcessor.java @@ -201,7 +201,7 @@ private void addReferenceToVulnerability(String parentName, Vulnerability vulner ref.setName(vulnerability.getName()); ref.setSource("bundle-audit"); ref.setUrl(url); - vulnerability.getReferences().add(ref); + vulnerability.addReference(ref); } LOGGER.debug("bundle-audit ({}): {}", parentName, nextLine); }