From cacd2487bd83603c0bacd88126c0fbaa4f9a6d13 Mon Sep 17 00:00:00 2001 From: Guillaume Dequenne Date: Mon, 10 Feb 2020 10:25:05 +0100 Subject: [PATCH 1/2] SONARPY-273 Update rule S2068 to avoid FPs on comparisons and method calls --- .../test/resources/expected/python-S2068.json | 7 -- .../hotspots/HardCodedCredentialsCheck.java | 66 +------------------ .../resources/checks/hardCodedCredentials.py | 50 ++++++-------- 3 files changed, 21 insertions(+), 102 deletions(-) diff --git a/its/ruling/src/test/resources/expected/python-S2068.json b/its/ruling/src/test/resources/expected/python-S2068.json index 471a637d26..5d4874b47c 100644 --- a/its/ruling/src/test/resources/expected/python-S2068.json +++ b/its/ruling/src/test/resources/expected/python-S2068.json @@ -14,10 +14,6 @@ 'project:buildbot-0.8.6p1/buildbot/status/client.py':[ 563, ], -'project:buildbot-0.8.6p1/buildbot/status/web/authz.py':[ -140, -161, -], 'project:buildbot-0.8.6p1/buildbot/test/unit/test_changes_pb.py':[ 44, 48, @@ -164,9 +160,6 @@ 259, 276, ], -'project:twisted-12.1.0/twisted/conch/test/test_ssh.py':[ -332, -], 'project:twisted-12.1.0/twisted/manhole/telnet.py':[ 95, ], diff --git a/python-checks/src/main/java/org/sonar/python/checks/hotspots/HardCodedCredentialsCheck.java b/python-checks/src/main/java/org/sonar/python/checks/hotspots/HardCodedCredentialsCheck.java index 5982ba6d24..d94081c818 100644 --- a/python-checks/src/main/java/org/sonar/python/checks/hotspots/HardCodedCredentialsCheck.java +++ b/python-checks/src/main/java/org/sonar/python/checks/hotspots/HardCodedCredentialsCheck.java @@ -37,7 +37,6 @@ import org.sonar.plugins.python.api.symbols.Symbol; import org.sonar.plugins.python.api.tree.Argument; import org.sonar.plugins.python.api.tree.AssignmentStatement; -import org.sonar.plugins.python.api.tree.BinaryExpression; import org.sonar.plugins.python.api.tree.CallExpression; import org.sonar.plugins.python.api.tree.DictionaryLiteral; import org.sonar.plugins.python.api.tree.DictionaryLiteralElement; @@ -48,7 +47,6 @@ import org.sonar.plugins.python.api.tree.Name; import org.sonar.plugins.python.api.tree.Parameter; import org.sonar.plugins.python.api.tree.ParameterList; -import org.sonar.plugins.python.api.tree.QualifiedExpression; import org.sonar.plugins.python.api.tree.RegularArgument; import org.sonar.plugins.python.api.tree.StringElement; import org.sonar.plugins.python.api.tree.StringLiteral; @@ -101,7 +99,7 @@ private Stream literalPatterns() { // Avoid raising on prepared statements String credentials = Stream.of(credentialWords.split(",")) .map(String::trim).collect(Collectors.joining("|")); - literalPatterns = toPatterns("=(?!.*(" + credentials + "))[^:%'?\\s]+"); + literalPatterns = toPatterns("=(?!.*(" + credentials + "))[^:%'?{\\s]+"); } return literalPatterns.stream(); } @@ -109,7 +107,6 @@ private Stream literalPatterns() { @Override public void initialize(Context context) { context.registerSyntaxNodeConsumer(Kind.ASSIGNMENT_STMT, ctx -> handleAssignmentStatement((AssignmentStatement) ctx.syntaxNode(), ctx)); - context.registerSyntaxNodeConsumer(Kind.COMPARISON, ctx -> handleBinaryExpression((BinaryExpression) ctx.syntaxNode(), ctx)); context.registerSyntaxNodeConsumer(Kind.STRING_LITERAL, ctx -> handleStringLiteral((StringLiteral) ctx.syntaxNode(), ctx)); context.registerSyntaxNodeConsumer(Kind.CALL_EXPR, ctx -> handleCallExpression((CallExpression) ctx.syntaxNode(), ctx)); context.registerSyntaxNodeConsumer(Kind.REGULAR_ARGUMENT, ctx -> handleRegularArgument((RegularArgument) ctx.syntaxNode(), ctx)); @@ -166,58 +163,12 @@ private void handleCallExpression(CallExpression callExpression, SubscriptionCon if (callExpression.arguments().isEmpty()) { return; } - // Raising issues on pwd.__eq__("literal") calls - if (callExpression.callee().is(Kind.QUALIFIED_EXPR)) { - QualifiedExpression qualifiedExpression = (QualifiedExpression) callExpression.callee(); - if (qualifiedExpression.name().name().equals("__eq__")) { - checkEqualsCall(callExpression, qualifiedExpression, ctx); - } - } Symbol calleeSymbol = callExpression.calleeSymbol(); if (calleeSymbol != null && sensitiveArgumentByFQN().containsKey(calleeSymbol.fullyQualifiedName())) { checkSensitiveArgument(callExpression, sensitiveArgumentByFQN().get(calleeSymbol.fullyQualifiedName()), ctx); } } - private void checkEqualsCall(CallExpression callExpression, QualifiedExpression qualifiedExpression, SubscriptionContext ctx) { - String matchedCredential = matchedCredentialFromQualifiedExpression(qualifiedExpression); - if (matchedCredential != null) { - if (isFirstArgumentAStringLiteral(callExpression)) { - ctx.addIssue(callExpression, String.format(MESSAGE, matchedCredential)); - } - } else if (qualifiedExpression.qualifier().is(Kind.STRING_LITERAL)) { - String matched = firstArgumentCredential(callExpression); - if (matched != null) { - ctx.addIssue(callExpression, String.format(MESSAGE, matched)); - } - } - } - - private String firstArgumentCredential(CallExpression callExpression) { - Argument argument = callExpression.arguments().get(0); - String matchedCredential = null; - if (argument.is(Kind.REGULAR_ARGUMENT)) { - RegularArgument regularArgument = (RegularArgument) argument; - if (regularArgument.expression().is(Kind.NAME)) { - matchedCredential = matchedCredential(((Name) regularArgument.expression()).name(), variablePatterns()); - } - } - return matchedCredential; - } - - private static boolean isFirstArgumentAStringLiteral(CallExpression callExpression) { - return callExpression.arguments().get(0).is(Kind.REGULAR_ARGUMENT) - && ((RegularArgument) callExpression.arguments().get(0)).expression().is(Kind.STRING_LITERAL); - } - - private String matchedCredentialFromQualifiedExpression(QualifiedExpression qualifiedExpression) { - String matchedCredential = null; - if (qualifiedExpression.qualifier().is(Kind.NAME)) { - matchedCredential = matchedCredential(((Name) qualifiedExpression.qualifier()).name(), variablePatterns()); - } - return matchedCredential; - } - private static void checkSensitiveArgument(CallExpression callExpression, int argNb, SubscriptionContext ctx) { for (int i = 0; i < callExpression.arguments().size(); i++) { Argument argument = callExpression.arguments().get(i); @@ -225,7 +176,7 @@ private static void checkSensitiveArgument(CallExpression callExpression, int ar RegularArgument regularArgument = (RegularArgument) argument; if (regularArgument.keywordArgument() != null) { return; - } else if (i == argNb && regularArgument.expression().is(Kind.STRING_LITERAL)) { + } else if (i == argNb && regularArgument.expression().is(Kind.STRING_LITERAL) && !((StringLiteral) regularArgument.expression()).trimmedQuotesValue().isEmpty()) { ctx.addIssue(regularArgument, "Review this potentially hard-coded credential."); } } @@ -261,19 +212,6 @@ private static boolean isURLWithCredentials(StringLiteral stringLiteral) { return false; } - private void handleBinaryExpression(BinaryExpression binaryExpression, SubscriptionContext ctx) { - String matchedCredential = null; - if (binaryExpression.leftOperand() instanceof HasSymbol && binaryExpression.rightOperand().is(Tree.Kind.STRING_LITERAL)) { - matchedCredential = credentialSymbolName(((HasSymbol) binaryExpression.leftOperand()).symbol()); - } - if (binaryExpression.rightOperand() instanceof HasSymbol && binaryExpression.leftOperand().is(Tree.Kind.STRING_LITERAL)) { - matchedCredential = credentialSymbolName(((HasSymbol) binaryExpression.rightOperand()).symbol()); - } - if (matchedCredential != null) { - ctx.addIssue(binaryExpression, String.format(MESSAGE, matchedCredential)); - } - } - private void handleAssignmentStatement(AssignmentStatement assignmentStatement, SubscriptionContext ctx) { ExpressionList lhs = assignmentStatement.lhsExpressions().get(0); Expression expression = lhs.expressions().get(0); diff --git a/python-checks/src/test/resources/checks/hardCodedCredentials.py b/python-checks/src/test/resources/checks/hardCodedCredentials.py index fdefa3e5d1..ec64e012c9 100644 --- a/python-checks/src/test/resources/checks/hardCodedCredentials.py +++ b/python-checks/src/test/resources/checks/hardCodedCredentials.py @@ -49,6 +49,8 @@ def a(self,pwd="azerty123", other=None): # Noncompliant {{"pwd" detected here, var1 = "&password=:password" # OK var1 = "&password=:param" # OK var1 = "&password='"+pwd+"'" # OK + var1 = f"&password={pwd}" # OK + var1 = "&password={something}" # OK url = "http://user:azerty123@domain.com" # Noncompliant {{Review this hard-coded URL, which may contain a credential.}} url = "https://user:azerty123@domain.com" # Noncompliant {{Review this hard-coded URL, which may contain a credential.}} @@ -57,6 +59,8 @@ def a(self,pwd="azerty123", other=None): # Noncompliant {{"pwd" detected here, url = "http://user@domain.com:80" # OK url = "http://user@domain.com" # OK url = "http://domain.com/user:azerty123" # OK + url = "ssh://domain.com/user:azerty123" # OK + url = "unknown://domain.com/user:azerty123" # OK username = 'admin' password = pwd @@ -85,49 +89,25 @@ def a(self,pwd="azerty123", other=None): # Noncompliant {{"pwd" detected here, CONNECTION_PASSWORD = "connection.password" # OK RESETPWD = "/users/resetUserPassword" # OK - if password == 'Password123': # Noncompliant - pass - elif 'Password123' == password: # Noncompliant - pass - elif password.__eq__('Password123'): # Noncompliant {{"password" detected here, review this potentially hard-coded credential.}} - pass - elif something.__eq__('Password123'): # OK - pass - elif password.__eq__(something): # OK - pass - elif password.__eq__(*unpack): # OK - pass - elif 'Password123'.__eq__(password): # Noncompliant {{"password" detected here, review this potentially hard-coded credential.}} - pass - elif 'Password123'.__eq__(something): # OK - pass - elif 'Password123'.__eq__(*unpack): # OK - pass - elif 'Password123'.__eq__("something"): # OK - pass - if password == None: # OK (FN?) - pass - if something == "something": # OK - pass - if password == '': # Noncompliant - pass - if password == "": # Noncompliant - pass - if password == pwd: # OK (FN?) + # To avoid FPs, no issues raised on equality tests + if password == 'Azerty123': # OK pass - if something.password == "Azerty123": # Noncompliant + elif password.__eq__('Azerty123'): # OK pass - if foo.bar == "Azerty123": # OK + elif 'Azerty123'.__eq__(password): # OK pass hash_map = { 'password': "azerty123"} # Noncompliant {{"password" detected here, review this potentially hard-coded credential.}} hash_map = { ("a", "b") : "c"} # OK hash_map = { something : "c"} # OK hash_map = {'admin_form' : adminForm, **self.admin.context(request),} # OK + hash_map = { 'password': pwd} # OK + hash_map = { 'password': "password"} # OK hash_map['db_password'] = "azerty123" # Noncompliant hash_map['db_password'] = pwd # OK hash_map['something'] = "azerty123" # OK hash_map[something] = "something" # OK + hash_map['password'] = 'password' # OK encoded_user = 'gUhd9TxpnQppnZVAf7cv9pa5sgRo2sFmShrr/NK9dz0=' encoded_password = 'gUhd9TxpnQppnZVAf7cv9uVnoE28Vq0bR2Cx6Ku1UQA=' # Noncompliant @@ -139,6 +119,8 @@ def db(self, pwd): mysql.connector.connection.MySQLConnection(host='localhost', user='root', password='password') # OK (avoid FPs) mysql.connector.connect(host='localhost', user='root', password=pwd) # OK mysql.connector.connection.MySQLConnection(host='localhost', user='root', password=pwd) # OK + mysql.connector.connection.MySQLConnection(host='localhost', user='root', password='') # OK + mysql.connector.connection.MySQLConnection(host='localhost', user='root', "") # OK pymysql.connect(host='localhost', user='root', password='Azerty123') # Noncompliant pymysql.connect('localhost', 'root', 'password') # Noncompliant {{Review this potentially hard-coded credential.}} @@ -149,6 +131,10 @@ def db(self, pwd): pymysql.connect('localhost', 'root', pwd) # OK pymysql.connections.Connection(host='localhost', user='root', password=pwd) # OK pymysql.connections.Connection('localhost', 'root', pwd) # OK + pymysql.connect('localhost', 'root', '') # Compliant + pymysql.connect(host='localhost', user='root', password='') # Compliant + pymysql.connections.Connection(host='localhost', user='root', password='') # Compliant + pymysql.connections.Connection('localhost', 'root', '') # Compliant psycopg2.connect(host='localhost', user='postgres', password='Azerty123') # Noncompliant psycopg2.connect(host='localhost', user='postgres', password=pwd,) # OK @@ -167,6 +153,8 @@ def db(self, pwd): pg.connect(None, 'localhost', 5432, None, 'postgres', 'password') # Noncompliant pg.connect(host='localhost', user='postgres', passwd=pwd) # OK pg.connect(None, 'localhost', 5432, None, 'postgres', pwd) # OK + pg.connect(host='localhost', user='postgres', passwd='') # Compliant + pg.connect(None, 'localhost', 5432, None, 'postgres', '') # Compliant random.call(None, password = 42) # OK random.call(None, password = "hello") # Noncompliant From 0d026ba0b45260493d14f20b44e25ff75027c991 Mon Sep 17 00:00:00 2001 From: Guillaume Dequenne Date: Mon, 10 Feb 2020 10:49:33 +0100 Subject: [PATCH 2/2] Rule S2068: Avoid raising issues on docstrings --- .../hotspots/HardCodedCredentialsCheck.java | 16 ++++++++++++++++ .../resources/checks/hardCodedCredentials.py | 14 +++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/python-checks/src/main/java/org/sonar/python/checks/hotspots/HardCodedCredentialsCheck.java b/python-checks/src/main/java/org/sonar/python/checks/hotspots/HardCodedCredentialsCheck.java index d94081c818..a91e505323 100644 --- a/python-checks/src/main/java/org/sonar/python/checks/hotspots/HardCodedCredentialsCheck.java +++ b/python-checks/src/main/java/org/sonar/python/checks/hotspots/HardCodedCredentialsCheck.java @@ -38,10 +38,13 @@ import org.sonar.plugins.python.api.tree.Argument; import org.sonar.plugins.python.api.tree.AssignmentStatement; import org.sonar.plugins.python.api.tree.CallExpression; +import org.sonar.plugins.python.api.tree.ClassDef; import org.sonar.plugins.python.api.tree.DictionaryLiteral; import org.sonar.plugins.python.api.tree.DictionaryLiteralElement; import org.sonar.plugins.python.api.tree.Expression; import org.sonar.plugins.python.api.tree.ExpressionList; +import org.sonar.plugins.python.api.tree.FileInput; +import org.sonar.plugins.python.api.tree.FunctionDef; import org.sonar.plugins.python.api.tree.HasSymbol; import org.sonar.plugins.python.api.tree.KeyValuePair; import org.sonar.plugins.python.api.tree.Name; @@ -53,6 +56,7 @@ import org.sonar.plugins.python.api.tree.SubscriptionExpression; import org.sonar.plugins.python.api.tree.Tree; import org.sonar.plugins.python.api.tree.Tree.Kind; +import org.sonar.python.tree.TreeUtils; @Rule(key = "S2068") public class HardCodedCredentialsCheck extends PythonSubscriptionCheck { @@ -184,6 +188,9 @@ private static void checkSensitiveArgument(CallExpression callExpression, int ar } private void handleStringLiteral(StringLiteral stringLiteral, SubscriptionContext ctx) { + if (isDocString(stringLiteral)) { + return; + } if (stringLiteral.stringElements().stream().anyMatch(StringElement::isInterpolated)) { return; } @@ -196,6 +203,15 @@ private void handleStringLiteral(StringLiteral stringLiteral, SubscriptionContex } } + private static boolean isDocString(StringLiteral stringLiteral) { + Tree parent = TreeUtils.firstAncestorOfKind(stringLiteral, Kind.FILE_INPUT, Kind.CLASSDEF, Kind.FUNCDEF); + return Optional.ofNullable(parent) + .map(p -> ((p.is(Kind.FILE_INPUT) && stringLiteral.equals(((FileInput) p).docstring())) + || (p.is(Kind.CLASSDEF) && stringLiteral.equals(((ClassDef) p).docstring())) + || (p.is(Kind.FUNCDEF) && stringLiteral.equals(((FunctionDef) p).docstring())))) + .orElse(false); + } + private static boolean isURLWithCredentials(StringLiteral stringLiteral) { if (!stringLiteral.trimmedQuotesValue().contains("://")) { return false; diff --git a/python-checks/src/test/resources/checks/hardCodedCredentials.py b/python-checks/src/test/resources/checks/hardCodedCredentials.py index ec64e012c9..4537364db1 100644 --- a/python-checks/src/test/resources/checks/hardCodedCredentials.py +++ b/python-checks/src/test/resources/checks/hardCodedCredentials.py @@ -1,3 +1,8 @@ +""" +some docstring +password=hello +no issue +""" from Crypto.Cipher import AES import base64 import os @@ -17,7 +22,10 @@ def getDecrypted(encodedtext): return cipher.decrypt(base64.b64decode(encodedtext)) class A: - + """ + password=azerty123 + OK + """ passed = "passed" password = "azerty123" # Noncompliant password = "azerty123" # Noncompliant @@ -27,6 +35,10 @@ class A: (a, b) = ("some", "thing") def __init__(self): + """ + password=azerty123 + OK + """ self.passed = "passed" fieldNameWithPasswordInIt = "azerty123" # Noncompliant {{"password" detected here, review this potentially hard-coded credential.}} fieldNameWithPasswordInIt = os.getenv("password", "") # OK