Skip to content

Commit 011739c

Browse files
committed
Merge branch 'main' into use-shared-guards-library
2 parents b0e9238 + 6eb2aad commit 011739c

File tree

18 files changed

+446
-120
lines changed

18 files changed

+446
-120
lines changed
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
.. _basic-query-for-rust-code:
2+
3+
Basic query for Rust code
4+
==========================
5+
6+
Learn to write and run a simple CodeQL query using Visual Studio Code with the CodeQL extension.
7+
8+
.. include:: ../reusables/vs-code-basic-instructions/setup-to-run-queries.rst
9+
10+
About the query
11+
---------------
12+
13+
The query we're going to run performs a basic search of the code for ``if`` expressions that are redundant, in the sense that they have an empty ``then`` branch. For example, code such as:
14+
15+
.. code-block:: rust
16+
17+
if error {
18+
// we should handle the error
19+
}
20+
21+
.. include:: ../reusables/vs-code-basic-instructions/find-database.rst
22+
23+
Running a quick query
24+
---------------------
25+
26+
.. include:: ../reusables/vs-code-basic-instructions/run-quick-query-1.rst
27+
28+
#. In the quick query tab, delete the content and paste in the following query.
29+
30+
.. code-block:: ql
31+
32+
import rust
33+
34+
from IfExpr ifExpr
35+
where ifExpr.getThen().(BlockExpr).getStmtList().getNumberOfStmtOrExpr() = 0
36+
select ifExpr, "This 'if' expression is redundant."
37+
38+
.. include:: ../reusables/vs-code-basic-instructions/run-quick-query-2.rst
39+
40+
.. image:: ../images/codeql-for-visual-studio-code/basic-rust-query-results-1.png
41+
:align: center
42+
43+
If any matching code is found, click a link in the ``ifExpr`` column to open the file and highlight the matching ``if`` expression.
44+
45+
.. image:: ../images/codeql-for-visual-studio-code/basic-rust-query-results-2.png
46+
:align: center
47+
48+
.. include:: ../reusables/vs-code-basic-instructions/note-store-quick-query.rst
49+
50+
About the query structure
51+
~~~~~~~~~~~~~~~~~~~~~~~~~
52+
53+
After the initial ``import`` statement, this simple query comprises three parts that serve similar purposes to the FROM, WHERE, and SELECT parts of an SQL query.
54+
55+
+----------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------+
56+
| Query part | Purpose | Details |
57+
+==================================================================================+===================================================================================================================+======================================================================================================+
58+
| ``import rust`` | Imports the standard CodeQL AST libraries for Rust. | Every query begins with one or more ``import`` statements. |
59+
+----------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------+
60+
| ``from IfExpr ifExpr`` | Defines the variables for the query. | We use: an ``IfExpr`` variable for ``if`` expressions. |
61+
| | Declarations are of the form: | |
62+
| | ``<type> <variable name>`` | |
63+
+----------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------+
64+
| ``where ifExpr.getThen().(BlockExpr).getStmtList().getNumberOfStmtOrExpr() = 0`` | Defines a condition on the variables. | ``ifExpr.getThen()``: gets the ``then`` branch of the ``if`` expression. |
65+
| | | ``.(BlockExpr)``: requires that the ``then`` branch is a block expression (``{ }``). |
66+
| | | ``.getStmtList()``: gets the list of things in the block. |
67+
| | | ``.getNumberOfStmtOrExpr() = 0``: requires that there are no statements or expressions in the block. |
68+
+----------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------+
69+
| ``select ifExpr, "This 'if' expression is redundant."`` | Defines what to report for each match. | Reports the resulting ``if`` expression with a string that explains the problem. |
70+
| | | |
71+
| | ``select`` statements for queries that are used to find instances of poor coding practice are always in the form: | |
72+
| | ``select <program element>, "<alert message>"`` | |
73+
+----------------------------------------------------------------------------------+-------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------+
74+
75+
Extend the query
76+
----------------
77+
78+
Query writing is an inherently iterative process. You write a simple query and then, when you run it, you discover examples that you had not previously considered, or opportunities for improvement.
79+
80+
Remove false positive results
81+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
82+
83+
Browsing the results of our basic query shows that it could be improved. Among the results you are likely to find examples of ``if`` expressions with an ``else`` branch, where an empty ``then`` branch does serve a purpose. For example:
84+
85+
.. code-block:: rust
86+
87+
if (option == "-verbose") {
88+
// nothing to do - handled earlier
89+
} else {
90+
handleError("unrecognized option")
91+
}
92+
93+
In this case, identifying the ``if`` expression with the empty ``then`` branch as redundant is a false positive. One solution to this is to modify the query to select ``if`` expressions where both the ``then`` and ``else`` branches are missing.
94+
95+
To exclude ``if`` expressions that have an ``else`` branch:
96+
97+
#. Add the following to the where clause:
98+
99+
.. code-block:: ql
100+
101+
and not exists(ifExpr.getElse())
102+
103+
The ``where`` clause is now:
104+
105+
.. code-block:: ql
106+
107+
where
108+
ifExpr.getThen().(BlockExpr).getStmtList().getNumberOfStmtOrExpr() = 0 and
109+
not exists(ifExpr.getElse())
110+
111+
#. Re-run the query.
112+
113+
There are now fewer results because ``if`` expressions with an ``else`` branch are no longer included.
114+
115+
Further reading
116+
---------------
117+
118+
.. include:: ../reusables/rust-further-reading.rst
119+
.. include:: ../reusables/codeql-ref-tools-further-reading.rst
120+
121+
.. Article-specific substitutions for the reusables used in docs/codeql/reusables/vs-code-basic-instructions
122+
123+
.. |language-text| replace:: Rust
124+
125+
.. |language-code| replace:: ``rust``
126+
127+
.. |example-url| replace:: https://github.com/rust-lang/rustlings
128+
129+
.. |image-quick-query| image:: ../images/codeql-for-visual-studio-code/quick-query-tab-rust.png
130+
131+
.. |result-col-1| replace:: The first column corresponds to the expression ``ifExpr`` and is linked to the location in the source code of the project where ``ifExpr`` occurs.

docs/codeql/codeql-language-guides/codeql-for-rust.rst

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,12 @@ Experiment and learn how to write effective and efficient queries for CodeQL dat
99
.. toctree::
1010
:hidden:
1111

12+
basic-query-for-rust-code
1213
codeql-library-for-rust
1314
analyzing-data-flow-in-rust
1415

16+
- :doc:`Basic query for Rust code <basic-query-for-rust-code>`: Learn to write and run a simple CodeQL query.
17+
1518
- :doc:`CodeQL library for Rust <codeql-library-for-rust>`: When analyzing Rust code, you can make use of the large collection of classes in the CodeQL library for Rust.
19+
1620
- :doc:`Analyzing data flow in Rust <analyzing-data-flow-in-rust>`: You can use CodeQL to track the flow of data through a Rust program to places where the data is used.
32.5 KB
Loading
32.3 KB
Loading
9.1 KB
Loading
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* `go/unvalidated-url-redirection` and `go/request-forgery` have a shared notion of a safe URL, which is known to not be malicious. Some URLs which were incorrectly considered safe are now correctly considered unsafe. This may lead to more alerts for those two queries.

go/ql/lib/semmle/go/security/OpenUrlRedirectCustomizations.qll

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
import go
88
import UrlConcatenation
9-
import SafeUrlFlowCustomizations
9+
private import SafeUrlFlowCustomizations
1010
import semmle.go.dataflow.barrierguardutil.RedirectCheckBarrierGuard
1111
import semmle.go.dataflow.barrierguardutil.RegexpCheck
1212
import semmle.go.dataflow.barrierguardutil.UrlCheck
@@ -121,21 +121,6 @@ module OpenUrlRedirect {
121121
/** A sink for an open redirect, considered as a sink for safe URL flow. */
122122
private class SafeUrlSink extends SafeUrlFlow::Sink instanceof OpenUrlRedirect::Sink { }
123123

124-
/**
125-
* A read of a field considered unsafe to redirect to, considered as a sanitizer for a safe
126-
* URL.
127-
*/
128-
private class UnsafeFieldReadSanitizer extends SafeUrlFlow::SanitizerEdge {
129-
UnsafeFieldReadSanitizer() {
130-
exists(DataFlow::FieldReadNode frn, string name |
131-
name = ["User", "RawQuery", "Fragment"] and
132-
frn.getField().hasQualifiedName("net/url", "URL")
133-
|
134-
this = frn.getBase()
135-
)
136-
}
137-
}
138-
139124
/**
140125
* Reinstate the usual field propagation rules for fields, which the OpenURLRedirect
141126
* query usually excludes, for fields of `Params` other than `Params.Fixed`.

go/ql/lib/semmle/go/security/RequestForgeryCustomizations.qll

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
import go
66
import UrlConcatenation
7-
import SafeUrlFlowCustomizations
7+
private import SafeUrlFlowCustomizations
88
import semmle.go.dataflow.barrierguardutil.RedirectCheckBarrierGuard
99
import semmle.go.dataflow.barrierguardutil.RegexpCheck
1010
import semmle.go.dataflow.barrierguardutil.UrlCheck
@@ -118,18 +118,3 @@ module RequestForgery {
118118

119119
/** A sink for request forgery, considered as a sink for safe URL flow. */
120120
private class SafeUrlSink extends SafeUrlFlow::Sink instanceof RequestForgery::Sink { }
121-
122-
/**
123-
* A read of a field considered unsafe for request forgery, considered as a sanitizer for a safe
124-
* URL.
125-
*/
126-
private class UnsafeFieldReadSanitizer extends SafeUrlFlow::SanitizerEdge {
127-
UnsafeFieldReadSanitizer() {
128-
exists(DataFlow::FieldReadNode frn, string name |
129-
(name = "RawQuery" or name = "Fragment" or name = "User") and
130-
frn.getField().hasQualifiedName("net/url", "URL")
131-
|
132-
this = frn.getBase()
133-
)
134-
}
135-
}

go/ql/lib/semmle/go/security/SafeUrlFlow.qll

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,10 @@ module SafeUrlFlow {
3030

3131
predicate isBarrierOut(DataFlow::Node node) {
3232
// block propagation of this safe value when its host is overwritten
33-
exists(Write w, Field f | f.hasQualifiedName("net/url", "URL", "Host") |
34-
w.writesField(node.getASuccessor(), f, _)
33+
exists(Write w, DataFlow::Node b, Field f |
34+
f.hasQualifiedName("net/url", "URL", "Host") and
35+
b = node.getASuccessor() and
36+
w.writesField(b, f, _)
3537
)
3638
or
3739
node instanceof SanitizerEdge

go/ql/lib/semmle/go/security/SafeUrlFlowCustomizations.qll

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,4 +40,19 @@ module SafeUrlFlow {
4040
private class StringSlicingEdge extends SanitizerEdge {
4141
StringSlicingEdge() { this = any(DataFlow::SliceNode sn) }
4242
}
43+
44+
/**
45+
* A read of a field considered unsafe to redirect to, considered as a sanitizer for a safe
46+
* URL.
47+
*/
48+
private class UnsafeFieldReadSanitizer extends SanitizerEdge {
49+
UnsafeFieldReadSanitizer() {
50+
exists(DataFlow::FieldReadNode frn, string name |
51+
name = ["Fragment", "RawQuery", "User"] and
52+
frn.getField().hasQualifiedName("net/url", "URL", name)
53+
|
54+
this = frn.getBase()
55+
)
56+
}
57+
}
4358
}

0 commit comments

Comments
 (0)