SONARPY-831 Rule S3752: Allowing both safe and unsafe HTTP methods is security-sensitive (Django)

Author: andreaguarino

its/ruling/src/test/resources/expected/python-S3752.json

@@ -0,0 +1,11 @@
+{
+'project:django-2.2.3/django/contrib/admin/sites.py':[
+368,
+],
+'project:django-2.2.3/django/contrib/flatpages/views.py':[
+22,
+],
+'project:django-2.2.3/django/views/i18n.py':[
+23,
+],
+}

python-checks/src/main/java/org/sonar/python/checks/CheckList.java

@@ -47,6 +47,7 @@
 import org.sonar.python.checks.hotspots.SecureCookieCheck;
 import org.sonar.python.checks.hotspots.StandardInputCheck;
 import org.sonar.python.checks.hotspots.StrongCryptographicKeysCheck;
+import org.sonar.python.checks.hotspots.UnsafeHttpMethodsCheck;
 import org.sonar.python.checks.hotspots.UnverifiedHostnameCheck;
 
 public final class CheckList {
@@ -211,6 +212,7 @@ public static Iterable<Class> getChecks() {
       UnreadPrivateAttributesCheck.class,
       UnreadPrivateInnerClassesCheck.class,
       UnreadPrivateMethodsCheck.class,
+      UnsafeHttpMethodsCheck.class,
       UndefinedSymbolsCheck.class,
       UnusedLocalVariableCheck.class,
       UnusedNestedDefinitionCheck.class,

python-checks/src/main/java/org/sonar/python/checks/hotspots/UnsafeHttpMethodsCheck.java

@@ -0,0 +1,124 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2021 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.hotspots;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.PythonSubscriptionCheck;
+import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.symbols.FunctionSymbol;
+import org.sonar.plugins.python.api.symbols.Symbol;
+import org.sonar.plugins.python.api.tree.Argument;
+import org.sonar.plugins.python.api.tree.CallExpression;
+import org.sonar.plugins.python.api.tree.Decorator;
+import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.FunctionDef;
+import org.sonar.plugins.python.api.tree.ListLiteral;
+import org.sonar.plugins.python.api.tree.RegularArgument;
+import org.sonar.plugins.python.api.tree.StringLiteral;
+import org.sonar.python.semantic.FunctionSymbolImpl;
+import org.sonar.python.tree.FunctionDefImpl;
+
+import static org.sonar.plugins.python.api.tree.Tree.Kind.CALL_EXPR;
+import static org.sonar.plugins.python.api.tree.Tree.Kind.FUNCDEF;
+import static org.sonar.plugins.python.api.tree.Tree.Kind.LIST_LITERAL;
+import static org.sonar.plugins.python.api.tree.Tree.Kind.REGULAR_ARGUMENT;
+import static org.sonar.plugins.python.api.tree.Tree.Kind.STRING_LITERAL;
+import static org.sonar.python.tree.TreeUtils.getSymbolFromTree;
+
+@Rule(key = "S3752")
+public class UnsafeHttpMethodsCheck extends PythonSubscriptionCheck {
+
+  private static final Set<String> SAFE_HTTP_METHODS = new HashSet<>(Arrays.asList("GET", "HEAD", "OPTIONS"));
+  private static final Set<String> UNSAFE_HTTP_METHODS = new HashSet<>(Arrays.asList("POST", "PUT", "DELETE"));
+  private static final Set<String> COMPLIANT_DECORATORS = new HashSet<>(Arrays.asList(
+    "django.views.decorators.http.require_POST",
+    "django.views.decorators.http.require_GET",
+    "django.views.decorators.http.require_safe"
+  ));
+  private static final String MESSAGE = "Make sure allowing safe and unsafe HTTP methods is safe here.";
+
+  @Override
+  public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(FUNCDEF, ctx -> {
+      FunctionDef functionDef = (FunctionDef) ctx.syntaxNode();
+      if (isDjangoView(functionDef)) {
+        checkDjangoView(functionDef, ctx);
+      }
+    });
+  }
+
+  private static void checkDjangoView(FunctionDef functionDef, SubscriptionContext ctx) {
+    for (Decorator decorator : functionDef.decorators()) {
+      if (getSymbolFromTree(decorator.expression())
+        .filter(symbol -> symbol.fullyQualifiedName() == null || COMPLIANT_DECORATORS.contains(symbol.fullyQualifiedName()))
+        .isPresent()) {
+        return;
+      }
+      if (decorator.expression().is(CALL_EXPR)) {
+        CallExpression callExpression = (CallExpression) decorator.expression();
+        Symbol symbol = callExpression.calleeSymbol();
+        if (symbol != null && "django.views.decorators.http.require_http_methods".equals(symbol.fullyQualifiedName())) {
+          checkRequireHttpMethodsDecorator(ctx, callExpression);
+          return;
+        }
+      }
+    }
+    ctx.addIssue(functionDef.name(), MESSAGE);
+  }
+
+  private static void checkRequireHttpMethodsDecorator(SubscriptionContext ctx, CallExpression callExpression) {
+    List<Argument> arguments = callExpression.arguments();
+    if (!arguments.isEmpty() && hasBothUnsafeAndSafeHttpMethods(arguments.get(0))) {
+      ctx.addIssue(callExpression, MESSAGE);
+    }
+  }
+
+  private static boolean hasBothUnsafeAndSafeHttpMethods(Argument argument) {
+    boolean hasSafeHttpMethod = false;
+    boolean hasUnsafeHttpMethod = false;
+    if (argument.is(REGULAR_ARGUMENT) && ((RegularArgument) argument).expression().is(LIST_LITERAL)) {
+      ListLiteral listLiteral = (ListLiteral) ((RegularArgument) argument).expression();
+      for (Expression expression : listLiteral.elements().expressions()) {
+        if (expression.is(STRING_LITERAL)) {
+          String value = ((StringLiteral) expression).trimmedQuotesValue();
+          if (SAFE_HTTP_METHODS.contains(value)) {
+            hasSafeHttpMethod = true;
+          } else if (UNSAFE_HTTP_METHODS.contains(value)) {
+            hasUnsafeHttpMethod = true;
+          }
+        }
+      }
+    }
+    return hasSafeHttpMethod && hasUnsafeHttpMethod;
+  }
+
+  private static boolean isDjangoView(FunctionDef functionDef) {
+    FunctionSymbol functionSymbol = ((FunctionDefImpl) functionDef).functionSymbol();
+    return Optional.ofNullable(functionSymbol)
+      .map(FunctionSymbolImpl.class::cast)
+      .filter(FunctionSymbolImpl::isDjangoView)
+      .isPresent();
+  }
+}

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

@@ -0,0 +1,77 @@
+<p>An HTTP method is safe when used to perform a read-only operation, such as retrieving information. In contrast, an unsafe HTTP method is used to
+change the state of an application, for instance to update a user's profile on a web application.</p>
+<p>Common safe HTTP methods are GET, HEAD, or OPTIONS.</p>
+<p>Common unsafe HTTP methods are POST, PUT and DELETE.</p>
+<p>Allowing both safe and unsafe HTTP methods to perform a specific operation on a web application could impact its security, for example CSRF
+protections are most of the time only protecting operations performed by unsafe HTTP methods.</p>
+<h2>Ask Yourself Whether</h2>
+<ul>
+  <li> HTTP methods are not defined at all for a route/controller of the application. </li>
+  <li> Safe HTTP methods are defined and used for a route/controller that can change the state of an application. </li>
+</ul>
+<p>There is a risk if you answered yes to any of those questions.</p>
+<h2>Recommended Secure Coding Practices</h2>
+<p>For all the routes/controllers of an application, the authorized HTTP methods should be explicitly defined and safe HTTP methods should only be
+used to perform read-only operations.</p>
+<h2>Sensitive Code Example</h2>
+<p>For <a href="https://www.djangoproject.com/">Django</a>:</p>
+<pre>
+# No method restriction
+def view(request):  # Sensitive
+    return HttpResponse("...")
+</pre>
+<pre>
+@require_http_methods(["GET", "POST"])  # Sensitive
+def view(request):
+    return HttpResponse("...")
+</pre>
+<p>For <a href="https://flask.palletsprojects.com/en/1.1.x/">Flask</a>:</p>
+<pre>
+@methods.route('/sensitive', methods=['GET', 'POST'])  # Sensitive
+def view():
+    return Response("...", 200)
+</pre>
+<h2>Compliant Solution</h2>
+<p>For <a href="https://www.djangoproject.com/">Django</a>:</p>
+<pre>
+@require_http_methods(["POST"])
+def view(request):
+    return HttpResponse("...")
+</pre>
+<pre>
+@require_POST
+def view(request):
+    return HttpResponse("...")
+</pre>
+<pre>
+@require_GET
+def view(request):
+    return HttpResponse("...")
+</pre>
+<pre>
+@require_safe
+def view(request):
+    return HttpResponse("...")
+</pre>
+<p>For <a href="https://flask.palletsprojects.com/en/1.1.x/">Flask</a>:</p>
+<pre>
+@methods.route('/compliant1')
+def view():
+    return Response("...", 200)
+</pre>
+<pre>
+@methods.route('/compliant2', methods=['GET'])
+def view():
+    return Response("...", 200)
+</pre>
+<h2>See</h2>
+<ul>
+  <li> <a href="https://owasp.org/www-project-top-ten/OWASP_Top_Ten_2017/Top_10-2017_A5-Broken_Access_Control">OWASP Top 10 2017 Category A5</a> -
+  Broken Access Control </li>
+  <li> <a href="https://cwe.mitre.org/data/definitions/352.html">MITRE, CWE-352</a> - Cross-Site Request Forgery (CSRF) </li>
+  <li> <a href="https://www.owasp.org/index.php/Cross-Site_Request_Forgery_%28CSRF%29">OWASP: Cross-Site Request Forgery</a> </li>
+  <li> <a href="https://www.sans.org/top25-software-errors/#cat1">SANS Top 25</a> - Insecure Interaction Between Components </li>
+  <li> <a href="https://docs.djangoproject.com/en/3.1/topics/http/decorators/#allowed-http-methods">Django</a> - Allowed HTTP Methods </li>
+  <li> <a href="https://flask.palletsprojects.com/en/1.1.x/quickstart/#http-methods">Flask</a> - HTTP Methods </li>
+</ul>
+

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S3752.json

@@ -0,0 +1,26 @@
+{
+  "title": "Allowing both safe and unsafe HTTP methods is security-sensitive",
+  "type": "SECURITY_HOTSPOT",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "cwe",
+    "sans-top25-insecure",
+    "owasp-a5"
+  ],
+  "defaultSeverity": "Minor",
+  "ruleSpecification": "RSPEC-3752",
+  "sqKey": "S3752",
+  "scope": "Main",
+  "securityStandards": {
+    "CWE": [
+      352
+    ],
+    "OWASP": [
+      "A5"
+    ]
+  }
+}

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/Sonar_way_profile.json

@@ -69,6 +69,7 @@
     "S3457",
     "S3516",
     "S3626",
+    "S3752",
     "S3776",
     "S3827",
     "S3862",

python-checks/src/test/java/org/sonar/python/checks/hotspots/UnsafeHttpMethodsCheckTest.java

@@ -0,0 +1,37 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) 2011-2021 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.hotspots;
+
+import java.util.Arrays;
+import org.junit.Test;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
+
+public class UnsafeHttpMethodsCheckTest {
+
+  @Test
+  public void test_django() {
+    PythonCheckVerifier.verify(Arrays.asList(
+      "src/test/resources/checks/hotspots/unsafeHttpMethods/django/urls.py",
+      "src/test/resources/checks/hotspots/unsafeHttpMethods/django/views.py",
+      "src/test/resources/checks/hotspots/unsafeHttpMethods/django/views_decorator_fqn_null.py"
+      ),
+      new UnsafeHttpMethodsCheck());
+  }
+}

python-checks/src/test/resources/checks/hotspots/unsafeHttpMethods/django/__init__.py

@@ -0,0 +1,20 @@
+from django.urls import path
+
+from . import views
+from . import views_decorator_fqn_null
+
+urlpatterns = [
+  path('sensitive1', views.sensitive1, name='sensitive1'),
+  path('sensitive2', views.sensitive2, name='sensitive2'),
+  path('compliant_require_http_methods', views.compliant_require_http_methods, name='compliant_require_http_methods'),
+  path('compliant_require_http_methods2', views.compliant_require_http_methods2, name='compliant_require_http_methods2'),
+  path('compliant_require_http_methods3', views.compliant_require_http_methods3, name='compliant_require_http_methods3'),
+  path('compliant_require_http_methods4', views.compliant_require_http_methods4, name='compliant_require_http_methods4'),
+  path('compliant_require_http_methods5', views.compliant_require_http_methods5, name='compliant_require_http_methods5'),
+  path('compliant_require_http_methods6', views.compliant_require_http_methods6, name='compliant_require_http_methods6'),
+  path('compliant', views_decorator_fqn_null.compliant, name='compliant'),
+  path('compliant_require_post', views.compliant_require_post, name='compliant_require_post'),
+  path('compliant_require_get', views.compliant_require_get, name='compliant_require_get'),
+  path('compliant_require_safe', views.compliant_require_safe, name='compliant_require_safe'),
+  path('sensitive_other_dec', views.sensitive_other_dec, name='sensitive_other_dec'),
+]

python-checks/src/test/resources/checks/hotspots/unsafeHttpMethods/django/urls.py

@@ -0,0 +1,51 @@
+from django.views.decorators.http import require_http_methods, require_POST, require_GET, require_safe, other_decorator
+
+def sensitive1(request):  # Noncompliant
+  ...
+
+@require_http_methods(["GET", "POST"])  # Noncompliant
+def sensitive2(request):
+  ...
+
+@require_http_methods(["POST"])  # Compliant
+def compliant_require_http_methods(request):
+  ...
+
+@require_http_methods(["GET"])  # Compliant
+def compliant_require_http_methods2(request):
+  ...
+@require_http_methods()  # Compliant
+def compliant_require_http_methods3(request):
+  ...
+
+methods = []
+@require_http_methods(*methods)  # Compliant
+def compliant_require_http_methods4(request):
+    ...
+
+@unknown(["GET", "POST"])
+def compliant_require_http_methods5(request): # Noncompliant
+    ...
+
+def foo(): ...
+@foo(["GET", "POST"])
+def compliant_require_http_methods6(request): # Noncompliant
+    ...
+
+@require_POST  # Compliant
+def compliant_require_post(request):
+  ...
+
+@require_GET  # Compliant
+def compliant_require_get(request):
+  ...
+
+@require_safe  # Compliant
+def compliant_require_safe(request):
+  ...
+@other_decorator
+def sensitive_other_dec(request): # Noncompliant
+  ...
+
+def other(request):
+  ...