remove sources which are contained from environment variables, fix some bugs thanks to @yoff

Author: am0o0

python/ql/src/experimental/Security/CWE-287-ConstantSecretKey/WebAppConstantSecretKey.ql

@@ -20,6 +20,26 @@ import semmle.python.dataflow.new.TaintTracking
 import WebAppConstantSecretKeyDjango
 import WebAppConstantSecretKeyFlask
 
+private predicate stringConstCompare(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
+  exists(CompareNode cn | cn = g |
+    exists(StrConst str_const, Cmpop op |
+      op = any(Eq eq) and branch = false
+      or
+      op = any(NotEq ne) and branch = true
+    |
+      cn.operands(str_const.getAFlowNode(), op, node)
+      or
+      cn.operands(node, op, str_const.getAFlowNode())
+    )
+  )
+}
+
+class StringConstCompareBarrier extends DataFlow::Node {
+  StringConstCompareBarrier() {
+    this = DataFlow::BarrierGuard<stringConstCompare/3>::getABarrierNode()
+  }
+}
+
 newtype TFrameWork =
   Flask() or
   Django()
@@ -39,7 +59,10 @@ module WebAppConstantSecretKeyConfig implements DataFlow::StateConfigSig {
     state = Django() and DjangoConstantSecretKeyConfig::isSink(sink)
   }
 
-  predicate isBarrier(DataFlow::Node node, FlowState state) { none() }
+  predicate isBarrier(DataFlow::Node sanitizer, FlowState state) {
+    (state = Flask() or state = Django()) and
+    sanitizer instanceof StringConstCompareBarrier
+  }
 
   predicate isAdditionalFlowStep(
     DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2

python/ql/src/experimental/Security/CWE-287-ConstantSecretKey/WebAppConstantSecretKeyDjango.qll

@@ -41,8 +41,7 @@ module DjangoConstantSecretKeyConfig {
               .asSink()
       )
       or
-      exists(API::Node n, DataFlow::AttrWrite attr |
-        attr.getObject().getALocalSource() = n.asSource() and
+      exists(DataFlow::AttrWrite attr |
         attr.getAttributeName() = ["SECRET_KEY_FALLBACKS", "SECRET_KEY"] and
         sink = attr.getValue()
       )

python/ql/src/experimental/Security/CWE-287-ConstantSecretKey/WebAppConstantSecretKeyFlask.qll

@@ -57,8 +57,8 @@ module FlaskConstantSecretKeyConfig {
           ]
       )
       or
-      exists(API::Node n, DataFlow::AttrWrite attr | n = flaskInstance() |
-        attr.getObject().getALocalSource() = n.getACall() and
+      exists(DataFlow::AttrWrite attr |
+        attr.getObject().getALocalSource() = flaskInstance().getACall() and
         attr.getAttributeName() = ["secret_key", "jwt_secret_key"] and
         sink = attr.getValue()
       )

python/ql/src/experimental/Security/CWE-287-ConstantSecretKey/WebAppConstantSecretKeySource.qll

@@ -6,55 +6,29 @@ import semmle.python.ApiGraphs
 class WebAppConstantSecretKeySource extends DataFlow::Node {
   WebAppConstantSecretKeySource() {
     (
-      // because Env return an Exeption if there isan't any value (instead of none) we should check whether
-      // there is a default value of there is a config file which mostly these config files have a default value
-      exists(API::Node env | env = API::moduleImport("environ").getMember("Env") |
-        (
-          // has default value
-          exists(env.getKeywordParameter("SECRET_KEY").asSource())
-          or
-          // get value from a config file which is not best security practice
-          exists(env.getReturn().getMember("read_env"))
+      // because Env return an Exeption if there isan't any value
+      // we should check whether there is a default value or not
+      exists(API::Node env |
+        env = API::moduleImport("environ").getMember("Env") and
+        // has default value
+        exists(API::Node param | param = env.getKeywordParameter("SECRET_KEY") |
+          param.asSink().asExpr().getASubExpression*().isConstant()
         ) and
         this = env.getReturn().getReturn().asSource()
       )
       or
       this.asExpr().isConstant()
       or
-      exists(API::Node cn |
-        cn =
-          [
-            API::moduleImport("configparser")
-                .getMember(["ConfigParser", "RawConfigParser"])
-                .getReturn(),
-            // legacy API https://docs.python.org/3/library/configparser.html#legacy-api-examples
-            API::moduleImport("configparser")
-                .getMember(["ConfigParser", "RawConfigParser"])
-                .getReturn()
-                .getMember("get")
-                .getReturn()
-          ] and
-        this = cn.asSource()
-      )
-      or
       exists(API::CallNode cn |
         cn =
           [
             API::moduleImport("os").getMember("getenv").getACall(),
             API::moduleImport("os").getMember("environ").getMember("get").getACall()
           ] and
-        (
-          // this can be ideal if we assume that best security practice is that
-          // we don't get SECRET_KEY from env and we always assign a secure generated random string to it
-          cn.getNumArgument() = 1
-          or
-          cn.getNumArgument() = 2 and
-          DataFlow::localFlow(any(DataFlow::Node n | n.asExpr().isConstant()), cn.getArg(1))
-        ) and
+        cn.getNumArgument() = 2 and
+        DataFlow::localFlow(any(DataFlow::Node n | n.asExpr().isConstant()), cn.getArg(1)) and
         this.asExpr() = cn.asExpr()
       )
-      or
-      this = API::moduleImport("os").getMember("environ").getASubscript().asSource()
     ) and
     // followings will sanitize the get_random_secret_key of django.core.management.utils and similar random generators which we have their source code and some of them can be tracking by taint tracking because they are initilized by a constant!
     exists(this.getScope().getLocation().getFile().getRelativePath()) and

python/ql/src/experimental/Security/CWE-287-ConstantSecretKey/examples/config.py

@@ -1,23 +1,14 @@
 """Flask App configuration."""
-from os import environ
 import os
 import random
-import configparser
 
 FLASK_DEBUG = True
 aConstant = 'CHANGEME2'
-config = configparser.ConfigParser()
 
 
 class Config:
-    SECRET_KEY = config["a"]["Secret"]
-    SECRET_KEY = config.get("key", "value")
-    SECRET_KEY = environ.get("envKey")
     SECRET_KEY = aConstant
-    SECRET_KEY = os.getenv('envKey')
-    SECRET_KEY = os.environ.get('envKey')
     SECRET_KEY = os.environ.get('envKey', random.randint)
     SECRET_KEY = os.getenv('envKey', random.randint)
     SECRET_KEY = os.getenv('envKey', aConstant)
     SECRET_KEY = os.environ.get('envKey', aConstant)
-    SECRET_KEY = os.environ['envKey']

python/ql/src/experimental/Security/CWE-287-ConstantSecretKey/examples/config2.py

@@ -1,5 +1,4 @@
 """Flask App configuration."""
-import environ
 
 aConstant = 'CHANGEME2'
 

python/ql/src/experimental/Security/CWE-287-ConstantSecretKey/examples/config3.py

@@ -3,5 +3,5 @@
 # General Config
 FLASK_DEBUG = True
 SECRET_KEY = "CHANGEME5"
-if SECRET_KEY == "CHANGEaME5":
+if SECRET_KEY == "CHANGEME5":
     raise "not possible"

python/ql/src/experimental/Security/CWE-287-ConstantSecretKey/examples/example_Django_unsafe.py

@@ -5,12 +5,11 @@
 from django.urls import path
 from django.http import HttpResponse
 
-
 env = environ.Env(
     SECRET_KEY=(str, "AConstantKey")
 )
 env.read_env(env_file='.env')
-# following is not safe if there is a call to read_env or if there is default value in Env(..)
+# following is not safe if there is default value in Env(..)
 settings.SECRET_KEY = env('SECRET_KEY')
 
 settings.configure(

python/ql/src/experimental/Security/CWE-287-ConstantSecretKey/examples/example_Flask_safe.py

@@ -10,7 +10,7 @@
 def DEB_EX():
     if 'logged_in' not in session:
         session['logged_in'] = 'value'
-    # debugging whether secret_key is secure or not
+    # debuggin whether secret_key is secure or not
     return app.secret_key