Setting a SECURITY_DESCRIPTOR’s DACL to NULL

.gitignore

@@ -12,6 +12,4 @@
 /.vs/ql/v15/Browse.VC.opendb
 /.vs/ql/v15/Browse.VC.db
 /.vs/ProjectSettings.json
-/.vs/slnx.sqlite-journal
-/.vs/ql2/v15/Browse.VC.opendb
-/.vs/ql2/v15/Browse.VC.db
+

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

@@ -0,0 +1,14 @@
+SECURITY_DESCRIPTOR  pSD;
+SECURITY_ATTRIBUTES  SA;
+
+if (!InitializeSecurityDescriptor(&pSD, SECURITY_DESCRIPTOR_REVISION))
+{
+    // error handling
+}
+if (!SetSecurityDescriptorDacl(&pSD,
+    TRUE,   // bDaclPresent - this value indicates the presence of a DACL in the security descriptor
+    NULL,   // pDacl - the pDacl parameter does not point to a DACL. All access will be allowed
+    FALSE))
+{
+    // error handling
+}

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

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+  <p>This query indicates that a call is setting the DACL field in a <code>SECURITY_DESCRIPTOR</code> to null.</p>
+  <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>
+  <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>
+  <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>
+</overview>
+
+<recommendation>
+<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>
+</recommendation>
+
+<example>
+<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>
+<sample src="UnsafeDaclSecurityDescriptor.cpp" />
+
+<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>
+</example>
+
+<references>
+  <li><a href="https://docs.microsoft.com/en-us/windows/desktop/api/securitybaseapi/nf-securitybaseapi-setsecuritydescriptordacl">SetSecurityDescriptorDacl function (Microsoft documentation).</a>
+  </li>
+</references>
+
+</qhelp>

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

@@ -0,0 +1,67 @@
+/**
+ * @name Setting a DACL to NULL in a SECURITY_DESCRIPTOR
+ * @description Setting a DACL to NULL in a SECURITY_DESCRIPTOR will result in an unprotected object.
+ *              If the DACL that belongs to the security descriptor of an object is set to NULL, a null DACL is created. 
+ *              A null DACL grants full access to any user who requests it;
+ *              normal security checking is not performed with respect to the object.
+ * @id cpp/unsafe-dacl-security-descriptor
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @tags security
+ *       external/cwe/cwe-732
+ *       external/microsoft/C6248
+ */
+import cpp
+import semmle.code.cpp.dataflow.DataFlow
+
+/**
+ * A function call to SetSecurityDescriptorDacl to set the ACL, specified by (2nd argument) bDaclPresent = TRUE
+ */
+class SetSecurityDescriptorDaclFunctionCall extends FunctionCall {
+  SetSecurityDescriptorDaclFunctionCall() {
+    this.getTarget().hasGlobalName("SetSecurityDescriptorDacl")
+    and this.getArgument(1).getValue().toInt() != 0
+  }
+}
+
+/**
+ * Dataflow that detects a call to SetSecurityDescriptorDacl with a NULL DACL as the pDacl argument
+ */
+class SetSecurityDescriptorDaclFunctionConfiguration extends DataFlow::Configuration {
+  SetSecurityDescriptorDaclFunctionConfiguration() {
+    this = "SetSecurityDescriptorDaclFunctionConfiguration"
+  }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists( 
+      NullValue nullExpr |
+      source.asExpr() = nullExpr 
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists( 
+      SetSecurityDescriptorDaclFunctionCall call, VariableAccess val |
+      val = sink.asExpr() |
+      val = call.getArgument(2) 
+    )
+  }
+}
+
+from SetSecurityDescriptorDaclFunctionCall call, string message
+where exists
+  ( 
+    NullValue nullExpr |
+    message = "Setting a DACL to NULL in a SECURITY_DESCRIPTOR will result in an unprotected object." |
+    call.getArgument(1).getValue().toInt() != 0
+    and call.getArgument(2) = nullExpr
+  ) or exists
+  ( 
+    Expr constassign, VariableAccess var, 
+    SetSecurityDescriptorDaclFunctionConfiguration config |
+    message = "Setting a DACL to NULL in a SECURITY_DESCRIPTOR using variable " + var + " that is set to NULL will result in an unprotected object." |
+    var = call.getArgument(2)
+    and config.hasFlow(DataFlow::exprNode(constassign), DataFlow::exprNode(var))
+  )
+select call, message

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

@@ -0,0 +1,92 @@
+// semmle-extractor-options: --microsoft
+typedef unsigned long       DWORD;
+typedef unsigned long       ULONG;
+typedef unsigned char       BYTE;
+typedef unsigned short      WORD;
+typedef int                 BOOL;
+typedef void                *PVOID;
+#define TRUE                1
+#define FALSE               0
+#define ERROR_SUCCESS       0L
+#define NULL                0
+
+typedef PVOID PSECURITY_DESCRIPTOR;
+
+typedef struct _ACL {
+    BYTE  AclRevision;
+    BYTE  Sbz1;
+    WORD   AclSize;
+    WORD   AceCount;
+    WORD   Sbz2;
+} ACL;
+typedef ACL *PACL;
+
+typedef enum _ACCESS_MODE
+{
+    NOT_USED_ACCESS = 0,
+    GRANT_ACCESS,
+    SET_ACCESS,
+    DENY_ACCESS,
+    REVOKE_ACCESS,
+    SET_AUDIT_SUCCESS,
+    SET_AUDIT_FAILURE
+} ACCESS_MODE;
+
+typedef int TRUSTEE_W;
+
+typedef struct _EXPLICIT_ACCESS_W
+{
+    DWORD        grfAccessPermissions;
+    ACCESS_MODE  grfAccessMode;
+    DWORD        grfInheritance;
+    TRUSTEE_W    Trustee;
+} EXPLICIT_ACCESS_W, *PEXPLICIT_ACCESS_W, EXPLICIT_ACCESSW, *PEXPLICIT_ACCESSW;
+
+BOOL
+SetSecurityDescriptorDacl(
+    PSECURITY_DESCRIPTOR pSecurityDescriptor,
+    BOOL bDaclPresent,
+    PACL pDacl,
+    BOOL bDaclDefaulted
+) {
+    return TRUE;
+}
+
+DWORD SetEntriesInAcl(
+    ULONG              cCountOfExplicitEntries,
+    PEXPLICIT_ACCESS_W pListOfExplicitEntries,
+    PACL               OldAcl,
+    PACL               *NewAcl
+)
+{
+    *NewAcl = (PACL)0xFFFFFF;
+    return ERROR_SUCCESS;
+}
+
+void Test()
+{
+    PSECURITY_DESCRIPTOR pSecurityDescriptor;
+    BOOL b;
+    b = SetSecurityDescriptorDacl(pSecurityDescriptor,
+        TRUE,       // Dacl Present
+        NULL,       // NULL pointer to DACL  == BUG
+        FALSE);
+
+    PACL pDacl = NULL;
+    b = SetSecurityDescriptorDacl(pSecurityDescriptor,
+        TRUE,       // Dacl Present
+        pDacl,      // NULL pointer to DACL  == BUG
+        FALSE);
+
+    SetEntriesInAcl(0, NULL, NULL, &pDacl);
+    b = SetSecurityDescriptorDacl(pSecurityDescriptor,
+        TRUE,       // Dacl Present
+        pDacl,      // Should have been set by SetEntriesInAcl ==> should not be flagged
+        FALSE);
+
+    b = SetSecurityDescriptorDacl(pSecurityDescriptor,
+        FALSE,      // Dacl is not Present
+        NULL,       // DACL is going to be removed from security descriptor. Default/inherited access ==> should not be flagged
+        FALSE);
+
+}

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

@@ -0,0 +1,2 @@
+| UnsafeDaclSecurityDescriptor.cpp:70:9:70:33 | call to SetSecurityDescriptorDacl | Setting a DACL to NULL in a SECURITY_DESCRIPTOR will result in an unprotected object. |
+| 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

@@ -0,0 +1 @@
+Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql