Skip to content

Commit 5653ef1

Browse files
ghislainpiotsonartech
ghislainpiot
authored and
sonartech
committed
SONARPY-1726 Create rule S6377 XML signatures should be validated securely (#199)
GitOrigin-RevId: 54594a66480dbe7e997ff38b126db64553c27aaf
1 parent adb7790 commit 5653ef1

12 files changed

Lines changed: 465 additions & 20 deletions

File tree

‎python-checks/src/main/java/org/sonar/python/checks/OpenSourceCheckList.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@
7070
import org.sonar.python.checks.hotspots.StrongCryptographicKeysCheck;
7171
import org.sonar.python.checks.hotspots.UnsafeHttpMethodsCheck;
7272
import org.sonar.python.checks.hotspots.UnverifiedHostnameCheck;
73+
import org.sonar.python.checks.hotspots.XMLSignatureValidationCheck;
7374
import org.sonar.python.checks.regex.AnchorPrecedenceCheck;
7475
import org.sonar.python.checks.regex.DuplicatesInCharacterClassCheck;
7576
import org.sonar.python.checks.regex.EmptyAlternativeCheck;
@@ -407,6 +408,7 @@ public Stream<Class<?>> getChecks() {
407408
WildcardImportCheck.class,
408409
WrongAssignmentOperatorCheck.class,
409410
XMLParserXXEVulnerableCheck.class,
411+
XMLSignatureValidationCheck.class,
410412
DjangoModelFormFieldsCheck.class,
411413
DjangoReceiverDecoratorCheck.class,
412414
DjangoModelStringFieldCheck.class,

‎python-checks/src/main/java/org/sonar/python/checks/hotspots/CommonValidationUtils.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.sonar.plugins.python.api.tree.Name;
2525
import org.sonar.plugins.python.api.tree.NumericLiteral;
2626
import org.sonar.plugins.python.api.tree.RegularArgument;
27+
import org.sonar.plugins.python.api.tree.StringLiteral;
2728
import org.sonar.plugins.python.api.tree.Tree;
2829
import org.sonar.python.checks.utils.Expressions;
2930
import org.sonar.python.tree.TreeUtils;
@@ -70,6 +71,15 @@ static boolean isEqualTo(Expression expression, int number) {
7071
}
7172
}
7273

74+
static String singleAssignedString(Expression expression) {
75+
if (expression instanceof Name name) {
76+
return Expressions.singleAssignedNonNameValue(name)
77+
.map(CommonValidationUtils::singleAssignedString)
78+
.orElse("");
79+
}
80+
return expression.is(Tree.Kind.STRING_LITERAL) ? ((StringLiteral) expression).trimmedQuotesValue() : "";
81+
}
82+
7383
interface CallValidator {
7484
void validate(SubscriptionContext ctx, CallExpression callExpression);
7585
}

‎python-checks/src/main/java/org/sonar/python/checks/hotspots/FastHashingOrPlainTextCheck.java‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@
3939
import org.sonar.plugins.python.api.types.v2.TriBool;
4040
import org.sonar.python.checks.hotspots.CommonValidationUtils.ArgumentValidator;
4141
import org.sonar.python.checks.hotspots.CommonValidationUtils.CallValidator;
42-
import org.sonar.python.checks.utils.Expressions;
4342
import org.sonar.python.tree.TreeUtils;
4443
import org.sonar.python.types.v2.TypeCheckBuilder;
4544
import org.sonar.python.types.v2.TypeCheckMap;
@@ -310,7 +309,7 @@ private static boolean subscriptionIsFlaskBcryptConfig(Expression expression, Ty
310309
var subscriptMatch = subscription.subscripts().expressions()
311310
.stream()
312311
.findFirst()
313-
.filter(expr -> "BCRYPT_LOG_ROUNDS".equals(singleAssignedString(expr)));
312+
.filter(expr -> "BCRYPT_LOG_ROUNDS".equals(CommonValidationUtils.singleAssignedString(expr)));
314313
return subscriptMatch.isPresent();
315314
}
316315

@@ -350,15 +349,6 @@ private void checkCallExpr(SubscriptionContext subscriptionContext) {
350349
}
351350
}
352351

353-
private static String singleAssignedString(Expression expression) {
354-
if (expression.is(Tree.Kind.NAME)) {
355-
return Expressions.singleAssignedNonNameValue(((Name) expression))
356-
.map(FastHashingOrPlainTextCheck::singleAssignedString)
357-
.orElse("");
358-
}
359-
return expression.is(Tree.Kind.STRING_LITERAL) ? ((StringLiteral) expression).trimmedQuotesValue() : "";
360-
}
361-
362352
record PBKDF2Validator(
363353
int algoPosition,
364354
String algoKeyword,
@@ -371,7 +361,7 @@ public void validate(SubscriptionContext ctx, CallExpression callExpression) {
371361
nthArgumentOrKeywordOptional(algoPosition, algoKeyword, callExpression.arguments());
372362
var algoString = algoArgument
373363
.map(RegularArgument::expression)
374-
.map(FastHashingOrPlainTextCheck::singleAssignedString)
364+
.map(CommonValidationUtils::singleAssignedString)
375365
.orElse("");
376366
if (!PBKDF2_ALGOS.contains(algoString)) {
377367
return;

‎python-checks/src/main/java/org/sonar/python/checks/hotspots/XMLSignatureValidationCheck.java‎

Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
/*
2+
* SonarQube Python Plugin
3+
* Copyright (C) 2011-2025 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
package org.sonar.python.checks.hotspots;
18+
19+
import java.util.Collection;
20+
import java.util.Collections;
21+
import java.util.HashMap;
22+
import java.util.List;
23+
import java.util.Map;
24+
import java.util.Optional;
25+
import java.util.stream.Collectors;
26+
import org.sonar.check.Rule;
27+
import org.sonar.plugins.python.api.PythonSubscriptionCheck;
28+
import org.sonar.plugins.python.api.SubscriptionContext;
29+
import org.sonar.plugins.python.api.tree.CallExpression;
30+
import org.sonar.plugins.python.api.tree.DictionaryLiteral;
31+
import org.sonar.plugins.python.api.tree.Expression;
32+
import org.sonar.plugins.python.api.tree.ExpressionList;
33+
import org.sonar.plugins.python.api.tree.KeyValuePair;
34+
import org.sonar.plugins.python.api.tree.ListLiteral;
35+
import org.sonar.plugins.python.api.tree.Name;
36+
import org.sonar.plugins.python.api.tree.QualifiedExpression;
37+
import org.sonar.plugins.python.api.tree.RegularArgument;
38+
import org.sonar.plugins.python.api.tree.Tree;
39+
import org.sonar.plugins.python.api.tree.UnpackingExpression;
40+
import org.sonar.plugins.python.api.types.v2.TriBool;
41+
import org.sonar.python.checks.utils.Expressions;
42+
import org.sonar.python.tree.TreeUtils;
43+
import org.sonar.python.types.v2.TypeCheckBuilder;
44+
import org.sonar.python.types.v2.TypeCheckMap;
45+
46+
import static org.sonar.python.checks.hotspots.CommonValidationUtils.singleAssignedString;
47+
48+
@Rule(key = "S6377")
49+
public class XMLSignatureValidationCheck extends PythonSubscriptionCheck {
50+
51+
private static final List<String> VERIFY_REQUIRED_ONE_OF = List.of(
52+
"x509_cert",
53+
"cert_subject_name",
54+
"cert_resolver",
55+
"ca_pem_file",
56+
"ca_path",
57+
"hmac_key"
58+
);
59+
private static final String MESSAGE = "Change this code to only accept signatures computed from a trusted party.";
60+
private static final String MESSAGE_SECONDARY = "Unsafe parameter set here";
61+
62+
private TypeCheckBuilder xmlVerifierVerifyTypeChecker;
63+
private TypeCheckBuilder dictTypeChecker;
64+
private TypeCheckBuilder signatureConfigurationTypeChecker;
65+
private TypeCheckMap<Boolean> signatureMethodTypeCheckMap;
66+
67+
@Override
68+
public void initialize(Context context) {
69+
context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, this::registerTypeCheckers);
70+
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, this::checkCallExpr);
71+
}
72+
73+
private void registerTypeCheckers(SubscriptionContext subscriptionContext) {
74+
xmlVerifierVerifyTypeChecker = subscriptionContext.typeChecker().typeCheckBuilder().isTypeWithFqn("signxml.XMLVerifier");
75+
dictTypeChecker = subscriptionContext.typeChecker().typeCheckBuilder().isTypeWithName("dict");
76+
signatureConfigurationTypeChecker = subscriptionContext.typeChecker().typeCheckBuilder().isTypeWithFqn("signxml.SignatureConfiguration");
77+
signatureMethodTypeCheckMap = TypeCheckMap.ofEntries(
78+
Map.entry(subscriptionContext.typeChecker().typeCheckBuilder().isTypeWithFqn("signxml.SignatureMethod.HMAC_SHA224"), true),
79+
Map.entry(subscriptionContext.typeChecker().typeCheckBuilder().isTypeWithFqn("signxml.SignatureMethod.HMAC_SHA256"), true),
80+
Map.entry(subscriptionContext.typeChecker().typeCheckBuilder().isTypeWithFqn("signxml.SignatureMethod.HMAC_SHA384"), true),
81+
Map.entry(subscriptionContext.typeChecker().typeCheckBuilder().isTypeWithFqn("signxml.SignatureMethod.HMAC_SHA512"), true)
82+
);
83+
}
84+
85+
private void checkCallExpr(SubscriptionContext subscriptionContext) {
86+
var callExpression = ((CallExpression) subscriptionContext.syntaxNode());
87+
var qualifiedExprCallee = Optional.of(callExpression)
88+
.map(CallExpression::callee)
89+
.flatMap(TreeUtils.toOptionalInstanceOfMapper(QualifiedExpression.class))
90+
.orElse(null);
91+
if (qualifiedExprCallee == null) {
92+
return;
93+
}
94+
var ok = qualifiedExpressionIsVerifyCall(qualifiedExprCallee);
95+
if (!ok) {
96+
return;
97+
}
98+
Map<String, Tree> argToTree = new HashMap<>();
99+
for (var arg : callExpression.arguments()) {
100+
if (arg instanceof RegularArgument regularArgument) {
101+
String argumentName = Optional.ofNullable(regularArgument.keywordArgument()).map(Name::name).orElse("");
102+
argToTree.put(argumentName, regularArgument.expression());
103+
} else {
104+
var keys = TreeUtils.toOptionalInstanceOf(UnpackingExpression.class, arg)
105+
.map(UnpackingExpression::expression)
106+
.flatMap(TreeUtils.toOptionalInstanceOfMapper(Name.class))
107+
.flatMap(Expressions::singleAssignedNonNameValue)
108+
.map(this::keysInUnpacking)
109+
.orElseGet(Map::of);
110+
argToTree.putAll(keys);
111+
}
112+
}
113+
114+
if (Collections.disjoint(argToTree.keySet(), VERIFY_REQUIRED_ONE_OF)) {
115+
subscriptionContext.addIssue(callExpression.callee(), MESSAGE);
116+
}
117+
118+
var expectConfigValue = argToTree.get("expect_config");
119+
if (expectConfigValue != null) {
120+
checkExpectConfig(subscriptionContext, expectConfigValue, callExpression);
121+
}
122+
}
123+
124+
private void checkExpectConfig(SubscriptionContext subscriptionContext, Tree expectConfigValue, CallExpression verifyCallExpression) {
125+
expectConfigValue = replaceBySingleAssigned(expectConfigValue);
126+
if (expectConfigValue instanceof CallExpression callExpression
127+
&& signatureConfigurationTypeChecker.check(callExpression.callee().typeV2()) == TriBool.TRUE) {
128+
TreeUtils.nthArgumentOrKeywordOptional(0, "require_x509", callExpression.arguments())
129+
.filter(arg -> Expressions.isFalsy(arg.expression()))
130+
.ifPresent(arg -> subscriptionContext.addIssue(verifyCallExpression, MESSAGE).secondary(arg, MESSAGE_SECONDARY));
131+
132+
TreeUtils.nthArgumentOrKeywordOptional(3, "signature_methods", callExpression.arguments())
133+
.map(this::getOffendingInList)
134+
.filter(list -> !list.isEmpty())
135+
.ifPresent(list -> {
136+
var issue = subscriptionContext.addIssue(verifyCallExpression, MESSAGE);
137+
list.forEach(secondary -> issue.secondary(secondary, MESSAGE_SECONDARY));
138+
});
139+
}
140+
}
141+
142+
private static Tree replaceBySingleAssigned(Tree expectConfigValue) {
143+
if (expectConfigValue.is(Tree.Kind.NAME)) {
144+
expectConfigValue = Expressions.singleAssignedNonNameValue((Name) expectConfigValue).orElse(null);
145+
}
146+
return expectConfigValue;
147+
}
148+
149+
private List<Expression> getOffendingInList(RegularArgument regularArgument) {
150+
var expression = replaceBySingleAssigned(regularArgument.expression());
151+
return TreeUtils.toOptionalInstanceOf(ListLiteral.class, expression)
152+
.map(ListLiteral::elements)
153+
.map(ExpressionList::expressions)
154+
.stream()
155+
.flatMap(Collection::stream)
156+
.filter(e -> signatureMethodTypeCheckMap.getOptionalForType(e.typeV2()).isEmpty())
157+
.toList();
158+
}
159+
160+
161+
private Map<String, Tree> keysInUnpacking(Expression expression) {
162+
if (expression instanceof CallExpression callExpression && dictTypeChecker.check(callExpression.callee().typeV2()) == TriBool.TRUE) {
163+
return callExpression.arguments().stream()
164+
.flatMap(TreeUtils.toStreamInstanceOfMapper(RegularArgument.class))
165+
.filter(regularArgument -> regularArgument.keywordArgument() != null)
166+
.collect(Collectors.toMap(
167+
regularArgument -> regularArgument.keywordArgument().name(),
168+
RegularArgument::expression
169+
));
170+
}
171+
if (expression instanceof DictionaryLiteral dictionaryLiteral) {
172+
var output = new HashMap<String, Tree>();
173+
for (var element : dictionaryLiteral.elements()) {
174+
if (element instanceof KeyValuePair keyValuePair) {
175+
176+
var key = singleAssignedString(keyValuePair.key());
177+
if (!key.isEmpty()) {
178+
output.put(key, keyValuePair.value());
179+
}
180+
} else {
181+
var unpackedKeys = TreeUtils.toOptionalInstanceOf(UnpackingExpression.class, element)
182+
.map(UnpackingExpression::expression)
183+
.flatMap(TreeUtils.toOptionalInstanceOfMapper(Name.class))
184+
.flatMap(Expressions::singleAssignedNonNameValue)
185+
.map(this::keysInUnpacking)
186+
.orElseGet(Map::of);
187+
output.putAll(unpackedKeys);
188+
}
189+
}
190+
return output;
191+
}
192+
return Map.of();
193+
}
194+
195+
private boolean qualifiedExpressionIsVerifyCall(QualifiedExpression qualifiedExprCallee) {
196+
if (qualifiedExprCallee.qualifier() instanceof CallExpression callExpression) {
197+
return xmlVerifierVerifyTypeChecker.check(callExpression.callee().typeV2()) == TriBool.TRUE;
198+
}
199+
if (qualifiedExprCallee.qualifier() instanceof Name name) {
200+
// This check can never be true because we don't have stubs for signxml.
201+
// Currently, if we try to instantiate the XMLVerifier and then call the verify method on the variable,
202+
// the name will have an unknown type.
203+
return xmlVerifierVerifyTypeChecker.check(name.typeV2()) == TriBool.TRUE;
204+
}
205+
return false;
206+
}
207+
}

‎python-checks/src/main/java/org/sonar/python/checks/utils/Expressions.java‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ private Expressions() {
6161
}
6262

6363
// https://docs.python.org/3/library/stdtypes.html#truth-value-testing
64-
public static boolean isFalsy(@Nullable Expression expression) {
64+
private static boolean isFalsyInternal(@Nullable Expression expression) {
6565
if (expression == null) {
6666
return false;
6767
}
@@ -77,8 +77,18 @@ public static boolean isFalsy(@Nullable Expression expression) {
7777
};
7878
}
7979

80+
public static boolean isFalsy(@Nullable Expression expression) {
81+
if (expression instanceof Name name) {
82+
if (Expressions.isFalsyInternal(expression)) {
83+
return true;
84+
}
85+
return Optional.ofNullable(Expressions.singleAssignedValue(name)).map(Expressions::isFalsyInternal).orElse(false);
86+
}
87+
return Expressions.isFalsyInternal(expression);
88+
}
89+
8090
// https://docs.python.org/3/library/stdtypes.html#truth-value-testing
81-
public static boolean isTruthy(@Nullable Expression expression) {
91+
private static boolean isTruthyInternal(@Nullable Expression expression) {
8292
if (expression == null) {
8393
return false;
8494
}
@@ -89,6 +99,16 @@ public static boolean isTruthy(@Nullable Expression expression) {
8999
};
90100
}
91101

102+
public static boolean isTruthy(@Nullable Expression expression) {
103+
if (expression instanceof Name name) {
104+
if (Expressions.isTruthyInternal(expression)) {
105+
return true;
106+
}
107+
return Optional.ofNullable(Expressions.singleAssignedValue(name)).map(Expressions::isTruthyInternal).orElse(false);
108+
}
109+
return Expressions.isTruthyInternal(expression);
110+
}
111+
92112
@CheckForNull
93113
public static Expression singleAssignedValue(Name name) {
94114
return singleAssignedValue(name, new HashSet<>());

‎python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S6377.html‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
<p>XML signatures are a method used to ensure the integrity and authenticity of XML documents. However, if XML signatures are not validated securely,
2+
it can lead to potential vulnerabilities.</p>
3+
<h2>Why is this an issue?</h2>
4+
<p>XML can be used for a wide variety of purposes. Using a signature on an XML message generally indicates this message requires authenticity and
5+
integrity. However, if the signature validation is not properly implemented this authenticity can not be guaranteed.</p>
6+
<h3>What is the potential impact?</h3>
7+
<p>By not enforcing secure validation, the XML Digital Signature API is more susceptible to attacks such as signature spoofing and injections.</p>
8+
<h3>Increased Vulnerability to Signature Spoofing</h3>
9+
<p>By disabling secure validation, the application becomes more susceptible to signature spoofing attacks. Attackers can potentially manipulate the
10+
XML signature in a way that bypasses the validation process, allowing them to forge or tamper with the signature. This can lead to the acceptance of
11+
invalid or maliciously modified signatures, compromising the integrity and authenticity of the XML documents.</p>
12+
<h3>Risk of Injection Attacks</h3>
13+
<p>Disabling secure validation can expose the application to injection attacks. Attackers can inject malicious code or entities into the XML document,
14+
taking advantage of the weakened validation process. In some cases, it can also expose the application to denial-of-service attacks. Attackers can
15+
exploit vulnerabilities in the validation process to cause excessive resource consumption or system crashes, leading to service unavailability or
16+
disruption.</p>
17+
<h2>How to fix it in SignXML</h2>
18+
<h3>Code examples</h3>
19+
<p>The following noncompliant code example verifies an XML signature without providing a trusted signing authority. This code will accept any
20+
signature created from a generally trusted certificate, for example, a Let’s encrypt one.</p>
21+
<h4>Noncompliant code example</h4>
22+
<pre data-diff-id="1" data-diff-type="noncompliant">
23+
from lxml import etree
24+
from signxml import XMLVerifier
25+
26+
xml_file = open("signed.xml", "rb")
27+
xml = etree.parse(xml_file)
28+
29+
XMLVerifier().verify(xml) # Noncompliant
30+
</pre>
31+
<h4>Compliant solution</h4>
32+
<pre data-diff-id="1" data-diff-type="compliant">
33+
from lxml import etree
34+
from signxml import XMLVerifier
35+
36+
xml_file = open("signed.xml", "rb")
37+
xml = etree.parse(xml_file)
38+
39+
cert_file = open("cert.pem", "rb")
40+
cert = cert_file.read()
41+
XMLVerifier().verify(xml, x509_cert=cert)
42+
</pre>
43+
<h3>How does this work?</h3>
44+
<p>Here, the compliant solution provides a trusted certificate to the signature validation function. This will ensure only signatures computed with
45+
the private key associated with the provided certificate will be accepted.</p>
46+
<h2>Resources</h2>
47+
<h3>Standards</h3>
48+
<ul>
49+
<li> OWASP - <a href="https://owasp.org/Top10/A02_2021-Cryptographic_Failures/">Top 10:2021 A02:2021 - Cryptographic Failures</a> </li>
50+
<li> OWASP - <a href="https://owasp.org/www-project-top-ten/2017/A3_2017-Sensitive_Data_Exposure">Top 10 2017 Category A3 - Sensitive Data
51+
Exposure</a> </li>
52+
<li> CWE - <a href="https://cwe.mitre.org/data/definitions/347">CWE-347 - Improper Verification of Cryptographic Signature</a> </li>
53+
<li> STIG Viewer - <a href="https://stigviewer.com/stig/application_security_and_development/2023-06-08/finding/V-222608">Application Security and
54+
Development: V-222608</a> - The application must not be vulnerable to XML-oriented attacks. </li>
55+
</ul>
56+

0 commit comments

Comments
 (0)