-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Azure python sanitizer upstream2 #21288
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
9912aaa
b8ba905
46a2a24
08b72d0
7db9779
265922d
88adb05
27e1981
97ddab0
97f19d0
42f6e6a
4f11913
f6c302b
85ae404
df54459
23bab81
9f8ed71
a91cf6b
4bb110b
9f9c353
4d4e7a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * Modified SSRF tests to use postprocessing to more easily debug results. | ||
| * Added new full SSRF sanitization barrier from the new AntiSSRF library. | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -176,4 +176,48 @@ module ServerSideRequestForgery { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| strNode = [call.getArg(0), call.getArgByName("string")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** A validation of a URI using the `AntiSSRF` library, considered as a full-ssrf sanitizer. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private class UriValidator extends FullUrlControlSanitizer { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| UriValidator() { this = DataFlow::BarrierGuard<uri_validator/3>::getABarrierNode() } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import semmle.python.dataflow.new.internal.DataFlowPublic | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private predicate uri_validator(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exists(DataFlow::CallCfgNode call, string funcs | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| funcs in ["in_domain", "in_azure_keyvault_domain", "in_azure_storage_domain"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| call = API::moduleImport("AntiSSRF").getMember("URIValidator").getMember(funcs).getACall() and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| call.getArg(0).asCfgNode() = node and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // validator used in a comparison | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exists(CompareNode cn, Cmpop op, Node n | cn = g and n.getALocalSource() = call | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // validator == true or validator == false or validator is True or validator is False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (op instanceof Eq or op instanceof Is) and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exists(ControlFlowNode l, boolean bool | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| l.getNode().(BooleanLiteral).booleanValue() = bool and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool in [true, false] and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| branch = bool and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cn.operands(n.asCfgNode(), op, l) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| or | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // validator != false or validator != true or validator is not True or validator is not False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (op instanceof NotEq or op instanceof IsNot) and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exists(ControlFlowNode l, boolean bool | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| l.getNode().(BooleanLiteral).booleanValue() = bool and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool in [true, false] and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| branch = bool.booleanNot() and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cn.operands(n.asCfgNode(), op, l) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| or | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // validator call directly (e.g., if URIValidator.in_domain(...) ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| g = call.asCfgNode() and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| branch = true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+194
to
+219
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, it's awkward. The present solution is an okay band-aid (especially after some cleanup), but it would be nice to have a generic solution that could also account for things like boolean operators, etc. I think the closest thing I'm aware of is https://github.com/github/codeql/blob/main/python/ql/lib/semmle/python/Comparisons.qll |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+187
to
+222
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think this should be equivalent, and hopefully a bit easier to read. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,4 @@ | ||
| Security/CWE-918/FullServerSideRequestForgery.ql | ||
| query: Security/CWE-918/FullServerSideRequestForgery.ql | ||
| postprocess: | ||
| - utils/test/InlineExpectationsTestQuery.ql | ||
| - utils/test/PrettyPrintModels.ql |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,4 @@ | ||
| Security/CWE-918/PartialServerSideRequestForgery.ql | ||
| query: Security/CWE-918/PartialServerSideRequestForgery.ql | ||
| postprocess: | ||
| - utils/test/InlineExpectationsTestQuery.ql | ||
| - utils/test/PrettyPrintModels.ql |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| from flask import request | ||
| from flask import request # $ Source | ||
|
|
||
| import requests | ||
| import re | ||
|
|
@@ -7,20 +7,24 @@ def full_ssrf(): | |
| user_input = request.args['untrusted_input'] | ||
| query_val = request.args['query_val'] | ||
|
|
||
| requests.get(user_input) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(user_input) # $ Alert[py/full-ssrf] | ||
|
|
||
| url = "https://" + user_input | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| # although the path `/foo` is added here, this can be circumvented such that the | ||
| # final URL is `https://evil.com/#/foo" -- since the fragment (#) is not sent to the | ||
| # server. | ||
| url = "https://" + user_input + "/foo" | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| # this might seem like a dummy test, but it serves to check how our sanitizers work. | ||
| url = "https://" + user_input + "/foo?key=" + query_val | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| # taint-steps are added as `fromNode -> toNode`, but when adding a sanitizer it's | ||
| # currently only possible to so on either `fromNode` or `toNode` (either all edges in | ||
|
|
@@ -39,72 +43,87 @@ def full_ssrf_format(): | |
|
|
||
| # using .format | ||
| url = "https://{}".format(user_input) | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| url = "https://{}/foo".format(user_input) | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| url = "https://{}/foo?key={}".format(user_input, query_val) | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| url = "https://{x}".format(x=user_input) | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| url = "https://{1}".format(0, user_input) | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| def full_ssrf_percent_format(): | ||
| user_input = request.args['untrusted_input'] | ||
| query_val = request.args['query_val'] | ||
|
|
||
| # using %-formatting | ||
| url = "https://%s" % user_input | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| url = "https://%s/foo" % user_input | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| url = "https://%s/foo/key=%s" % (user_input, query_val) | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full and partial control | ||
| requests.get(url) # $ Alert[py/partial-ssrf] $ MISSING: Alert[py/full-ssrf] | ||
|
|
||
| def full_ssrf_f_strings(): | ||
| user_input = request.args['untrusted_input'] | ||
| query_val = request.args['query_val'] | ||
|
|
||
| # using f-strings | ||
| url = f"https://{user_input}" | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| url = f"https://{user_input}/foo" | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
| url = f"https://{user_input}/foo?key={query_val}" | ||
| requests.get(url) # NOT OK -- user has full control | ||
| # NOT OK -- user has full control | ||
| requests.get(url) # $ Alert[py/full-ssrf] | ||
|
|
||
|
|
||
| def partial_ssrf_1(): | ||
| user_input = request.args['untrusted_input'] | ||
|
|
||
| url = "https://example.com/foo?" + user_input | ||
| requests.get(url) # NOT OK -- user controls query parameters | ||
| # NOT OK -- user controls query parameters | ||
| requests.get(url) # $ Alert[py/partial-ssrf] | ||
|
|
||
| def partial_ssrf_2(): | ||
| user_input = request.args['untrusted_input'] | ||
|
|
||
| url = "https://example.com/" + user_input | ||
| requests.get(url) # NOT OK -- user controls path | ||
| # NOT OK -- user controls path | ||
| requests.get(url) # $ Alert[py/partial-ssrf] | ||
|
|
||
| def partial_ssrf_3(): | ||
| user_input = request.args['untrusted_input'] | ||
|
|
||
| url = "https://example.com/" + user_input | ||
| requests.get(url) # NOT OK -- user controls path | ||
| # NOT OK -- user controls path | ||
| requests.get(url) # $ Alert[py/partial-ssrf] | ||
|
|
||
| def partial_ssrf_4(): | ||
| user_input = request.args['untrusted_input'] | ||
|
|
||
| url = "https://example.com/foo#{}".format(user_input) | ||
| requests.get(url) # NOT OK -- user contollred fragment | ||
| # NOT OK -- user controlled fragment | ||
| requests.get(url) # $ Alert[py/partial-ssrf] | ||
|
Comment on lines
124
to
126
|
||
|
|
||
| def partial_ssrf_5(): | ||
| user_input = request.args['untrusted_input'] | ||
|
|
@@ -113,20 +132,22 @@ def partial_ssrf_5(): | |
| # controlled | ||
|
|
||
| url = "https://example.com/foo#%s" % user_input | ||
| requests.get(url) # NOT OK -- user contollred fragment | ||
| # NOT OK -- user controlled fragment | ||
| requests.get(url) # $ Alert[py/partial-ssrf] | ||
|
Comment on lines
134
to
136
|
||
|
|
||
| def partial_ssrf_6(): | ||
| user_input = request.args['untrusted_input'] | ||
|
|
||
| url = f"https://example.com/foo#{user_input}" | ||
| requests.get(url) # NOT OK -- user only controlled fragment | ||
| # NOT OK -- user only controlled fragment | ||
| requests.get(url) # $ Alert[py/partial-ssrf] | ||
|
|
||
| def partial_ssrf_7(): | ||
| user_input = request.args['untrusted_input'] | ||
|
|
||
| if user_input.isalnum(): | ||
| url = f"https://example.com/foo#{user_input}" | ||
| requests.get(url) # OK - user input can only contain alphanumerical characters | ||
| requests.get(url) # OK - user input can only contain alphanumerical characters | ||
|
|
||
| if user_input.isalpha(): | ||
| url = f"https://example.com/foo#{user_input}" | ||
|
|
@@ -154,7 +175,8 @@ def partial_ssrf_7(): | |
|
|
||
| if re.fullmatch(r'.*[a-zA-Z0-9]+.*', user_input): | ||
| url = f"https://example.com/foo#{user_input}" | ||
| requests.get(url) # NOT OK, but NOT FOUND - user input can contain arbitrary characters | ||
| # NOT OK, but NOT FOUND - user input can contain arbitrary characters | ||
| requests.get(url) # $ MISSING: Alert[py/partial-ssrf] | ||
|
|
||
|
|
||
| if re.match(r'^[a-zA-Z0-9]+$', user_input): | ||
|
|
@@ -163,7 +185,8 @@ def partial_ssrf_7(): | |
|
|
||
| if re.match(r'[a-zA-Z0-9]+', user_input): | ||
| url = f"https://example.com/foo#{user_input}" | ||
| requests.get(url) # NOT OK, but NOT FOUND - user input can contain arbitrary character as a suffix. | ||
| # NOT OK, but NOT FOUND - user input can contain arbitrary character as a suffix. | ||
| requests.get(url) # $ MISSING: Alert[py/partial-ssrf] | ||
|
|
||
| reg = re.compile(r'^[a-zA-Z0-9]+$') | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this makes sense to put in the change notes, as it isn't really a user-facing change. I don't have strong feelings about it, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.