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

C++: Model Microsoft's "Active Template Library" #18136

Merged
merged 65 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
fe9feb9
C++: We will need all these types.
MathiasVP Nov 26, 2024
16e5fa3
C++: Add failing tests with U_STRINGorID.
MathiasVP Nov 26, 2024
bf36f00
C++: Add model. Observe that flow still fails.
MathiasVP Nov 26, 2024
f688470
C++: Since isConstructedFrom only holds for templates we need to expl…
MathiasVP Nov 26, 2024
749602c
C++: Add failing tests with CA2AEX and friends.
MathiasVP Nov 26, 2024
763b991
C++: Add models.
MathiasVP Nov 26, 2024
2c7d0de
C++: Accept test changes.
MathiasVP Nov 26, 2024
c00f84d
C++: Work around the 'wrong' function name for conversion operators.
MathiasVP Nov 26, 2024
4f2cd81
C++: Accept test changes.
MathiasVP Nov 26, 2024
1cd426e
C++: Add failing tests with 'CAtlArray'.
MathiasVP Nov 27, 2024
0f8df1c
C++: Add MaD model for 'CAtlArray'.
MathiasVP Nov 27, 2024
c604a93
C++: Add failing tests with 'CAtlList'.
MathiasVP Nov 27, 2024
2b8ef5a
C++: Add MaD model for 'CAtlList'.
MathiasVP Nov 27, 2024
68ee8da
C++: Add failing tests with 'CComBSTR'.
MathiasVP Nov 27, 2024
9b00484
C++: Add MaD model for 'CComBSTR'.
MathiasVP Nov 27, 2024
948be09
C++: Add an taint step from object to field for 'CComBSTR's.
MathiasVP Nov 27, 2024
e831cb5
C++: Add failing tests with 'CComSafeArray'.
MathiasVP Nov 27, 2024
5f05417
C++: Add MaD model for 'CComSafeArray'.
MathiasVP Nov 27, 2024
1a79290
C++: Add failing tests with 'CPathT'.
MathiasVP Nov 27, 2024
3543619
C++: Add MaD model for 'CPathT'.
MathiasVP Nov 27, 2024
c61395b
C++: Add implicit read of the 'm_strPath' member.
MathiasVP Nov 27, 2024
029c013
C++: Add failing tests with 'CSimpleArray'.
MathiasVP Nov 27, 2024
02b88d5
C++: Add MaD model for 'CSimpleArray'.
MathiasVP Nov 27, 2024
12674ea
C++: Add failing tests with 'CSimpleMap'.
MathiasVP Nov 27, 2024
74b6c9d
C++: Add MaD model for 'CSimpleMap'.
MathiasVP Nov 27, 2024
1ea879a
C++: Add failing tests for 'CUrl'.
MathiasVP Nov 27, 2024
300e3ea
C++: Add MaD model for 'CUrl'.
MathiasVP Nov 27, 2024
e73fccd
C++: Add more types that we'll need for later.
MathiasVP Nov 27, 2024
dee47f2
C++: Add a failing test with 'CAtlFile'.
MathiasVP Nov 27, 2024
74eae4a
C++: Add a MaD model for 'CAtlFile' and mark reads as local flow sour…
MathiasVP Nov 27, 2024
ac0599c
C++: Add a failing test with 'CAtlFileMapping'.
MathiasVP Nov 27, 2024
3709151
C++: Add a MaD model for 'CAtlFileMappingBase' and mark reads as loca…
MathiasVP Nov 27, 2024
67ba85a
C++: Add failing tests for 'CAtlTemporaryFile'.
MathiasVP Nov 27, 2024
33212da
C++: Add a MaD model for 'CAtlTemporaryFile' and mark reads as local …
MathiasVP Nov 27, 2024
5aada39
C++: Add failing tests for 'CRegKey'.
MathiasVP Nov 27, 2024
d69de0c
C++: Add a MaD model for 'CRegKey' and mark query calls as local flow…
MathiasVP Nov 27, 2024
19e7c37
C++: Update the final test changes. Nothing exciting here.
MathiasVP Nov 27, 2024
0242874
C++: Add change note.
MathiasVP Nov 27, 2024
3c0af49
C++: Fix bug introduced in an earlier commit and accept test changes.…
MathiasVP Nov 27, 2024
2c58279
C++: Add QLDoc to 'isClassConstructedFrom' and 'isFunctionConstructed…
MathiasVP Dec 3, 2024
0c8245f
Update cpp/ql/test/library-tests/dataflow/taint-tests/atl.cpp
MathiasVP Dec 3, 2024
593e223
C++: Update test changes after 0c8245f727e8fc7fac2b5731dcbc5e4d7bd86fdc.
MathiasVP Dec 3, 2024
3abb904
C++: Fix testcase to reveal problematic models.
MathiasVP Dec 3, 2024
c3086d4
C++: Fix models and accept test changes.
MathiasVP Dec 3, 2024
8d035e6
C++: Fix test.
MathiasVP Dec 4, 2024
de75e03
C++: Remove taint to POSITIONs.
MathiasVP Dec 4, 2024
9dc3aec
C++: Remove more taint to POSITIONs.
MathiasVP Dec 4, 2024
c7dee4b
C++: Remove more taint to POSITIONs.
MathiasVP Dec 4, 2024
279a30c
C++: Make 'SetAt' a value-preserving step.
MathiasVP Dec 4, 2024
4f00e22
C++: Accept more test changes.
MathiasVP Dec 4, 2024
d0bf3b8
C++: Add missing MaD row for move constructor.
MathiasVP Dec 6, 2024
904db38
C++: Add missing space between type name and '&'.
MathiasVP Dec 6, 2024
f7b55e0
C++: 'Attach' is value-preserving.
MathiasVP Dec 6, 2024
6388a9a
C++: Delete duplicated MaD row.
MathiasVP Dec 6, 2024
66de42c
C++: Fix MaD row for 'operator&' on 'CComBSTR's.
MathiasVP Dec 6, 2024
3d0a205
C++: Fix 'BSTRToArray' stub and MaD model.
MathiasVP Dec 6, 2024
59f4b3c
C++: Get rid of the model for 'Create'.
MathiasVP Dec 6, 2024
d735a14
C++: Also flow to the return value of 'operator='.
MathiasVP Dec 9, 2024
d3dc318
C++: Make 'GetValueAt' a value-preserving step.
MathiasVP Dec 9, 2024
db86f6a
C++: Fix annotation.
MathiasVP Dec 9, 2024
674dbce
C++: Add taint flow through 'CRegKey::Create'.
MathiasVP Dec 9, 2024
7f87a25
C++: Fix 'QueryMultiStringValue' model.
MathiasVP Dec 9, 2024
184dfc2
C++: Fix 'QueryStringValue' model.
MathiasVP Dec 9, 2024
5f33733
C++: Fix 'QueryValue' model.
MathiasVP Dec 9, 2024
8bdd10c
C++: Fix spurious columns in 'CRegKey'.
MathiasVP Dec 9, 2024
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
19 changes: 19 additions & 0 deletions cpp/ql/lib/ext/CRegKey.model.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
extensions:
- addsTo:
pack: codeql/cpp-all
extensible: summaryModel
data: # namespace, type, subtypes, name, signature, ext, input, output, kind, provenance
- ["", "CRegKey", True, "CRegKey", "(CRegKey&)", "", "Argument[*0]", "Argument[-1]", "value", "manual"]
- ["", "CRegKey", True, "CRegKey", "(HKEY)", "", "Argument[0]", "Argument[-1]", "taint", "manual"]
- ["", "CRegKey", True, "Attach", "", "", "Argument[0]", "Argument[-1]", "taint", "manual"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel we should also model CRegKey::Create, which I believe is commonly used when adding new data structures in the registry. The lpszKeyName (Argument[*1]) should have taint flow into the qualifier.

(it could also be a query sink, you would not want untrusted data controlling this operation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I've added this summary model in 674dbce. I think this is one that could also be treated with MapKey flow at some point and be made value-preserving.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it will be value preserving as you're still appending the key name to an existing registry path represented by the current state of the key. Unless maybe you consider the key to be a set of path edges, in which case I suppose you're adding one value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless maybe you consider the key to be a set of path edges, in which case I suppose you're adding one value.

Yeah, that was my thinking. But I guess we can settle on those details later. I agree that there are multiple ways to think of this.

- ["", "CRegKey", True, "QueryBinaryValue", "", "", "Argument[*0]", "Argument[*1]", "taint", "manual"]
- ["", "CRegKey", True, "QueryDWORDValue", "", "", "Argument[*0]", "Argument[*1]", "taint", "manual"]
- ["", "CRegKey", True, "QueryMultiStringValue", "", "", "Argument[*0]", "Argument[**1]", "taint", "manual"]
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved
- ["", "CRegKey", True, "QueryQWORDValue", "", "", "Argument[*0]", "Argument[*1]", "taint", "manual"]
- ["", "CRegKey", True, "QueryStringValue", "", "", "Argument[*0]", "Argument[**1]", "taint", "manual"]
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved
- ["", "CRegKey", True, "QueryValue", "(LPCTSTR,DWORD *,void *,ULONG *)", "", "Argument[*0]", "Argument[*2]", "taint", "manual"]
- ["", "CRegKey", True, "QueryValue", "(DWORD&,LPCTSTR)", "", "Argument[*1]", "Argument[*0]", "taint", "manual"]
- ["", "CRegKey", True, "QueryValue", "(LPTSTR,LPCTSTR,DWORD *)", "", "Argument[*1]", "Argument[**0]", "taint", "manual"]
- ["", "CRegKey", True, "QueryValue", "operator HKEY", "", "Argument[-1]", "ReturnValue", "taint", "manual"]
- ["", "CRegKey", True, "QueryValue", "operator=", "", "Argument[*0]", "ReturnValue[*]", "value", "manual"]
- ["", "CRegKey", True, "QueryValue", "operator=", "", "Argument[*0]", "Argument[-1]", "value", "manual"]
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions cpp/ql/lib/semmle/code/cpp/models/Models.qll
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,4 @@ private import implementations.CPathT
private import implementations.CAtlFile
private import implementations.CAtlFileMapping
private import implementations.CAtlTemporaryFile
private import implementations.CRegKey
87 changes: 87 additions & 0 deletions cpp/ql/lib/semmle/code/cpp/models/implementations/CRegKey.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
private import cpp
private import semmle.code.cpp.models.interfaces.FlowSource
private import semmle.code.cpp.ir.dataflow.FlowSteps
private import semmle.code.cpp.dataflow.new.DataFlow

/** The `CRegKey` class from the Microsoft "Active Template Library". */
class CRegKey extends Class {
CRegKey() { this.hasGlobalName("CRegKey") }
}

module CRegKey {
/** The `m_hKey` member on a object of type `CRegKey`. */
class MhKey extends Field {
MhKey() {
this.getDeclaringType() instanceof CRegKey and
this.getName() = "m_hKey"
}
}

private class MhKeyPathTaintInheritingContent extends TaintInheritingContent,
DataFlow::FieldContent
{
MhKeyPathTaintInheritingContent() { this.getField() instanceof MhKey }
}

private class CRegKeyMemberFunction extends MemberFunction {
string name;

CRegKeyMemberFunction() { this.getClassAndName(name) instanceof CRegKey }
}

abstract private class CRegKeyFlowSource extends CRegKeyMemberFunction, LocalFlowSourceFunction {
FunctionOutput output;

final override predicate hasLocalFlowSource(FunctionOutput output_, string description) {
output_ = output and
description = "registry string read by " + name
}
}

/** The `CRegKey::QueryBinaryValue` function from Win32. */
class QueryBinaryValue extends CRegKeyFlowSource {
QueryBinaryValue() { name = "QueryBinaryValue" and output.isParameterDeref(1) }
}

/** The `CRegKey::QueryDWORDValue` function from Win32. */
class QueryDwordValue extends CRegKeyFlowSource {
QueryDwordValue() { name = "QueryDWORDValue" and output.isParameterDeref(1) }
}

/** The `CRegKey::QueryGUIDValue` function from Win32. */
class QueryGuidValue extends CRegKeyFlowSource {
QueryGuidValue() { name = "QueryGUIDValue" and output.isParameterDeref(1) }
}

/** The `CRegKey::QueryMultiStringValue` function from Win32. */
class QueryMultiStringValue extends CRegKeyFlowSource {
QueryMultiStringValue() { name = "QueryMultiStringValue" and output.isParameterDeref(1) }
}

/** The `CRegKey::QueryQWORDValue` function from Win32. */
class QueryQwordValue extends CRegKeyFlowSource {
QueryQwordValue() { name = "QueryQWORDValue" and output.isParameterDeref(1) }
}

/** The `CRegKey::QueryStringValue` function from Win32. */
class QueryStringValue extends CRegKeyFlowSource {
QueryStringValue() { name = "QueryStringValue" and output.isParameterDeref(1) }
}

/** The `CRegKey::QueryValue` function from Win32. */
class QueryValue extends CRegKeyFlowSource {
QueryValue() {
name = "QueryValue" and
(
this.getNumberOfParameters() = 4 and
output.isParameterDeref(2)
or
this.getNumberOfParameters() = 2 and
output.isParameterDeref(0)
or
this.getNumberOfParameters() = 3 and
output.isParameterDeref(0)
)
}
}
}
18 changes: 9 additions & 9 deletions cpp/ql/test/library-tests/dataflow/source-sink-tests/atl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -364,26 +364,26 @@ void test_CRegKey() {
CRegKey key;
char data[1024];
ULONG bytesRead;
key.QueryBinaryValue("foo", data, &bytesRead); // $ MISSING: local_source
key.QueryBinaryValue("foo", data, &bytesRead); // $ local_source

DWORD value;
key.QueryDWORDValue("foo", value); // $ MISSING: local_source
key.QueryDWORDValue("foo", value); // $ local_source

GUID guid;
key.QueryGUIDValue("foo", guid); // $ MISSING: local_source
key.QueryGUIDValue("foo", guid); // $ local_source

key.QueryMultiStringValue("foo", data, &bytesRead); // $ MISSING: local_source
key.QueryMultiStringValue("foo", data, &bytesRead); // $ local_source

ULONGLONG qword;
key.QueryQWORDValue("foo", qword); // $ MISSING: local_source
key.QueryQWORDValue("foo", qword); // $ local_source

key.QueryStringValue("foo", data, &bytesRead); // $ MISSING: local_source
key.QueryStringValue("foo", data, &bytesRead); // $ local_source

key.QueryValue(data, "foo", &bytesRead); // $ MISSING: local_source
key.QueryValue(data, "foo", &bytesRead); // $ local_source

DWORD type;
key.QueryValue("foo", &type, data, &bytesRead); // $ MISSING: local_source
key.QueryValue("foo", &type, data, &bytesRead); // $ local_source

DWORD value2;
key.QueryValue(value2, "foo"); // $ MISSING: local_source
key.QueryValue(value2, "foo"); // $ local_source
}