Skip to content

Commit

Permalink
Fix a subtle issue with missed dependency upgrades due to advisory da…
Browse files Browse the repository at this point in the history 
…tabase sometimes missing a ".RELEASE" suffix in its fixed versions.
  • Loading branch information
@sambsnyd
sambsnyd committed Jul 23, 2024
1 parent 4a776f8 commit 560a374
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 23 deletions.
77 changes: 58 additions & 19 deletions src/main/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,22 @@ public Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
gav.getValue().getVersion(), null, overrideTransitive, null)
.getVisitor(acc.getMavenUdvAcc())
.visitNonNull(t, ctx);
if (t == t2 && gav.getValue().isDotReleaseAmbiguous()) {
t2 = new UpgradeDependencyVersion(gav.getKey().getGroupId(), gav.getKey().getArtifactId(),
gav.getValue().getVersion() + ".RELEASE", null, overrideTransitive, null)
.getVisitor(acc.getMavenUdvAcc())
.visitNonNull(t, ctx);
}
if (t == t2) {
if (Boolean.TRUE.equals(overrideTransitive)) {
AddManagedDependency amd = new AddManagedDependency(gav.getKey().getGroupId(), gav.getKey().getArtifactId(), gav.getValue().getVersion(), null, null, null, null, null, null, null);
t2 = amd.getVisitor(amd.getInitialValue(ctx))
.visitNonNull(t, ctx);
if (t == t2 && gav.getValue().isDotReleaseAmbiguous()) {
amd = new AddManagedDependency(gav.getKey().getGroupId(), gav.getKey().getArtifactId(), gav.getValue().getVersion() + ".RELEASE", null, null, null, null, null, null, null);
t2 = amd.getVisitor(amd.getInitialValue(ctx))
.visitNonNull(t, ctx);
}
if (t != t2) {
t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, requireNonNull(gav.getValue().getBecause()));
}
Expand All @@ -262,23 +273,35 @@ public Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
if (!vulnerabilities.getGavToVulnerabilities().isEmpty()) {
t = enumerateFixesGradleGroovy(vulnerabilities.getGavToVulnerabilities(), acc.getScope(), fixes).visitNonNull(t, ctx);
for (Map.Entry<GroupArtifact, VersionBecause> gaToVb : fixes.entrySet()) {
String because = gaToVb.getValue().getBecause();
VersionBecause vb = gaToVb.getValue();
Tree t2 = new org.openrewrite.gradle.UpgradeDependencyVersion(gaToVb.getKey().getGroupId(), gaToVb.getKey().getArtifactId(),
gaToVb.getValue().getVersion(), null)
vb.getVersion(), null)
.getVisitor(acc.getGradleUdvAcc())
.visitNonNull(t, ctx);
if (t == t2 && vb.isDotReleaseAmbiguous()) {
t2 = new org.openrewrite.gradle.UpgradeDependencyVersion(gaToVb.getKey().getGroupId(), gaToVb.getKey().getArtifactId(),
vb.getVersion() + ".RELEASE", null)
.getVisitor(acc.getGradleUdvAcc())
.visitNonNull(t, ctx);
}
if (t == t2) {
if (Boolean.TRUE.equals(overrideTransitive)) {
t2 = new UpgradeTransitiveDependencyVersion(gaToVb.getKey().getGroupId(), gaToVb.getKey().getArtifactId(),
gaToVb.getValue().getVersion(), null, because, null)
vb.getVersion(), null, vb.getBecause(), null)
.getVisitor()
.visitNonNull(t, ctx);
if(t == t2 && vb.isDotReleaseAmbiguous()) {
t2 = new UpgradeTransitiveDependencyVersion(gaToVb.getKey().getGroupId(), gaToVb.getKey().getArtifactId(),
vb.getVersion() + ".RELEASE", null, vb.getBecause(), null)
.getVisitor()
.visitNonNull(t, ctx);
}
if (t != t2) {
t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, because);
t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, vb.getBecause());
}
}
} else {
t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, because);
t = CommitMessage.message(t2, DependencyVulnerabilityCheck.this, vb.getBecause());
}
}
}
Expand Down Expand Up @@ -363,6 +386,8 @@ private static class VersionBecause {

@Nullable
String because;

boolean dotReleaseAmbiguous;
}

private void analyzeDependency(
Expand All @@ -376,20 +401,26 @@ private void analyzeDependency(

nextVulnerability:
for (Vulnerability v : vs) {
// Some dependencies have a ".RELEASE" suffix.
// For example spring-security-core had a .RELEASE suffix for versions >=2.0.5 and <5.4.0. No suffixes since then
// The vulnerability database is inconsistent about whether the ".RELEASE" is included in the fixed version
// This inconsistency complicates comparisons because "5.3.0" != "5.3.0.RELEASE"
// This inconsistency complicates dependency upgrade since we don't know which version number format to request
// Therefore ignore the suffix during comparison but record it so that version upgrades can try both with and without the suffix
// The edge case of ".RELEASE" being introduced into a version scheme between patch versions is possible but hopefully rare
boolean dotReleaseAmbiguous = resolvedDependency.getVersion().endsWith(".RELEASE") && !v.getFixedVersion().endsWith(".RELEASE");
boolean isLessThanFixed = StringUtils.isBlank(v.getFixedVersion());
if (!isLessThanFixed) {
if (resolvedDependency.getVersion().endsWith(".RELEASE")) {
String versionWithoutRelease = resolvedDependency.getVersion().substring(0,
resolvedDependency.getVersion().length() - ".RELEASE".length());
if (vc.compare(versionParser.transform(v.getFixedVersion()), versionParser.transform(versionWithoutRelease)) > 0) {
isLessThanFixed = true;
}
} else if (vc.compare(versionParser.transform(v.getFixedVersion()), versionParser.transform(resolvedDependency.getVersion())) > 0) {
isLessThanFixed = true;
}
if (!isLessThanFixed
&& vc.compare(
versionParser.transform(stripExtraneousVersionSuffix(v.getFixedVersion())),
versionParser.transform(stripExtraneousVersionSuffix(resolvedDependency.getVersion()))) > 0) {
isLessThanFixed = true;
}

if (isLessThanFixed && vc.compare(versionParser.transform(v.getIntroducedVersion()), versionParser.transform(resolvedDependency.getVersion())) <= 0) {
if (isLessThanFixed
&& vc.compare(
versionParser.transform(stripExtraneousVersionSuffix(v.getIntroducedVersion())),
versionParser.transform(stripExtraneousVersionSuffix(resolvedDependency.getVersion()))) <= 0) {
if (gavVs == null) {
gavVs = vulnerabilities.computeIfAbsent(resolvedDependency.getGav(), ga -> new TreeSet<>(
Comparator.comparing((MinimumDepthVulnerability vDep) -> vDep.getVulnerability().getSeverity()).reversed()
Expand All @@ -403,7 +434,7 @@ private void analyzeDependency(
}
}

gavVs.add(new MinimumDepthVulnerability(resolvedDependency.getDepth(), v));
gavVs.add(new MinimumDepthVulnerability(resolvedDependency.getDepth(), v, dotReleaseAmbiguous));
}
}
}
Expand Down Expand Up @@ -435,7 +466,7 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
if (!StringUtils.isBlank(v.getFixedVersion()) &&
new LatestPatch(null).isValid(gav.getVersion(), v.getFixedVersion())) {
if (fixVersion == null || new StaticVersionComparator().compare(versionParser.transform(v.getFixedVersion()), versionParser.transform(fixVersion)) > 0) {
fixes.put(ga, new VersionBecause(v.getFixedVersion(), v.getCve()));
fixes.put(ga, new VersionBecause(v.getFixedVersion(), v.getCve(), vDepth.dotReleaseAmbiguous));
}
}
}
Expand Down Expand Up @@ -501,7 +532,7 @@ public G.CompilationUnit visitCompilationUnit(G.CompilationUnit cu, ExecutionCon
if (!StringUtils.isBlank(v.getFixedVersion()) &&
new LatestPatch(null).isValid(gav.getVersion(), v.getFixedVersion())) {
if (fixVersion == null || new StaticVersionComparator().compare(versionParser.transform(v.getFixedVersion()), versionParser.transform(fixVersion)) > 0) {
fixes.put(ga, new VersionBecause(v.getFixedVersion(), v.getCve()));
fixes.put(ga, new VersionBecause(v.getFixedVersion(), v.getCve(), vDepth.dotReleaseAmbiguous));
}
}
}
Expand All @@ -519,5 +550,13 @@ public static class MinimumDepthVulnerability {
@NonFinal
int minDepth;
Vulnerability vulnerability;
boolean dotReleaseAmbiguous;
}

private static String stripExtraneousVersionSuffix(String version) {
if (version.endsWith(".RELEASE")) {
return version.substring(0, version.length() - ".RELEASE".length());
}
return version;
}
}
2 changes: 1 addition & 1 deletion src/main/java/org/openrewrite/java/dependencies/Vulnerability.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import java.time.ZonedDateTime;

@Value
@EqualsAndHashCode(onlyExplicitlyIncluded = false)
@EqualsAndHashCode
public class Vulnerability {
@EqualsAndHashCode.Include
String cve;
Expand Down
8 changes: 5 additions & 3 deletions src/test/java/org/openrewrite/java/dependencies/DependencyVulnerabilityCheckTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,13 @@ void maven() {
<artifactId>my-app</artifactId>
<version>1</version>
<dependencies>
<!--~~(This dependency includes org.springframework:spring-context:4.3.23.RELEASE which has the following vulnerabilities:
CVE-2022-22968 (HIGH severity, fixed in 5.2.21.RELEASE) - Improper handling of case sensitivity in Spring Framework)~~>--><dependency>
<!--~~(This dependency includes org.springframework.security:spring-security-core:4.2.13.RELEASE which has the following vulnerabilities:
CVE-2022-22978 (CRITICAL severity, fixed in 5.4.11) - Authorization bypass in Spring Security
CVE-2024-22257 (HIGH severity, fixed in 5.7.12) - Erroneous authentication pass in Spring Security
CVE-2020-5408 (MODERATE severity, fixed in 4.2.16) - Insufficient Entropy in Spring Security)~~>--><dependency>
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-core</artifactId>
<version>4.2.13.RELEASE</version>
<version>4.2.16.RELEASE</version>
</dependency>
<!--~~(This dependency includes org.apache.logging.log4j:log4j:2.13.1 which has the following vulnerabilities:
CVE-2020-9488 (LOW severity, fixed in 2.13.2) - Improper validation of certificate with host mismatch in Apache Log4j SMTP appender)~~>--><dependency>
Expand Down

0 comments on commit 560a374

Please sign in to comment.