From 48c99fb1d11db7c1d17ec203b691afaf10d3cf32 Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Thu, 20 Sep 2018 16:28:37 -0700 Subject: [PATCH 1/8] =?UTF-8?q?Setting=20a=20SECURITY=5FDESCRIPTOR?= =?UTF-8?q?=E2=80=99s=20DACL=20to=20NULL=20Closing=20the=20gap=20between?= =?UTF-8?q?=20Semmle=20&=20PreFAST=20This=20rule=20is=20equivalent=20to=20?= =?UTF-8?q?C6248?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .gitignore | 4 + .../CWE-732/UnsafeDaclSecurityDescriptor.cpp | 14 +++ .../UnsafeDaclSecurityDescriptor.qhelp | 29 ++++++ .../CWE-732/UnsafeDaclSecurityDescriptor.ql | 61 +++++++++++++ .../CWE-732/UnsafeDaclSecurityDescriptor.cpp | 91 +++++++++++++++++++ .../UnsafeDaclSecurityDescriptor.expected | 2 + .../UnsafeDaclSecurityDescriptor.qlref | 1 + 7 files changed, 202 insertions(+) create mode 100644 cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp create mode 100644 cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp create mode 100644 cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected create mode 100644 cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qlref diff --git a/.gitignore b/.gitignore index 31f8ccd9abf2..f77f6b11d935 100644 --- a/.gitignore +++ b/.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 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..590f1482e8f6 --- /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..79e2285201aa --- /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 SECURITY_DESCRIPTOR's DACL field 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..19551a46c5be --- /dev/null +++ b/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 \ 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..9fe34cf5d5cc --- /dev/null +++ b/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); + +} \ 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..dd4b1db43c11 --- /dev/null +++ b/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. | 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 From 75ef377ac1b14483186f58bd6fdc2c7d34535097 Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Fri, 21 Sep 2018 11:34:22 -0700 Subject: [PATCH 2/8] Replace Unicode apostrophe with ANSI single quote --- .gitignore | 3 +++ .../Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql | 8 ++++---- .../CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 4b055e55a091..effd82ac428a 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,6 @@ /.vs/ql/v15/Browse.VC.opendb /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json +/.vs/ql5/v15/Browse.VC.opendb +/.vs/ql5/v15/Browse.VC.db +/.vs/ql5/v15/.suo diff --git a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql index 19551a46c5be..a7d17f4de329 100644 --- a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql +++ b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql @@ -1,6 +1,6 @@ /** - * @name Setting a SECURITY_DESCRIPTOR’s DACL to NULL - * @description Setting a SECURITY_DESCRIPTOR’s DACL to NULL will result in an unprotected object. + * @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. @@ -49,12 +49,12 @@ class SetSecurityDescriptorDaclFunctionConfiguration extends DataFlow::Configura from SetSecurityDescriptorDaclFunctionCall call, string message where exists( NullValue nullExpr | - message = "Setting a SECURITY_DESCRIPTOR’s DACL to NULL will result in an unprotected object." | + 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." | + 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)) ) 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 index dd4b1db43c11..b759bd907d83 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected @@ -1,2 +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. | +| UnsafeDaclSecurityDescriptor.cpp:69:6:69:30 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR's DACL to NULL will result in an unprotected object. | +| UnsafeDaclSecurityDescriptor.cpp:75:6:75:30 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR's DACL using variable pDacl that is set to NULL will result in an unprotected object. | From c22787293e225a2bbda375b4e8a54779866399a2 Mon Sep 17 00:00:00 2001 From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com> Date: Fri, 21 Sep 2018 11:35:43 -0700 Subject: [PATCH 3/8] Update .gitignore --- .gitignore | 3 --- 1 file changed, 3 deletions(-) diff --git a/.gitignore b/.gitignore index effd82ac428a..4b055e55a091 100644 --- a/.gitignore +++ b/.gitignore @@ -12,6 +12,3 @@ /.vs/ql/v15/Browse.VC.opendb /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json -/.vs/ql5/v15/Browse.VC.opendb -/.vs/ql5/v15/Browse.VC.db -/.vs/ql5/v15/.suo From 8519f1a9e19d303b0ba9da9b83b1b2416984c355 Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Fri, 21 Sep 2018 13:07:39 -0700 Subject: [PATCH 4/8] Fixing tabs replaced to spaces --- .../CWE-732/UnsafeDaclSecurityDescriptor.cpp | 12 +-- .../CWE-732/UnsafeDaclSecurityDescriptor.cpp | 102 +++++++++--------- 2 files changed, 57 insertions(+), 57 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp index 590f1482e8f6..1324185481f4 100644 --- a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp +++ b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp @@ -3,12 +3,12 @@ SECURITY_ATTRIBUTES SA; if (!InitializeSecurityDescriptor(&pSD, SECURITY_DESCRIPTOR_REVISION)) { - // error handling + // 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)) +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 + // error handling } 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 index 9fe34cf5d5cc..62de434be970 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp @@ -1,91 +1,91 @@ typedef unsigned long DWORD; -typedef unsigned long ULONG; +typedef unsigned long ULONG; typedef unsigned char BYTE; typedef unsigned short WORD; typedef int BOOL; -typedef void *PVOID; +typedef void *PVOID; #define TRUE 1 -#define FALSE 0 +#define FALSE 0 #define ERROR_SUCCESS 0L -#define NULL 0 +#define NULL 0 typedef PVOID PSECURITY_DESCRIPTOR; typedef struct _ACL { - BYTE AclRevision; - BYTE Sbz1; - WORD AclSize; - WORD AceCount; - WORD Sbz2; + 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 + 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; + 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 + PSECURITY_DESCRIPTOR pSecurityDescriptor, + BOOL bDaclPresent, + PACL pDacl, + BOOL bDaclDefaulted ) { - return TRUE; + return TRUE; } DWORD SetEntriesInAcl( - ULONG cCountOfExplicitEntries, - PEXPLICIT_ACCESS_W pListOfExplicitEntries, - PACL OldAcl, - PACL *NewAcl + ULONG cCountOfExplicitEntries, + PEXPLICIT_ACCESS_W pListOfExplicitEntries, + PACL OldAcl, + PACL *NewAcl ) { - *NewAcl = (PACL)0xFFFFFF; - return ERROR_SUCCESS; + *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); + 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); + 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); + 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); + 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 From 925c3b51f9a601c6d1e84ed617bf8e158d8a863c Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Fri, 21 Sep 2018 15:21:07 -0700 Subject: [PATCH 5/8] Adding semmle-extractor-options: --microsoft to test --- .gitignore | 3 +++ .../Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp | 1 + 2 files changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index 4b055e55a091..effd82ac428a 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,6 @@ /.vs/ql/v15/Browse.VC.opendb /.vs/ql/v15/Browse.VC.db /.vs/ProjectSettings.json +/.vs/ql5/v15/Browse.VC.opendb +/.vs/ql5/v15/Browse.VC.db +/.vs/ql5/v15/.suo 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 index 62de434be970..8c5e0e632d84 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp @@ -1,3 +1,4 @@ +// semmle-extractor-options: --microsoft typedef unsigned long DWORD; typedef unsigned long ULONG; typedef unsigned char BYTE; From c75019726cfd31c594f1179a32497af3d45f517e Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Mon, 24 Sep 2018 10:10:58 -0700 Subject: [PATCH 6/8] Removing tabs & reformatting --- .../CWE-732/UnsafeDaclSecurityDescriptor.ql | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql index a7d17f4de329..d2165187b423 100644 --- a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql +++ b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql @@ -32,15 +32,17 @@ class SetSecurityDescriptorDaclFunctionConfiguration extends DataFlow::Configura SetSecurityDescriptorDaclFunctionConfiguration() { this = "SetSecurityDescriptorDaclFunctionConfiguration" } - + override predicate isSource(DataFlow::Node source) { - exists( NullValue nullExpr | + exists( + NullValue nullExpr | source.asExpr() = nullExpr ) } override predicate isSink(DataFlow::Node sink) { - exists( SetSecurityDescriptorDaclFunctionCall call, VariableAccess val | + exists( + SetSecurityDescriptorDaclFunctionCall call, VariableAccess val | val = sink.asExpr() | val = call.getArgument(2) ) @@ -48,14 +50,18 @@ class SetSecurityDescriptorDaclFunctionConfiguration extends DataFlow::Configura } 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)) - ) +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 \ No newline at end of file From a566ffae4a18d0dbb253e1befaf0c90d19b26d0b Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Mon, 24 Sep 2018 10:18:39 -0700 Subject: [PATCH 7/8] Fixed the test .expected file --- .../CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index b759bd907d83..41b2e91da4cf 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected @@ -1,2 +1,2 @@ -| UnsafeDaclSecurityDescriptor.cpp:69:6:69:30 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR's DACL to NULL will result in an unprotected object. | -| UnsafeDaclSecurityDescriptor.cpp:75:6:75:30 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR's DACL using variable pDacl that is set to NULL will result in an unprotected object. | +| UnsafeDaclSecurityDescriptor.cpp:70:9:70:33 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR\u2019s DACL to NULL will result in an unprotected object. | +| UnsafeDaclSecurityDescriptor.cpp:76:9:76:33 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR\u2019s DACL using variable pDacl that is set to NULL will result in an unprotected object. | From d6d27df27b87303059f7269fd890af7c467f37ad Mon Sep 17 00:00:00 2001 From: Raul Garcia Date: Tue, 25 Sep 2018 10:50:34 -0700 Subject: [PATCH 8/8] Removing all usage of single quotes --- .../CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp | 2 +- .../Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql | 8 ++++---- .../CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp index 79e2285201aa..519d21fd8c17 100644 --- a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp +++ b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp @@ -4,7 +4,7 @@ -

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

    +

    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.

    diff --git a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql index d2165187b423..068c4e442891 100644 --- a/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql +++ b/cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql @@ -1,6 +1,6 @@ /** - * @name Setting a SECURITY_DESCRIPTOR's DACL to NULL - * @description Setting a SECURITY_DESCRIPTOR's DACL to NULL will result in an unprotected object. + * @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. @@ -53,14 +53,14 @@ from SetSecurityDescriptorDaclFunctionCall call, string message where exists ( NullValue nullExpr | - message = "Setting a SECURITY_DESCRIPTOR's DACL to NULL will result in an unprotected object." | + 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 SECURITY_DESCRIPTOR's DACL using variable " + var + " that is set to NULL will result in an unprotected object." | + 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)) ) 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 index 41b2e91da4cf..e596bae3a0f0 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected @@ -1,2 +1,2 @@ -| UnsafeDaclSecurityDescriptor.cpp:70:9:70:33 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR\u2019s DACL to NULL will result in an unprotected object. | -| UnsafeDaclSecurityDescriptor.cpp:76:9:76:33 | call to SetSecurityDescriptorDacl | Setting a SECURITY_DESCRIPTOR\u2019s DACL using variable pDacl that is set to NULL will result in an unprotected object. | +| 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. |