|
| 1 | +/** |
| 2 | + * @name Initializing SECRET_KEY of Flask application with Constant value |
| 3 | + * @description Initializing SECRET_KEY of Flask application with Constant value |
| 4 | + * files can lead to Authentication bypass |
| 5 | + * @kind path-problem |
| 6 | + * @id py/ConstantSecretKey |
| 7 | + * @problem.severity error |
| 8 | + * @security-severity 8.5 |
| 9 | + * @precision high |
| 10 | + * @tags security |
| 11 | + * experimental |
| 12 | + * external/cwe/cwe-287 |
| 13 | + */ |
| 14 | + |
| 15 | +import python |
| 16 | +import experimental.semmle.python.Concepts |
| 17 | +import semmle.python.dataflow.new.DataFlow |
| 18 | +import semmle.python.ApiGraphs |
| 19 | +import semmle.python.dataflow.new.TaintTracking |
| 20 | + |
| 21 | +/** |
| 22 | + * `flask.Flask()` |
| 23 | + */ |
| 24 | +API::Node flaskInstance() { result = API::moduleImport("flask").getMember("Flask").getASubclass*() } |
| 25 | + |
| 26 | +/** |
| 27 | + * with using flask-session package, there is no jwt exists in cookies in user side |
| 28 | + * ```python |
| 29 | + *import os |
| 30 | + *from flask import Flask, session |
| 31 | + *app = Flask(__name__) |
| 32 | + * ``` |
| 33 | + */ |
| 34 | +module FlaskConstantSecretKeyConfig implements DataFlow::ConfigSig { |
| 35 | + /** |
| 36 | + * Sources are Constants that without any Tainting reach the Sinks. |
| 37 | + * Also Sources can be the default value of getenv or similar methods |
| 38 | + * in a case that no value is assigned to Desired SECRET_KEY environment variable |
| 39 | + */ |
| 40 | + predicate isSource(DataFlow::Node source) { |
| 41 | + ( |
| 42 | + source.asExpr().isConstant() |
| 43 | + or |
| 44 | + exists(API::Node cn | |
| 45 | + cn = |
| 46 | + [ |
| 47 | + API::moduleImport("configparser") |
| 48 | + .getMember(["ConfigParser", "RawConfigParser"]) |
| 49 | + .getReturn(), |
| 50 | + // legacy API https://docs.python.org/3/library/configparser.html#legacy-api-examples |
| 51 | + API::moduleImport("configparser") |
| 52 | + .getMember(["ConfigParser", "RawConfigParser"]) |
| 53 | + .getReturn() |
| 54 | + .getMember("get") |
| 55 | + .getReturn() |
| 56 | + ] and |
| 57 | + source = cn.asSource() |
| 58 | + ) |
| 59 | + or |
| 60 | + exists(API::CallNode cn | |
| 61 | + cn = |
| 62 | + [ |
| 63 | + API::moduleImport("os").getMember("getenv").getACall(), |
| 64 | + API::moduleImport("os").getMember("environ").getMember("get").getACall() |
| 65 | + ] and |
| 66 | + ( |
| 67 | + // this can be ideal if we assume that best security practice is that |
| 68 | + // we don't get SECRET_KEY from env and we always assign a secure generated random string to it |
| 69 | + cn.getNumArgument() = 1 |
| 70 | + or |
| 71 | + cn.getNumArgument() = 2 and |
| 72 | + DataFlow::localFlow(any(DataFlow::Node n | n.asExpr().isConstant()), cn.getArg(1)) |
| 73 | + ) and |
| 74 | + source.asExpr() = cn.asExpr() |
| 75 | + ) |
| 76 | + or |
| 77 | + exists(DataFlow::LocalSourceNode lsn | |
| 78 | + lsn = API::moduleImport("os").getMember("environ").getASubscript().asSource() and |
| 79 | + source.asExpr() = lsn.asExpr() |
| 80 | + ) |
| 81 | + ) and |
| 82 | + not source.getScope().getLocation().getFile().inStdlib() |
| 83 | + } |
| 84 | + |
| 85 | + /** |
| 86 | + * Sinks are one of the following kinds, some of them are directly connected to a flask Instance like |
| 87 | + * ```python |
| 88 | + * app.config['SECRET_KEY'] = 'CHANGEME1' |
| 89 | + * app.secret_key = 'CHANGEME2' |
| 90 | + * app.config.update(SECRET_KEY="CHANGEME3") |
| 91 | + * app.config.from_mapping(SECRET_KEY="CHANGEME4") |
| 92 | + * ``` |
| 93 | + * other Sinks are SECRET_KEY Constants Variables that are defined in seperate files or a class in those files like: |
| 94 | + * ```python |
| 95 | + * app.config.from_pyfile("config.py") |
| 96 | + * app.config.from_object('config.Config') |
| 97 | + *``` |
| 98 | + * we find these files with `FromObjectFileName` DataFlow Configuration |
| 99 | + * note that "JWT_SECRET_KEY" is same as "SECRET_KEY" but it is belong to popular flask-jwt-extended library |
| 100 | + */ |
| 101 | + predicate isSink(DataFlow::Node sink) { |
| 102 | + ( |
| 103 | + exists(API::Node n | |
| 104 | + n = flaskInstance() and |
| 105 | + flask_sessionSanitizer(n.getReturn().asSource()) |
| 106 | + | |
| 107 | + sink = |
| 108 | + [ |
| 109 | + n.getReturn().getAMember().getSubscript(["SECRET_KEY", "JWT_SECRET_KEY"]).asSink(), |
| 110 | + n.getReturn().getMember(["SECRET_KEY", "JWT_SECRET_KEY"]).asSink(), |
| 111 | + n.getReturn() |
| 112 | + .getMember("config") |
| 113 | + .getMember(["update", "from_mapping"]) |
| 114 | + .getACall() |
| 115 | + .getArgByName(["SECRET_KEY", "JWT_SECRET_KEY"]) |
| 116 | + ] |
| 117 | + ) |
| 118 | + or |
| 119 | + // this query checks for Django SecretKey too |
| 120 | + if exists(API::moduleImport("django")) |
| 121 | + then |
| 122 | + exists(AssignStmt e | e.getTarget(0).toString() = "SECRET_KEY" | |
| 123 | + sink.asExpr() = e.getValue() |
| 124 | + // and sanitizer(e.getTarget(0)) |
| 125 | + ) |
| 126 | + else |
| 127 | + exists(SecretKeyAssignStmt e | |
| 128 | + sink.asExpr() = e.getValue() |
| 129 | + // | sanitizer(e.getTarget(0)) |
| 130 | + ) |
| 131 | + ) and |
| 132 | + not sink.getScope().getLocation().getFile().inStdlib() |
| 133 | + } |
| 134 | +} |
| 135 | + |
| 136 | +// using flask_session library is safe |
| 137 | +predicate flask_sessionSanitizer(DataFlow::Node source) { |
| 138 | + not DataFlow::localFlow(source, |
| 139 | + API::moduleImport("flask_session").getMember("Session").getACall().getArg(0)) |
| 140 | +} |
| 141 | + |
| 142 | +// *it seems that sanitizer have a lot of performance issues* |
| 143 | +// for case check whether SECRECT_KEY is empty or not |
| 144 | +predicate sanitizer(Expr sourceExpr) { |
| 145 | + exists(DataFlow::Node source, DataFlow::Node sink, If i | |
| 146 | + source.asExpr() = sourceExpr and |
| 147 | + DataFlow::localFlow(source, sink) |
| 148 | + | |
| 149 | + not i.getASubExpression().getAChildNode*().(Compare) = sink.asExpr() and |
| 150 | + not sink.getScope().getLocation().getFile().inStdlib() and |
| 151 | + not source.getScope().getLocation().getFile().inStdlib() and |
| 152 | + not i.getScope().getLocation().getFile().inStdlib() |
| 153 | + ) |
| 154 | +} |
| 155 | + |
| 156 | +/** |
| 157 | + * Assignments like `SECRET_KEY = ConstantValue` |
| 158 | + * which ConstantValue will be found by another DataFlow Configuration |
| 159 | + * and `SECRET_KEY` location must be a argument of `from_object` or `from_pyfile` methods |
| 160 | + * the argument/location value will be found by another Taint Tracking Configuration. |
| 161 | + */ |
| 162 | +class SecretKeyAssignStmt extends AssignStmt { |
| 163 | + SecretKeyAssignStmt() { |
| 164 | + exists( |
| 165 | + string configFileName, string fileNamehelper, DataFlow::Node n1, FromObjectFileName config |
| 166 | + | |
| 167 | + config.hasFlow(n1, _) and |
| 168 | + n1.asExpr().isConstant() and |
| 169 | + fileNamehelper = n1.asExpr().(StrConst).getS() and |
| 170 | + // because of `from_object` we want first part of `Config.AClassName` which `Config` is a python file name |
| 171 | + configFileName = fileNamehelper.splitAt(".") and |
| 172 | + // after spliting, don't look at %py% pattern |
| 173 | + configFileName != "py" |
| 174 | + | |
| 175 | + this.getLocation().getFile().getShortName().matches("%" + configFileName + "%") and |
| 176 | + this.getTarget(0).toString() = ["SECRET_KEY", "JWT_SECRET_KEY"] |
| 177 | + ) and |
| 178 | + not this.getScope().getLocation().getFile().inStdlib() |
| 179 | + } |
| 180 | +} |
| 181 | + |
| 182 | +/** |
| 183 | + * we have some file name that telling us the SECRET_KEY location |
| 184 | + * which have determined by these two methods |
| 185 | + * `app.config.from_pyfile("configFileName.py")` or `app.config.from_object("configFileName.ClassName")` |
| 186 | + * this is a helper configuration that help us skip the SECRET_KEY variables that are not related to Flask. |
| 187 | + */ |
| 188 | +class FromObjectFileName extends TaintTracking::Configuration { |
| 189 | + FromObjectFileName() { this = "FromObjectFileName" } |
| 190 | + |
| 191 | + override predicate isSource(DataFlow::Node source) { |
| 192 | + source.asExpr().isConstant() and |
| 193 | + not source.getScope().getLocation().getFile().inStdlib() |
| 194 | + } |
| 195 | + |
| 196 | + override predicate isSink(DataFlow::Node sink) { |
| 197 | + exists(API::Node n | |
| 198 | + n = flaskInstance() and |
| 199 | + flask_sessionSanitizer(n.getReturn().asSource()) |
| 200 | + | |
| 201 | + sink = |
| 202 | + n.getReturn() |
| 203 | + .getMember("config") |
| 204 | + .getMember(["from_object", "from_pyfile"]) |
| 205 | + .getACall() |
| 206 | + .getArg(0) |
| 207 | + ) and |
| 208 | + not sink.getScope().getLocation().getFile().inStdlib() |
| 209 | + } |
| 210 | +} |
| 211 | + |
| 212 | +module FlaskConstantSecretKey = TaintTracking::Global<FlaskConstantSecretKeyConfig>; |
| 213 | + |
| 214 | +import FlaskConstantSecretKey::PathGraph |
| 215 | + |
| 216 | +from FlaskConstantSecretKey::PathNode source, FlaskConstantSecretKey::PathNode sink |
| 217 | +where FlaskConstantSecretKey::flowPath(source, sink) |
| 218 | +select sink, source, sink, "The SECRET_KEY config variable has assigned by $@.", source, |
| 219 | + " this constant String" |
0 commit comments