Skip to content

Commit da6107f

Browse files
maksim-grebeniuk-sonarsourcesonartech
maksim-grebeniuk-sonarsource
authored and
sonartech
committed
SONARPY-1689 [S4830] Improve pyOpenSSL support (#200)
GitOrigin-RevId: 466fea567f5a91aeb7f66c79d042ae2ead6fcfdf
1 parent 12d3e89 commit da6107f

2 files changed

Lines changed: 127 additions & 47 deletions

File tree

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

Lines changed: 95 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,14 @@
1616
*/
1717
package org.sonar.python.checks;
1818

19+
import java.util.ArrayList;
1920
import java.util.Arrays;
2021
import java.util.Collection;
2122
import java.util.Collections;
2223
import java.util.Comparator;
2324
import java.util.HashSet;
2425
import java.util.List;
2526
import java.util.Map;
26-
import java.util.Objects;
2727
import java.util.Optional;
2828
import java.util.Set;
2929
import java.util.function.Predicate;
@@ -34,17 +34,19 @@
3434
import org.sonar.plugins.python.api.symbols.Usage;
3535
import org.sonar.plugins.python.api.tree.Argument;
3636
import org.sonar.plugins.python.api.tree.AssignmentStatement;
37+
import org.sonar.plugins.python.api.tree.BaseTreeVisitor;
3738
import org.sonar.plugins.python.api.tree.BinaryExpression;
3839
import org.sonar.plugins.python.api.tree.CallExpression;
3940
import org.sonar.plugins.python.api.tree.DictionaryLiteral;
4041
import org.sonar.plugins.python.api.tree.Expression;
4142
import org.sonar.plugins.python.api.tree.ExpressionList;
42-
import org.sonar.plugins.python.api.tree.HasSymbol;
43+
import org.sonar.plugins.python.api.tree.FunctionDef;
4344
import org.sonar.plugins.python.api.tree.KeyValuePair;
4445
import org.sonar.plugins.python.api.tree.Name;
4546
import org.sonar.plugins.python.api.tree.NumericLiteral;
4647
import org.sonar.plugins.python.api.tree.QualifiedExpression;
4748
import org.sonar.plugins.python.api.tree.RegularArgument;
49+
import org.sonar.plugins.python.api.tree.ReturnStatement;
4850
import org.sonar.plugins.python.api.tree.StringLiteral;
4951
import org.sonar.plugins.python.api.tree.Token;
5052
import org.sonar.plugins.python.api.tree.Tree;
@@ -53,28 +55,29 @@
5355
import org.sonar.plugins.python.api.tree.WithStatement;
5456
import org.sonar.plugins.python.api.types.v2.TriBool;
5557
import org.sonar.python.checks.utils.Expressions;
56-
import org.sonar.python.tree.RegularArgumentImpl;
58+
import org.sonar.python.semantic.v2.SymbolV2;
59+
import org.sonar.python.semantic.v2.UsageV2;
5760
import org.sonar.python.tree.TreeUtils;
5861
import org.sonar.python.types.v2.TypeCheckBuilder;
5962
import org.sonar.python.types.v2.TypeCheckMap;
6063

61-
import static java.util.Optional.ofNullable;
62-
6364
// https://jira.sonarsource.com/browse/RSPEC-4830
6465
@Rule(key = "S4830")
6566
public class VerifiedSslTlsCertificateCheck extends PythonSubscriptionCheck {
6667

6768
private static final String MESSAGE = "Enable server certificate validation on this SSL/TLS connection.";
68-
private static final String VERIFY_NONE = Fqn.ssl("VERIFY_NONE");
69+
private static final String VERIFY_NONE = "OpenSSL.SSL.VERIFY_NONE";
6970

7071
private TypeCheckBuilder requestsSessionTypeCheck;
72+
private TypeCheckBuilder isOpenSslContextCheck;
73+
private TypeCheckBuilder isOpenSslContextSetVerifyCheck;
7174
private TypeCheckMap<Set<String>> requestsVerifyArguments;
7275

7376
@Override
7477
public void initialize(Context context) {
7578
context.registerSyntaxNodeConsumer(Tree.Kind.FILE_INPUT, this::initTypeChecks);
7679
context.registerSyntaxNodeConsumer(Tree.Kind.WITH_STMT, VerifiedSslTlsCertificateCheck::verifyAioHttpWithSession);
77-
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, VerifiedSslTlsCertificateCheck::sslSetVerifyCheck);
80+
context.registerSyntaxNodeConsumer(Tree.Kind.ASSIGNMENT_STMT, this::openSslContextCheck);
7881
context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, this::requestsCheck);
7982
context.registerSyntaxNodeConsumer(Tree.Kind.REGULAR_ARGUMENT, VerifiedSslTlsCertificateCheck::standardSslCheckForRegularArgument);
8083
context.registerSyntaxNodeConsumer(Tree.Kind.ASSIGNMENT_STMT, VerifiedSslTlsCertificateCheck::standardSslCheckForAssignmentStatement);
@@ -83,6 +86,8 @@ public void initialize(Context context) {
8386

8487
private void initTypeChecks(SubscriptionContext ctx) {
8588
requestsSessionTypeCheck = ctx.typeChecker().typeCheckBuilder().isInstanceOf(REQUESTS_SESSIONS_FQN);
89+
isOpenSslContextCheck = ctx.typeChecker().typeCheckBuilder().isInstanceOf(OPENSSL_CONTEXT_FQN);
90+
isOpenSslContextSetVerifyCheck = ctx.typeChecker().typeCheckBuilder().isTypeWithName(OPENSSL_CONTEXT_SET_VERIFY_FQN);
8691
requestsVerifyArguments = new TypeCheckMap<>();
8792
CALLS_WHERE_TO_ENFORCE_TRUE_ARGUMENT.forEach(fqn -> {
8893
var check = ctx.typeChecker().typeCheckBuilder().isTypeWithFqn(fqn);
@@ -94,22 +99,24 @@ private static void verifyAioHttpWithSession(SubscriptionContext ctx) {
9499
var withStatement = (WithStatement) ctx.syntaxNode();
95100
withStatement.withItems()
96101
.stream()
97-
.filter(item -> Optional.of(item)
98-
.map(WithItem::test)
99-
.flatMap(TreeUtils.toOptionalInstanceOfMapper(CallExpression.class))
100-
.map(CallExpression::calleeSymbol)
101-
.map(Symbol::fullyQualifiedName)
102-
.filter("aiohttp.ClientSession"::equals)
103-
.isPresent())
102+
.filter(VerifiedSslTlsCertificateCheck::isAioHttpClientSessionCall)
104103
.map(WithItem::expression)
105-
.map(TreeUtils.toOptionalInstanceOfMapper(Name.class))
104+
.map(TreeUtils::getSymbolFromTree)
106105
.filter(Optional::isPresent)
107106
.map(Optional::get)
108-
.map(HasSymbol::symbol)
109-
.filter(Objects::nonNull)
110107
.forEach(symbol -> verifyAioHttpSessionSymbolUsages(ctx, symbol));
111108
}
112109

110+
private static boolean isAioHttpClientSessionCall(WithItem item) {
111+
return Optional.of(item)
112+
.map(WithItem::test)
113+
.flatMap(TreeUtils.toOptionalInstanceOfMapper(CallExpression.class))
114+
.map(CallExpression::calleeSymbol)
115+
.map(Symbol::fullyQualifiedName)
116+
.filter("aiohttp.ClientSession"::equals)
117+
.isPresent();
118+
}
119+
113120
private static void verifyAioHttpSessionSymbolUsages(SubscriptionContext ctx, Symbol sessionSymbol) {
114121
sessionSymbol.usages()
115122
.stream()
@@ -122,10 +129,8 @@ private static void verifyAioHttpSessionSymbolUsages(SubscriptionContext ctx, Sy
122129
.forEach(sessionCallExpr -> verifyVulnerableMethods(ctx, sessionCallExpr, VERIFY_SSL_ARG_NAMES));
123130
}
124131

125-
/**
126-
* Fully qualified name of the <code>set_verify</code> used in <code>sslSetVerifyCheck</code>.
127-
*/
128-
private static final String SET_VERIFY = Fqn.context("set_verify");
132+
private static final String OPENSSL_CONTEXT_SET_VERIFY_FQN = "OpenSSL.SSL.Context.set_verify";
133+
private static final String OPENSSL_CONTEXT_FQN = "OpenSSL.SSL.Context";
129134

130135
/**
131136
* Check for the <code>OpenSSL.SSL.Context.set_verify</code> flag settings.
@@ -135,38 +140,81 @@ private static void verifyAioHttpSessionSymbolUsages(SubscriptionContext ctx, Sy
135140
*
136141
* @param subscriptionContext the subscription context passed by <code>Context.registerSyntaxNodeConsumer</code>.
137142
*/
138-
private static void sslSetVerifyCheck(SubscriptionContext subscriptionContext) {
143+
private void openSslContextCheck(SubscriptionContext subscriptionContext) {
144+
var assignmentStatement = (AssignmentStatement) subscriptionContext.syntaxNode();
145+
var assignedValue = assignmentStatement.lhsExpressions().get(0).expressions().get(0);
146+
Optional.of(assignedValue)
147+
.flatMap(TreeUtils.toOptionalInstanceOfMapper(Name.class))
148+
.filter(n -> isOpenSslContextCheck.check(n.typeV2()) == TriBool.TRUE)
149+
.map(Name::symbolV2)
150+
.ifPresent(symbol -> checkContextSymbol(subscriptionContext, symbol, assignedValue));
151+
}
139152

140-
CallExpression callExpr = (CallExpression) subscriptionContext.syntaxNode();
153+
private void checkContextSymbol(SubscriptionContext subscriptionContext, SymbolV2 symbol, Expression assignedValue) {
154+
var setVerifyCallExpressions = symbol
155+
.usages()
156+
.stream()
157+
.filter(Predicate.not(UsageV2::isBindingUsage))
158+
.map(UsageV2::tree)
159+
.map(Tree::parent)
160+
.flatMap(TreeUtils.toStreamInstanceOfMapper(QualifiedExpression.class))
161+
.filter(qe -> isOpenSslContextSetVerifyCheck.check(qe.typeV2()) == TriBool.TRUE)
162+
.map(Tree::parent)
163+
.flatMap(TreeUtils.toStreamInstanceOfMapper(CallExpression.class))
164+
.toList();
165+
if (setVerifyCallExpressions.isEmpty()) {
166+
subscriptionContext.addIssue(assignedValue, MESSAGE);
167+
} else {
168+
setVerifyCallExpressions
169+
.forEach(setVerifyCallExpression -> checkSetVerifyArguments(subscriptionContext, setVerifyCallExpression));
170+
}
171+
}
141172

142-
boolean isSetVerifyInvocation = ofNullable(callExpr.calleeSymbol())
143-
.map(Symbol::fullyQualifiedName)
144-
.filter(SET_VERIFY::equals)
145-
.isPresent();
173+
private static void checkSetVerifyArguments(SubscriptionContext ctx, CallExpression setVerifyCallExpression) {
174+
TreeUtils.nthArgumentOrKeywordOptional(0, "mode", setVerifyCallExpression.arguments())
175+
.map(RegularArgument::expression)
176+
.map(VerifiedSslTlsCertificateCheck::extractFlags)
177+
.flatMap(VerifiedSslTlsCertificateCheck::checkFlagSettings)
178+
.ifPresent(issueReport -> ctx.addIssue(issueReport.token, MESSAGE));
146179

147-
if (isSetVerifyInvocation) {
148-
List<Argument> args = callExpr.arguments();
149-
if (!args.isEmpty()) {
150-
Tree flagsArgument = args.get(0);
151-
if (flagsArgument.is(Tree.Kind.REGULAR_ARGUMENT)) {
152-
Set<QualifiedExpression> flags = extractFlags(((RegularArgumentImpl) flagsArgument).expression());
153-
checkFlagSettings(flags).ifPresent(issue -> subscriptionContext.addIssue(issue.token, MESSAGE));
154-
}
155-
}
156-
}
180+
TreeUtils.nthArgumentOrKeywordOptional(1, "callback", setVerifyCallExpression.arguments())
181+
.map(RegularArgument::expression)
182+
.filter(callbackArgument -> findCallbackDefinition(callbackArgument)
183+
.filter(VerifiedSslTlsCertificateCheck::isAlwaysTruthyCallback)
184+
.isPresent())
185+
.ifPresent(callbackArgument -> ctx.addIssue(callbackArgument, MESSAGE));
157186
}
158187

159-
/**
160-
* Helper methods for generating FQNs frequently used in this check.
161-
*/
162-
private static class Fqn {
163-
private static String context(@SuppressWarnings("SameParameterValue") String method) {
164-
return ssl("Context." + method);
165-
}
188+
private static Optional<FunctionDef> findCallbackDefinition(Expression callbackArgument) {
189+
return TreeUtils.toOptionalInstanceOf(Name.class, callbackArgument)
190+
.map(Name::symbolV2)
191+
.map(SymbolV2::usages)
192+
.stream()
193+
.flatMap(Collection::stream)
194+
.filter(usageV2 -> usageV2.kind() == UsageV2.Kind.FUNC_DECLARATION)
195+
.map(UsageV2::tree)
196+
.map(Tree::parent)
197+
.flatMap(TreeUtils.toStreamInstanceOfMapper(FunctionDef.class))
198+
.findFirst();
199+
}
166200

167-
private static String ssl(String property) {
168-
return "OpenSSL.SSL." + property;
169-
}
201+
private static boolean isAlwaysTruthyCallback(FunctionDef callbackDefinition) {
202+
var returns = new ArrayList<ReturnStatement>();
203+
var returnsCollector = new BaseTreeVisitor() {
204+
@Override
205+
public void visitReturnStatement(ReturnStatement returnStatement) {
206+
returns.add(returnStatement);
207+
}
208+
};
209+
callbackDefinition.accept(returnsCollector);
210+
return !returns.isEmpty()
211+
&& returns.stream()
212+
.allMatch(VerifiedSslTlsCertificateCheck::isTruthyReturnStatement);
213+
}
214+
215+
private static boolean isTruthyReturnStatement(ReturnStatement returnStatement) {
216+
return returnStatement.expressions().size() == 1
217+
&& Expressions.isTruthy(returnStatement.expressions().get(0));
170218
}
171219

172220
/**

‎python-checks/src/test/resources/checks/verifiedSslTlsCertificateCheck.py‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,38 @@ def pyopensslTest():
2626
ctxC1.set_verify(**kwargs)
2727
ctxC1.set_verify(noSSL.THIS_DOESNT_EXIST)
2828

29+
def truthy_verify_callback_local(a):
30+
if a:
31+
return True
32+
else:
33+
return 1
34+
35+
def verify_callback_local(a):
36+
if a:
37+
return False
38+
else:
39+
return 1
40+
41+
def tuple_return_verify_callback_local(a, b):
42+
return a, b
43+
44+
locally_defined_non_function_callback = ""
45+
ctx2 = SSL.Context(SSL.TLSv1_2_METHOD)
46+
ctx2.set_verify(SSL.VERIFY_PEER, truthy_verify_callback_local) # Noncompliant
47+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
48+
49+
ctx3 = SSL.Context(SSL.TLSv1_2_METHOD)
50+
ctx3.set_verify(SSL.VERIFY_PEER, tuple_return_verify_callback_local) # Compliant
51+
ctx3.set_verify(SSL.VERIFY_PEER, verify_callback_local) # Compliant
52+
ctx3.set_verify(SSL.VERIFY_PEER, locally_defined_non_function_callback) # Compliant
53+
ctx3.set_verify(SSL.VERIFY_PEER, something.unknown) # Compliant
54+
55+
ctx4 = SSL.Context(SSL.TLSv1_2_METHOD) # Noncompliant
56+
57+
ctx5 = SSL.Context(SSL.TLSv1_2_METHOD)
58+
ctx5.use_privatekey(SSL.VERIFY_NONE) # Check that other random method call is not raised with wrong arguments
59+
ctx5.set_verify(SSL.VERIFY_PEER, verify_callback_local) # Compliant
60+
2961
def requestsTests():
3062
# Mutably borrowed from here:
3163
# https://github.com/SonarSource/security-expected-issues/blob/master/python/rules/vulnerabilities/\

0 commit comments

Comments
 (0)