Skip to content
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

Rust: Add some flow source models #18069

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions rust/ql/lib/codeql/rust/Concepts.qll
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

private import codeql.rust.dataflow.DataFlow
private import codeql.threatmodels.ThreatModels
private import codeql.rust.Frameworks

/**
* A data flow source for a specific threat-model.
Expand Down Expand Up @@ -46,6 +47,45 @@
ActiveThreatModelSource() { currentThreatModel(this.getThreatModel()) }
}

/**
* A data flow source corresponding to the program's command line arguments or path.
*/
final class CommandLineArgsSource = CommandLineArgsSource::Range;

module CommandLineArgsSource {

Check warning on line 55 in rust/ql/lib/codeql/rust/Concepts.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for module Concepts::CommandLineArgsSource
abstract class Range extends ThreatModelSource::Range {

Check warning on line 56 in rust/ql/lib/codeql/rust/Concepts.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for class Concepts::CommandLineArgsSource::Range
override string getThreatModel() { result = "commandargs" }

override string getSourceType() { result = "CommandLineArgs" }
}
}

/**
* A data flow source corresponding to the program's environment.
*/
final class EnvironmentSource = EnvironmentSource::Range;

module EnvironmentSource {

Check warning on line 68 in rust/ql/lib/codeql/rust/Concepts.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for module Concepts::EnvironmentSource
abstract class Range extends ThreatModelSource::Range {

Check warning on line 69 in rust/ql/lib/codeql/rust/Concepts.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for class Concepts::EnvironmentSource::Range
override string getThreatModel() { result = "environment" }

override string getSourceType() { result = "EnvironmentSource" }
}
}

/**
* A data flow source for remote (network) data.
*/
final class RemoteSource = RemoteSource::Range;

module RemoteSource {

Check warning on line 81 in rust/ql/lib/codeql/rust/Concepts.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for module Concepts::RemoteSource
abstract class Range extends ThreatModelSource::Range {

Check warning on line 82 in rust/ql/lib/codeql/rust/Concepts.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for class Concepts::RemoteSource::Range
override string getThreatModel() { result = "remote" }

override string getSourceType() { result = "RemoteSource" }
}
}

/**
* A data-flow node that constructs a SQL statement.
*
Expand Down
6 changes: 6 additions & 0 deletions rust/ql/lib/codeql/rust/Frameworks.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* This file imports all models of frameworks and libraries.
*/

private import codeql.rust.frameworks.Reqwest
private import codeql.rust.frameworks.stdlib.Env
16 changes: 16 additions & 0 deletions rust/ql/lib/codeql/rust/frameworks/Reqwest.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* Provides modeling for the `reqwest` library.
*/

private import rust
private import codeql.rust.Concepts

/**
* A call to `reqwest::get` or `reqwest::blocking::get`.
*/
private class ReqwestGet extends RemoteSource::Range {
ReqwestGet() {
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved
this.asExpr().getExpr().(CallExpr).getExpr().(PathExpr).getPath().getResolvedPath() =
["crate::get", "crate::blocking::get"]
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved
}
}
33 changes: 33 additions & 0 deletions rust/ql/lib/codeql/rust/frameworks/stdlib/Env.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Provides modeling for the `std::env` library.
*/

private import rust
private import codeql.rust.Concepts

/**
* A call to `std::env::args` or `std::env::args_os`.
*/
private class StdEnvArgs extends CommandLineArgsSource::Range {
StdEnvArgs() {
this.asExpr().getExpr().(CallExpr).getExpr().(PathExpr).getPath().getResolvedPath() = ["crate::env::args", "crate::env::args_os"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redsun82 these paths don't look right - I assume the correct path would be something like std::env::args or crate::std::env::args? They do match both what we get in the tests and what we see in real databases at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(fixing this will probably be follow-up work, what we have works well enough right now and is needed to get queries working)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, this might have to do with the prelude, I'll have a look

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, no, currently that is expected.

The thing is that a path only makes sense in the context of a crate (there is no such thing as a global concept of path across crates). So that path is the correct one relative to the std crate.

The other piece of information is the (highly non-standard and prone to be changed) getCrateOrigin, which gives lang:std for the std lib.

we could theoretically combine the two, it's just that while I'm quite confident of the correctness of the resolved path (up to the fact that it may not cover all entities yet), I'm still unsure about the effectiveness of getCrateOrigin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. It sounds like we could check getCrateOrigin for models where we're worried about ambiguity ("crate::get" makes me nervous), but perhaps not check it for cases where we aren't (e.g. "crate::env::vars") since that will be more robust if getCrateOrigin is potentially prone to change. Alternatively we could fall back on other details such as types to ensure our models are matching the right things - to the extent that we extract enough information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've push a commit adding the restriction I described above.

I've also added an agenda item next week to discuss crate origins, how we can get reliable information and how they will interact with models-as-data.

}
}

/**
* A call to `std::env::current_dir`, `std::env::current_exe` or `std::env::home_dir`.
*/
private class StdEnvDir extends CommandLineArgsSource::Range {
StdEnvDir() {
this.asExpr().getExpr().(CallExpr).getExpr().(PathExpr).getPath().getResolvedPath() = ["crate::env::current_dir", "crate::env::current_exe", "crate::env::home_dir"]
}
}

/**
* A call to `std::env::var`, `std::env::var_os`, `std::env::vars` or `std::env::vars_os`.
*/
private class StdEnvVar extends EnvironmentSource::Range {
StdEnvVar() {
this.asExpr().getExpr().(CallExpr).getExpr().(PathExpr).getPath().getResolvedPath() = ["crate::env::var", "crate::env::var_os", "crate::env::vars", "crate::env::vars_os"]
}
}
5 changes: 5 additions & 0 deletions rust/ql/src/queries/summary/SummaryStats.ql
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import rust
import codeql.rust.Concepts
import codeql.rust.Diagnostics
import Stats

Expand Down Expand Up @@ -37,4 +38,8 @@ where
key = "Inconsistencies - CFG" and value = getTotalCfgInconsistencies()
or
key = "Inconsistencies - data flow" and value = getTotalDataFlowInconsistencies()
or
key = "Taint sources - total" and value = count(ThreatModelSource s)
or
key = "Taint sources - active" and value = count(ActiveThreatModelSource s)
select key, value
17 changes: 17 additions & 0 deletions rust/ql/src/queries/summary/TaintSources.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* @name Taint Sources
* @description List all sources of untrusted input that have been idenfitied
* in the database.
* @kind problem
* @problem.severity info
* @id rust/summary/taint-sources
* @tags summary
*/

import rust
import codeql.rust.Concepts

from ThreatModelSource s, string defaultString
where
if s instanceof ActiveThreatModelSource then defaultString = ", DEFAULT" else defaultString = ""
select s, s.getSourceType() + " (" + s.getThreatModel() + defaultString + ")"
Fixed Show fixed Hide fixed
Empty file.
21 changes: 21 additions & 0 deletions rust/ql/test/library-tests/dataflow/sources/InlineFlow.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import rust
import codeql.rust.dataflow.DataFlow
import codeql.rust.Concepts
import utils.InlineFlowTest

/**
* Configuration for flow from any threat model source to an argument of a function called `sink`.
*/
module MyFlowConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof ThreatModelSource }

predicate isSink(DataFlow::Node sink) {
any(CallExpr call | call.getExpr().(PathExpr).getPath().toString() = "sink")
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved
.getArgList()
.getAnArg() = sink.asExpr().getExpr()
}
}

module MyFlowTest = TaintFlowTest<MyFlowConfig>;

import MyFlowTest
17 changes: 17 additions & 0 deletions rust/ql/test/library-tests/dataflow/sources/TaintSources.expected
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
| test.rs:8:10:8:30 | CallExpr | EnvironmentSource (environment) |
| test.rs:9:10:9:33 | CallExpr | EnvironmentSource (environment) |
| test.rs:11:16:11:36 | CallExpr | EnvironmentSource (environment) |
| test.rs:12:16:12:39 | CallExpr | EnvironmentSource (environment) |
| test.rs:17:25:17:40 | CallExpr | EnvironmentSource (environment) |
| test.rs:22:25:22:43 | CallExpr | EnvironmentSource (environment) |
| test.rs:29:29:29:44 | CallExpr | CommandLineArgs (commandargs) |
| test.rs:32:16:32:31 | CallExpr | CommandLineArgs (commandargs) |
| test.rs:33:16:33:34 | CallExpr | CommandLineArgs (commandargs) |
| test.rs:40:16:40:31 | CallExpr | CommandLineArgs (commandargs) |
| test.rs:44:16:44:34 | CallExpr | CommandLineArgs (commandargs) |
| test.rs:50:15:50:37 | CallExpr | CommandLineArgs (commandargs) |
| test.rs:51:15:51:37 | CallExpr | CommandLineArgs (commandargs) |
| test.rs:52:16:52:35 | CallExpr | CommandLineArgs (commandargs) |
| test.rs:60:26:60:70 | CallExpr | RemoteSource (remote, DEFAULT) |
| test.rs:63:26:63:70 | CallExpr | RemoteSource (remote, DEFAULT) |
| test.rs:66:26:66:60 | CallExpr | RemoteSource (remote, DEFAULT) |
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: queries/summary/TaintSources.ql
postprocess: utils/InlineExpectationsTestQuery.ql
3 changes: 3 additions & 0 deletions rust/ql/test/library-tests/dataflow/sources/options.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
qltest_cargo_check: true
qltest_dependencies:
- reqwest = { version = "0.12.9", features = ["blocking"] }
36 changes: 36 additions & 0 deletions rust/ql/test/library-tests/dataflow/sources/reqwest.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@

// --- stubs for the "reqwest" library ---

/*
--- we don't seem to have a way to use this, hence we currently test against the real reqwest library
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redsun82 I tried to stub reqwest, it has a much simpler interface than sqlx. But I ran into a problem that it's not possible (I think) to emulate the correct paths for the library and import it into a test.rs at the moment. When we can, we can switch to a stubbing approach for reqwest at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(fixing this will probably be follow-up work, what we have works well enough right now)

#[derive(Debug)]
pub struct Error { }
pub mod blocking {
pub struct Response { }
impl Response {
pub fn text(self) -> Result<String, super::Error> {
Ok("".to_string())
}
}
pub fn get<T>(url: T) -> Result<Response, super::Error> {
let _url = url;
Ok(Response {})
}
}
pub struct Response { }
impl Response {
pub async fn text(self) -> Result<String, Error> {
Ok("".to_string())
}
}
pub async fn get<T>(url: T) -> Result<Response, Error> {
let _url = url;
Ok(Response {})
}
*/
70 changes: 70 additions & 0 deletions rust/ql/test/library-tests/dataflow/sources/test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#![allow(deprecated)]

fn sink<T>(_: T) { }

// --- tests ---

fn test_env_vars() {
sink(std::env::var("HOME")); // $ Alert[rust/summary/taint-sources] hasTaintFlow
sink(std::env::var_os("PATH")); // $ Alert[rust/summary/taint-sources] hasTaintFlow

let var1 = std::env::var("HOME").expect("HOME not set"); // $ Alert[rust/summary/taint-sources]
let var2 = std::env::var_os("PATH").unwrap(); // $ Alert[rust/summary/taint-sources]

sink(var1); // $ MISSING: hasTaintFlow
sink(var2); // $ MISSING: hasTaintFlow
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved

for (key, value) in std::env::vars() { // $ Alert[rust/summary/taint-sources]
sink(key); // $ MISSING: hasTaintFlow
sink(value); // $ MISSING: hasTaintFlow
}

for (key, value) in std::env::vars_os() { // $ Alert[rust/summary/taint-sources]
sink(key); // $ MISSING: hasTaintFlow
sink(value); // $ MISSING: hasTaintFlow
}
}

fn test_env_args() {
let args: Vec<String> = std::env::args().collect(); // $ Alert[rust/summary/taint-sources]
let my_path = &args[0];
let arg1 = &args[1];
let arg2 = std::env::args().nth(2).unwrap(); // $ Alert[rust/summary/taint-sources]
let arg3 = std::env::args_os().nth(3).unwrap(); // $ Alert[rust/summary/taint-sources]

sink(my_path); // $ MISSING: hasTaintFlow
sink(arg1); // $ MISSING: hasTaintFlow
sink(arg2); // $ MISSING: hasTaintFlow
sink(arg3); // $ MISSING: hasTaintFlow

for arg in std::env::args() { // $ Alert[rust/summary/taint-sources]
sink(arg); // $ MISSING: hasTaintFlow
}

for arg in std::env::args_os() { // $ Alert[rust/summary/taint-sources]
sink(arg); // $ MISSING: hasTaintFlow
}
}

fn test_env_dirs() {
let dir = std::env::current_dir().expect("FAILED"); // $ Alert[rust/summary/taint-sources]
let exe = std::env::current_exe().expect("FAILED"); // $ Alert[rust/summary/taint-sources]
let home = std::env::home_dir().expect("FAILED"); // $ Alert[rust/summary/taint-sources]

sink(dir); // $ MISSING: hasTaintFlow
sink(exe); // $ MISSING: hasTaintFlow
sink(home); // $ MISSING: hasTaintFlow
}

async fn test_reqwest() -> Result<(), reqwest::Error> {
let remote_string1 = reqwest::blocking::get("http://example.com/")?.text()?; // $ Alert[rust/summary/taint-sources]
sink(remote_string1); // $ MISSING: hasTaintFlow

let remote_string2 = reqwest::blocking::get("http://example.com/").unwrap().text().unwrap(); // $ Alert[rust/summary/taint-sources]
sink(remote_string2); // $ MISSING: hasTaintFlow

let remote_string3 = reqwest::get("http://example.com/").await?.text().await?; // $ Alert[rust/summary/taint-sources]
sink(remote_string3); // $ MISSING: hasTaintFlow

Ok(())
}
2 changes: 2 additions & 0 deletions rust/ql/test/query-tests/diagnostics/SummaryStats.expected
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@
| Inconsistencies - data flow | 0 |
| Lines of code extracted | 59 |
| Lines of user code extracted | 59 |
| Taint sources - active | 0 |
| Taint sources - total | 0 |
Loading