Setting a SECURITY_DESCRIPTOR’s DACL to NULL

Closing the gap between Semmle & PreFAST
This rule is equivalent to C6248

Author: raulgarciamsft

.gitignore

@@ -8,3 +8,7 @@
 # qltest projects and artifacts
 */ql/test/**/*.testproj
 */ql/test/**/*.actual
+/.vs/slnx.sqlite
+/.vs/ql3/v15/Browse.VC.opendb
+/.vs/ql3/v15/Browse.VC.db
+/.vs/ProjectSettings.json

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 <code>SECURITY_DESCRIPTOR</code>'s DACL field 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,61 @@
+/**
+ * @name Setting a SECURITY_DESCRIPTOR’s DACL to NULL
+ * @description Setting a SECURITY_DESCRIPTOR’s DACL to NULL 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 SECURITY_DESCRIPTOR’s DACL to NULL 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 SECURITY_DESCRIPTOR’s DACL 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,91 @@
+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:69:6:69:30 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR\u2019s DACL to NULL will result in an unprotected object. |
+| UnsafeDaclSecurityDescriptor.cpp:75:6:75:30 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR\u2019s DACL 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