From: Ted Kremenek Date: Wed, 2 Sep 2009 02:47:41 +0000 (+0000) Subject: Implement: CWE-338: Use of cryptographically weak prng X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=2465047c6f5b9a865f63ae1402fccb95abab9e28;p=clang Implement: CWE-338: Use of cryptographically weak prng Patch by Geoff Keating! git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@80752 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Analysis/CheckSecuritySyntaxOnly.cpp b/lib/Analysis/CheckSecuritySyntaxOnly.cpp index 3684360835..1bed9d1a5b 100644 --- a/lib/Analysis/CheckSecuritySyntaxOnly.cpp +++ b/lib/Analysis/CheckSecuritySyntaxOnly.cpp @@ -23,12 +23,15 @@ namespace { class VISIBILITY_HIDDEN WalkAST : public StmtVisitor { BugReporter &BR; IdentifierInfo *II_gets; - enum { num_ids = 6 }; - IdentifierInfo *II_setid[num_ids]; + enum { num_rands = 9 }; + IdentifierInfo *II_rand[num_rands]; + IdentifierInfo *II_random; + enum { num_setids = 6 }; + IdentifierInfo *II_setid[num_setids]; public: WalkAST(BugReporter &br) : BR(br), - II_gets(0), II_setid() {} + II_gets(0), II_rand(), II_random(0), II_setid() {} // Statement visitor methods. void VisitCallExpr(CallExpr *CE); @@ -44,6 +47,8 @@ public: // Checker-specific methods. void CheckLoopConditionForFloat(const ForStmt *FS); void CheckCall_gets(const CallExpr *CE, const FunctionDecl *FD); + void CheckCall_rand(const CallExpr *CE, const FunctionDecl *FD); + void CheckCall_random(const CallExpr *CE, const FunctionDecl *FD); void CheckUncheckedReturnValue(CallExpr *CE); }; } // end anonymous namespace @@ -71,7 +76,9 @@ void WalkAST::VisitChildren(Stmt *S) { void WalkAST::VisitCallExpr(CallExpr *CE) { if (const FunctionDecl *FD = CE->getDirectCallee()) { - CheckCall_gets(CE, FD); + CheckCall_gets(CE, FD); + CheckCall_rand(CE, FD); + CheckCall_random(CE, FD); } // Recurse and check children. @@ -240,6 +247,96 @@ void WalkAST::CheckCall_gets(const CallExpr *CE, const FunctionDecl *FD) { CE->getLocStart(), &R, 1); } +//===----------------------------------------------------------------------===// +// Check: Linear congruent random number generators should not be used +// Originally: +// CWE-338: Use of cryptographically weak prng +//===----------------------------------------------------------------------===// + +void WalkAST::CheckCall_rand(const CallExpr *CE, const FunctionDecl *FD) { + if (II_rand[0] == NULL) { + // This check applies to these functions + static const char * const identifiers[num_rands] = { + "drand48", "erand48", "jrand48", "lrand48", "mrand48", "nrand48", + "lcong48", + "rand", "rand_r" + }; + + for (size_t i = 0; i < num_rands; i++) + II_rand[i] = &BR.getContext().Idents.get(identifiers[i]); + } + + const IdentifierInfo *id = FD->getIdentifier(); + size_t identifierid; + + for (identifierid = 0; identifierid < num_rands; identifierid++) + if (id == II_rand[identifierid]) + break; + + if (identifierid >= num_rands) + return; + + const FunctionProtoType *FTP = dyn_cast(FD->getType()); + if (!FTP) + return; + + if (FTP->getNumArgs() == 1) { + // Is the argument an 'unsigned short *'? + // (Actually any integer type is allowed.) + const PointerType *PT = dyn_cast(FTP->getArgType(0)); + if (!PT) + return; + + if (! PT->getPointeeType()->isIntegerType()) + return; + } + else if (FTP->getNumArgs() != 0) + return; + + // Issue a warning. + std::string buf1; + llvm::raw_string_ostream os1(buf1); + os1 << "'" << FD->getNameAsString() << "' is a poor random number generator"; + + std::string buf2; + llvm::raw_string_ostream os2(buf2); + os2 << "Function '" << FD->getNameAsString() + << "' is obsolete because it implements a poor random number generator." + << " Use 'arc4random' instead"; + + SourceRange R = CE->getCallee()->getSourceRange(); + + BR.EmitBasicReport(os1.str().c_str(), "Security", os2.str().c_str(), + CE->getLocStart(), &R, 1); +} + +//===----------------------------------------------------------------------===// +// Check: 'random' should not be used +// Originally: +//===----------------------------------------------------------------------===// + +void WalkAST::CheckCall_random(const CallExpr *CE, const FunctionDecl *FD) { + if (FD->getIdentifier() != GetIdentifier(II_random, "random")) + return; + + const FunctionProtoType *FTP = dyn_cast(FD->getType()); + if (!FTP) + return; + + // Verify that the function takes no argument. + if (FTP->getNumArgs() != 0) + return; + + // Issue a warning. + SourceRange R = CE->getCallee()->getSourceRange(); + BR.EmitBasicReport("'random' is not a secure random number generator", + "Security", + "The 'random' function produces a sequence of values that " + "an adversary may be able to predict. Use 'arc4random' " + "instead", + CE->getLocStart(), &R, 1); +} + //===----------------------------------------------------------------------===// // Check: Should check whether privileges are dropped successfully. // Originally: @@ -251,23 +348,23 @@ void WalkAST::CheckUncheckedReturnValue(CallExpr *CE) { return; if (II_setid[0] == NULL) { - static const char * const identifiers[num_ids] = { + static const char * const identifiers[num_setids] = { "setuid", "setgid", "seteuid", "setegid", "setreuid", "setregid" }; - for (size_t i = 0; i < num_ids; i++) + for (size_t i = 0; i < num_setids; 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++) + for (identifierid = 0; identifierid < num_setids; identifierid++) if (id == II_setid[identifierid]) break; - if (identifierid >= num_ids) + if (identifierid >= num_setids) return; const FunctionProtoType *FTP = dyn_cast(FD->getType()); diff --git a/test/Analysis/security-syntax-checks.m b/test/Analysis/security-syntax-checks.m index df17926a15..ebd7d172b5 100644 --- a/test/Analysis/security-syntax-checks.m +++ b/test/Analysis/security-syntax-checks.m @@ -60,3 +60,32 @@ void test_setuid() 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}} } + +// CWE-338: Use of cryptographically weak prng +int rand(void); +double drand48(void); +double erand48(unsigned short[3]); +long jrand48(unsigned short[3]); +void lcong48(unsigned short[7]); +long lrand48(void); +long mrand48(void); +long nrand48(unsigned short[3]); +long random(void); +int rand_r(unsigned *); + +void test_rand() +{ + unsigned short a[7]; + unsigned b; + + rand(); // expected-warning{{Function 'rand' is obsolete because it implements a poor random number generator. Use 'arc4random' instead}} + drand48(); // expected-warning{{Function 'drand48' is obsolete because it implements a poor random number generator. Use 'arc4random' instead}} + erand48(a); // expected-warning{{Function 'erand48' is obsolete because it implements a poor random number generator. Use 'arc4random' instead}} + jrand48(a); // expected-warning{{Function 'jrand48' is obsolete because it implements a poor random number generator. Use 'arc4random' instead}} + lcong48(a); // expected-warning{{Function 'lcong48' is obsolete because it implements a poor random number generator. Use 'arc4random' instead}} + lrand48(); // expected-warning{{Function 'lrand48' is obsolete because it implements a poor random number generator. Use 'arc4random' instead}} + mrand48(); // expected-warning{{Function 'mrand48' is obsolete because it implements a poor random number generator. Use 'arc4random' instead}} + nrand48(a); // expected-warning{{Function 'nrand48' is obsolete because it implements a poor random number generator. Use 'arc4random' instead}} + rand_r(&b); // expected-warning{{Function 'rand_r' is obsolete because it implements a poor random number generator. Use 'arc4random' instead}} + random(); // expected-warning{{The 'random' function produces a sequence of values that an adversary may be able to predict. Use 'arc4random' instead}} +}