diff --git a/clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp b/clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp index 96c42f8ba01c..a33271fc653d 100644 --- a/clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp +++ b/clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp @@ -22,14 +22,18 @@ using namespace clang; namespace { class VISIBILITY_HIDDEN WalkAST : public StmtVisitor { 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(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: +//===----------------------------------------------------------------------===// + +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(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. //===----------------------------------------------------------------------===// diff --git a/clang/test/Analysis/security-syntax-checks.m b/clang/test/Analysis/security-syntax-checks.m index 7e2a03d81cb5..df17926a15e0 100644 --- a/clang/test/Analysis/security-syntax-checks.m +++ b/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 // 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}} } + +// 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}} +}