Skip to content

Commit

Permalink
Fix vuln aliases not being detached when converting to policy proto
Browse files Browse the repository at this point in the history 
Signed-off-by: nscuro <[email protected]>
  • Loading branch information
@nscuro
nscuro committed Dec 20, 2023
1 parent 50d54fa commit d718026
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 9 deletions.
25 changes: 16 additions & 9 deletions ...main/java/org/dependencytrack/event/kafka/processor/VulnerabilityScanResultProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,12 +201,26 @@ private Set<Vulnerability> syncVulnerabilities(final QueryManager qm, final Scan
}

try {
syncedVulns.add(syncVulnerability(qm, vuln, scannerResult.getScanner()));
final Vulnerability syncedVuln = syncVulnerability(qm, vuln, scannerResult.getScanner());

// Detach vulnerabilities from JDO persistence context.
// We do not want to trigger any DB interactions by accessing their fields later.
// Note that even PersistenceManager#detachCopy will load / unload fields based
// on the current FetchPlan. But we just want to keep the data we already have,
// and #makeTransientAll does exactly that.
qm.getPersistenceManager().makeTransient(syncedVuln);

if (vuln.getAliases() != null && !vuln.getAliases().isEmpty()) {
final var syncedAliases = new ArrayList<VulnerabilityAlias>();
for (VulnerabilityAlias alias : vuln.getAliases()) {
qm.synchronizeVulnerabilityAlias(alias);
final VulnerabilityAlias syncedAlias = qm.synchronizeVulnerabilityAlias(alias);
qm.getPersistenceManager().makeTransient(syncedAlias);
syncedAliases.add(syncedAlias);
}
syncedVuln.setAliases(syncedAliases);
}

syncedVulns.add(syncedVuln);
} catch (RuntimeException e) {
// Use a broad catch here, so we can still try to process other
// vulnerabilities, even though processing one of them failed.
Expand All @@ -216,13 +230,6 @@ private Set<Vulnerability> syncVulnerabilities(final QueryManager qm, final Scan
}
}

// Detach vulnerabilities from JDO persistence context.
// We do not want to trigger any DB interactions by accessing their fields.
// Note that even PersistenceManager#detachCopy will load / unload fields based
// on the current FetchPlan. But we just want to keep the data we already have,
// and #makeTransientAll does exactly that.
qm.getPersistenceManager().makeTransientAll(syncedVulns);

return syncedVulns;
}

Expand Down
102 changes: 102 additions & 0 deletions .../java/org/dependencytrack/event/kafka/processor/VulnerabilityScanResultProcessorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.cyclonedx.proto.v1_4.Property;
import org.cyclonedx.proto.v1_4.Source;
import org.cyclonedx.proto.v1_4.VulnerabilityRating;
import org.cyclonedx.proto.v1_4.VulnerabilityReference;
import org.dependencytrack.AbstractPostgresEnabledTest;
import org.dependencytrack.TestCacheManager;
import org.dependencytrack.event.kafka.KafkaEventHeaders;
Expand All @@ -37,6 +38,7 @@
import org.dependencytrack.model.Project;
import org.dependencytrack.model.Severity;
import org.dependencytrack.model.Vulnerability;
import org.dependencytrack.model.VulnerabilityAlias;
import org.dependencytrack.model.VulnerabilityAnalysisLevel;
import org.dependencytrack.model.VulnerabilityPolicy;
import org.dependencytrack.notification.NotificationConstants;
Expand Down Expand Up @@ -985,6 +987,106 @@ public void analysisThroughPolicyExistingEqualAnalysisTest() {
assertThat(kafkaMockProducer.history()).isEmpty();
}

@Test
public void analysisThroughPolicyWithAliasesTest() {
final var project = new Project();
project.setName("acme-app");
project.setVersion("1.0.0");
qm.persist(project);

final var component = new Component();
component.setName("acme-lib");
component.setVersion("1.1.0");
component.setProject(project);
qm.persist(component);

// Create a vulnerability for which no aliases are currently known.
// Aliases will be reported by the ScanResult.
final var vulnA = new Vulnerability();
vulnA.setVulnId("CVE-100");
vulnA.setSource(Vulnerability.Source.NVD);
qm.persist(vulnA);

// Create a vulnerability for which an alias is already known.
// The same alias will be reported by the ScanResult.
final var vulnB = new Vulnerability();
vulnB.setVulnId("CVE-200");
vulnB.setSource(Vulnerability.Source.NVD);
qm.persist(vulnB);
final var vulnAliasB = new VulnerabilityAlias();
vulnAliasB.setCveId("CVE-200");
vulnAliasB.setGhsaId("GHSA-200");
qm.synchronizeVulnerabilityAlias(vulnAliasB);

// Create a policy that suppresses any finding with the alias GHSA-100 or GHSA-200.
final var policyAnalysis = new VulnerabilityPolicyAnalysis();
policyAnalysis.setState(VulnerabilityPolicyAnalysis.State.FALSE_POSITIVE);
policyAnalysis.setSuppress(true);
final var policy = new VulnerabilityPolicy();
policy.setName("Foo");
policy.setConditions(new String[]{"vuln.aliases.exists(alias, alias.id == \"GHSA-100\" || alias.id == \"GHSA-200\")"});
policy.setAnalysis(policyAnalysis);
qm.createVulnerabilityPolicy(policy, null);

// Report three vulnerabilities for the component:
// - CVE-100 with alias GHSA-100 (vuln already in DB, alias is new)
// - CVE-200 with alias GHSA-200 (vuln and alias already in DB)
// - CVE-300 without alias (vuln already in DB)
final var componentUuid = component.getUuid();
final var scanToken = UUID.randomUUID().toString();
final var scanKey = ScanKey.newBuilder().setScanToken(scanToken).setComponentUuid(componentUuid.toString()).build();
final var scanResult = ScanResult.newBuilder()
.setKey(scanKey)
.addScannerResults(ScannerResult.newBuilder()
.setScanner(SCANNER_INTERNAL)
.setStatus(SCAN_STATUS_SUCCESSFUL)
.setBom(Bom.newBuilder().addAllVulnerabilities(List.of(
org.cyclonedx.proto.v1_4.Vulnerability.newBuilder()
.setId("CVE-100")
.setSource(Source.newBuilder().setName("NVD"))
.addReferences(VulnerabilityReference.newBuilder()
.setId("GHSA-100")
.setSource(Source.newBuilder().setName("GITHUB")))
.build(),
org.cyclonedx.proto.v1_4.Vulnerability.newBuilder()
.setId("CVE-200")
.setSource(Source.newBuilder().setName("NVD"))
.addReferences(VulnerabilityReference.newBuilder()
.setId("GHSA-200")
.setSource(Source.newBuilder().setName("GITHUB")))
.build(),
org.cyclonedx.proto.v1_4.Vulnerability.newBuilder()
.setId("CVE-300")
.setSource(Source.newBuilder().setName("NVD"))
.build()
))))
.build();
inputTopic.pipeInput(new TestRecord<>(scanKey, scanResult));
assertThat(outputTopic.readValuesToList()).containsOnly(scanResult);

qm.getPersistenceManager().evictAll();
assertThat(component.getVulnerabilities()).satisfiesExactlyInAnyOrder(
v -> {
assertThat(v.getVulnId()).isEqualTo("CVE-100");
assertThat(qm.getAnalysis(component, v)).satisfies(analysis -> {
assertThat(analysis.getAnalysisState()).isEqualTo(AnalysisState.FALSE_POSITIVE);
assertThat(analysis.isSuppressed()).isTrue();
});
},
v -> {
assertThat(v.getVulnId()).isEqualTo("CVE-200");
assertThat(qm.getAnalysis(component, v)).satisfies(analysis -> {
assertThat(analysis.getAnalysisState()).isEqualTo(AnalysisState.FALSE_POSITIVE);
assertThat(analysis.isSuppressed()).isTrue();
});
},
v -> {
assertThat(v.getVulnId()).isEqualTo("CVE-300");
assertThat(qm.getAnalysis(component, v)).isNull();
}
);
}

private org.cyclonedx.proto.v1_4.Vulnerability createVuln(final String id, final String source) {
return org.cyclonedx.proto.v1_4.Vulnerability.newBuilder()
.setId(id)
Expand Down

0 comments on commit d718026

Please sign in to comment.