Skip to content

Commit 338754f

Browse files
hvitvedhvitved
authored
Merge pull request #800 from calumgrant/cs/winforms
C#: Add sources from System.Windows.Forms controls
2 parents 23e94c2 + 790db3a commit 338754f

File tree

22 files changed

+256
-14
lines changed

22 files changed

+256
-14
lines changed

‎change-notes/1.20/analysis-csharp.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@
1414
| Off-by-one comparison against container length (cs/index-out-of-bounds) | Fewer false positives | Results have been removed when there are additional guards on the index. |
1515
| Dereferenced variable is always null (cs/dereferenced-value-is-always-null) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. |
1616
| Dereferenced variable may be null (cs/dereferenced-value-may-be-null) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. |
17-
17+
| SQL query built from user-controlled sources (cs/sql-injection), Improper control of generation of code (cs/code-injection), Uncontrolled format string (cs/uncontrolled-format-string), Clear text storage of sensitive information (cs/cleartext-storage-of-sensitive-information), Exposure of private information (cs/exposure-of-sensitive-information) | More results | Data sources have been added from user controls in `System.Windows.Forms`. |
18+
1819
## Changes to code extraction
1920

2021
* Fix extraction of `for` statements where the condition declares new variables using `is`.

‎csharp/ql/src/Security Features/CWE-089/SqlInjection.ql‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,13 @@ import csharp
1414
import semmle.code.csharp.security.dataflow.SqlInjection::SqlInjection
1515
import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph
1616

17+
string getSourceType(DataFlow::Node node) {
18+
result = node.(RemoteFlowSource).getSourceType()
19+
or
20+
result = node.(LocalFlowSource).getSourceType()
21+
}
22+
1723
from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink
1824
where c.hasFlowPath(source, sink)
1925
select sink.getNode(), source, sink, "Query might include code from $@.", source,
20-
("this " + source.getNode().(RemoteFlowSource).getSourceType())
26+
("this " + getSourceType(source.getNode()))

‎csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,19 @@
1212

1313
import csharp
1414
import semmle.code.csharp.dataflow.flowsources.Remote
15+
import semmle.code.csharp.dataflow.flowsources.Local
1516
import semmle.code.csharp.dataflow.TaintTracking
1617
import semmle.code.csharp.frameworks.Format
1718
import DataFlow::PathGraph
1819

1920
class FormatStringConfiguration extends TaintTracking::Configuration {
2021
FormatStringConfiguration() { this = "FormatStringConfiguration" }
2122

22-
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
23+
override predicate isSource(DataFlow::Node source) {
24+
source instanceof RemoteFlowSource
25+
or
26+
source instanceof LocalFlowSource
27+
}
2328

2429
override predicate isSink(DataFlow::Node sink) {
2530
sink.asExpr() = any(FormatCall call).getFormatExpr()

‎csharp/ql/src/semmle/code/csharp/dataflow/flowsources/Local.qll‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* Provides classes representing sources of local input.
3+
*/
4+
5+
import csharp
6+
private import semmle.code.csharp.frameworks.system.windows.Forms
7+
8+
/** A data flow source of local data. */
9+
abstract class LocalFlowSource extends DataFlow::Node {
10+
/** Gets a string that describes the type of this local flow source. */
11+
abstract string getSourceType();
12+
}
13+
14+
/** A data flow source of local user input. */
15+
abstract class LocalUserInputSource extends LocalFlowSource { }
16+
17+
/** The text of a `TextBox`. */
18+
class TextFieldSource extends LocalUserInputSource {
19+
TextFieldSource() { this.asExpr() = any(TextControl control).getARead() }
20+
21+
override string getSourceType() { result = "TextBox text" }
22+
}

‎csharp/ql/src/semmle/code/csharp/frameworks/system/windows/Forms.qll‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,66 @@ class SystemWindowsFormsHtmlElement extends SystemWindowsFormsClass {
2323
/** Gets the `SetAttribute` method. */
2424
Method getSetAttributeMethod() { result = this.getAMethod("SetAttribute") }
2525
}
26+
27+
/** The `System.Windows.Forms.TextBoxBase` class. */
28+
class SystemWindowsFormsTextBoxBase extends SystemWindowsFormsClass {
29+
SystemWindowsFormsTextBoxBase() { this.hasName("TextBoxBase") }
30+
31+
/** Gets the `Text` property. */
32+
Property getTextProperty() { result = this.getProperty("Text") }
33+
}
34+
35+
/** The `System.Windows.Forms.RichTextBox` class. */
36+
class SystemWindowsFormsRichTextBox extends SystemWindowsFormsClass {
37+
SystemWindowsFormsRichTextBox() { this.hasName("RichTextBox") }
38+
39+
/** Gets the `Rtf` property. */
40+
Property getRtfProperty() { result = this.getProperty("Rtf") }
41+
42+
/** Gets the `SelectedText` property. */
43+
Property getSelectedTextProperty() { result = this.getProperty("SelectedText") }
44+
45+
/** Gets the 'SelectedRtf' property. */
46+
Property getSelectedRtfProperty() { result = this.getProperty("SelectedRtf") }
47+
}
48+
49+
/** The `System.Windows.Forms.HtmlDocument` class. */
50+
class SystemWindowsFormsHtmlDocumentClass extends SystemWindowsFormsClass {
51+
SystemWindowsFormsHtmlDocumentClass() { this.hasName("HtmlDocument") }
52+
53+
/** Gets the `Write` method. */
54+
Method getWriteMethod() { result = this.getAMethod("Write") }
55+
}
56+
57+
/** The `System.Windows.Forms.WebBrowser` class. */
58+
class SystemWindowsFormsWebBrowserClass extends SystemWindowsFormsClass {
59+
SystemWindowsFormsWebBrowserClass() { this.hasName("WebBrowser") }
60+
61+
/** Gets the `DocumentText` property. */
62+
Property getDocumentTextProperty() { result = this.getProperty("DocumentText") }
63+
}
64+
65+
private class TextProperty extends Property {
66+
TextProperty() {
67+
exists(SystemWindowsFormsRichTextBox c |
68+
this = c.getRtfProperty() or
69+
this = c.getSelectedTextProperty() or
70+
this = c.getSelectedRtfProperty()
71+
)
72+
or
73+
exists(SystemWindowsFormsTextBoxBase tb | this = tb.getTextProperty().getAnOverrider*())
74+
}
75+
}
76+
77+
/** A variable that contains a text control. */
78+
class TextControl extends Variable {
79+
TextControl() {
80+
this.getType().(ValueOrRefType).getBaseClass*() instanceof SystemWindowsFormsTextBoxBase
81+
}
82+
83+
/** Gets a read of the text property. */
84+
PropertyRead getARead() {
85+
result.getTarget() instanceof TextProperty and
86+
result.getQualifier() = this.getAnAccess()
87+
}
88+
}

‎csharp/ql/src/semmle/code/csharp/security/PrivateData.qll‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
*/
1111

1212
import csharp
13+
import semmle.code.csharp.frameworks.system.windows.Forms
1314

1415
/** A string for `match` that identifies strings that look like they represent private data. */
1516
private string privateNames() {
@@ -58,3 +59,10 @@ class PrivateVariableAccess extends PrivateDataExpr, VariableAccess {
5859
exists(string s | this.getTarget().getName().toLowerCase() = s | s.matches(privateNames()))
5960
}
6061
}
62+
63+
/** Reading the text property of a control that might contain private data. */
64+
class PrivateControlAccess extends PrivateDataExpr {
65+
PrivateControlAccess() {
66+
exists(TextControl c | this = c.getARead() and c.getName().toLowerCase().matches(privateNames()))
67+
}
68+
}

‎csharp/ql/src/semmle/code/csharp/security/SensitiveActions.qll‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
*/
1111

1212
import csharp
13+
import semmle.code.csharp.frameworks.system.windows.Forms
1314

1415
/**
1516
* A string for `match` that identifies strings that look like they represent secret data.
@@ -108,7 +109,11 @@ private predicate expressionHasName(Expr expr, string name) {
108109

109110
/** An expression that may contain a password. */
110111
class PasswordExpr extends Expr {
111-
PasswordExpr() { exists(string name | expressionHasName(this, name) and isPassword(name)) }
112+
PasswordExpr() {
113+
exists(string name | expressionHasName(this, name) and isPassword(name))
114+
or
115+
this instanceof PasswordTextboxText
116+
}
112117
}
113118

114119
/** An expression that might contain sensitive data. */
@@ -130,6 +135,23 @@ class SensitiveVariableAccess extends SensitiveExpr, VariableAccess {
130135
SensitiveVariableAccess() { isSuspicious(this.getTarget().getName()) }
131136
}
132137

138+
/** Reading the `Text` property of a password text box. */
139+
class PasswordTextboxText extends SensitiveExpr, PropertyRead {
140+
PasswordTextboxText() { this = any(PasswordField p).getARead() }
141+
}
142+
143+
/** A field containing a text box used as a password. */
144+
class PasswordField extends TextControl {
145+
PasswordField() {
146+
isSuspicious(this.getName())
147+
or
148+
exists(PropertyWrite write | write.getQualifier() = this.getAnAccess() |
149+
write.getTarget().getName() = "UseSystemPasswordChar" or
150+
write.getTarget().getName() = "PasswordChar"
151+
)
152+
}
153+
}
154+
133155
/** A method that may produce sensitive data. */
134156
abstract class SensitiveDataMethod extends Method { }
135157

‎csharp/ql/src/semmle/code/csharp/security/dataflow/CodeInjection.qll‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import csharp
66

77
module CodeInjection {
88
import semmle.code.csharp.dataflow.flowsources.Remote
9+
import semmle.code.csharp.dataflow.flowsources.Local
910
import semmle.code.csharp.frameworks.system.codedom.Compiler
1011
import semmle.code.csharp.security.Sanitizers
1112

@@ -40,6 +41,9 @@ module CodeInjection {
4041
/** A source of remote user input. */
4142
class RemoteSource extends Source { RemoteSource() { this instanceof RemoteFlowSource } }
4243

44+
/** A source of local user input. */
45+
class LocalSource extends Source { LocalSource() { this instanceof LocalFlowSource } }
46+
4347
private class SimpleTypeSanitizer extends Sanitizer, SimpleTypeSanitizedExpr { }
4448

4549
private class GuidSanitizer extends Sanitizer, GuidSanitizedExpr { }

‎csharp/ql/src/semmle/code/csharp/security/dataflow/ResourceInjection.qll‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import csharp
66

77
module ResourceInjection {
88
import semmle.code.csharp.dataflow.flowsources.Remote
9+
import semmle.code.csharp.dataflow.flowsources.Local
910
import semmle.code.csharp.frameworks.system.Data
1011
import semmle.code.csharp.security.Sanitizers
1112

@@ -40,6 +41,9 @@ module ResourceInjection {
4041
/** A source of remote user input. */
4142
class RemoteSource extends Source { RemoteSource() { this instanceof RemoteFlowSource } }
4243

44+
/** A source of local user input. */
45+
class LocalSource extends Source { LocalSource() { this instanceof LocalFlowSource } }
46+
4347
/** An argument to the `ConnectionString` property on a data connection class. */
4448
class SqlConnectionStringSink extends Sink {
4549
SqlConnectionStringSink() {

‎csharp/ql/src/semmle/code/csharp/security/dataflow/SqlInjection.qll‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import csharp
66

77
module SqlInjection {
88
import semmle.code.csharp.dataflow.flowsources.Remote
9+
import semmle.code.csharp.dataflow.flowsources.Local
910
import semmle.code.csharp.frameworks.Sql
1011
import semmle.code.csharp.security.Sanitizers
1112

@@ -40,6 +41,9 @@ module SqlInjection {
4041
/** A source of remote user input. */
4142
class RemoteSource extends Source { RemoteSource() { this instanceof RemoteFlowSource } }
4243

44+
/** A source of local user input. */
45+
class LocalSource extends Source { LocalSource() { this instanceof LocalFlowSource } }
46+
4347
/** An SQL expression passed to an API call that executes SQL. */
4448
class SqlInjectionExprSink extends Sink {
4549
SqlInjectionExprSink() { exists(SqlExpr s | this.getExpr() = s.getSql()) }

0 commit comments

Comments
 (0)