Skip to content

ThreatModels: Add stdin kind #17209

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

Merged
merged 10 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from 8 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
6 changes: 3 additions & 3 deletions csharp/ql/lib/ext/System.model.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ extensions:
pack: codeql/csharp-all
extensible: sourceModel
data:
- ["System", "Console", False, "Read", "", "", "ReturnValue", "local", "manual"]
- ["System", "Console", False, "ReadKey", "", "", "ReturnValue", "local", "manual"]
- ["System", "Console", False, "ReadLine", "", "", "ReturnValue", "local", "manual"]
- ["System", "Console", False, "Read", "", "", "ReturnValue", "stdin", "manual"]
- ["System", "Console", False, "ReadKey", "", "", "ReturnValue", "stdin", "manual"]
- ["System", "Console", False, "ReadLine", "", "", "ReturnValue", "stdin", "manual"]
- ["System", "Environment", False, "ExpandEnvironmentVariables", "(System.String)", "", "ReturnValue", "environment", "manual"]
- ["System", "Environment", False, "GetCommandLineArgs", "()", "", "ReturnValue", "commandargs", "manual"]
- ["System", "Environment", False, "get_CommandLine", "()", "", "ReturnValue", "commandargs", "manual"]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,16 @@ abstract class WindowsRegistrySource extends LocalFlowSource {
private class ExternalWindowsRegistrySource extends WindowsRegistrySource {
ExternalWindowsRegistrySource() { sourceNode(this, "windows-registry") }
}

/**
* A dataflow source that represents the reading from stdin.
*/
abstract class StdinSource extends LocalFlowSource {
override string getThreatModel() { result = "stdin" }

override string getSourceType() { result = "read from stdin" }
}

private class ExternalStdinSource extends StdinSource {
ExternalStdinSource() { sourceNode(this, "stdin") }
}
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,10 @@ source
| System.IO;StreamWriter;StreamWriter;(System.String,System.IO.FileStreamOptions);Argument[this];file-write;manual |
| System.IO;StreamWriter;StreamWriter;(System.String,System.Text.Encoding,System.IO.FileStreamOptions);Argument[this];file-write;manual |
| System.Net.Sockets;TcpClient;GetStream;();ReturnValue;remote;manual |
| System;Console;Read;();ReturnValue;local;manual |
| System;Console;ReadKey;();ReturnValue;local;manual |
| System;Console;ReadKey;(System.Boolean);ReturnValue;local;manual |
| System;Console;ReadLine;();ReturnValue;local;manual |
| System;Console;Read;();ReturnValue;stdin;manual |
| System;Console;ReadKey;();ReturnValue;stdin;manual |
| System;Console;ReadKey;(System.Boolean);ReturnValue;stdin;manual |
| System;Console;ReadLine;();ReturnValue;stdin;manual |
| System;Environment;ExpandEnvironmentVariables;(System.String);ReturnValue;environment;manual |
| System;Environment;GetCommandLineArgs;();ReturnValue;commandargs;manual |
| System;Environment;GetEnvironmentVariable;(System.String);ReturnValue;environment;manual |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
| SqlInjection.cs:83:50:83:55 | access to local variable query1 | SqlInjection.cs:82:21:82:29 | access to property Text : String | SqlInjection.cs:83:50:83:55 | access to local variable query1 | This query depends on $@. | SqlInjection.cs:82:21:82:29 | access to property Text : String | this TextBox text |
| SqlInjection.cs:93:42:93:52 | access to local variable queryString | SqlInjection.cs:92:21:92:29 | access to property Text : String | SqlInjection.cs:93:42:93:52 | access to local variable queryString | This query depends on $@. | SqlInjection.cs:92:21:92:29 | access to property Text : String | this TextBox text |
| SqlInjection.cs:94:50:94:52 | access to local variable cmd | SqlInjection.cs:92:21:92:29 | access to property Text : String | SqlInjection.cs:94:50:94:52 | access to local variable cmd | This query depends on $@. | SqlInjection.cs:92:21:92:29 | access to property Text : String | this TextBox text |
| SqlInjection.cs:104:42:104:52 | access to local variable queryString | SqlInjection.cs:103:21:103:38 | call to method ReadLine : String | SqlInjection.cs:104:42:104:52 | access to local variable queryString | This query depends on $@. | SqlInjection.cs:103:21:103:38 | call to method ReadLine : String | this external |
| SqlInjection.cs:105:50:105:52 | access to local variable cmd | SqlInjection.cs:103:21:103:38 | call to method ReadLine : String | SqlInjection.cs:105:50:105:52 | access to local variable cmd | This query depends on $@. | SqlInjection.cs:103:21:103:38 | call to method ReadLine : String | this external |
| SqlInjection.cs:104:42:104:52 | access to local variable queryString | SqlInjection.cs:103:21:103:38 | call to method ReadLine : String | SqlInjection.cs:104:42:104:52 | access to local variable queryString | This query depends on $@. | SqlInjection.cs:103:21:103:38 | call to method ReadLine : String | this read from stdin |
| SqlInjection.cs:105:50:105:52 | access to local variable cmd | SqlInjection.cs:103:21:103:38 | call to method ReadLine : String | SqlInjection.cs:105:50:105:52 | access to local variable cmd | This query depends on $@. | SqlInjection.cs:103:21:103:38 | call to method ReadLine : String | this read from stdin |
| SqlInjectionDapper.cs:21:55:21:59 | access to local variable query | SqlInjectionDapper.cs:20:86:20:94 | access to property Text : String | SqlInjectionDapper.cs:21:55:21:59 | access to local variable query | This query depends on $@. | SqlInjectionDapper.cs:20:86:20:94 | access to property Text : String | this TextBox text |
| SqlInjectionDapper.cs:30:66:30:70 | access to local variable query | SqlInjectionDapper.cs:29:86:29:94 | access to property Text : String | SqlInjectionDapper.cs:30:66:30:70 | access to local variable query | This query depends on $@. | SqlInjectionDapper.cs:29:86:29:94 | access to property Text : String | this TextBox text |
| SqlInjectionDapper.cs:39:63:39:67 | access to local variable query | SqlInjectionDapper.cs:38:86:38:94 | access to property Text : String | SqlInjectionDapper.cs:39:63:39:67 | access to local variable query | This query depends on $@. | SqlInjectionDapper.cs:38:86:38:94 | access to property Text : String | this TextBox text |
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#select
| ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | ConsoleUncontrolledFormatString.cs:8:22:8:39 | call to method ReadLine : String | ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | This format string depends on $@. | ConsoleUncontrolledFormatString.cs:8:22:8:39 | call to method ReadLine | thisexternal |
| ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | ConsoleUncontrolledFormatString.cs:8:22:8:39 | call to method ReadLine : String | ConsoleUncontrolledFormatString.cs:11:31:11:36 | access to local variable format | This format string depends on $@. | ConsoleUncontrolledFormatString.cs:8:22:8:39 | call to method ReadLine | thisread from stdin |
| UncontrolledFormatString.cs:12:23:12:26 | access to local variable path | UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:12:23:12:26 | access to local variable path | This format string depends on $@. | UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString | thisASP.NET query string |
| UncontrolledFormatString.cs:15:46:15:49 | access to local variable path | UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString : NameValueCollection | UncontrolledFormatString.cs:15:46:15:49 | access to local variable path | This format string depends on $@. | UncontrolledFormatString.cs:9:23:9:45 | access to property QueryString | thisASP.NET query string |
| UncontrolledFormatString.cs:32:23:32:31 | access to property Text | UncontrolledFormatString.cs:32:23:32:31 | access to property Text | UncontrolledFormatString.cs:32:23:32:31 | access to property Text | This format string depends on $@. | UncontrolledFormatString.cs:32:23:32:31 | access to property Text | thisTextBox text |
Expand Down
6 changes: 3 additions & 3 deletions csharp/ql/test/utils/modelgenerator/dataflow/Sources.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@ public class NewSources


// New source
// source=Sources;NewSources;false;WrapConsoleReadLine;();;ReturnValue;local;df-generated
// source=Sources;NewSources;false;WrapConsoleReadLine;();;ReturnValue;stdin;df-generated
// neutral=Sources;NewSources;WrapConsoleReadLine;();summary;df-generated
public string? WrapConsoleReadLine()
{
return Console.ReadLine();
}

// New source
// source=Sources;NewSources;false;WrapConsoleReadLineAndProcees;(System.String);;ReturnValue;local;df-generated
// source=Sources;NewSources;false;WrapConsoleReadLineAndProcees;(System.String);;ReturnValue;stdin;df-generated
// neutral=Sources;NewSources;WrapConsoleReadLineAndProcees;(System.String);summary;df-generated
public string WrapConsoleReadLineAndProcees(string prompt)
{
Expand All @@ -37,7 +37,7 @@ public string WrapConsoleReadLineAndProcees(string prompt)
}

// New source
// source=Sources;NewSources;false;WrapConsoleReadKey;();;ReturnValue;local;df-generated
// source=Sources;NewSources;false;WrapConsoleReadKey;();;ReturnValue;stdin;df-generated
// neutral=Sources;NewSources;WrapConsoleReadKey;();summary;df-generated
public ConsoleKeyInfo WrapConsoleReadKey()
{
Expand Down
2 changes: 1 addition & 1 deletion docs/codeql/reusables/threat-model-description.rst
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ A threat model is a named class of dataflow sources that can be enabled or disab
The ``kind`` property of the ``sourceModel`` determines which threat model a source is associated with. There are two main categories:

- ``remote`` which represents requests and responses from the network.
- ``local`` which represents data from local files (``file``), command-line arguments (``commandargs``), database reads (``database``), environment variables(``environment``) and Windows registry values ("windows-registry"). Currently, Windows registry values are used by C# only.
- ``local`` which represents data from local files (``file``), command-line arguments (``commandargs``), database reads (``database``), environment variables(``environment``), standard input (``stdin``) and Windows registry values ("windows-registry"). Currently, Windows registry values are used by C# only.

Note that subcategories can be turned included or excluded separately, so you can specify ``local`` without ``database``, or just ``commandargs`` and ``environment`` without the rest of ``local``.

Expand Down
4 changes: 4 additions & 0 deletions java/ql/lib/change-notes/2024-08-13-stdin-threat-model.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Threat-model for `System.in` changed from `commandargs` to newly created `stdin` (both subgroups of `local`).
16 changes: 13 additions & 3 deletions java/ql/lib/semmle/code/java/dataflow/FlowSources.qll
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,8 @@ deprecated class EnvInput extends DataFlow::Node {
EnvInput() {
this instanceof EnvironmentInput or
this instanceof CliInput or
this instanceof FileInput
this instanceof FileInput or
this instanceof StdinInput
}
}

Expand All @@ -234,12 +235,21 @@ private class CliInput extends LocalUserInput {
exists(Field f | this.asExpr() = f.getAnAccess() |
f.getAnAnnotation().getType().getQualifiedName() = "org.kohsuke.args4j.Argument"
)
or
}

override string getThreatModel() { result = "commandargs" }
}

/**
* A node with input from stdin.
*/
private class StdinInput extends LocalUserInput {
StdinInput() {
// Access to `System.in`.
exists(Field f | this.asExpr() = f.getAnAccess() | f instanceof SystemIn)
}

override string getThreatModel() { result = "commandargs" }
override string getThreatModel() { result = "stdin" }
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public void M4(Statement handle) throws Exception {
}

public void M5(Statement handle) throws Exception {
// Only a source if "commandargs" is a selected threat model.
// Only a source if "stdin" is a selected threat model.
byte[] data = new byte[1024];
System.in.read(data);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ extensions:
extensible: threatModelConfiguration
data:
- ["environment", true, 0]
- ["commandargs", true, 0]
- ["stdin", true, 0]

- addsTo:
pack: codeql/java-all
Expand Down
1 change: 1 addition & 0 deletions shared/mad/codeql/mad/ModelValidation.qll
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ module KindValidation<KindValidationConfigSig Config> {
[
// shared
"local", "remote", "file", "commandargs", "database", "environment", "reverse-dns",
"stdin",
// Java
"android-external-storage-dir", "contentprovider",
// C#
Expand Down
1 change: 1 addition & 0 deletions shared/threat-models/ext/threat-model-grouping.model.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ extensions:
- ["database", "local"]
- ["commandargs", "local"]
- ["environment", "local"]
- ["stdin", "local"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall I think this is a good decision.

But I'm debating whether it makes more sense as a completely separate threat model kind versus a sub-kind of file (my understanding of how threatModelEnabled works is that it supports sub-kinds).

Either way I agree with @michaelnebel that this should have a change note.

- ["file", "local"]
- ["windows-registry", "local"]

Expand Down
Loading