[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske closed 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/91445

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 01/10] [clang][analyzer] Add checker
 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/91445

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 1/9] [clang][analyzer] Add checker
 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,75 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-analyzer-output=text -verify %s
+
+typedef int uid_t;
+typedef int gid_t;
+
+int setuid(uid_t);
+int setgid(gid_t);
+
+uid_t getuid();
+gid_t getgid();
+
+
+
+void test_note_1() {
+  if (setuid(getuid()) == -1) // expected-note{{Assuming the condition is 
false}} \
+  // expected-note{{Taking false branch}}
+return;
+  if (setuid(getuid()) == -1) // expected-note{{Call to 'setuid' found here 
that removes superuser privileges}} \
+  // expected-note{{Assuming the condition is 
false}} \
+  // expected-note{{Taking false branch}}
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}} \
+  // expected-note{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+}
+
+void test_note_2() {
+  if (setuid(getuid()) == -1) // expected-note{{Call to 'setuid' found here 
that removes superuser privileges}} \
+  // expected-note{{Assuming the condition is 
false}} \
+  // expected-note{{Taking false branch}} \
+  // expected-note{{Assuming the condition is 
false}} \
+  // expected-note{{Taking false branch}}

steakhal wrote:

```suggestion
  // expected-note 2 {{Assuming the condition is 
false}} \
  // expected-note 2 {{Taking false branch}}
```

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal approved this pull request.

LGTM now, modulo the license concern mentioned 
[here](https://github.com/llvm/llvm-project/pull/91445#discussion_r1599766388).
I approve this, given that is checked/resolved.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/91445

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 1/8] [clang][analyzer] Add checker
 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker : public Checker 
{
+  const BugType BT{this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  const CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+  const BugType *getBT() const { return  }
+
+private:
+  void processSetuid(ProgramStateRef State, const CallEvent ,
+ CheckerContext ) const;
+  void processSetgid(ProgramStateRef State, const CallEvent ,
+ CheckerContext ) const;
+  void processOther(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call), and for identification of note
+/// tags.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+processOther(State, Call, C);
+  }
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {
+  SValBuilder  = State->getStateManager().getSValBuilder();
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return State;
+
+  // Check if the most recent call to 'setuid(getuid())' is assumed to be != 0.
+  // It should be only -1 at failure, but we want to accept a "!= 0" check too.
+  // (But now an invalid failure check like "!= 1" will be recognized as 
correct
+  // too. The "invalid failure check" is a different bug that is not the 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker : public Checker 
{
+  const BugType BT{this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  const CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+  const BugType *getBT() const { return  }

steakhal wrote:

I guess you dont need this.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal requested changes to this pull request.

NoteTags, yeey.

Please add tests for the Notes added by the NoteTag callbacks. Inother words, 
add a test with the text diagnostic output.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

balazske wrote:

I added the `NoteTag` support now (instead of a next PR). The 
`checkDeadSymbols` is removed, it does really not matter if the data remains in 
the GDM and this way it is used to display the note tag only for the last 
`setuid` call.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/91445

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 1/7] [clang][analyzer] Add checker
 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+

steakhal wrote:

I think the docs should explicitly mention how to fix/suppress this issue.
Like: One should call `setgid(getgid())` first and then `setuid(getuid())`.
Maybe also mention to pay attention to not mix up the APIs so that user ID and 
group IDs are not swapped, etc. Also add a remark to not forget to check the 
return value of these APIs. 

And this example looks suspiciously similar to the one present in the CERT docs.
Does it raise license and copy-right issues? If that's a concern, please rework 
the example.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal approved this pull request.

I think this looks good now.
I think to really reach the full potential of this checker, we must have a 
visitor/note tag for highlighting the places of the last `setgid` or `setuid` 
call in the report.

I also think that the docs could be improved.

Anyways. I think it's already pretty good, and I don't plan to do another round 
of reviews.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,

steakhal wrote:

```suggestion
  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and "
   "'setuid(getuid())' (CERT: POS36-C)">,
```
The previous line-brake seemed so arbitrary. This way we at least obey the 80 
column rule.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -30,23 +30,20 @@ enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid 
};
 
 class SetgidSetuidOrderChecker
 : public Checker {
-  const BugType BT_WrongRevocationOrder{
-  this, "Possible wrong order of privilege revocation"};
+  const BugType BT{this, "Possible wrong order of privilege revocation"};
 
   const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
   const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
 
   const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
   const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
 
-  CallDescriptionSet OtherSetPrivilegeDesc{
+  CallDescriptionSet const OtherSetPrivilegeDesc{

steakhal wrote:

I think in LLVM we usually use west-const.
```suggestion
  const CallDescriptionSet OtherSetPrivilegeDesc{
```

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/91445

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 1/6] [clang][analyzer] Add checker
 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});

balazske wrote:

I prefer this way because for me it is not obvious that `SymbolRef` is a 
pointer.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

@@ -0,0 +1,185 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+typedef int uid_t;
+typedef int gid_t;
+
+int setuid(uid_t);
+int setgid(gid_t);
+int seteuid(uid_t);
+int setegid(gid_t);
+int setreuid(uid_t, uid_t);
+int setregid(gid_t, gid_t);
+int setresuid(uid_t, uid_t, uid_t);
+int setresgid(gid_t, gid_t, gid_t);
+
+uid_t getuid();
+gid_t getgid();
+
+
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void incorrect_order() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void warn_at_second_time() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+}
+
+uid_t f_uid();
+gid_t f_gid();
+
+void setuid_other() {
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_other() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(f_gid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setuid_other_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_with_getuid() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getuid()) == -1)
+return;
+}
+
+void setuid_with_getgid() {
+  if (setuid(getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+int f_setuid() {
+  return setuid(getuid());
+}
+
+int f_setgid() {
+  return setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+}
+
+void function_calls() {
+  if (f_setuid() == -1)
+return;
+  if (f_setgid() == -1)
+return;
+}
+
+void seteuid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (seteuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setegid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setegid(getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setreuid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setreuid(getuid(), getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setregid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setregid(getgid(), getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setresuid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setresuid(getuid(), getuid(), getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setresgid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setresgid(getgid(), getgid(), getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void other_system_function_between() {
+  if (setuid(getuid()) == -1)
+return;
+  gid_t g = getgid();
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+}
+
+void f_extern();
+
+void other_unknown_function_between() {
+  if (setuid(getuid()) == -1)
+return;
+  f_extern();
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}

balazske wrote:

This was a question for me to filter out this case from warning generation. But 
it is likely that if the extern function calls the `setgid(getgid())` there 
should be not a next call to this again.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""

steakhal wrote:

I don't have a preference, I was just asking.
I just felt this is a natural question, and you seem to care about alpha 
checkers and documentation/discoverability, and this seemed like a subject to 
discuss. @NagyDonat WDYT?

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balázs Kéri via cfe-commits]

@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""

balazske wrote:

Move the new checker into `unix`, or move the chroot checker into `security` 
(or `alpha.security`)?

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder' (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 


https://github.com/steakhal edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,185 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+typedef int uid_t;
+typedef int gid_t;
+
+int setuid(uid_t);
+int setgid(gid_t);
+int seteuid(uid_t);
+int setegid(gid_t);
+int setreuid(uid_t, uid_t);
+int setregid(gid_t, gid_t);
+int setresuid(uid_t, uid_t, uid_t);
+int setresgid(gid_t, gid_t, gid_t);
+
+uid_t getuid();
+gid_t getgid();
+
+
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void incorrect_order() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void warn_at_second_time() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+}
+
+uid_t f_uid();
+gid_t f_gid();
+
+void setuid_other() {
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_other() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(f_gid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setuid_other_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_with_getuid() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getuid()) == -1)
+return;
+}
+
+void setuid_with_getgid() {
+  if (setuid(getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+int f_setuid() {
+  return setuid(getuid());
+}
+
+int f_setgid() {
+  return setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+}
+
+void function_calls() {
+  if (f_setuid() == -1)
+return;
+  if (f_setgid() == -1)
+return;
+}
+
+void seteuid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (seteuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setegid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setegid(getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setreuid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setreuid(getuid(), getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setregid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setregid(getgid(), getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setresuid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setresuid(getuid(), getuid(), getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setresgid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setresgid(getgid(), getgid(), getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void other_system_function_between() {
+  if (setuid(getuid()) == -1)
+return;
+  gid_t g = getgid();

steakhal wrote:

Did you mean to use `g` on the next line? If not, then have you considered just 
casting this to void instead of introducing a new variable?

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""

steakhal wrote:

Give that the 
[`alpha.unix.chroot`](https://clang.llvm.org/docs/analyzer/checkers.html#alpha-unix-chroot-c)
 checker does something similar, I wonder if this checker should share the same 
parent package with that one to aid discoverability.
WDYT?

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+   

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{

steakhal wrote:

Why is this field not constant, like the others?

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}

steakhal wrote:

Couldn't we just omit this, and leave the compiler generate this for us?

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,185 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+typedef int uid_t;
+typedef int gid_t;
+
+int setuid(uid_t);
+int setgid(gid_t);
+int seteuid(uid_t);
+int setegid(gid_t);
+int setreuid(uid_t, uid_t);
+int setregid(gid_t, gid_t);
+int setresuid(uid_t, uid_t, uid_t);
+int setresgid(gid_t, gid_t, gid_t);
+
+uid_t getuid();
+gid_t getgid();
+
+
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void incorrect_order() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void warn_at_second_time() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+}
+
+uid_t f_uid();
+gid_t f_gid();
+
+void setuid_other() {
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_other() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(f_gid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setuid_other_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_with_getuid() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getuid()) == -1)
+return;
+}
+
+void setuid_with_getgid() {
+  if (setuid(getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+int f_setuid() {
+  return setuid(getuid());
+}
+
+int f_setgid() {
+  return setgid(getgid()); // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+}
+
+void function_calls() {
+  if (f_setuid() == -1)
+return;
+  if (f_setgid() == -1)
+return;
+}
+
+void seteuid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (seteuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setegid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setegid(getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setreuid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setreuid(getuid(), getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setregid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setregid(getgid(), getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setresuid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setresuid(getuid(), getuid(), getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setresgid_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setresgid(getgid(), getgid(), getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void other_system_function_between() {
+  if (setuid(getuid()) == -1)
+return;
+  gid_t g = getgid();
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+}
+
+void f_extern();
+
+void other_unknown_function_between() {
+  if (setuid(getuid()) == -1)
+return;
+  f_extern();
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}

steakhal wrote:

I was thinking about this for a minute. I think it's fine, but I'll leave here 
my thought anyway:
Technically, `f_extern` could have already drop the privileges. But I get it 
that would imply that this checker is basically useless after the first opaque 
call - which is not practical.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});

steakhal wrote:

```suggestion
  State = State->set(nullptr);
```

This comes up a few times in this patch. Personally, I prefer to explicitly say 
"nullptr" is something is that - and I'm not writing 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+   

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,185 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+typedef int uid_t;
+typedef int gid_t;
+
+int setuid(uid_t);
+int setgid(gid_t);
+int seteuid(uid_t);
+int setegid(gid_t);
+int setreuid(uid_t, uid_t);
+int setregid(gid_t, gid_t);
+int setresuid(uid_t, uid_t, uid_t);
+int setresgid(gid_t, gid_t, gid_t);
+
+uid_t getuid();
+gid_t getgid();
+
+
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void incorrect_order() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void warn_at_second_time() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}

steakhal wrote:

I'm not sure of the value proposition with this second warning.

To me, the code has a sequence (the middle 2 calls), that should satisfy the 
CERT rule.
After seeing a "good" pattern, I'd expect the checker to ignore the rest, as we 
already dropped the privileges, right?

So the question is: is it valuable to report this?

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+   

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,170 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+#include "Inputs/system-header-simulator-setgid-setuid.h"
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void incorrect_order() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void warn_at_second_time() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+}
+
+uid_t f_uid();
+gid_t f_gid();
+
+void setuid_other() {
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_other() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(f_gid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setuid_other_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_with_getuid() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getuid()) == -1)
+return;
+}
+
+void setuid_with_getgid() {
+  if (setuid(getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}

steakhal wrote:

Tidy would a nice fit, yes.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+   

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,170 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+#include "Inputs/system-header-simulator-setgid-setuid.h"
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;

steakhal wrote:

I also find it weird that we start the test file with an edge-case :D

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+   

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());

steakhal wrote:

```suggestion
  C.addTransition(State);
```

https://github.com/llvm/llvm-project/pull/91445

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balazs Benics via cfe-commits]

=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= ,
=?utf-8?q?Balázs_Kéri?= 
Message-ID:
In-Reply-To: 



@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+   

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat commented:

Thanks for updating your commit! Now there are only two remaining issues and 
they are both very minor (marked by inline comments: renaming `CallExpr *CE` 
and explaining the reason why "trying to set the gid again" appears as a 
special case in the SEI-CERT standard).

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
+  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
+
+  const CallDescription GetuidDesc{CDM::CLibrary, {"getuid"}, 0};
+  const CallDescription GetgidDesc{CDM::CLibrary, {"getgid"}, 0};
+
+  CallDescriptionSet OtherSetPrivilegeDesc{
+  {CDM::CLibrary, {"seteuid"}, 1},   {CDM::CLibrary, {"setegid"}, 1},
+  {CDM::CLibrary, {"setreuid"}, 2},  {CDM::CLibrary, {"setregid"}, 2},
+  {CDM::CLibrary, {"setresuid"}, 3}, {CDM::CLibrary, {"setresgid"}, 3}};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(const CallDescription ,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (OtherSetPrivilegeDesc.contains(Call)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {
+  SValBuilder  = State->getStateManager().getSValBuilder();
+  SymbolRef 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,170 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+#include "Inputs/system-header-simulator-setgid-setuid.h"
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;

NagyDonat wrote:

But why do the SEI-CERT best practices mandate that "this should not be 
recognized as an error"? Could you briefly explain this in a comment? (E.g. 
"Special case: calling `setgid(getgid())` after an earlier `setgid(getgid()); 
setuid(getuid())` combination is legitimate, because it... ")

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};

NagyDonat wrote:

`BT` is fairly traditional in the codebase: there are 40+ checker source files 
that use it as a variable name for _the_ bug type. However, this longer name is 
also fine if you prefer it.  

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Endre Fülöp via cfe-commits]

https://github.com/gamesh411 approved this pull request.

Emitting a note to the location where the first part of the detected pattern 
(the `setuid(getuid())` call) seems like useful information, but this patch is 
great as it is.
You could also add it in another patch if it is not trivial.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {
+  SValBuilder  = State->getStateManager().getSValBuilder();
+  SymbolRef LastSetuidSym = 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

@@ -0,0 +1,170 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+#include "Inputs/system-header-simulator-setgid-setuid.h"
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;

balazske wrote:

Here is a sequence of `setuid(getuid())` and `setgid(getgid())` in the code, 
but still this should be not recognized as error.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/91445

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 1/5] [clang][analyzer] Add checker
 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

balazske wrote:

Is it useful to add a note tag to the previous `setuid(getuid())` call? It can 
be (theoretically) in another function or otherwise in a remote place in the 
source code.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske deleted 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };

balazske wrote:

This position is used because it is before GDM registrations where the type is 
used. (If it must be used in a function it must be moved before the checker 
anyway.)

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};

balazske wrote:

`BT` does not look informative enough, and `BugType` is not usable as name. I 
do not see a problem with this name except that it is a bit long.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/91445

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 1/4] [clang][analyzer] Add checker
 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {

NagyDonat wrote:

@balazske Thanks for the explanation.

This question is 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat commented:

You forgot to add `CDM::CLibrary` in the definition of `SetuidDesc` and 
`SetgidDesc` (see the new inline comment).

There are also several inline comments from my previous review where I'm 
expecting an answer (not necessarily a code change -- in each case I can accept 
the current solution if you would prefer it). 

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,196 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};

NagyDonat wrote:

```suggestion
  const CallDescription SetuidDesc{CDM::CLibrary, {"setuid"}, 1};
  const CallDescription SetgidDesc{CDM::CLibrary, {"setgid"}, 1};
```

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/91445

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 1/3] [clang][analyzer] Add checker
 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {

balazske wrote:

In this case it would be possible to examine what is 

[clang] [clang][analyzer] Add checker 'security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {
+  SValBuilder  = State->getStateManager().getSValBuilder();
+  SymbolRef LastSetuidSym = 

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};

NagyDonat wrote:

Please use the `MatchAs` argument of the `CallDescription` constructor, which 
was recently introduced by my changes. In this case you'd probably use the 
matching mode `CDM::CLibrary`.

Without this these call descriptions would match even a C++ method if it has 
the right name and number of arguments.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };

NagyDonat wrote:

Perhaps place this `enum` declaration above the checker class. (When I was 
looking for its definition, I automatically scrolled to the top, and I was 
surprised that I didn't see it there.)

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};

NagyDonat wrote:

Use `BT` instead of this long variable name -- this is the only bug type in 
this class.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {

NagyDonat wrote:

It is very strange that this `evalAssume` callback 

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {
+  SValBuilder  = State->getStateManager().getSValBuilder();
+  SymbolRef LastSetuidSym = 

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+SetregidDesc, SetresuidDesc, SetresgidDesc)) {

NagyDonat wrote:

`matchesAny` is a nice tool, but it would be even better to use a 
`CallDescriptionSet` and its `contains` method, because then you wouldn't need 
to introduce individual variable names for these call descriptions. 

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,170 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+#include "Inputs/system-header-simulator-setgid-setuid.h"
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void incorrect_order() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void warn_at_second_time() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1) // expected-warning{{A 'setgid(getgid())' call 
following a 'setuid(getuid())' call is likely to fail}}
+return;
+}
+
+uid_t f_uid();
+gid_t f_gid();
+
+void setuid_other() {
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_other() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(f_gid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setuid_other_between() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setuid(f_uid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}
+
+void setgid_with_getuid() {
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getuid()) == -1)
+return;
+}
+
+void setuid_with_getgid() {
+  if (setuid(getgid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;
+}

NagyDonat wrote:

By  the way it would be nice to create an unrelated checker that emits warnings 
for these, because these are very hard to spot and they could be typos.

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {
+  SValBuilder  = State->getStateManager().getSValBuilder();
+  SymbolRef LastSetuidSym = 

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,170 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,security.SetgidSetuidOrder 
-verify %s
+
+#include "Inputs/system-header-simulator-setgid-setuid.h"
+
+void correct_order() {
+  if (setgid(getgid()) == -1)
+return;
+  if (setuid(getuid()) == -1)
+return;
+  if (setgid(getgid()) == -1)
+return;

NagyDonat wrote:

Why is this block here? Should this "try to set the gid again" attempt appear 
in normal code?

(I see that this is intentionally allowed in the checker, but it would be good 
to add a short comment that explains the rationale behind this.)

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

@@ -0,0 +1,197 @@
+//===-- SetgidSetuidOrderChecker.cpp - check privilege revocation calls 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines a checker to detect possible reversed order of privilege
+//  revocations when 'setgid' and 'setuid' is used.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+  this, "Possible wrong order of privilege revocation"};
+
+  const CallDescription SetuidDesc{{"setuid"}, 1};
+  const CallDescription SetgidDesc{{"setgid"}, 1};
+  const CallDescription SeteuidDesc{{"seteuid"}, 1};
+  const CallDescription SetegidDesc{{"setegid"}, 1};
+  const CallDescription SetreuidDesc{{"setreuid"}, 2};
+  const CallDescription SetregidDesc{{"setregid"}, 2};
+  const CallDescription SetresuidDesc{{"setresuid"}, 3};
+  const CallDescription SetresgidDesc{{"setresgid"}, 3};
+
+public:
+  SetgidSetuidOrderChecker() {}
+
+  void checkPostCall(const CallEvent , CheckerContext ) const;
+  void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+  ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
+private:
+  ProgramStateRef processSetuid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processSetgid(ProgramStateRef State, const CallEvent ,
+CheckerContext ) const;
+  ProgramStateRef processOther(ProgramStateRef State, const CallEvent ,
+   CheckerContext ) const;
+  /// Check if a function like \c getuid or \c getgid is called directly from
+  /// the first argument of function called from \a Call.
+  bool isFunctionCalledInArg(llvm::StringRef FnName,
+ const CallEvent ) const;
+  void emitReport(ProgramStateRef State, CheckerContext ) const;
+};
+
+enum SetPrivilegeFunctionKind { Irrelevant, Setuid, Setgid };
+
+} // end anonymous namespace
+
+/// Store if there was a call to 'setuid(getuid())' or 'setgid(getgid())' not
+/// followed by other different privilege-change functions.
+/// If the value \c Setuid is stored and a 'setgid(getgid())' call is found we
+/// have found the bug to be reported. Value \c Setgid is used too to prevent
+/// warnings at a setgid-setuid-setgid sequence.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetPrivilegeCall, 
SetPrivilegeFunctionKind)
+/// Store the symbol value of the last 'setuid(getuid())' call. This is used to
+/// detect if the result is compared to -1 and avoid warnings on that branch
+/// (which is the failure branch of the call).
+REGISTER_TRAIT_WITH_PROGRAMSTATE(LastSetuidCallSVal, SymbolRef)
+
+void SetgidSetuidOrderChecker::checkPostCall(const CallEvent ,
+ CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+  if (SetuidDesc.matches(Call)) {
+State = processSetuid(State, Call, C);
+  } else if (SetgidDesc.matches(Call)) {
+State = processSetgid(State, Call, C);
+  } else if (matchesAny(Call, SeteuidDesc, SetegidDesc, SetreuidDesc,
+SetregidDesc, SetresuidDesc, SetresgidDesc)) {
+State = processOther(State, Call, C);
+  }
+  if (State)
+C.addTransition(State);
+}
+
+void SetgidSetuidOrderChecker::checkDeadSymbols(SymbolReaper ,
+CheckerContext ) const {
+  ProgramStateRef State = C.getState();
+
+  SymbolRef LastSetuidSym = State->get();
+  if (!LastSetuidSym)
+return;
+
+  if (!SymReaper.isDead(LastSetuidSym))
+return;
+
+  State = State->set(SymbolRef{});
+  C.addTransition(State, C.getPredecessor());
+}
+
+ProgramStateRef SetgidSetuidOrderChecker::evalAssume(ProgramStateRef State,
+ SVal Cond,
+ bool Assumption) const {
+  SValBuilder  = State->getStateManager().getSValBuilder();
+  SymbolRef LastSetuidSym = 

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat commented:

It's good to see that this checker is finished. I added several inline 
comments, but they are not serious issues -- most are connected to 
`CallDescription`s where I'm now very familiar with the available options (and 
I refactored the code, so others are not so familiar with it...).

https://github.com/llvm/llvm-project/pull/91445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/91445

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH 1/2] [clang][analyzer] Add checker
 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[via cfe-commits]

llvmbot wrote:



@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Balázs Kéri (balazske)


Changes



---
Full diff: https://github.com/llvm/llvm-project/pull/91445.diff


6 Files Affected:

- (modified) clang/docs/analyzer/checkers.rst (+28) 
- (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+5) 
- (modified) clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt (+1) 
- (added) clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp (+196) 
- (added) clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h 
(+15) 
- (added) clang/test/Analysis/setgid-setuid-order.c (+170) 


``diff
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SetgidSetuidOrderChecker
+: public Checker {
+  const BugType BT_WrongRevocationOrder{
+ 

[clang] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'. (PR #91445)

[Balázs Kéri via cfe-commits]

https://github.com/balazske created 
https://github.com/llvm/llvm-project/pull/91445

None

From d839faf7a30851a172d812137b30635c741870f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 May 2024 10:10:24 +0200
Subject: [PATCH] [clang][analyzer] Add checker 'Security.SetgidSetuidOrder'.

---
 clang/docs/analyzer/checkers.rst  |  28 +++
 .../clang/StaticAnalyzer/Checkers/Checkers.td |   5 +
 .../StaticAnalyzer/Checkers/CMakeLists.txt|   1 +
 .../Checkers/SetgidSetuidOrderChecker.cpp | 196 ++
 .../system-header-simulator-setgid-setuid.h   |  15 ++
 clang/test/Analysis/setgid-setuid-order.c | 170 +++
 6 files changed, 415 insertions(+)
 create mode 100644 
clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
 create mode 100644 
clang/test/Analysis/Inputs/system-header-simulator-setgid-setuid.h
 create mode 100644 clang/test/Analysis/setgid-setuid-order.c

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 0d87df36ced0e..d0c0c7a535c62 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1179,6 +1179,34 @@ security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C)
strncpy(buf, "a", 1); // warn
  }
 
+security.SetgidSetuidOrder (C)
+""
+When dropping user-level and group-level privileges in a program by using
+``setuid`` and ``setgid`` calls, it is important to reset the group-level
+privileges (with ``setgid``) first. Function ``setgid`` will likely fail if
+the superuser privileges are already dropped.
+
+The checker checks for sequences of ``setuid(getuid())`` and
+``setgid(getgid())`` calls (in this order). If such a sequence is found and
+there is no other privilege-changing function call (``seteuid``, ``setreuid``,
+``setresuid`` and the GID versions of these) in between, a warning is
+generated. The checker finds only exactly ``setuid(getuid())`` calls (and the
+GID versions), not for example if the result of ``getuid()`` is stored in a
+variable.
+
+This check corresponds to SEI CERT Rule `POS36-C 
`_.
+
+.. code-block:: c
+
+ void test1() {
+   if (setuid(getuid()) == -1) {
+ /* handle error condition */
+   }
+   if (setgid(getgid()) == -1) { // warn
+ /* handle error condition */
+   }
+ }
+
 .. _unix-checkers:
 
 unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fd..cc954828901af 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1011,6 +1011,11 @@ def FloatLoopCounter : Checker<"FloatLoopCounter">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation;
 
+def SetgidSetuidOrderChecker : Checker<"SetgidSetuidOrder">,
+  HelpText<"Warn on possible reversed order of 'setgid(getgid()))' and 
'setuid(getuid())' (CERT: "
+   "POS36-C)">,
+  Documentation;
+
 } // end "security"
 
 let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt 
b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 4443ffd092938..45d3788f105dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -103,6 +103,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
+  SetgidSetuidOrderChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
new file mode 100644
index 0..11cc748cb40b1
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp
@@ -0,0 +1,196 @@
+//===-- ChrootChecker.cpp - chroot usage checks 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"