From 84a362d9adfd45d92634f64be5e4350dbd40d3d7 Mon Sep 17 00:00:00 2001 From: Hans Aikema Date: Wed, 6 Oct 2021 18:16:31 +0200 Subject: [PATCH 1/5] Add an integrationtest that triggers the issue in the current codebase --- .../invoker.properties | 19 ++++++++ .../it/3679-classifier-in-dependency/pom.xml | 34 +++++++++++++++ .../postbuild.groovy | 43 +++++++++++++++++++ 3 files changed, 96 insertions(+) create mode 100644 maven/src/it/3679-classifier-in-dependency/invoker.properties create mode 100644 maven/src/it/3679-classifier-in-dependency/pom.xml create mode 100644 maven/src/it/3679-classifier-in-dependency/postbuild.groovy diff --git a/maven/src/it/3679-classifier-in-dependency/invoker.properties b/maven/src/it/3679-classifier-in-dependency/invoker.properties new file mode 100644 index 00000000000..19bdedfee05 --- /dev/null +++ b/maven/src/it/3679-classifier-in-dependency/invoker.properties @@ -0,0 +1,19 @@ +# +# This file is part of dependency-check-maven. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Copyright (c) 2014 Jeremy Long. All Rights Reserved. +# + +invoker.goals = --no-transfer-progress --batch-mode -Danalyzer.ossindex.enabled=false -Danalyzer.central.enabled=false ${project.groupId}:${project.artifactId}:${project.version}:check -Dformat=XML -Dcve.startyear=2018 diff --git a/maven/src/it/3679-classifier-in-dependency/pom.xml b/maven/src/it/3679-classifier-in-dependency/pom.xml new file mode 100644 index 00000000000..995884ecfb7 --- /dev/null +++ b/maven/src/it/3679-classifier-in-dependency/pom.xml @@ -0,0 +1,34 @@ + + + + 4.0.0 + org.owasp.test + 3679-classifier-dependency + 1.0.0-SNAPSHOT + jar + + + com.google.inject + guice + 4.2.2 + no_aop + provided + + + diff --git a/maven/src/it/3679-classifier-in-dependency/postbuild.groovy b/maven/src/it/3679-classifier-in-dependency/postbuild.groovy new file mode 100644 index 00000000000..5a54a1619c8 --- /dev/null +++ b/maven/src/it/3679-classifier-in-dependency/postbuild.groovy @@ -0,0 +1,43 @@ +/* + * This file is part of dependency-check-maven. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * Copyright (c) 2014 Jeremy Long. All Rights Reserved. + */ + +import org.apache.commons.io.FileUtils; +import org.w3c.dom.NodeList; + +import java.nio.charset.Charset; +import javax.xml.xpath.* +import javax.xml.parsers.DocumentBuilderFactory + +// Check to see if jackson-databind-2.5.3.jar was identified with a known CVE - using CVE-2018-7489. + +def countMatches(String xml, String xpathQuery) { + def xpath = XPathFactory.newInstance().newXPath() + def builder = DocumentBuilderFactory.newInstance().newDocumentBuilder() + def inputStream = new ByteArrayInputStream( xml.bytes ) + def records = builder.parse(inputStream).documentElement + NodeList nodes = xpath.evaluate( xpathQuery, records, XPathConstants.NODESET ) as NodeList + nodes.getLength(); +} + +String log = FileUtils.readFileToString(new File(basedir, "target/dependency-check-report.xml"), Charset.defaultCharset().name()); +int count = countMatches(log,"/analysis/dependencies/dependency[./fileName = 'guice-2.4.4-no_aop.jar']"); +if (count != 1){ + System.out.println(String.format("google guice no_aop was identified %s times, expected 1", count)); + return false; +} +return true; From aa5e2d3eda006b2d047ce3803d113aea6d50af0e Mon Sep 17 00:00:00 2001 From: Hans Aikema Date: Wed, 6 Oct 2021 18:18:04 +0200 Subject: [PATCH 2/5] Work around MSHARED-998 by resolving all dependencies when we need to resolve a DependencyNode with a classifier --- .../maven/BaseDependencyCheckMojo.java | 58 +++++++++++++++---- 1 file changed, 48 insertions(+), 10 deletions(-) diff --git a/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java b/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java index ea798d0a757..76c639aef71 100644 --- a/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java +++ b/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java @@ -87,6 +87,7 @@ import java.util.HashSet; import java.util.List; import java.util.Locale; +import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -1328,17 +1329,29 @@ private ExceptionCollection collectMavenDependencies(Engine engine, MavenProject final List dependencies = project.getDependencies(); final List managedDependencies = project.getDependencyManagement() == null ? null : project.getDependencyManagement().getDependencies(); - final TransformableFilter filter = new PatternInclusionsFilter( - Collections.singletonList(TransferUtils.toArtifactCoordinate(dependencyNode.getArtifact()).toString())); - final Iterable singleResult = - dependencyResolver.resolveDependencies(buildingRequest, dependencies, managedDependencies, filter); - - if (singleResult.iterator().hasNext()) { - final ArtifactResult first = singleResult.iterator().next(); - result = first.getArtifact(); + final ArtifactCoordinate theCoord = TransferUtils.toArtifactCoordinate(dependencyNode.getArtifact()); + if (theCoord.getClassifier() != null) { + // This would trigger NPE when using the filter - MSHARED-998 + getLog().debug("Expensive lookup as workaround for MSHARED-998 for "+theCoord); + final Iterable allDeps = + dependencyResolver.resolveDependencies(buildingRequest, dependencies, managedDependencies, + null); + result = findClassifierArtifactInAllDeps(allDeps, theCoord); } else { - throw new DependencyNotFoundException(String.format("Failed to resolve dependency %s with " - + "dependencyResolver", coordinate)); + final TransformableFilter filter = new PatternInclusionsFilter( + Collections.singletonList( + TransferUtils.toArtifactCoordinate(dependencyNode.getArtifact()).toString())); + final Iterable singleResult = + dependencyResolver.resolveDependencies(buildingRequest, dependencies, managedDependencies, + filter); + + if (singleResult.iterator().hasNext()) { + final ArtifactResult first = singleResult.iterator().next(); + result = first.getArtifact(); + } else { + throw new DependencyNotFoundException(String.format("Failed to resolve dependency %s with " + + "dependencyResolver", coordinate)); + } } } catch (DependencyNotFoundException | DependencyResolverException ex) { getLog().debug(String.format("Aggregate : %s", aggregate)); @@ -1440,6 +1453,31 @@ && addSnapshotReactorDependency(engine, dependencyNode.getArtifact())) { return exCol; } + /** Utility method for a work-around to MSHARED-998 */ + private Artifact findClassifierArtifactInAllDeps(final Iterable allDeps, final ArtifactCoordinate theCoord) { + Artifact result = null; + for (final ArtifactResult res : allDeps) { + if (sameArtifact(res, theCoord)) { + result = res.getArtifact(); + break; + } + } + return result; + } + + /** Utility method for a work-around to MSHARED-998 */ + private boolean sameArtifact(final ArtifactResult res, final ArtifactCoordinate theCoord) { + if (res == null || res.getArtifact() == null || theCoord == null) { + return false; + } + boolean result = Objects.equals(res.getArtifact().getGroupId(), theCoord.getGroupId()); + result &= Objects.equals(res.getArtifact().getArtifactId(), theCoord.getArtifactId()); + result &= Objects.equals(res.getArtifact().getVersion(), theCoord.getVersion()); + result &= Objects.equals(res.getArtifact().getClassifier(), theCoord.getClassifier()); + result &= Objects.equals(res.getArtifact().getType(), theCoord.getExtension()); + return result; + } + /** * @param project the {@link MavenProject} * @param dependencyNode the {@link DependencyNode} From f65a9886aa841b836f1ec487853da3ddd7f48419 Mon Sep 17 00:00:00 2001 From: Hans Aikema Date: Wed, 6 Oct 2021 18:45:26 +0200 Subject: [PATCH 3/5] Use the proper expected filename in the integration test --- maven/src/it/3679-classifier-in-dependency/postbuild.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/maven/src/it/3679-classifier-in-dependency/postbuild.groovy b/maven/src/it/3679-classifier-in-dependency/postbuild.groovy index 5a54a1619c8..d264f127797 100644 --- a/maven/src/it/3679-classifier-in-dependency/postbuild.groovy +++ b/maven/src/it/3679-classifier-in-dependency/postbuild.groovy @@ -35,7 +35,7 @@ def countMatches(String xml, String xpathQuery) { } String log = FileUtils.readFileToString(new File(basedir, "target/dependency-check-report.xml"), Charset.defaultCharset().name()); -int count = countMatches(log,"/analysis/dependencies/dependency[./fileName = 'guice-2.4.4-no_aop.jar']"); +int count = countMatches(log,"/analysis/dependencies/dependency[./fileName = 'guice-4.2.2-no_aop.jar']"); if (count != 1){ System.out.println(String.format("google guice no_aop was identified %s times, expected 1", count)); return false; From 2d09f3226672e3f9ba6407841180d2fcf084d7d4 Mon Sep 17 00:00:00 2001 From: Hans Aikema Date: Wed, 6 Oct 2021 19:26:53 +0200 Subject: [PATCH 4/5] Add an accidentally forgotten throw of DependencyNotFoundException when the artifact cannot be resolved --- .../dependencycheck/maven/BaseDependencyCheckMojo.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java b/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java index 76c639aef71..0cc9dd2c108 100644 --- a/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java +++ b/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java @@ -1454,7 +1454,8 @@ && addSnapshotReactorDependency(engine, dependencyNode.getArtifact())) { } /** Utility method for a work-around to MSHARED-998 */ - private Artifact findClassifierArtifactInAllDeps(final Iterable allDeps, final ArtifactCoordinate theCoord) { + private Artifact findClassifierArtifactInAllDeps(final Iterable allDeps, final ArtifactCoordinate theCoord) + throws DependencyNotFoundException { Artifact result = null; for (final ArtifactResult res : allDeps) { if (sameArtifact(res, theCoord)) { @@ -1462,6 +1463,10 @@ private Artifact findClassifierArtifactInAllDeps(final Iterable break; } } + if (result == null) { + throw new DependencyNotFoundException(String.format("Expected dependency not found in resolved artifacts for " + + "dependency %s", theCoord)); + } return result; } From 93ac23e4ca33f24b2653d16423f864eb08f19fdb Mon Sep 17 00:00:00 2001 From: Hans Aikema Date: Wed, 6 Oct 2021 19:31:15 +0200 Subject: [PATCH 5/5] Process the Checkstyle remarks --- .../maven/BaseDependencyCheckMojo.java | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java b/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java index 0cc9dd2c108..9d2f92ace45 100644 --- a/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java +++ b/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java @@ -1332,7 +1332,7 @@ private ExceptionCollection collectMavenDependencies(Engine engine, MavenProject final ArtifactCoordinate theCoord = TransferUtils.toArtifactCoordinate(dependencyNode.getArtifact()); if (theCoord.getClassifier() != null) { // This would trigger NPE when using the filter - MSHARED-998 - getLog().debug("Expensive lookup as workaround for MSHARED-998 for "+theCoord); + getLog().debug("Expensive lookup as workaround for MSHARED-998 for " + theCoord); final Iterable allDeps = dependencyResolver.resolveDependencies(buildingRequest, dependencies, managedDependencies, null); @@ -1453,7 +1453,14 @@ && addSnapshotReactorDependency(engine, dependencyNode.getArtifact())) { return exCol; } - /** Utility method for a work-around to MSHARED-998 */ + /** + * Utility method for a work-around to MSHARED-998 + * @param allDeps The Iterable of the resolved artifacts for all dependencies + * @param theCoord The ArtifactCoordinate of the artifact-with-classifier we intended to resolve + * @return the resolved artifact matching with {@code theCoord} + * @throws DependencyNotFoundException Not expected to be thrown, but will be thrown if {@code theCoord} could not be + * found within {@code allDeps} + */ private Artifact findClassifierArtifactInAllDeps(final Iterable allDeps, final ArtifactCoordinate theCoord) throws DependencyNotFoundException { Artifact result = null; @@ -1470,7 +1477,12 @@ private Artifact findClassifierArtifactInAllDeps(final Iterable return result; } - /** Utility method for a work-around to MSHARED-998 */ + /** + * Utility method for a work-around to MSHARED-998 + * @param res A single ArtifactResult obtained from the DependencyResolver + * @param theCoord The coordinates of the Artifact that we try to find + * @return {@code true} when theCoord is non-null and matches with the artifact of res + */ private boolean sameArtifact(final ArtifactResult res, final ArtifactCoordinate theCoord) { if (res == null || res.getArtifact() == null || theCoord == null) { return false;