mirror of
https://github.com/github/codeql.git
synced 2025-12-24 04:36:35 +01:00
Merge remote-tracking branch 'origin/main' into jorgectf/python/insecure-cookie
This commit is contained in:
jorgectf
@@ -1,13 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
|
||||
<overview>
|
||||
<p>about General Class-Level Information</p>
|
||||
|
||||
<!--TOC-->
|
||||
|
||||
</overview>
|
||||
</qhelp>
|
||||
@@ -1,13 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
|
||||
<overview>
|
||||
<p>about Architecture</p>
|
||||
|
||||
<!--TOC-->
|
||||
|
||||
</overview>
|
||||
</qhelp>
|
||||
@@ -1,16 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
|
||||
<overview>
|
||||
<p>about best practices</p>
|
||||
|
||||
<!--TOC-->
|
||||
|
||||
|
||||
|
||||
|
||||
</overview>
|
||||
</qhelp>
|
||||
@@ -1,42 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
This metric measures the number of lines of text that have been added, deleted
|
||||
or modified in files below this location in the tree.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
Code churn is known to be a good (if not the best) predictor of defects in a
|
||||
code component (see e.g. [Nagappan] or [Khoshgoftaar]). The intuition is that
|
||||
files, packages or projects that have experienced a disproportionately high
|
||||
amount of churn for the amount of code involved may have been harder to write,
|
||||
and are thus likely to contain more bugs.
|
||||
</p>
|
||||
|
||||
</overview>
|
||||
<recommendation>
|
||||
|
||||
<p>
|
||||
It is a fact of life that some code is going to be changed more than the rest,
|
||||
and little can be done to change this. However, bearing in mind code churn's
|
||||
effectiveness as a defect predictor, code that has been repeatedly changed
|
||||
should be subjected to vigorous testing and code review.
|
||||
</p>
|
||||
|
||||
</recommendation>
|
||||
<references>
|
||||
|
||||
|
||||
<li>
|
||||
N. Nagappan et al. <em>Change Bursts as Defect Predictors</em>. In Proceedings of the 21st IEEE International Symposium on Software Reliability Engineering, 2010.
|
||||
</li>
|
||||
<li>
|
||||
T. M. Khoshgoftaar and R. M. Szabo. <em>Improving code churn predictions during the system test and maintenance phases</em>. In ICSM '94, 1994, pp. 58-67.
|
||||
</li>
|
||||
|
||||
|
||||
</references>
|
||||
</qhelp>
|
||||
@@ -1,6 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<include src="HChurn.qhelp" />
|
||||
</qhelp>
|
||||
@@ -1,6 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<include src="HChurn.qhelp" />
|
||||
</qhelp>
|
||||
@@ -1,48 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
This metric measures the number of different authors (by examining the
|
||||
version control history)
|
||||
for files below this location in the tree. (This is a better version
|
||||
of the metric that counts the number of different authors using Javadoc
|
||||
tags.)
|
||||
</p>
|
||||
|
||||
<p>
|
||||
Files that have been changed by a large number of different authors are
|
||||
by definition the product of many minds. New authors working on a file
|
||||
may be less familiar with the design and implementation of the code than
|
||||
the original authors, which can be a potential source of bugs. Furthermore,
|
||||
code that has been worked on by many people, if not carefully maintained,
|
||||
often ends up lacking conceptual integrity. For both of these reasons, any
|
||||
code that has been worked on by an unusually high number of different people
|
||||
merits careful inspection in code reviews.
|
||||
</p>
|
||||
|
||||
</overview>
|
||||
<recommendation>
|
||||
|
||||
<p>
|
||||
There is clearly no way to reduce the number of authors that have worked
|
||||
on a file - it is impossible to rewrite history. However, files highlighted
|
||||
by this metric should be given special attention in a code review, and may
|
||||
ultimately be good candidates for refactoring/rewriting by an individual,
|
||||
experienced developer.
|
||||
</p>
|
||||
|
||||
|
||||
|
||||
</recommendation>
|
||||
<references>
|
||||
|
||||
|
||||
<li>
|
||||
F. P. Brooks Jr. <em>The Mythical Man-Month</em>, Chapter 4. Addison-Wesley, 1974.
|
||||
</li>
|
||||
|
||||
|
||||
</references>
|
||||
</qhelp>
|
||||
@@ -1,30 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
This metric measures the total number of file-level changes made to files
|
||||
below this location in the tree. For an individual file, it measures the
|
||||
number of commits that have affected that file. For a directory of files, it
|
||||
measures the sum of the file-level changes for each of the files in the
|
||||
directory.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
For example, suppose we have a directory containing two files, A and B. If the
|
||||
number of file-level changes to A is <code>100</code>, and the number of
|
||||
file-level changes to B is <code>80</code>, then the total number of
|
||||
file-level changes to the directory is <code>180</code>. Note that this is
|
||||
likely to be different (in some cases very different) from the number of
|
||||
commits that affected any file in the directory, since more than one file can
|
||||
be changed by a single commit. (Note what would happen if we performed
|
||||
<code>80</code> commits on A and B, followed by another <code>20</code>
|
||||
commits on A alone - the total number of file-level changes would be
|
||||
<code>180</code>, but the number of commits involved would be
|
||||
<code>100</code>.)
|
||||
</p>
|
||||
|
||||
|
||||
</overview>
|
||||
</qhelp>
|
||||
@@ -1,51 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
This metric measures the average number of co-committed files for the files
|
||||
below this location in the tree.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
A co-committed file is one that is committed at the same time as a given file.
|
||||
For instance, if you commit files A, B and C together, then B and C would be
|
||||
the co-committed files of A for that commit. The value of the metric for an
|
||||
individual file is the average number of such co-committed files over all
|
||||
commits. The value of the metric for a directory is the aggregation of these
|
||||
averages - for instance, if we are using <code>max</code> as our aggregation
|
||||
function, the value would be the maximum of the average number of co-commits
|
||||
over all files in the directory.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
An unusually high value for this metric may indicate that the file in question
|
||||
is too tightly-coupled to other files, and it is difficult to change it in
|
||||
isolation. Alternatively, it may just be an indication that you commit lots of
|
||||
unrelated changes at the same time.
|
||||
</p>
|
||||
|
||||
</overview>
|
||||
<recommendation>
|
||||
|
||||
<p>
|
||||
Examine the file in question to see what the problem is.
|
||||
</p>
|
||||
|
||||
<ul>
|
||||
<li>
|
||||
If the file is too tightly coupled, it will have high values for its afferent
|
||||
and/or efferent coupling metrics, and you should apply the advice given there.
|
||||
</li>
|
||||
|
||||
<li>
|
||||
If the file is not tightly coupled, but you find that you are committing lots
|
||||
of unrelated changes at the same time, then you may want to revisit your commit
|
||||
practices.
|
||||
</li>
|
||||
</ul>
|
||||
|
||||
|
||||
</recommendation>
|
||||
</qhelp>
|
||||
@@ -1,53 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
This metric measures the number of file re-commits that have occurred below
|
||||
this location in the tree. A re-commit is taken to mean a commit to a file
|
||||
that was touched less than five days ago.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
In a system that is being developed using a controlled change process (where
|
||||
changes are not committed until they are in some sense 'complete'), re-commits
|
||||
can be (but are not always) an indication that an initial change was not
|
||||
successful and had to be revisited within a short time period. The intuition
|
||||
is that the original change may have been difficult to get right, and hence
|
||||
the code in the file may be more than usually defect-prone. The concept is
|
||||
somewhat similar to that of 'change bursts', as described in [Nagappan].
|
||||
</p>
|
||||
|
||||
</overview>
|
||||
<recommendation>
|
||||
|
||||
<p>
|
||||
High numbers of re-commits can be addressed on two levels: preventative and
|
||||
corrective.
|
||||
</p>
|
||||
|
||||
<ul>
|
||||
<li>
|
||||
On the preventative side, a high number of re-commits may be an indication
|
||||
that your code review process needs an overhaul.
|
||||
</li>
|
||||
|
||||
<li>
|
||||
On the corrective side, code that has experienced a high number of re-commits
|
||||
should be vigorously code reviewed and tested.
|
||||
</li>
|
||||
</ul>
|
||||
|
||||
|
||||
</recommendation>
|
||||
<references>
|
||||
|
||||
|
||||
<li>
|
||||
N. Nagappan et al. <em>Change Bursts as Defect Predictors</em>. In Proceedings of the 21st IEEE International Symposium on Software Reliability Engineering, 2010.
|
||||
</li>
|
||||
|
||||
|
||||
</references>
|
||||
</qhelp>
|
||||
@@ -1,63 +0,0 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
This metric measures the number of recent changes to files that have occurred
|
||||
below this location in the tree. A recent change is taken to mean a change
|
||||
that has occurred in the last <code>180</code> days.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
All code that has changed a great deal may be more than usually prone to
|
||||
defects, but this is particularly true of code that has been changing
|
||||
dramatically in the recent past, because it has not yet had a chance to be
|
||||
properly field-tested in order to iron out the bugs.
|
||||
</p>
|
||||
|
||||
</overview>
|
||||
<recommendation>
|
||||
|
||||
<p>
|
||||
There is more than one reason why a file may have been changing a lot
|
||||
recently:
|
||||
</p>
|
||||
|
||||
<ul>
|
||||
<li>
|
||||
The file may be part of a new subsystem that is being written. New code is
|
||||
always going to change a lot in a short period of time, but it is important
|
||||
to ensure that it is properly code reviewed and unit tested before integrating
|
||||
it into a working product.
|
||||
</li>
|
||||
|
||||
<li>
|
||||
The file may be being heavily refactored. Large refactorings are sometimes
|
||||
essential, but they are also quite risky. You should write proper regression
|
||||
tests before starting on a major refactoring, and check that they still pass
|
||||
once you're done.
|
||||
</li>
|
||||
|
||||
<li>
|
||||
The same bit of code may be being changed repeatedly because it is difficult
|
||||
to get right. Aside from vigorous code reviewing and testing, it may be a good
|
||||
idea to rethink the system design - if something is that hard
|
||||
to get right (and it's not an inherently difficult concept), you might be making life unnecessarily hard for yourself and
|
||||
risking introducing insidious defects.
|
||||
</li>
|
||||
</ul>
|
||||
|
||||
|
||||
|
||||
</recommendation>
|
||||
<references>
|
||||
|
||||
|
||||
<li>
|
||||
N. Nagappan et al. <em>Change Bursts as Defect Predictors</em>. In Proceedings of the 21st IEEE International Symposium on Software Reliability Engineering, 2010.
|
||||
</li>
|
||||
|
||||
|
||||
</references>
|
||||
</qhelp>
|
||||
@@ -158,8 +158,10 @@ Python built-in support
|
||||
Flask, Web framework
|
||||
Tornado, Web framework
|
||||
Twisted, Web framework
|
||||
PyYAML, Serialization
|
||||
starlette, Asynchronous Server Gateway Interface (ASGI)
|
||||
dill, Serialization
|
||||
PyYAML, Serialization
|
||||
ruamel.yaml, Serialization
|
||||
simplejson, Serialization
|
||||
ujson, Serialization
|
||||
fabric, Utility library
|
||||
|
||||
@@ -9,7 +9,7 @@ cn.hutool.core.codec,,,1,,,,,,,,,,,,,,,,,,,,,1,
|
||||
com.esotericsoftware.kryo.io,,,1,,,,,,,,,,,,,,,,,,,,,1,
|
||||
com.esotericsoftware.kryo5.io,,,1,,,,,,,,,,,,,,,,,,,,,1,
|
||||
com.fasterxml.jackson.core,,,1,,,,,,,,,,,,,,,,,,,,,1,
|
||||
com.fasterxml.jackson.databind,,,5,,,,,,,,,,,,,,,,,,,,,5,
|
||||
com.fasterxml.jackson.databind,,,6,,,,,,,,,,,,,,,,,,,,,6,
|
||||
com.google.common.base,,,85,,,,,,,,,,,,,,,,,,,,,62,23
|
||||
com.google.common.cache,,,17,,,,,,,,,,,,,,,,,,,,,,17
|
||||
com.google.common.collect,,,553,,,,,,,,,,,,,,,,,,,,,2,551
|
||||
@@ -90,3 +90,12 @@ org.springframework.web.util,,,163,,,,,,,,,,,,,,,,,,,,,138,25
|
||||
org.xml.sax,,,1,,,,,,,,,,,,,,,,,,,,,1,
|
||||
org.xmlpull.v1,,3,,,,,,,,,,,,,,,,,,,,,3,,
|
||||
play.mvc,,4,,,,,,,,,,,,,,,,,,,,,4,,
|
||||
ratpack.core.form,,,3,,,,,,,,,,,,,,,,,,,,,3,
|
||||
ratpack.core.handling,,6,4,,,,,,,,,,,,,,,,,,,,6,4,
|
||||
ratpack.core.http,,10,10,,,,,,,,,,,,,,,,,,,,10,10,
|
||||
ratpack.exec,,,26,,,,,,,,,,,,,,,,,,,,,,26
|
||||
ratpack.form,,,3,,,,,,,,,,,,,,,,,,,,,3,
|
||||
ratpack.func,,,5,,,,,,,,,,,,,,,,,,,,,,5
|
||||
ratpack.handling,,6,4,,,,,,,,,,,,,,,,,,,,6,4,
|
||||
ratpack.http,,10,10,,,,,,,,,,,,,,,,,,,,10,10,
|
||||
ratpack.util,,,5,,,,,,,,,,,,,,,,,,,,,,5
|
||||
|
||||
|
@@ -18,6 +18,6 @@ Java framework & library support
|
||||
Java Standard Library,``java.*``,3,510,30,13,,,7,,,10
|
||||
Java extensions,"``javax.*``, ``jakarta.*``",54,552,32,,,4,,1,1,2
|
||||
`Spring <https://spring.io/>`_,``org.springframework.*``,29,469,91,,,,19,14,,29
|
||||
Others,"``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jooq``, ``org.mvel2``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``",7,28,151,,,,14,18,,
|
||||
Totals,,143,5261,408,13,6,10,107,33,1,66
|
||||
Others,"``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jooq``, ``org.mvel2``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``, ``ratpack.core.form``, ``ratpack.core.handling``, ``ratpack.core.http``, ``ratpack.exec``, ``ratpack.form``, ``ratpack.func``, ``ratpack.handling``, ``ratpack.http``, ``ratpack.util``",39,99,151,,,,14,18,,
|
||||
Totals,,175,5332,408,13,6,10,107,33,1,66
|
||||
|
||||
|
||||
2
python/change-notes/2021-10-26-ruamel.yaml-modeling.md
Normal file
2
python/change-notes/2021-10-26-ruamel.yaml-modeling.md
Normal file
@@ -0,0 +1,2 @@
|
||||
lgtm,codescanning
|
||||
* Added modeling of the `ruamel.yaml` PyPI package, resulting in additional sinks for the _Deserializing untrusted input_ (`py/unsafe-deserialization`) query (since `ruamel.yaml.load` can lead to code execution).
|
||||
@@ -25,6 +25,7 @@ private import semmle.python.frameworks.Peewee
|
||||
private import semmle.python.frameworks.Psycopg2
|
||||
private import semmle.python.frameworks.PyMySQL
|
||||
private import semmle.python.frameworks.Rsa
|
||||
private import semmle.python.frameworks.RuamelYaml
|
||||
private import semmle.python.frameworks.Simplejson
|
||||
private import semmle.python.frameworks.SqlAlchemy
|
||||
private import semmle.python.frameworks.Stdlib
|
||||
|
||||
@@ -519,4 +519,32 @@ module Flask {
|
||||
|
||||
override DataFlow::Node getValueArg() { none() }
|
||||
}
|
||||
|
||||
/**
|
||||
* A call to `flask.send_from_directory`.
|
||||
*
|
||||
* See https://flask.palletsprojects.com/en/1.1.x/api/#flask.send_from_directory
|
||||
*/
|
||||
class FlaskSendFromDirectory extends FileSystemAccess::Range, DataFlow::CallCfgNode {
|
||||
FlaskSendFromDirectory() {
|
||||
this = API::moduleImport("flask").getMember("send_from_directory").getACall()
|
||||
}
|
||||
|
||||
override DataFlow::Node getAPathArgument() {
|
||||
result in [this.getArg(_), this.getArgByName(["directory", "filename"])]
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A call to `flask.send_file`.
|
||||
*
|
||||
* See https://flask.palletsprojects.com/en/1.1.x/api/#flask.send_file
|
||||
*/
|
||||
class FlaskSendFile extends FileSystemAccess::Range, DataFlow::CallCfgNode {
|
||||
FlaskSendFile() { this = API::moduleImport("flask").getMember("send_file").getACall() }
|
||||
|
||||
override DataFlow::Node getAPathArgument() {
|
||||
result in [this.getArg(0), this.getArgByName("filename_or_fp")]
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
57
python/ql/lib/semmle/python/frameworks/RuamelYaml.qll
Normal file
57
python/ql/lib/semmle/python/frameworks/RuamelYaml.qll
Normal file
@@ -0,0 +1,57 @@
|
||||
/**
|
||||
* Provides classes modeling security-relevant aspects of the `ruamel.yaml` PyPI package
|
||||
*
|
||||
* See
|
||||
* - https://pypi.org/project/ruamel.yaml/
|
||||
*/
|
||||
|
||||
private import python
|
||||
private import semmle.python.dataflow.new.DataFlow
|
||||
private import semmle.python.Concepts
|
||||
private import semmle.python.ApiGraphs
|
||||
|
||||
/**
|
||||
* Provides models for the `ruamel.yaml` PyPI package.
|
||||
*
|
||||
* See
|
||||
* - https://pypi.org/project/ruamel.yaml/
|
||||
*/
|
||||
private module RuamelYaml {
|
||||
// Note: `ruamel.yaml` is a fork of the `PyYAML` PyPI package, so that's why the
|
||||
// interface is so similar.
|
||||
/**
|
||||
* A call to any of the loading functions in `yaml` (`load`, `load_all`, `safe_load`, `safe_load_all`)
|
||||
*
|
||||
* See https://pyyaml.org/wiki/PyYAMLDocumentation (you will have to scroll down).
|
||||
*/
|
||||
private class RuamelYamlLoadCall extends Decoding::Range, DataFlow::CallCfgNode {
|
||||
string func_name;
|
||||
|
||||
RuamelYamlLoadCall() {
|
||||
func_name in ["load", "load_all", "safe_load", "safe_load_all"] and
|
||||
this = API::moduleImport("ruamel").getMember("yaml").getMember(func_name).getACall()
|
||||
}
|
||||
|
||||
override predicate mayExecuteInput() {
|
||||
func_name in ["load", "load_all"] and
|
||||
// If the `Loader` argument is not set, the default loader will be used, which is
|
||||
// not safe. The only safe loaders are `SafeLoader` or `BaseLoader` (and their
|
||||
// variants with C implementation).
|
||||
not exists(DataFlow::Node loader_arg |
|
||||
loader_arg in [this.getArg(1), this.getArgByName("Loader")]
|
||||
|
|
||||
loader_arg =
|
||||
API::moduleImport("ruamel")
|
||||
.getMember("yaml")
|
||||
.getMember(["SafeLoader", "BaseLoader", "CSafeLoader", "CBaseLoader"])
|
||||
.getAUse()
|
||||
)
|
||||
}
|
||||
|
||||
override DataFlow::Node getAnInput() { result in [this.getArg(0), this.getArgByName("stream")] }
|
||||
|
||||
override DataFlow::Node getOutput() { result = this }
|
||||
|
||||
override string getFormat() { result = "YAML" }
|
||||
}
|
||||
}
|
||||
@@ -9,7 +9,6 @@
|
||||
|
||||
private import python
|
||||
private import semmle.python.dataflow.new.DataFlow
|
||||
private import semmle.python.dataflow.new.RemoteFlowSources
|
||||
private import semmle.python.Concepts
|
||||
private import semmle.python.ApiGraphs
|
||||
|
||||
@@ -41,11 +40,17 @@ private module Yaml {
|
||||
}
|
||||
|
||||
/**
|
||||
* This function was thought safe from the 5.1 release in 2017, when the default loader was changed to `FullLoader`.
|
||||
* In 2020 new exploits were found, meaning it's not safe. The Current plan is to change the default to `SafeLoader` in release 6.0
|
||||
* (as explained in https://github.com/yaml/pyyaml/issues/420#issuecomment-696752389).
|
||||
* Until 6.0 is released, we will mark `yaml.load` as possibly leading to arbitrary code execution.
|
||||
* See https://github.com/yaml/pyyaml/wiki/PyYAML-yaml.load(input)-Deprecation for more details.
|
||||
* This function was thought safe from the 5.1 release in 2017, when the default
|
||||
* loader was changed to `FullLoader` (see
|
||||
* https://github.com/yaml/pyyaml/wiki/PyYAML-yaml.load(input)-Deprecation).
|
||||
*
|
||||
* In 2020 new exploits were found, meaning it's not safe. With the 6.0 release (see
|
||||
* https://github.com/yaml/pyyaml/commit/8cdff2c80573b8be8e8ad28929264a913a63aa33),
|
||||
* when using `load` and `load_all` you are now required to specify a Loader. But
|
||||
* from what I (@RasmusWL) can gather, `FullLoader` is not to be considered safe,
|
||||
* although known exploits have been mitigated (is at least my impression). Also see
|
||||
* https://github.com/yaml/pyyaml/issues/420#issuecomment-696752389 for more
|
||||
* details.
|
||||
*/
|
||||
override predicate mayExecuteInput() {
|
||||
func_name in ["full_load", "full_load_all", "unsafe_load", "unsafe_load_all"]
|
||||
@@ -63,7 +68,7 @@ private module Yaml {
|
||||
)
|
||||
}
|
||||
|
||||
override DataFlow::Node getAnInput() { result = this.getArg(0) }
|
||||
override DataFlow::Node getAnInput() { result in [this.getArg(0), this.getArgByName("stream")] }
|
||||
|
||||
override DataFlow::Node getOutput() { result = this }
|
||||
|
||||
|
||||
@@ -1,6 +1,17 @@
|
||||
from flask import Flask, request
|
||||
from flask import Flask, request, send_from_directory, send_file
|
||||
app = Flask(__name__)
|
||||
|
||||
@app.route("/save-uploaded-file") # $routeSetup="/save-uploaded-file"
|
||||
def test_taint(): # $requestHandler
|
||||
request.files['key'].save("path") # $ getAPathArgument="path"
|
||||
|
||||
|
||||
@app.route("/path-injection") # $routeSetup="/path-injection"
|
||||
def test_path(): # $requestHandler
|
||||
|
||||
send_from_directory("filepath","file") # $ getAPathArgument="filepath" getAPathArgument="file"
|
||||
send_file("file") # $ getAPathArgument="file"
|
||||
|
||||
send_from_directory(directory="filepath","file") # $ getAPathArgument="filepath" getAPathArgument="file"
|
||||
send_from_directory(filename="filepath","file") # $ getAPathArgument="filepath" getAPathArgument="file"
|
||||
send_file(filename_or_fp="file") # $ getAPathArgument="file"
|
||||
|
||||
@@ -0,0 +1,2 @@
|
||||
import python
|
||||
import experimental.meta.ConceptsTest
|
||||
@@ -0,0 +1,33 @@
|
||||
import ruamel.yaml
|
||||
|
||||
# Unsafe:
|
||||
ruamel.yaml.load(payload) # $ decodeInput=payload decodeOutput=ruamel.yaml.load(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
ruamel.yaml.load(stream=payload) # $ decodeInput=payload decodeOutput=ruamel.yaml.load(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
ruamel.yaml.load(payload, ruamel.yaml.Loader) # $ decodeInput=payload decodeOutput=ruamel.yaml.load(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
|
||||
# Safe:
|
||||
ruamel.yaml.load(payload, ruamel.yaml.SafeLoader) # $ decodeInput=payload decodeOutput=ruamel.yaml.load(..) decodeFormat=YAML
|
||||
ruamel.yaml.load(payload, Loader=ruamel.yaml.SafeLoader) # $ decodeInput=payload decodeOutput=ruamel.yaml.load(..) decodeFormat=YAML
|
||||
ruamel.yaml.load(payload, ruamel.yaml.BaseLoader) # $ decodeInput=payload decodeOutput=ruamel.yaml.load(..) decodeFormat=YAML
|
||||
ruamel.yaml.safe_load(payload) # $ decodeInput=payload decodeOutput=ruamel.yaml.safe_load(..) decodeFormat=YAML
|
||||
|
||||
################################################################################
|
||||
# load_all variants
|
||||
################################################################################
|
||||
|
||||
# Unsafe:
|
||||
ruamel.yaml.load_all(payload) # $ decodeInput=payload decodeOutput=ruamel.yaml.load_all(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
|
||||
# Safe:
|
||||
ruamel.yaml.safe_load_all(payload) # $ decodeInput=payload decodeOutput=ruamel.yaml.safe_load_all(..) decodeFormat=YAML
|
||||
|
||||
################################################################################
|
||||
# C-based loaders with `libyaml`
|
||||
################################################################################
|
||||
|
||||
# Unsafe:
|
||||
ruamel.yaml.load(payload, ruamel.yaml.CLoader) # $ decodeInput=payload decodeOutput=ruamel.yaml.load(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
|
||||
# Safe:
|
||||
ruamel.yaml.load(payload, ruamel.yaml.CSafeLoader) # $ decodeInput=payload decodeOutput=ruamel.yaml.load(..) decodeFormat=YAML
|
||||
ruamel.yaml.load(payload, ruamel.yaml.CBaseLoader) # $ decodeInput=payload decodeOutput=ruamel.yaml.load(..) decodeFormat=YAML
|
||||
63
python/ql/test/library-tests/frameworks/ruamel.yaml/PoC
Normal file
63
python/ql/test/library-tests/frameworks/ruamel.yaml/PoC
Normal file
@@ -0,0 +1,63 @@
|
||||
#!/usr/bin/env python3
|
||||
|
||||
# this file doesn't have a .py extension so the extractor doesn't pick it up, so it
|
||||
# doesn't have to be annotated
|
||||
|
||||
# This file is just a Proof of Concept for how code execution can be triggered.
|
||||
|
||||
import os
|
||||
import ruamel.yaml
|
||||
|
||||
class Exploit(object):
|
||||
def __reduce__(self):
|
||||
return (os.system, ('ls',))
|
||||
|
||||
data = Exploit()
|
||||
serialized_data = ruamel.yaml.dump(data)
|
||||
|
||||
# All these will execute `ls`
|
||||
print("!!! ruamel.yaml.load")
|
||||
ruamel.yaml.load(serialized_data)
|
||||
|
||||
print("!!! ruamel.yaml.load kwarg")
|
||||
ruamel.yaml.load(stream=serialized_data)
|
||||
|
||||
print("!!! ruamel.yaml.load with Loader=ruamel.yaml.Loader")
|
||||
ruamel.yaml.load(serialized_data, ruamel.yaml.Loader)
|
||||
|
||||
print("!!! ruamel.yaml.load with Loader=ruamel.yaml.UnsafeLoader")
|
||||
ruamel.yaml.load(serialized_data, ruamel.yaml.UnsafeLoader)
|
||||
|
||||
print("!!! ruamel.yaml.load with Loader=ruamel.yaml.CLoader")
|
||||
ruamel.yaml.load(serialized_data, ruamel.yaml.CLoader)
|
||||
|
||||
# you need to iterate through the result for it to execute... but it still works
|
||||
print("!!! ruamel.yaml.load_all")
|
||||
for _ in ruamel.yaml.load_all(serialized_data):
|
||||
pass
|
||||
|
||||
# check that the safe version is actually safe
|
||||
print("\n" + "-"*80)
|
||||
print("safe versions")
|
||||
print("-" * 80)
|
||||
|
||||
print("!!! ruamel.yaml.safe_load")
|
||||
try:
|
||||
ruamel.yaml.safe_load(serialized_data)
|
||||
raise Exception("should not happen")
|
||||
except ruamel.yaml.constructor.ConstructorError:
|
||||
pass
|
||||
|
||||
print("!!! ruamel.yaml.load with Loader=ruamel.yaml.SafeLoader")
|
||||
try:
|
||||
ruamel.yaml.load(serialized_data, ruamel.yaml.SafeLoader)
|
||||
raise Exception("should not happen")
|
||||
except ruamel.yaml.constructor.ConstructorError:
|
||||
pass
|
||||
|
||||
print("!!! ruamel.yaml.load with Loader=ruamel.yaml.CSafeLoader")
|
||||
try:
|
||||
ruamel.yaml.load(serialized_data, ruamel.yaml.CSafeLoader)
|
||||
raise Exception("should not happen")
|
||||
except ruamel.yaml.constructor.ConstructorError:
|
||||
pass
|
||||
@@ -2,6 +2,7 @@ import yaml
|
||||
|
||||
# Unsafe:
|
||||
yaml.load(payload) # $decodeInput=payload decodeOutput=yaml.load(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
yaml.load(stream=payload) # $decodeInput=payload decodeOutput=yaml.load(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
yaml.load(payload, yaml.Loader) # $decodeInput=payload decodeOutput=yaml.load(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
yaml.unsafe_load(payload) # $ decodeInput=payload decodeOutput=yaml.unsafe_load(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
yaml.full_load(payload) # $ decodeInput=payload decodeOutput=yaml.full_load(..) decodeFormat=YAML decodeMayExecuteInput
|
||||
|
||||
58
python/ql/test/library-tests/frameworks/yaml/PoC
Normal file
58
python/ql/test/library-tests/frameworks/yaml/PoC
Normal file
@@ -0,0 +1,58 @@
|
||||
#!/usr/bin/env python3
|
||||
|
||||
# this file doesn't have a .py extension so the extractor doesn't pick it up, so it
|
||||
# doesn't have to be annotated
|
||||
|
||||
# This file is just a Proof of Concept for how code execution can be triggered.
|
||||
|
||||
|
||||
import os
|
||||
import yaml
|
||||
|
||||
class Exploit(object):
|
||||
def __reduce__(self):
|
||||
return (os.system, ('ls',))
|
||||
|
||||
data = Exploit()
|
||||
serialized_data = yaml.dump(data)
|
||||
|
||||
# All these will execute `ls`
|
||||
print("!!! yaml.unsafe_load")
|
||||
yaml.unsafe_load(serialized_data)
|
||||
|
||||
print("!!! yaml.unsafe_load kwarg")
|
||||
yaml.unsafe_load(stream=serialized_data)
|
||||
|
||||
print("!!! yaml.load with Loader=yaml.UnsafeLoader")
|
||||
yaml.load(serialized_data, yaml.UnsafeLoader)
|
||||
|
||||
# you need to iterate through the result for it to execute... but it still works
|
||||
print("!!! yaml.unsafe_load_all")
|
||||
for _ in yaml.unsafe_load_all(serialized_data):
|
||||
pass
|
||||
|
||||
# check that the safe version is actually safe
|
||||
print("\n" + "-"*80)
|
||||
print("safe versions")
|
||||
print("-" * 80)
|
||||
|
||||
print("!!! yaml.load")
|
||||
try:
|
||||
yaml.load(serialized_data)
|
||||
raise Exception("should not happen")
|
||||
except yaml.constructor.ConstructorError:
|
||||
pass
|
||||
|
||||
print("!!! yaml.safe_load")
|
||||
try:
|
||||
yaml.safe_load(serialized_data)
|
||||
raise Exception("should not happen")
|
||||
except yaml.constructor.ConstructorError:
|
||||
pass
|
||||
|
||||
print("!!! yaml.load with Loader=yaml.SafeLoader")
|
||||
try:
|
||||
yaml.load(serialized_data, yaml.SafeLoader)
|
||||
raise Exception("should not happen")
|
||||
except yaml.constructor.ConstructorError:
|
||||
pass
|
||||
BIN
ruby/Cargo.lock
generated
BIN
ruby/Cargo.lock
generated
Binary file not shown.
Reference in New Issue
Block a user