Skip to content

Commit d196704

Browse files
henrymercerhenrymercer
authored
Merge pull request #11574 from github/henrymercer/check-query-ids
Add a PR check to ensure query IDs are unique
2 parents 2ab05a8 + 3036b15 commit d196704

File tree

8 files changed

+121
-3
lines changed

8 files changed

+121
-3
lines changed

‎.github/workflows/check-query-ids.yml

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
name: Check query IDs
2+
3+
on:
4+
pull_request:
5+
paths:
6+
- "**/src/**/*.ql"
7+
- misc/scripts/check-query-ids.py
8+
- .github/workflows/check-query-ids.yml
9+
branches:
10+
- main
11+
- "rc/*"
12+
workflow_dispatch:
13+
14+
jobs:
15+
check:
16+
name: Check query IDs
17+
runs-on: ubuntu-latest
18+
steps:
19+
- uses: actions/checkout@v3
20+
- name: Check for duplicate query IDs
21+
run: python3 misc/scripts/check-query-ids.py

‎config/identical-files.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -470,6 +470,10 @@
470470
"javascript/ql/src/Comments/CommentedOutCodeReferences.inc.qhelp",
471471
"python/ql/src/Lexical/CommentedOutCodeReferences.inc.qhelp"
472472
],
473+
"ThreadResourceAbuse qhelp": [
474+
"java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.qhelp",
475+
"java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qhelp"
476+
],
473477
"IDE Contextual Queries": [
474478
"cpp/ql/lib/IDEContextual.qll",
475479
"csharp/ql/lib/IDEContextual.qll",

‎java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.qhelp

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
7+
<overview>
8+
<p>The <code>Thread.sleep</code> method is used to pause the execution of current thread for
9+
specified time. When the sleep time is user-controlled, especially in the web application context,
10+
it can be abused to cause all of a server's threads to sleep, leading to denial of service.</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>To guard against this attack, consider specifying an upper range of allowed sleep time or adopting
15+
the producer/consumer design pattern with <code>Object.wait</code> method to avoid performance
16+
problems or even resource exhaustion. For more information, refer to the concurrency tutorial of Oracle
17+
listed below or <code>java/ql/src/Likely Bugs/Concurrency</code> queries of CodeQL.</p>
18+
</recommendation>
19+
20+
<example>
21+
<p>The following example shows a bad situation and a good situation respectively. In the bad situation,
22+
a thread sleep time comes directly from user input. In the good situation, an upper
23+
range check on the maximum sleep time allowed is enforced.</p>
24+
<sample src="ThreadResourceAbuse.java" />
25+
</example>
26+
27+
<references>
28+
<li>
29+
Snyk:
30+
<a href="https://snyk.io/vuln/SNYK-JAVA-COMGOOGLECODEGWTUPLOAD-569506">Denial of Service (DoS)
31+
in com.googlecode.gwtupload:gwtupload</a>.
32+
</li>
33+
<li>
34+
gwtupload:
35+
<a href="https://github.com/manolo/gwtupload/issues/33">[Fix DOS issue] Updating the
36+
AbstractUploadListener.java file</a>.
37+
</li>
38+
<li>
39+
The blog of a gypsy engineer:
40+
<a href="https://blog.gypsyengineer.com/en/security/cve-2019-17555-dos-via-retry-after-header-in-apache-olingo.html">
41+
CVE-2019-17555: DoS via Retry-After header in Apache Olingo</a>.
42+
</li>
43+
<li>
44+
Oracle:
45+
<a href="https://docs.oracle.com/javase/tutorial/essential/concurrency/guardmeth.html">The Java Concurrency Tutorials</a>
46+
</li>
47+
</references>
48+
</qhelp>

‎java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* @description Using user input directly to control a thread's sleep time could lead to
44
* performance problems or even resource exhaustion.
55
* @kind path-problem
6-
* @id java/thread-resource-abuse
6+
* @id java/local-thread-resource-abuse
77
* @problem.severity recommendation
88
* @tags security
99
* external/cwe/cwe-400

‎misc/scripts/check-query-ids.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
from pathlib import Path
2+
import re
3+
import sys
4+
from typing import Dict, List, Optional
5+
6+
ID = re.compile(r" +\* +@id\s*(.*)")
7+
8+
def get_query_id(query_path: Path) -> Optional[str]:
9+
with open(query_path) as f:
10+
for line in f:
11+
m = ID.match(line)
12+
if m:
13+
return m.group(1)
14+
return None
15+
16+
def main():
17+
# Map query IDs to paths of queries with those IDs. We want to check that this is a 1:1 map.
18+
query_ids: Dict[str, List[str]] = {}
19+
20+
# Just check src folders for now to avoid churn
21+
for query_path in Path().glob("**/src/**/*.ql"):
22+
# Skip compiled query packs
23+
if any(p == ".codeql" for p in query_path.parts):
24+
continue
25+
query_id = get_query_id(query_path)
26+
if query_id is not None:
27+
query_ids.setdefault(query_id, []).append(str(query_path))
28+
29+
fail = False
30+
for query_id, query_paths in query_ids.items():
31+
if len(query_paths) > 1:
32+
fail = True
33+
print(f"Query ID {query_id} is used in multiple queries:")
34+
for query_path in query_paths:
35+
print(f" - {query_path}")
36+
37+
if fail:
38+
print("FAIL: duplicate query IDs found in src folders. Please assign these queries unique IDs.")
39+
sys.exit(1)
40+
else:
41+
print("PASS: no duplicate query IDs found in src folders.")
42+
43+
if __name__ == "__main__":
44+
main()

‎python/ql/src/experimental/Security/CWE-022bis/TarSlip.qhelp renamed to ‎python/ql/src/experimental/Security/CWE-022bis/TarSlipImprov.qhelp.disabled

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
<!DOCTYPE qhelp PUBLIC
22
"-//Semmle//qhelp//EN"
33
"qhelp.dtd">
4+
<!-- Disabled since it refers to examples which do not exist. -->
45
<qhelp>
56

67
<overview>

‎python/ql/src/experimental/Security/CWE-022bis/TarSlipImprov.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* destination file path is within the destination directory can cause files outside
55
* the destination directory to be overwritten.
66
* @kind path-problem
7-
* @id py/tarslip
7+
* @id py/tarslip-extended
88
* @problem.severity error
99
* @security-severity 7.5
1010
* @precision high

‎python/ql/src/experimental/Security/CWE-079/ReflectedXSS.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* @problem.severity error
77
* @security-severity 2.9
88
* @sub-severity high
9-
* @id py/reflective-xss
9+
* @id py/reflective-xss-email
1010
* @tags security
1111
* external/cwe/cwe-079
1212
* external/cwe/cwe-116

0 commit comments

Comments
 (0)