Skip to content

SONARPY-358 RSPEC-4423 weak protocol version #279

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.

Sign up for GitHub

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

Merged
benzonico merged 1 commit into from
Aug 30, 2019
Merged

SONARPY-358 RSPEC-4423 weak protocol version #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions its/ruling/src/test/resources/expected/python-S4423.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
{
'project:buildbot-0.8.6p1/buildbot/status/mail.py':[
40,
749,
],
'project:docker-compose-1.24.1/tests/unit/cli/docker_client_test.py':[
174,
213,
],
'project:tornado-2.3/tornado/simple_httpclient.py':[
210,
],
'project:tornado-2.3/tornado/test/httpserver_test.py':[
110,
115,
120,
125,
],
'project:twisted-12.1.0/doc/core/examples/echoserv_ssl.py':[
15,
],
'project:twisted-12.1.0/doc/core/howto/tutorial/listings/finger/finger/finger.py':[
324,
],
'project:twisted-12.1.0/doc/core/howto/tutorial/listings/finger/finger22.py':[
315,
],
'project:twisted-12.1.0/doc/mail/examples/smtpclient_tls.py':[
9,
40,
],
'project:twisted-12.1.0/twisted/internet/_sslverify.py':[
600,
],
'project:twisted-12.1.0/twisted/internet/endpoints.py':[
1091,
],
'project:twisted-12.1.0/twisted/internet/ssl.py':[
54,
106,
],
'project:twisted-12.1.0/twisted/mail/imap4.py':[
2739,
],
'project:twisted-12.1.0/twisted/mail/pop3client.py':[
428,
],
'project:twisted-12.1.0/twisted/mail/protocols.py':[
222,
],
'project:twisted-12.1.0/twisted/mail/smtp.py':[
1802,
],
'project:twisted-12.1.0/twisted/mail/test/pop3testserver.py':[
211,
],
'project:twisted-12.1.0/twisted/protocols/test/test_tls.py':[
20,
83,
],
'project:twisted-12.1.0/twisted/protocols/tls.py':[
40,
43,
],
'project:twisted-12.1.0/twisted/test/ssl_helpers.py':[
14,
23,
],
'project:twisted-12.1.0/twisted/test/test_ssl.py':[
273,
665,
713,
],
'project:twisted-12.1.0/twisted/test/test_sslverify.py':[
264,
285,
],
}
1 change: 1 addition & 0 deletions python-checks/src/main/java/org/sonar/python/checks/CheckList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ public static Iterable<Class> getChecks() {
UnusedLocalVariableCheck.class,
UselessParenthesisAfterKeywordCheck.class,
UselessParenthesisCheck.class,
WeakSSLProtocolCheck.class,
XPathCheck.class
)));
}
Expand Down
68 changes: 68 additions & 0 deletions python-checks/src/main/java/org/sonar/python/checks/WeakSSLProtocolCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* SonarQube Python Plugin
* Copyright (C) 2011-2019 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonar.python.checks;

import java.util.Arrays;
import java.util.List;
import java.util.function.Predicate;
import javax.annotation.Nullable;
import org.sonar.check.Rule;
import org.sonar.python.PythonSubscriptionCheck;
import org.sonar.python.api.tree.PyNameTree;
import org.sonar.python.api.tree.Tree;
import org.sonar.python.semantic.Symbol;

@Rule(key = "S4423")
public class WeakSSLProtocolCheck extends PythonSubscriptionCheck {
private static final List<String> WEAK_PROTOCOL_CONSTANTS = Arrays.asList(
"ssl.PROTOCOL_SSLv2",
"ssl.PROTOCOL_SSLv3",
"ssl.PROTOCOL_SSLv23",
"ssl.PROTOCOL_TLS",
"ssl.PROTOCOL_TLSv1",
"ssl.PROTOCOL_TLSv1_1",
"OpenSSL.SSL.SSLv2_METHOD",
"OpenSSL.SSL.SSLv3_METHOD",
"OpenSSL.SSL.SSLv23_METHOD",
"OpenSSL.SSL.TLSv1_METHOD",
"OpenSSL.SSL.TLSv1_1_METHOD"
);

@Override
public void initialize(Context context) {
context.registerSyntaxNodeConsumer(Tree.Kind.NAME, ctx -> {
PyNameTree pyNameTree = (PyNameTree) ctx.syntaxNode();
Symbol symbol = ctx.symbolTable().getSymbol(pyNameTree);
if (isWeakProtocol(pyNameTree, symbol)) {
ctx.addIssue(pyNameTree, "Change this code to use a stronger protocol.");
}
});
}

private static boolean isWeakProtocol(PyNameTree pyNameTree, @Nullable Symbol symbol) {
Predicate<String> matchWeakProtocol;
if (symbol == null) {
matchWeakProtocol = s -> s.substring(s.lastIndexOf('.') + 1).equals(pyNameTree.name());
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why we need that. I think we have a problem because, in the unit test, this method returns true only when we go through this branch. The other branch, where we use the symbol, doesn't seem to raise issues. Maybe it's related to https://github.com/SonarSource/sonar-python/blob/master/python-squid/src/main/java/org/sonar/python/semantic/SymbolTableBuilderVisitor.java#L329

Maybe we should either:

  • create a ticket to fix the problem later
  • or fix the problem in the symbol table right away

Copy link
Copy Markdown
Contributor Author

@benzonico benzonico Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is deeper : this is completely linked to semantic: this is about solving dotted names : in order to get fully qualified name of the constant we need to figure out fqn of its parent and right now the SymbolTableVisitor does not deal with this kind of constructs (if i'm correct, it only deals with the first part of a dotted name).
As I did not want to address semantic in the implementation of this new rule and as I managed to make it work I believe this is ok.
Not sure which kind of ticket I should create for such case to be honest.

Copy link
Copy Markdown
Contributor

@andrea-guarino-sonarsource andrea-guarino-sonarsource Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me jump in the discussion. Just to clarify, currently the semantic has some limitations and inconsistencies regarding symbol resolution:

from foo import A
from foo.A import fn

A.fn() # Here "getSymbol()" will retrieve the correct symbol for "A.fn" PythonGrammar.ATTRIBUTE_REF (corresponding to PyQualifiedExpressionTree), but not for "fn" PythonGrammar.ATOM node

fn() # Here "getSymbol()" will retrieve the correct symbol for "fn" PythonGrammar.ATOM node

I agree we shouldn't fix the semantics right now, I can create a ticket for this.

Copy link
Copy Markdown
Contributor Author

@benzonico benzonico Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrea-guarino-sonarsource be my guest and please link it here !

} else {
matchWeakProtocol = symbol.qualifiedName()::equals;
}
return WEAK_PROTOCOL_CONSTANTS.stream().anyMatch(matchWeakProtocol);
}
}
44 changes: 44 additions & 0 deletions python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S4423.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<p>Older versions of SSL/TLS protocol like "SSLv3" have been proven to be insecure.</p>
<p>This rule raises an issue when an SSL/TLS context is created with an insecure protocol version, i.e. when one of the following constants is
detected in the code:</p>
<ul>
<li> <code>OpenSSL.SSL.SSLv3_METHOD</code> (Use instead <code>OpenSSL.SSL.TLSv1_2_METHOD</code>) </li>
<li> <code>ssl.PROTOCOL_SSLv3</code> (Use instead <code>ssl.PROTOCOL_TLSv1_2</code>) </li>
</ul>
<p>Protocol versions different from TLSv1.2 and TLSv1.3 are considered insecure.</p>
<h2>Noncompliant Code Example</h2>
<pre>
from OpenSSL import SSL

SSL.Context(SSL.SSLv3_METHOD) # Noncompliant
</pre>
<pre>
import ssl

ssl.SSLContext(ssl.PROTOCOL_SSLv3) # Noncompliant
</pre>
<h2>Compliant Solution</h2>
<pre>
from OpenSSL import SSL

SSL.Context(SSL.TLSv1_2_METHOD) # Compliant
</pre>
<pre>
import ssl

ssl.SSLContext(ssl.PROTOCOL_TLSv1_2) # Compliant
</pre>
<h2>See</h2>
<ul>
<li> <a href="https://www.owasp.org/index.php/Top_10-2017_A3-Sensitive_Data_Exposure">OWASP Top 10 2017 Category A3</a> - Sensitive Data Exposure
</li>
<li> <a href="https://www.owasp.org/index.php/Top_10-2017_A6-Security_Misconfiguration">OWASP Top 10 2017 Category A6</a> - Security
Misconfiguration </li>
<li> <a href="http://cwe.mitre.org/data/definitions/326.html">MITRE, CWE-327</a> - Inadequate Encryption Strength </li>
<li> <a href="http://cwe.mitre.org/data/definitions/327.html">MITRE, CWE-326</a> - Use of a Broken or Risky Cryptographic Algorithm </li>
<li> <a href="https://www.sans.org/top25-software-errors/#cat3">SANS Top 25</a> - Porous Defenses </li>
<li> <a href="https://blogs.oracle.com/java-platform-group/diagnosing-tls,-ssl,-and-https">Diagnosing TLS, SSL, and HTTPS</a> </li>
<li> <a href="https://github.com/ssllabs/research/wiki/SSL-and-TLS-Deployment-Best-Practices#22-use-secure-protocols">SSL and TLS Deployment Best
Practices - Use secure protocols</a> </li>
</ul>

34 changes: 34 additions & 0 deletions python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S4423.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{
"title": "Weak SSL\/TLS protocols should not be used",
"type": "VULNERABILITY",
Copy link
Copy Markdown
Contributor

@pynicolas pynicolas Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be a hotspot?

Copy link
Copy Markdown
Contributor Author

@benzonico benzonico Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a vulnerability as per PM specification, summoning @pierre-loup-tristant-sonarsource for confirmation.

Copy link
Copy Markdown

@pierre-loup-tristant-sonarsource pierre-loup-tristant-sonarsource Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a vulnerability for Python as it also is for Java and PHP.

"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "2min"
},
"tags": [
"cwe",
"owasp-a6",
"sans-top25-porous",
"owasp-a3"
],
"standards": [
"CWE",
"OWASP Top 10",
"SANS Top 25"
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-4423",
"sqKey": "S4423",
"scope": "Main",
"securityStandards": {
"CWE": [
327,
326
],
"OWASP": [
"A3",
"A6"
]
}
}
1 change: 1 addition & 0 deletions python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"S2734",
"S2772",
"S3776",
"S4423",
"S4507",
"S4721",
"S4784",
Expand Down
32 changes: 32 additions & 0 deletions python-checks/src/test/java/org/sonar/python/checks/WeakSSLProtocolCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* SonarQube Python Plugin
* Copyright (C) 2011-2019 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonar.python.checks;

import org.junit.Test;
import org.sonar.python.checks.utils.PythonCheckVerifier;

public class WeakSSLProtocolCheckTest {

@Test
public void test() {
PythonCheckVerifier.verify("src/test/resources/checks/weakSSLProtocol.py", new WeakSSLProtocolCheck());
}

}
56 changes: 56 additions & 0 deletions python-checks/src/test/resources/checks/weakSSLProtocol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
from OpenSSL import SSL
import ssl
from requests.adapters import HTTPAdapter
from requests.packages.urllib3.poolmanager import PoolManager

SSL.Context(SSL.SSLv2_METHOD) # Noncompliant {{Change this code to use a stronger protocol.}}
# ^^^^^^^^^^^^
# Keyword argument
SSL.Context(method=SSL.SSLv2_METHOD) # Noncompliant
SSL.Context(SSL.TLSv1_2_METHOD) # Compliant
SSL.Context(method=SSL.TLSv1_2_METHOD) # Compliant

# ssl.SSLContext()
ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv2) # Noncompliant
ctx = ssl.SSLContext(protocol=ssl.PROTOCOL_SSLv2) # Noncompliant

# ssl.wrap_socket()
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
ssl.wrap_socket(s, ssl_version=ssl.PROTOCOL_SSLv2) # Noncompliant
# ^^^^^^^^^^^^^^

# ssl.SSLContext()
ctx = ssl.SSLContext(ssl.PROTOCOL_TLSv1_2) # Compliant
ctx = ssl.SSLContext(protocol=ssl.PROTOCOL_TLSv1_2) # Compliant


# ssl.wrap_socket()
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
ssl.wrap_socket(s, ssl_version=ssl.PROTOCOL_TLSv1_2) # Compliant


class Ssl3Adapter(HTTPAdapter):
""""Transport adapter that forces SSLv3"""

def init_poolmanager(self, *pool_args, **pool_kwargs):

self.poolmanager = PoolManager(
*pool_args,
ssl_version=ssl.PROTOCOL_SSLv3, # Noncompliant
**pool_kwargs)

class Tls12Adapter(HTTPAdapter):
""""Transport adapter that forces TLSv1.2"""

def init_poolmanager(self, *pool_args, **pool_kwargs):
self.poolmanager = PoolManager(
*pool_args,
ssl_version=ssl.PROTOCOL_TLSv1_2,
**pool_kwargs)


class unrelated():
someClass.S = toto
PROTOCOL_SSLv2 = "someconstant" # Noncompliant FP due to lack of semantic
def met():
foo(PROTOCOL_SSLv2) # compliant, symbol does not match qualified name
2 changes: 1 addition & 1 deletion sonar-python-plugin/src/test/java/org/sonar/plugins/python/PythonRuleRepositoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void createRulesTest() {

List<RulesDefinition.Rule> rules = repository.rules();
assertThat(rules).isNotNull();
assertThat(rules).hasSize(64);
assertThat(rules).hasSize(65);

RulesDefinition.Rule s1578 = repository.rule("S1578");
assertThat(s1578).isNotNull();
Expand Down