-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Refactor] tighten resource constraints for XML/XPath collector #3999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -39,6 +39,7 @@ | |||||||||||||||||||||||||||||||||||
| import java.util.Set; | ||||||||||||||||||||||||||||||||||||
| import java.util.stream.Collectors; | ||||||||||||||||||||||||||||||||||||
| import javax.net.ssl.SSLException; | ||||||||||||||||||||||||||||||||||||
| import javax.xml.XMLConstants; | ||||||||||||||||||||||||||||||||||||
| import javax.xml.parsers.DocumentBuilder; | ||||||||||||||||||||||||||||||||||||
| import javax.xml.parsers.DocumentBuilderFactory; | ||||||||||||||||||||||||||||||||||||
| import javax.xml.xpath.XPath; | ||||||||||||||||||||||||||||||||||||
|
|
@@ -106,6 +107,22 @@ | |||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| @Slf4j | ||||||||||||||||||||||||||||||||||||
| public class HttpCollectImpl extends AbstractCollect { | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Pre-compiled regex patterns for dangerous XPath detection. | ||||||||||||||||||||||||||||||||||||
| * Compiled once at class load for performance. | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| private static final List<Pattern> DANGEROUS_XPATH_PATTERNS; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| static { | ||||||||||||||||||||||||||||||||||||
| List<Pattern> patterns = new ArrayList<>(); | ||||||||||||||||||||||||||||||||||||
| for (String pattern : CollectorConstants.DANGEROUS_XPATH_PATTERNS) { | ||||||||||||||||||||||||||||||||||||
| // Use CASE_INSENSITIVE to prevent bypass via case variations (e.g., //TEXT() vs //text()) | ||||||||||||||||||||||||||||||||||||
| patterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE | Pattern.DOTALL)); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| DANGEROUS_XPATH_PATTERNS = Collections.unmodifiableList(patterns); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private final Set<Integer> defaultSuccessStatusCodes = Set.of( | ||||||||||||||||||||||||||||||||||||
| HttpStatus.SC_OK, | ||||||||||||||||||||||||||||||||||||
| HttpStatus.SC_CREATED, | ||||||||||||||||||||||||||||||||||||
|
|
@@ -357,21 +374,83 @@ private void parseResponseBySiteMap(String resp, List<String> aliasFields, | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Validates the XPath expression to prevent DoS attacks. | ||||||||||||||||||||||||||||||||||||
| * Checks for dangerous patterns that could traverse the entire XML document. | ||||||||||||||||||||||||||||||||||||
| * Uses pre-compiled patterns for performance and case-insensitive matching for security. | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * @param xpathExpression the XPath expression to validate | ||||||||||||||||||||||||||||||||||||
| * @throws IllegalArgumentException if the expression contains dangerous patterns | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| private void validateXPathExpression(String xpathExpression) throws IllegalArgumentException { | ||||||||||||||||||||||||||||||||||||
| if (!StringUtils.hasText(xpathExpression)) { | ||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Check against dangerous patterns using pre-compiled regex | ||||||||||||||||||||||||||||||||||||
| for (Pattern pattern : DANGEROUS_XPATH_PATTERNS) { | ||||||||||||||||||||||||||||||||||||
| Matcher matcher = pattern.matcher(xpathExpression); | ||||||||||||||||||||||||||||||||||||
| if (matcher.find()) { | ||||||||||||||||||||||||||||||||||||
| throw new IllegalArgumentException( | ||||||||||||||||||||||||||||||||||||
| "XPath expression contains dangerous pattern that may cause DoS: " + pattern.pattern() | ||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Check for excessive wildcard usage (more than 3 // or * operators) | ||||||||||||||||||||||||||||||||||||
| long descendantAxisCount = xpathExpression.chars().filter(ch -> ch == '/').count(); | ||||||||||||||||||||||||||||||||||||
| long wildcardCount = xpathExpression.chars().filter(ch -> ch == '*').count(); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (descendantAxisCount > 10 || wildcardCount > 5) { | ||||||||||||||||||||||||||||||||||||
| throw new IllegalArgumentException( | ||||||||||||||||||||||||||||||||||||
| "XPath expression contains too many wildcards or descendant axes, potential DoS risk" | ||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| log.debug("XPath expression validation passed: {}", xpathExpression); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| private void parseResponseByXmlPath(String resp, Metrics metrics, | ||||||||||||||||||||||||||||||||||||
| CollectRep.MetricsData.Builder builder, Long responseTime) { | ||||||||||||||||||||||||||||||||||||
| HttpProtocol http = metrics.getHttp(); | ||||||||||||||||||||||||||||||||||||
| List<String> aliasFields = metrics.getAliasFields(); | ||||||||||||||||||||||||||||||||||||
| String xpathExpression = http.getParseScript(); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Layer 1: Validate XPath expression is not empty | ||||||||||||||||||||||||||||||||||||
| if (!StringUtils.hasText(xpathExpression)) { | ||||||||||||||||||||||||||||||||||||
| log.warn("Http collect parse type is xmlPath, but the xpath expression is empty."); | ||||||||||||||||||||||||||||||||||||
| builder.setCode(CollectRep.Code.FAIL); | ||||||||||||||||||||||||||||||||||||
| builder.setMsg("XPath expression is empty"); | ||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Layer 2: Check XML response size to prevent memory exhaustion | ||||||||||||||||||||||||||||||||||||
| if (resp != null && resp.length() > CollectorConstants.MAX_XML_RESPONSE_SIZE) { | ||||||||||||||||||||||||||||||||||||
| log.warn("XML response size {} bytes exceeds maximum allowed size {} bytes", | ||||||||||||||||||||||||||||||||||||
| resp.length(), CollectorConstants.MAX_XML_RESPONSE_SIZE); | ||||||||||||||||||||||||||||||||||||
| builder.setCode(CollectRep.Code.FAIL); | ||||||||||||||||||||||||||||||||||||
| builder.setMsg("XML response exceeds maximum allowed size of " | ||||||||||||||||||||||||||||||||||||
| + (CollectorConstants.MAX_XML_RESPONSE_SIZE / 1024 / 1024) + "MB"); | ||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+428
to
+434
|
||||||||||||||||||||||||||||||||||||
| if (resp != null && resp.length() > CollectorConstants.MAX_XML_RESPONSE_SIZE) { | |
| log.warn("XML response size {} bytes exceeds maximum allowed size {} bytes", | |
| resp.length(), CollectorConstants.MAX_XML_RESPONSE_SIZE); | |
| builder.setCode(CollectRep.Code.FAIL); | |
| builder.setMsg("XML response exceeds maximum allowed size of " | |
| + (CollectorConstants.MAX_XML_RESPONSE_SIZE / 1024 / 1024) + "MB"); | |
| return; | |
| if (resp != null) { | |
| int respSizeBytes = resp.getBytes(StandardCharsets.UTF_8).length; | |
| if (respSizeBytes > CollectorConstants.MAX_XML_RESPONSE_SIZE) { | |
| log.warn("XML response size {} bytes exceeds maximum allowed size {} bytes", | |
| respSizeBytes, CollectorConstants.MAX_XML_RESPONSE_SIZE); | |
| builder.setCode(CollectRep.Code.FAIL); | |
| builder.setMsg("XML response exceeds maximum allowed size of " | |
| + (CollectorConstants.MAX_XML_RESPONSE_SIZE / 1024 / 1024) + "MB"); | |
| return; | |
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -49,4 +49,28 @@ public interface CollectorConstants extends NetworkConstants { | |||||
|
|
||||||
| String STATUS_CODE = "statusCode"; | ||||||
|
|
||||||
| /** | ||||||
| * Maximum XML response size in bytes (10MB) to prevent DoS attacks | ||||||
| */ | ||||||
| int MAX_XML_RESPONSE_SIZE = 10 * 1024 * 1024; | ||||||
|
|
||||||
| /** | ||||||
| * Maximum number of nodes returned by XPath query to prevent excessive resource consumption | ||||||
| */ | ||||||
| int MAX_XPATH_RESULT_NODES = 1000; | ||||||
|
|
||||||
| /** | ||||||
| * Dangerous XPath expression patterns that could cause DoS attacks | ||||||
| * These patterns match expressions that traverse the entire XML document | ||||||
| */ | ||||||
| String[] DANGEROUS_XPATH_PATTERNS = { | ||||||
| "//\\*\\s*\\|\\s*//@\\*\\s*\\|\\s*//text\\(\\)", // //* | //@* | //text() | ||||||
| "//\\*\\s*\\|", // //* | ... | ||||||
| "//@\\*\\s*\\|", // //@* | ... | ||||||
| "//node\\(\\)\\s*\\|", // //node() | ... | ||||||
| "descendant-or-self::node\\(\\)\\s*\\|", // descendant-or-self::node() | ... | ||||||
| "/descendant-or-self::node\\(\\)", // /descendant-or-self::node() | ||||||
| "//\\*[\\s\\S]*//\\*" // //** with multiple wildcards | ||||||
|
||||||
| "//\\*[\\s\\S]*//\\*" // //** with multiple wildcards | |
| "//\\*[\\s\\S]*//\\*" // //* ... //* (two wildcard-descendant segments with arbitrary content between) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "Check for excessive wildcard usage (more than 3 // or * operators)" does not match the implementation: the code counts every '/' (not specifically the '//' descendant axis) and uses thresholds of 10 for '/' and 5 for '' instead of 3. Please either adjust the thresholds/logic to match the documented rule, or update the comment so it accurately describes the condition being enforced (e.g., clarify it's counting total '/' and '' characters with the given limits).