Merge pull request #11574 from github/henrymercer/check-query-ids

Add a PR check to ensure query IDs are unique

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

@@ -0,0 +1,21 @@
+name: Check query IDs
+
+on:
+  pull_request:
+    paths:
+      - "**/src/**/*.ql"
+      - misc/scripts/check-query-ids.py
+      - .github/workflows/check-query-ids.yml
+    branches:
+      - main
+      - "rc/*"
+  workflow_dispatch:
+
+jobs:
+  check:
+    name: Check query IDs
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Check for duplicate query IDs
+        run: python3 misc/scripts/check-query-ids.py

config/identical-files.json

@@ -470,6 +470,10 @@
     "javascript/ql/src/Comments/CommentedOutCodeReferences.inc.qhelp",
     "python/ql/src/Lexical/CommentedOutCodeReferences.inc.qhelp"
   ],
+  "ThreadResourceAbuse qhelp": [
+    "java/ql/src/experimental/Security/CWE/CWE-400/LocalThreadResourceAbuse.qhelp",
+    "java/ql/src/experimental/Security/CWE/CWE-400/ThreadResourceAbuse.qhelp"
+  ],
   "IDE Contextual Queries": [
     "cpp/ql/lib/IDEContextual.qll",
     "csharp/ql/lib/IDEContextual.qll",

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

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>The <code>Thread.sleep</code> method is used to pause the execution of current thread for 
+specified time. When the sleep time is user-controlled, especially in the web application context, 
+it can be abused to cause all of a server's threads to sleep, leading to denial of service.</p>
+</overview>
+
+<recommendation>
+<p>To guard against this attack, consider specifying an upper range of allowed sleep time or adopting
+the producer/consumer design pattern with <code>Object.wait</code> method to avoid performance 
+problems or even resource exhaustion. For more information, refer to the concurrency tutorial of Oracle
+listed below or <code>java/ql/src/Likely Bugs/Concurrency</code> queries of CodeQL.</p>
+</recommendation>
+
+<example>
+<p>The following example shows a bad situation and a good situation respectively. In the bad situation,
+a thread sleep time comes directly from user input. In the good situation, an upper 
+range check on the maximum sleep time allowed is enforced.</p>
+<sample src="ThreadResourceAbuse.java" />
+</example>
+
+<references>
+<li>
+Snyk:
+<a href="https://snyk.io/vuln/SNYK-JAVA-COMGOOGLECODEGWTUPLOAD-569506">Denial of Service (DoS)
+in com.googlecode.gwtupload:gwtupload</a>.
+</li>
+<li>
+gwtupload:
+<a href="https://github.com/manolo/gwtupload/issues/33">[Fix DOS issue] Updating the 
+AbstractUploadListener.java file</a>.
+</li>
+<li>
+The blog of a gypsy engineer:
+<a href="https://blog.gypsyengineer.com/en/security/cve-2019-17555-dos-via-retry-after-header-in-apache-olingo.html">
+CVE-2019-17555: DoS via Retry-After header in Apache Olingo</a>.
+</li>
+<li>
+Oracle:
+<a href="https://docs.oracle.com/javase/tutorial/essential/concurrency/guardmeth.html">The Java Concurrency Tutorials</a>
+</li>
+</references>
+</qhelp>

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

@@ -3,7 +3,7 @@
  * @description Using user input directly to control a thread's sleep time could lead to
  *              performance problems or even resource exhaustion.
  * @kind path-problem
- * @id java/thread-resource-abuse
+ * @id java/local-thread-resource-abuse
  * @problem.severity recommendation
  * @tags security
  *       external/cwe/cwe-400

misc/scripts/check-query-ids.py

@@ -0,0 +1,44 @@
+from pathlib import Path
+import re
+import sys
+from typing import Dict, List, Optional
+
+ID = re.compile(r" +\* +@id\s*(.*)")
+
+def get_query_id(query_path: Path) -> Optional[str]:
+    with open(query_path) as f:
+        for line in f:
+            m = ID.match(line)
+            if m:
+                return m.group(1)
+        return None
+
+def main():
+    # Map query IDs to paths of queries with those IDs. We want to check that this is a 1:1 map.
+    query_ids: Dict[str, List[str]] = {}
+
+    # Just check src folders for now to avoid churn
+    for query_path in Path().glob("**/src/**/*.ql"):
+        # Skip compiled query packs
+        if any(p == ".codeql" for p in query_path.parts):
+            continue
+        query_id = get_query_id(query_path)
+        if query_id is not None:
+            query_ids.setdefault(query_id, []).append(str(query_path))
+
+    fail = False
+    for query_id, query_paths in query_ids.items():
+        if len(query_paths) > 1:
+            fail = True
+            print(f"Query ID {query_id} is used in multiple queries:")
+            for query_path in query_paths:
+                print(f" - {query_path}")
+
+    if fail:
+        print("FAIL: duplicate query IDs found in src folders. Please assign these queries unique IDs.")
+        sys.exit(1)
+    else:
+        print("PASS: no duplicate query IDs found in src folders.")
+
+if __name__ == "__main__":
+    main()

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

@@ -1,6 +1,7 @@
 <!DOCTYPE qhelp PUBLIC
   "-//Semmle//qhelp//EN"
   "qhelp.dtd">
+<!-- Disabled since it refers to examples which do not exist. -->
 <qhelp>
 
 <overview>

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

@@ -4,7 +4,7 @@
  *              destination file path is within the destination directory can cause files outside
  *              the destination directory to be overwritten.
  * @kind path-problem
- * @id py/tarslip
+ * @id py/tarslip-extended
  * @problem.severity error
  * @security-severity 7.5
  * @precision high

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

@@ -6,7 +6,7 @@
  * @problem.severity error
  * @security-severity 2.9
  * @sub-severity high
- * @id py/reflective-xss
+ * @id py/reflective-xss-email
  * @tags security
  *       external/cwe/cwe-079
  *       external/cwe/cwe-116