From 13116c5381db476263ede5cca9180bb3fe6366d5 Mon Sep 17 00:00:00 2001 From: Jeremy Long Date: Sun, 22 Jun 2014 16:34:39 -0400 Subject: [PATCH] added support for suppression by GAV (issue #124), created base suppression.xml (issue #123), and fixed false positives related to spring security (issue #130) Former-commit-id: 330134211d022fec336dc1ca39205a94a088ee84 --- .../analyzer/AbstractSuppressionAnalyzer.java | 13 ++- .../suppression/SuppressionHandler.java | 12 ++- .../suppression/SuppressionRule.java | 81 ++++++++++++++++--- .../dependencycheck-base-suppression.xml | 12 +++ .../AbstractSuppressionAnalyzerTest.java | 6 +- .../suppression/SuppressionRuleTest.java | 68 ++++++++++++++-- src/site/markdown/suppression.md | 9 +++ 7 files changed, 172 insertions(+), 29 deletions(-) create mode 100644 dependency-check-core/src/main/resources/dependencycheck-base-suppression.xml diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java index 466c1d96d..3c66004e1 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java @@ -98,11 +98,18 @@ public abstract class AbstractSuppressionAnalyzer extends AbstractAnalyzer { * @throws SuppressionParseException thrown if the XML cannot be parsed. */ private void loadSuppressionData() throws SuppressionParseException { + final SuppressionParser parser = new SuppressionParser(); + File file = null; + file = new File(this.getClass().getClassLoader().getResource("dependencycheck-base-suppression.xml").getPath()); + try { + rules = parser.parseSuppressionRules(file); + } catch (SuppressionParseException ex) { + LOGGER.log(Level.FINE, "Unable to parse the base suppression data file", ex); + } final String suppressionFilePath = Settings.getString(Settings.KEYS.SUPPRESSION_FILE); if (suppressionFilePath == null) { return; } - File file = null; boolean deleteTempFile = false; try { final Pattern uriRx = Pattern.compile("^(https?|file)\\:.*", Pattern.CASE_INSENSITIVE); @@ -132,9 +139,9 @@ public abstract class AbstractSuppressionAnalyzer extends AbstractAnalyzer { } if (file != null) { - final SuppressionParser parser = new SuppressionParser(); try { - rules = parser.parseSuppressionRules(file); + //rules = parser.parseSuppressionRules(file); + rules.addAll(parser.parseSuppressionRules(file)); LOGGER.log(Level.FINE, rules.size() + " suppression rules were loaded."); } catch (SuppressionParseException ex) { final String msg = String.format("Unable to parse suppression xml file '%s'", file.getPath()); diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/suppression/SuppressionHandler.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/suppression/SuppressionHandler.java index 498390b9b..4906f4e19 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/suppression/SuppressionHandler.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/suppression/SuppressionHandler.java @@ -54,6 +54,10 @@ public class SuppressionHandler extends DefaultHandler { * The CWE element name. */ public static final String CWE = "cwe"; + /** + * The GAV element name. + */ + public static final String GAV = "gav"; /** * The cvssBelow element name. */ @@ -95,13 +99,10 @@ public class SuppressionHandler extends DefaultHandler { */ @Override public void startElement(String uri, String localName, String qName, Attributes attributes) throws SAXException { - currentAttributes = null; + currentAttributes = attributes; currentText = new StringBuffer(); - if (SUPPRESS.equals(qName)) { rule = new SuppressionRule(); - } else if (FILE_PATH.equals(qName)) { - currentAttributes = attributes; } } @@ -123,6 +124,9 @@ public class SuppressionHandler extends DefaultHandler { rule.setFilePath(pt); } else if (SHA1.equals(qName)) { rule.setSha1(currentText.toString()); + } else if (GAV.equals(qName)) { + final PropertyType pt = processPropertyType(); + rule.setGav(pt); } else if (CPE.equals(qName)) { final PropertyType pt = processPropertyType(); rule.addCpe(pt); diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/suppression/SuppressionRule.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/suppression/SuppressionRule.java index a0e94f3f2..9e1261e31 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/suppression/SuppressionRule.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/suppression/SuppressionRule.java @@ -20,6 +20,7 @@ package org.owasp.dependencycheck.suppression; import java.util.ArrayList; import java.util.Iterator; import java.util.List; +import java.util.logging.Logger; import org.owasp.dependencycheck.dependency.Dependency; import org.owasp.dependencycheck.dependency.Identifier; import org.owasp.dependencycheck.dependency.Vulnerability; @@ -30,6 +31,10 @@ import org.owasp.dependencycheck.dependency.Vulnerability; */ public class SuppressionRule { + /** + * The Logger for use throughout the class + */ + private static final Logger LOGGER = Logger.getLogger(SuppressionRule.class.getName()); /** * The file path for the suppression. */ @@ -280,16 +285,19 @@ public class SuppressionRule { return; } if (gav != null) { + LOGGER.info(this.toString()); final Iterator itr = dependency.getIdentifiers().iterator(); - boolean hasMatch = false; + boolean gavFound = false; while (itr.hasNext()) { final Identifier i = itr.next(); + LOGGER.info(String.format("%nChecking %s for gav:%s", i.getValue(), this.gav)); if (identifierMatches("maven", this.gav, i)) { - hasMatch = true; + LOGGER.info("GAV Matched!"); + gavFound = true; break; } } - if (!hasMatch) { + if (!gavFound) { return; } } @@ -298,8 +306,17 @@ public class SuppressionRule { final Iterator itr = dependency.getIdentifiers().iterator(); while (itr.hasNext()) { final Identifier i = itr.next(); + if (this.gav != null) { + LOGGER.info(String.format("%nProcessesing %s", i.getValue())); + } for (PropertyType c : this.cpe) { + if (this.gav != null) { + LOGGER.info(String.format("%nChecking %s for cpe:%s", i.getValue(), c.getValue())); + } if (identifierMatches("cpe", c, i)) { + if (this.gav != null) { + LOGGER.info(String.format("%nRemoving %s", i.getValue())); + } dependency.addSuppressedIdentifier(i); itr.remove(); break; @@ -355,7 +372,7 @@ public class SuppressionRule { boolean cpeHasNoVersion(PropertyType c) { if (c.isRegex()) { return false; - } // cpe:/a:jboss:jboss:1.0.0: + } // cpe:/a:jboss:jboss:1.0.0 if (countCharacter(c.getValue(), ':') == 3) { return true; } @@ -390,20 +407,62 @@ public class SuppressionRule { if (identifierType.equals(identifier.getType())) { if (suppressionEntry.matches(identifier.getValue())) { return true; - } else if (cpeHasNoVersion(suppressionEntry)) { + } else if ("cpe".equals(identifierType) && cpeHasNoVersion(suppressionEntry)) { if (suppressionEntry.isCaseSensitive()) { - if (identifier.getValue().startsWith(suppressionEntry.getValue())) { - return true; - } + return identifier.getValue().startsWith(suppressionEntry.getValue()); } else { final String id = identifier.getValue().toLowerCase(); final String check = suppressionEntry.getValue().toLowerCase(); - if (id.startsWith(check)) { - return true; - } + return id.startsWith(check); } } } return false; } + + @Override + public String toString() { + StringBuilder sb = new StringBuilder(); + sb.append("SuppressionRule{"); + if (filePath != null) { + sb.append("filePath=").append(filePath).append(","); + } + if (sha1 != null) { + sb.append("sha1=").append(sha1).append(","); + } + if (gav != null) { + sb.append("gav=").append(gav).append(","); + } + if (cpe != null && cpe.size() > 0) { + sb.append("cpe={"); + for (PropertyType pt : cpe) { + sb.append(pt).append(","); + } + sb.append("}"); + } + if (cwe != null && cwe.size() > 0) { + sb.append("cwe={"); + for (String s : cwe) { + sb.append(s).append(","); + } + sb.append("}"); + } + if (cve != null && cve.size() > 0) { + sb.append("cve={"); + for (String s : cve) { + sb.append(s).append(","); + } + sb.append("}"); + } + if (cvssBelow != null && cvssBelow.size() > 0) { + sb.append("cvssBelow={"); + for (Float s : cvssBelow) { + sb.append(s).append(","); + } + sb.append("}"); + } + sb.append("}"); + return sb.toString(); + } + } diff --git a/dependency-check-core/src/main/resources/dependencycheck-base-suppression.xml b/dependency-check-core/src/main/resources/dependencycheck-base-suppression.xml new file mode 100644 index 000000000..3a01a55f5 --- /dev/null +++ b/dependency-check-core/src/main/resources/dependencycheck-base-suppression.xml @@ -0,0 +1,12 @@ + + + + + org\.springframework\.security:spring.* + cpe:/a:mod_security:mod_security + cpe:/a:springsource:spring_framework + cpe:/a:vmware:springsource_spring_framework + + \ No newline at end of file diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzerTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzerTest.java index f20a13d44..94ab3c317 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzerTest.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzerTest.java @@ -34,8 +34,8 @@ import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; /** * @author Jeremy Long @@ -67,7 +67,7 @@ public class AbstractSuppressionAnalyzerTest extends BaseTest { instance.initialize(); int expCount = 5; List result = instance.getRules(); - assertEquals(expCount, result.size()); + assertTrue(expCount <= result.size()); } /** @@ -79,7 +79,7 @@ public class AbstractSuppressionAnalyzerTest extends BaseTest { instance.initialize(); int expCount = 5; List result = instance.getRules(); - assertEquals(expCount, result.size()); + assertTrue(expCount <= result.size()); } @Test(expected = SuppressionParseException.class) diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/suppression/SuppressionRuleTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/suppression/SuppressionRuleTest.java index a5cfc3068..86fb99bf9 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/suppression/SuppressionRuleTest.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/suppression/SuppressionRuleTest.java @@ -343,51 +343,66 @@ public class SuppressionRuleTest { */ @Test public void testCpeMatches() { - Identifier identifier = new Identifier("cwe", "cpe:/a:microsoft:.net_framework:4.5", "some url not needed for this test"); + Identifier identifier = new Identifier("cpe", "cpe:/a:microsoft:.net_framework:4.5", "some url not needed for this test"); PropertyType cpe = new PropertyType(); cpe.setValue("cpe:/a:microsoft:.net_framework:4.5"); SuppressionRule instance = new SuppressionRule(); boolean expResult = true; - boolean result = instance.identifierMatches(cpe, identifier); + boolean result = instance.identifierMatches("cpe", cpe, identifier); assertEquals(expResult, result); cpe.setValue("cpe:/a:microsoft:.net_framework:4.0"); expResult = false; - result = instance.identifierMatches(cpe, identifier); + result = instance.identifierMatches("cpe", cpe, identifier); assertEquals(expResult, result); cpe.setValue("CPE:/a:microsoft:.net_framework:4.5"); cpe.setCaseSensitive(true); expResult = false; - result = instance.identifierMatches(cpe, identifier); + result = instance.identifierMatches("cpe", cpe, identifier); assertEquals(expResult, result); cpe.setValue("cpe:/a:microsoft:.net_framework"); cpe.setCaseSensitive(false); expResult = true; - result = instance.identifierMatches(cpe, identifier); + result = instance.identifierMatches("cpe", cpe, identifier); assertEquals(expResult, result); cpe.setValue("cpe:/a:microsoft:.*"); cpe.setRegex(true); expResult = true; - result = instance.identifierMatches(cpe, identifier); + result = instance.identifierMatches("cpe", cpe, identifier); assertEquals(expResult, result); cpe.setValue("CPE:/a:microsoft:.*"); cpe.setRegex(true); cpe.setCaseSensitive(true); expResult = false; - result = instance.identifierMatches(cpe, identifier); + result = instance.identifierMatches("cpe", cpe, identifier); assertEquals(expResult, result); cpe.setValue("cpe:/a:apache:.*"); cpe.setRegex(true); cpe.setCaseSensitive(false); expResult = false; - result = instance.identifierMatches(cpe, identifier); + result = instance.identifierMatches("cpe", cpe, identifier); + assertEquals(expResult, result); + + identifier = new Identifier("maven", "org.springframework:spring-core:2.5.5", "https://repository.sonatype.org/service/local/artifact/maven/redirect?r=central-proxy&g=org.springframework&a=spring-core&v=2.5.5&e=jar"); + cpe.setValue("org.springframework:spring-core:2.5.5"); + cpe.setRegex(false); + cpe.setCaseSensitive(false); + expResult = true; + result = instance.identifierMatches("maven", cpe, identifier); + assertEquals(expResult, result); + + cpe.setValue("org\\.springframework\\.security:spring.*"); + cpe.setRegex(true); + cpe.setCaseSensitive(false); + expResult = false; + result = instance.identifierMatches("maven", cpe, identifier); assertEquals(expResult, result); } @@ -467,6 +482,43 @@ public class SuppressionRuleTest { assertTrue(dependency.getSuppressedIdentifiers().size() == 3); } + /** + * Test of process method, of class SuppressionRule. + */ + @Test + public void testProcessGAV() { + File spring = new File(this.getClass().getClassLoader().getResource("spring-security-web-3.0.0.RELEASE.jar").getPath()); + Dependency dependency = new Dependency(spring); + dependency.addIdentifier("cpe", "cpe:/a:vmware:springsource_spring_framework:3.0.0", "some url not needed for this test"); + dependency.addIdentifier("cpe", "cpe:/a:springsource:spring_framework:3.0.0", "some url not needed for this test"); + dependency.addIdentifier("cpe", "cpe:/a:mod_security:mod_security:3.0.0", "some url not needed for this test"); + dependency.addIdentifier("cpe", "cpe:/a:vmware:springsource_spring_security:3.0.0", "some url not needed for this test"); + dependency.addIdentifier("maven", "org.springframework.security:spring-security-web:3.0.0.RELEASE", "some url not needed for this test"); + + //cpe + SuppressionRule instance = new SuppressionRule(); + PropertyType pt = new PropertyType(); + + pt.setValue("org\\.springframework\\.security:spring.*"); + pt.setRegex(true); + pt.setCaseSensitive(false); + instance.setGav(pt); + + pt = new PropertyType(); + pt.setValue("cpe:/a:mod_security:mod_security"); + instance.addCpe(pt); + pt = new PropertyType(); + pt.setValue("cpe:/a:springsource:spring_framework"); + instance.addCpe(pt); + pt = new PropertyType(); + pt.setValue("cpe:/a:vmware:springsource_spring_framework"); + instance.addCpe(pt); + + instance.process(dependency); + assertEquals(2, dependency.getIdentifiers().size()); + + } + private Vulnerability createVulnerability() { Vulnerability v = new Vulnerability(); v.setCwe("CWE-287 Improper Authentication"); diff --git a/src/site/markdown/suppression.md b/src/site/markdown/suppression.md index 5d10a6dc5..d3f789a75 100644 --- a/src/site/markdown/suppression.md +++ b/src/site/markdown/suppression.md @@ -64,6 +64,15 @@ HTML version of the report. The other common scenario would be to ignore all CVE ]]> 7 + + + org\.springframework\.security:spring.* + cpe:/a:vmware:springsource_spring_framework + cpe:/a:springsource:spring_framework + cpe:/a:mod_security:mod_security + ```