CVE-2020-13756: Improper Control of Generation of Code ('Code Injection') (CWE-94)
value: Sabberworm PHP CSS Parser before 8.3.1 calls eval on uncontrolled data, possibly leading to remote code execution if the function allSelectors() or getSelectorsBySpecificity() is called with input from an attacker.
CWE description: The product constructs all or part of a code segment using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the syntax or behavior of the intended code segment.
CVSS v3 base score: 9.8
Published: 2020-06-03T14:15Z
Repository: https://github.com/sabberworm/PHP-CSS-Parser
Language: PHP
Files changed: lib/Sabberworm/CSS/CSSList/CSSBlockList.php, lib/Sabberworm/CSS/CSSList/Document.php, tests/Sabberworm/CSS/ParserTest.php
Commit date: 2020-06-01 11:03:20 +0200
Commit message: Don’t use eval

Diff: diff --git a/lib/Sabberworm/CSS/CSSList/CSSBlockList.php b/lib/Sabberworm/CSS/CSSList/CSSBlockList.php
index 55a594f..233eb54 100644
--- a/lib/Sabberworm/CSS/CSSList/CSSBlockList.php
+++ b/lib/Sabberworm/CSS/CSSList/CSSBlockList.php
@@ -63,13 +63,38 @@ abstract class CSSBlockList extends CSSList {
 		foreach ($aDeclarationBlocks as $oBlock) {
 			foreach ($oBlock->getSelectors() as $oSelector) {
 				if ($sSpecificitySearch === null) {
 					$aResult[] = $oSelector;
 				} else {
-					$sComparison = "\$bRes = {$oSelector->getSpecificity()} $sSpecificitySearch;";
-					eval($sComparison);
-					if ($bRes) {
+					$sComparator = '===';
+					$aSpecificitySearch = explode(' ', $sSpecificitySearch);
+					$iTargetSpecificity = $aSpecificitySearch[0];
+					if(count($aSpecificitySearch) > 1) {
+						$sComparator = $aSpecificitySearch[0];
+						$iTargetSpecificity = $aSpecificitySearch[1];
+					}
+					$iTargetSpecificity = (int)$iTargetSpecificity;
+					$iSelectorSpecificity = $oSelector->getSpecificity();
+					$bMatches = false;
+					switch($sComparator) {
+						case '<=':
+							$bMatches = $iSelectorSpecificity <= $iTargetSpecificity;
+						break;
+						case '<':
+							$bMatches = $iSelectorSpecificity < $iTargetSpecificity;
+						break;
+						case '>=':
+							$bMatches = $iSelectorSpecificity >= $iTargetSpecificity;
+						break;
+						case '>':
+							$bMatches = $iSelectorSpecificity > $iTargetSpecificity;
+						break;
+						default:
+							$bMatches = $iSelectorSpecificity === $iTargetSpecificity;
+						break;
+					}
+					if ($bMatches) {
 						$aResult[] = $oSelector;
 					}
 				}
 			}
 		}
diff --git a/lib/Sabberworm/CSS/CSSList/Document.php b/lib/Sabberworm/CSS/CSSList/Document.php
index 265c9ce..fee661c 100644
--- a/lib/Sabberworm/CSS/CSSList/Document.php
+++ b/lib/Sabberworm/CSS/CSSList/Document.php
@@ -55,13 +55,10 @@ class Document extends CSSBlockList {
      * Note that this does not yield the full DeclarationBlock that the selector belongs to (and, currently, there is no way to get to that).
      * @param $sSpecificitySearch An optional filter by specificity. May contain a comparison operator and a number or just a number (defaults to "==").
      * @example getSelectorsBySpecificity('>= 100')
      */
     public function getSelectorsBySpecificity($sSpecificitySearch = null) {
-        if (is_numeric($sSpecificitySearch) || is_numeric($sSpecificitySearch[0])) {
-            $sSpecificitySearch = "== $sSpecificitySearch";
-        }
         $aResult = array();
         $this->allSelectors($aResult, $sSpecificitySearch);
         return $aResult;
     }
 
diff --git a/tests/Sabberworm/CSS/ParserTest.php b/tests/Sabberworm/CSS/ParserTest.php
index 9e8b964..ad20b0f 100644
--- a/tests/Sabberworm/CSS/ParserTest.php
+++ b/tests/Sabberworm/CSS/ParserTest.php
@@ -144,10 +144,17 @@ class ParserTest extends \PHPUnit_Framework_TestCase {
 				default:
 					$this->fail("specificity: untested selector " . $oSelector->getSelector());
 			}
 		}
 		$this->assertEquals(array(new Selector('#test .help', true)), $oDoc->getSelectorsBySpecificity('> 100'));
+		$this->assertEquals(array(new Selector('#test .help', true), new Selector('#file', true)), $oDoc->getSelectorsBySpecificity('>= 100'));
+		$this->assertEquals(array(new Selector('#file', true)), $oDoc->getSelectorsBySpecificity('=== 100'));
+		$this->assertEquals(array(new Selector('#file', true)), $oDoc->getSelectorsBySpecificity('== 100'));
+		$this->assertEquals(array(new Selector('#file', true), new Selector('.help:hover', true), new Selector('li.green', true), new Selector('ol li::before', true)), $oDoc->getSelectorsBySpecificity('<= 100'));
+		$this->assertEquals(array(new Selector('.help:hover', true), new Selector('li.green', true), new Selector('ol li::before', true)), $oDoc->getSelectorsBySpecificity('< 100'));
+		$this->assertEquals(array(new Selector('li.green', true)), $oDoc->getSelectorsBySpecificity('11'));
+		$this->assertEquals(array(new Selector('ol li::before', true)), $oDoc->getSelectorsBySpecificity(3));
 	}
 
 	function testManipulation() {
 		$oDoc = $this->parsedStructureForFile('atrules');
 		$this->assertSame('@charset "utf-8";@font-face {font-family: "CrassRoots";src: url("../media/cr.ttf");}html, body {font-size: 1.6em;}

Vulnerable code: 					$sComparison = "\$bRes = {$oSelector->getSpecificity()} $sSpecificitySearch;";
					eval($sComparison);
					if ($bRes) {
        if (is_numeric($sSpecificitySearch) || is_numeric($sSpecificitySearch[0])) {
            $sSpecificitySearch = "== $sSpecificitySearch";
        }
Fixed code: 					$sComparator = '===';
					$aSpecificitySearch = explode(' ', $sSpecificitySearch);
					$iTargetSpecificity = $aSpecificitySearch[0];
					if(count($aSpecificitySearch) > 1) {
						$sComparator = $aSpecificitySearch[0];
						$iTargetSpecificity = $aSpecificitySearch[1];
					}
					$iTargetSpecificity = (int)$iTargetSpecificity;
					$iSelectorSpecificity = $oSelector->getSpecificity();
					$bMatches = false;
					switch($sComparator) {
						case '<=':
							$bMatches = $iSelectorSpecificity <= $iTargetSpecificity;
						break;
						case '<':
							$bMatches = $iSelectorSpecificity < $iTargetSpecificity;
						break;
						case '>=':
							$bMatches = $iSelectorSpecificity >= $iTargetSpecificity;
						break;
						case '>':
							$bMatches = $iSelectorSpecificity > $iTargetSpecificity;
						break;
						default:
							$bMatches = $iSelectorSpecificity === $iTargetSpecificity;
						break;
					}
					if ($bMatches) {
		$this->assertEquals(array(new Selector('#test .help', true), new Selector('#file', true)), $oDoc->getSelectorsBySpecificity('>= 100'));
		$this->assertEquals(array(new Selector('#file', true)), $oDoc->getSelectorsBySpecificity('=== 100'));
		$this->assertEquals(array(new Selector('#file', true)), $oDoc->getSelectorsBySpecificity('== 100'));
		$this->assertEquals(array(new Selector('#file', true), new Selector('.help:hover', true), new Selector('li.green', true), new Selector('ol li::before', true)), $oDoc->getSelectorsBySpecificity('<= 100'));
		$this->assertEquals(array(new Selector('.help:hover', true), new Selector('li.green', true), new Selector('ol li::before', true)), $oDoc->getSelectorsBySpecificity('< 100'));
		$this->assertEquals(array(new Selector('li.green', true)), $oDoc->getSelectorsBySpecificity('11'));
		$this->assertEquals(array(new Selector('ol li::before', true)), $oDoc->getSelectorsBySpecificity(3));