Implement: <rdar://problem/6337132> CWE-273: Failure to Check Whether Privileges

Were Dropped Successfully

Patch by Geoff Keating!

llvm-svn: 80313

clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp

@@ -22,14 +22,18 @@ using namespace clang;
 namespace {
 class VISIBILITY_HIDDEN WalkAST : public StmtVisitor<WalkAST> {
   BugReporter &BR;  
-  IdentifierInfo *II_gets;  
+  IdentifierInfo *II_gets;
+  enum { num_ids = 6 };
+  IdentifierInfo *II_setid[num_ids];
+  
 public:
   WalkAST(BugReporter &br) : BR(br),
-    II_gets(0) {}
+    II_gets(0), II_setid() {}
   
   // Statement visitor methods.
   void VisitCallExpr(CallExpr *CE);
   void VisitForStmt(ForStmt *S);
+  void VisitCompoundStmt (CompoundStmt *S);
   void VisitStmt(Stmt *S) { VisitChildren(S); }
 
   void VisitChildren(Stmt *S);
@@ -40,6 +44,7 @@ public:
   // Checker-specific methods.
   void CheckLoopConditionForFloat(const ForStmt *FS);
   void CheckCall_gets(const CallExpr *CE, const FunctionDecl *FD);
+  void CheckUncheckedReturnValue(CallExpr *CE);
 };
 } // end anonymous namespace
 
@@ -73,6 +78,16 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
   VisitChildren(CE);
 }
 
+void WalkAST::VisitCompoundStmt(CompoundStmt *S) {
+  for (Stmt::child_iterator I = S->child_begin(), E = S->child_end(); I!=E; ++I)
+    if (Stmt *child = *I)
+      {
+	if (CallExpr *CE = dyn_cast<CallExpr>(child))
+	  CheckUncheckedReturnValue(CE);
+	Visit(child);
+      }
+}
+
 void WalkAST::VisitForStmt(ForStmt *FS) {
   CheckLoopConditionForFloat(FS);  
 
@@ -225,6 +240,69 @@ void WalkAST::CheckCall_gets(const CallExpr *CE, const FunctionDecl *FD) {
                      CE->getLocStart(), &R, 1);
 }
 
+//===----------------------------------------------------------------------===//
+// Check: Should check whether privileges are dropped successfully.
+// Originally: <rdar://problem/6337132>
+//===----------------------------------------------------------------------===//
+
+void WalkAST::CheckUncheckedReturnValue(CallExpr *CE) {
+  const FunctionDecl *FD = CE->getDirectCallee();
+  if (!FD)
+    return;
+
+  if (II_setid[0] == NULL) {
+    static const char * const identifiers[num_ids] = {
+      "setuid", "setgid", "seteuid", "setegid",
+      "setreuid", "setregid"
+    };
+      
+    for (size_t i = 0; i < num_ids; i++)
+      II_setid[i] = &BR.getContext().Idents.get(identifiers[i]);  
+  }
+  
+  const IdentifierInfo *id = FD->getIdentifier();
+  size_t identifierid;
+
+  for (identifierid = 0; identifierid < num_ids; identifierid++)
+    if (id == II_setid[identifierid])
+      break;
+
+  if (identifierid >= num_ids)
+    return;
+  
+  const FunctionProtoType *FTP = dyn_cast<FunctionProtoType>(FD->getType());
+  if (!FTP)
+    return;
+  
+  /* Verify that the function takes one or two arguments (depending on
+     the function).  */
+  if (FTP->getNumArgs() != (identifierid < 4 ? 1 : 2))
+    return;
+
+  // The arguments must be integers.
+  for (unsigned i = 0; i < FTP->getNumArgs(); i++)
+    if (! FTP->getArgType(i)->isIntegerType())
+      return;
+
+  // Issue a warning.
+  std::string buf1;
+  llvm::raw_string_ostream os1(buf1);
+  os1 << "Return value is not checked in call to '" << FD->getNameAsString()
+     << "'";
+
+  std::string buf2;
+  llvm::raw_string_ostream os2(buf2);
+  os2 << "The return value from the call to '" << FD->getNameAsString()
+      << "' is not checked.  If an error occurs in '"
+      << FD->getNameAsString()
+      << "', the following code may execute with unexpected privileges";
+
+  SourceRange R = CE->getCallee()->getSourceRange();
+  
+  BR.EmitBasicReport(os1.str().c_str(), "Security", os2.str().c_str(),
+                     CE->getLocStart(), &R, 1);
+}
+
 //===----------------------------------------------------------------------===//
 // Entry point for check.
 //===----------------------------------------------------------------------===//

clang/test/Analysis/security-syntax-checks.m

@@ -1,4 +1,4 @@
-// RUN: clang-cc -analyze -warn-security-syntactic %s -verify
+// RUN: clang-cc -triple i386-apple-darwin10 -analyze -warn-security-syntactic %s -verify
 
 // <rdar://problem/6336718> rule request: floating point used as loop 
 //  condition (FLP30-C, FLP-30-CPP)
@@ -29,3 +29,34 @@ void test_gets() {
   char buff[1024];
   gets(buff); // expected-warning{{Call to function 'gets' is extremely insecure as it can always result in a buffer overflow}}
 }
+
+// <rdar://problem/6337132> CWE-273: Failure to Check Whether Privileges Were
+//  Dropped Successfully
+typedef unsigned int __uint32_t;
+typedef __uint32_t __darwin_uid_t;
+typedef __uint32_t __darwin_gid_t;
+typedef __darwin_uid_t uid_t;
+typedef __darwin_gid_t gid_t;
+int setuid(uid_t);
+int setregid(gid_t, gid_t);
+int setreuid(uid_t, uid_t);
+extern void check(int);
+
+void test_setuid() 
+{
+  setuid(2); // expected-warning{{The return value from the call to 'setuid' is not checked.  If an error occurs in 'setuid', the following code may execute with unexpected privileges}}
+  setuid(0); // expected-warning{{The return value from the call to 'setuid' is not checked.  If an error occurs in 'setuid', the following code may execute with unexpected privileges}}
+  if (setuid (2) != 0)
+    abort();
+
+  // Currently the 'setuid' check is not flow-sensitive, and only looks
+  // at whether the function was called in a compound statement.  This
+  // will lead to false negatives, but there should be no false positives.
+  int t = setuid(2);  // no-warning
+  (void)setuid (2); // no-warning
+
+  check(setuid (2)); // no-warning
+
+  setreuid(2,2); // expected-warning{{The return value from the call to 'setreuid' is not checked.  If an error occurs in 'setreuid', the following code may execute with unexpected privileges}}
+  setregid(2,2); // expected-warning{{The return value from the call to 'setregid' is not checked.  If an error occurs in 'setregid', the following code may execute with unexpected privileges}}
+}