diff --git a/.gitignore b/.gitignore index fdcd174a97c6..7e82b2f488ca 100644 --- a/.gitignore +++ b/.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 + diff --git a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp new file mode 100644 index 000000000000..1324185481f4 --- /dev/null +++ b/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 +} diff --git a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp new file mode 100644 index 000000000000..519d21fd8c17 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp @@ -0,0 +1,29 @@ + + + + +

This query indicates that a call is setting the DACL field in a SECURITY_DESCRIPTOR to null.

+

When using SetSecurityDescriptorDacl to set a discretionary access control (DACL), setting the bDaclPresent argument to TRUE indicates the prescence of a DACL in the security description in the argument pDacl.

+

When the pDacl parameter does not point to a DACL (i.e. it is NULL) and the bDaclPresent flag is TRUE, a NULL DACL is specified.

+

A NULL DACL grants full access to any user who requests it; normal security checking is not performed with respect to the object.

+
+ + +

You should not use a NULL DACL with an object because any user can change the DACL and owner of the security descriptor.

+
+ + +

In the following example, the call to SetSecurityDescriptorDacl is setting an unsafe DACL (NULL DACL) to the security descriptor.

+ + +

To fix this issue, pDacl argument should be a pointer to an ACL structure that specifies the DACL for the security descriptor.

+
+ + +
  • SetSecurityDescriptorDacl function (Microsoft documentation). +
  • +
    + +
    diff --git a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql new file mode 100644 index 000000000000..068c4e442891 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp new file mode 100644 index 000000000000..8c5e0e632d84 --- /dev/null +++ b/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); + +} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected new file mode 100644 index 000000000000..e596bae3a0f0 --- /dev/null +++ b/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. | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qlref new file mode 100644 index 000000000000..6d8a0fc40192 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql \ No newline at end of file