Skip to content

Commit 397c8b5

Browse files
jbjjbj
authored
Merge pull request #212 from raulgarciamsft/master
Setting a SECURITY_DESCRIPTOR’s DACL to NULL
2 parents 7c006d4 + 54493eb commit 397c8b5

File tree

7 files changed

+206
-3
lines changed

7 files changed

+206
-3
lines changed

‎.gitignore‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,4 @@
1212
/.vs/ql/v15/Browse.VC.opendb
1313
/.vs/ql/v15/Browse.VC.db
1414
/.vs/ProjectSettings.json
15-
/.vs/slnx.sqlite-journal
16-
/.vs/ql2/v15/Browse.VC.opendb
17-
/.vs/ql2/v15/Browse.VC.db
15+

‎cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
SECURITY_DESCRIPTOR pSD;
2+
SECURITY_ATTRIBUTES SA;
3+
4+
if (!InitializeSecurityDescriptor(&pSD, SECURITY_DESCRIPTOR_REVISION))
5+
{
6+
// error handling
7+
}
8+
if (!SetSecurityDescriptorDacl(&pSD,
9+
TRUE, // bDaclPresent - this value indicates the presence of a DACL in the security descriptor
10+
NULL, // pDacl - the pDacl parameter does not point to a DACL. All access will be allowed
11+
FALSE))
12+
{
13+
// error handling
14+
}

‎cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>This query indicates that a call is setting the DACL field in a <code>SECURITY_DESCRIPTOR</code> to null.</p>
8+
<p>When using <code>SetSecurityDescriptorDacl</code> to set a discretionary access control (DACL), setting the <code>bDaclPresent</code> argument to <code>TRUE</code> indicates the prescence of a DACL in the security description in the argument <code>pDacl</code>.</p>
9+
<p>When the <code>pDacl</code> parameter does not point to a DACL (i.e. it is <code>NULL</code>) and the <code>bDaclPresent</code> flag is <code>TRUE</code>, a <code>NULL DACL</code> is specified.</p>
10+
<p>A <code>NULL DACL</code> grants full access to any user who requests it; normal security checking is not performed with respect to the object.</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>You should not use a <code>NULL DACL</code> with an object because any user can change the DACL and owner of the security descriptor.</p>
15+
</recommendation>
16+
17+
<example>
18+
<p>In the following example, the call to <code>SetSecurityDescriptorDacl</code> is setting an unsafe DACL (<code>NULL DACL</code>) to the security descriptor.</p>
19+
<sample src="UnsafeDaclSecurityDescriptor.cpp" />
20+
21+
<p>To fix this issue, <code>pDacl</code> argument should be a pointer to an <code>ACL</code> structure that specifies the DACL for the security descriptor.</p>
22+
</example>
23+
24+
<references>
25+
<li><a href="https://docs.microsoft.com/en-us/windows/desktop/api/securitybaseapi/nf-securitybaseapi-setsecuritydescriptordacl">SetSecurityDescriptorDacl function (Microsoft documentation).</a>
26+
</li>
27+
</references>
28+
29+
</qhelp>

‎cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* @name Setting a DACL to NULL in a SECURITY_DESCRIPTOR
3+
* @description Setting a DACL to NULL in a SECURITY_DESCRIPTOR will result in an unprotected object.
4+
* If the DACL that belongs to the security descriptor of an object is set to NULL, a null DACL is created.
5+
* A null DACL grants full access to any user who requests it;
6+
* normal security checking is not performed with respect to the object.
7+
* @id cpp/unsafe-dacl-security-descriptor
8+
* @kind problem
9+
* @problem.severity error
10+
* @precision high
11+
* @tags security
12+
* external/cwe/cwe-732
13+
* external/microsoft/C6248
14+
*/
15+
import cpp
16+
import semmle.code.cpp.dataflow.DataFlow
17+
18+
/**
19+
* A function call to SetSecurityDescriptorDacl to set the ACL, specified by (2nd argument) bDaclPresent = TRUE
20+
*/
21+
class SetSecurityDescriptorDaclFunctionCall extends FunctionCall {
22+
SetSecurityDescriptorDaclFunctionCall() {
23+
this.getTarget().hasGlobalName("SetSecurityDescriptorDacl")
24+
and this.getArgument(1).getValue().toInt() != 0
25+
}
26+
}
27+
28+
/**
29+
* Dataflow that detects a call to SetSecurityDescriptorDacl with a NULL DACL as the pDacl argument
30+
*/
31+
class SetSecurityDescriptorDaclFunctionConfiguration extends DataFlow::Configuration {
32+
SetSecurityDescriptorDaclFunctionConfiguration() {
33+
this = "SetSecurityDescriptorDaclFunctionConfiguration"
34+
}
35+
36+
override predicate isSource(DataFlow::Node source) {
37+
exists(
38+
NullValue nullExpr |
39+
source.asExpr() = nullExpr
40+
)
41+
}
42+
43+
override predicate isSink(DataFlow::Node sink) {
44+
exists(
45+
SetSecurityDescriptorDaclFunctionCall call, VariableAccess val |
46+
val = sink.asExpr() |
47+
val = call.getArgument(2)
48+
)
49+
}
50+
}
51+
52+
from SetSecurityDescriptorDaclFunctionCall call, string message
53+
where exists
54+
(
55+
NullValue nullExpr |
56+
message = "Setting a DACL to NULL in a SECURITY_DESCRIPTOR will result in an unprotected object." |
57+
call.getArgument(1).getValue().toInt() != 0
58+
and call.getArgument(2) = nullExpr
59+
) or exists
60+
(
61+
Expr constassign, VariableAccess var,
62+
SetSecurityDescriptorDaclFunctionConfiguration config |
63+
message = "Setting a DACL to NULL in a SECURITY_DESCRIPTOR using variable " + var + " that is set to NULL will result in an unprotected object." |
64+
var = call.getArgument(2)
65+
and config.hasFlow(DataFlow::exprNode(constassign), DataFlow::exprNode(var))
66+
)
67+
select call, message

‎cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
// semmle-extractor-options: --microsoft
2+
typedef unsigned long DWORD;
3+
typedef unsigned long ULONG;
4+
typedef unsigned char BYTE;
5+
typedef unsigned short WORD;
6+
typedef int BOOL;
7+
typedef void *PVOID;
8+
#define TRUE 1
9+
#define FALSE 0
10+
#define ERROR_SUCCESS 0L
11+
#define NULL 0
12+
13+
typedef PVOID PSECURITY_DESCRIPTOR;
14+
15+
typedef struct _ACL {
16+
BYTE AclRevision;
17+
BYTE Sbz1;
18+
WORD AclSize;
19+
WORD AceCount;
20+
WORD Sbz2;
21+
} ACL;
22+
typedef ACL *PACL;
23+
24+
typedef enum _ACCESS_MODE
25+
{
26+
NOT_USED_ACCESS = 0,
27+
GRANT_ACCESS,
28+
SET_ACCESS,
29+
DENY_ACCESS,
30+
REVOKE_ACCESS,
31+
SET_AUDIT_SUCCESS,
32+
SET_AUDIT_FAILURE
33+
} ACCESS_MODE;
34+
35+
typedef int TRUSTEE_W;
36+
37+
typedef struct _EXPLICIT_ACCESS_W
38+
{
39+
DWORD grfAccessPermissions;
40+
ACCESS_MODE grfAccessMode;
41+
DWORD grfInheritance;
42+
TRUSTEE_W Trustee;
43+
} EXPLICIT_ACCESS_W, *PEXPLICIT_ACCESS_W, EXPLICIT_ACCESSW, *PEXPLICIT_ACCESSW;
44+
45+
BOOL
46+
SetSecurityDescriptorDacl(
47+
PSECURITY_DESCRIPTOR pSecurityDescriptor,
48+
BOOL bDaclPresent,
49+
PACL pDacl,
50+
BOOL bDaclDefaulted
51+
) {
52+
return TRUE;
53+
}
54+
55+
DWORD SetEntriesInAcl(
56+
ULONG cCountOfExplicitEntries,
57+
PEXPLICIT_ACCESS_W pListOfExplicitEntries,
58+
PACL OldAcl,
59+
PACL *NewAcl
60+
)
61+
{
62+
*NewAcl = (PACL)0xFFFFFF;
63+
return ERROR_SUCCESS;
64+
}
65+
66+
void Test()
67+
{
68+
PSECURITY_DESCRIPTOR pSecurityDescriptor;
69+
BOOL b;
70+
b = SetSecurityDescriptorDacl(pSecurityDescriptor,
71+
TRUE, // Dacl Present
72+
NULL, // NULL pointer to DACL == BUG
73+
FALSE);
74+
75+
PACL pDacl = NULL;
76+
b = SetSecurityDescriptorDacl(pSecurityDescriptor,
77+
TRUE, // Dacl Present
78+
pDacl, // NULL pointer to DACL == BUG
79+
FALSE);
80+
81+
SetEntriesInAcl(0, NULL, NULL, &pDacl);
82+
b = SetSecurityDescriptorDacl(pSecurityDescriptor,
83+
TRUE, // Dacl Present
84+
pDacl, // Should have been set by SetEntriesInAcl ==> should not be flagged
85+
FALSE);
86+
87+
b = SetSecurityDescriptorDacl(pSecurityDescriptor,
88+
FALSE, // Dacl is not Present
89+
NULL, // DACL is going to be removed from security descriptor. Default/inherited access ==> should not be flagged
90+
FALSE);
91+
92+
}

‎cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
| UnsafeDaclSecurityDescriptor.cpp:70:9:70:33 | call to SetSecurityDescriptorDacl | Setting a DACL to NULL in a SECURITY_DESCRIPTOR will result in an unprotected object. |
2+
| UnsafeDaclSecurityDescriptor.cpp:76:9:76:33 | call to SetSecurityDescriptorDacl | Setting a DACL to NULL in a SECURITY_DESCRIPTOR using variable pDacl that is set to NULL will result in an unprotected object. |

‎cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql

0 commit comments

Comments
 (0)