Author: Artem Dergachev Date: 2019-12-19T14:54:29-08:00 New Revision: b284005072122fe4af879725e3c8090009f89ca0
URL: https://github.com/llvm/llvm-project/commit/b284005072122fe4af879725e3c8090009f89ca0 DIFF: https://github.com/llvm/llvm-project/commit/b284005072122fe4af879725e3c8090009f89ca0.diff LOG: [analyzer] Add a syntactic security check for ObjC NSCoder API. Method '-[NSCoder decodeValueOfObjCType:at:]' is not only deprecated but also a security hazard, hence a loud check. Differential Revision: https://reviews.llvm.org/D71728 Added: clang/test/Analysis/security-syntax-checks-nscoder.m Modified: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/Driver/ToolChains/Clang.cpp clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp clang/www/analyzer/available_checks.html Removed: ################################################################################ diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 43632e801d2b..5b23de418999 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -773,6 +773,11 @@ def DeprecatedOrUnsafeBufferHandling : Dependencies<[SecuritySyntaxChecker]>, Documentation<HasDocumentation>; +def decodeValueOfObjCType : Checker<"decodeValueOfObjCType">, + HelpText<"Warn on uses of the '-decodeValueOfObjCType:at:' method">, + Dependencies<[SecuritySyntaxChecker]>, + Documentation<HasDocumentation>; + } // end "security.insecureAPI" let ParentPackage = Security in { diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index 6b93dc2939e1..1347c3de913e 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -2789,8 +2789,11 @@ static void RenderAnalyzerOptions(const ArgList &Args, ArgStringList &CmdArgs, CmdArgs.push_back("-analyzer-disable-checker=unix.Vfork"); } - if (Triple.isOSDarwin()) + if (Triple.isOSDarwin()) { CmdArgs.push_back("-analyzer-checker=osx"); + CmdArgs.push_back( + "-analyzer-checker=security.insecureAPI.decodeValueOfObjCType"); + } CmdArgs.push_back("-analyzer-checker=deadcode"); diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp index 260a2896e78c..d9ffa562c0aa 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp @@ -49,6 +49,7 @@ struct ChecksFilter { DefaultBool check_vfork; DefaultBool check_FloatLoopCounter; DefaultBool check_UncheckedReturn; + DefaultBool check_decodeValueOfObjCType; CheckerNameRef checkName_bcmp; CheckerNameRef checkName_bcopy; @@ -63,6 +64,7 @@ struct ChecksFilter { CheckerNameRef checkName_vfork; CheckerNameRef checkName_FloatLoopCounter; CheckerNameRef checkName_UncheckedReturn; + CheckerNameRef checkName_decodeValueOfObjCType; }; class WalkAST : public StmtVisitor<WalkAST> { @@ -83,6 +85,7 @@ class WalkAST : public StmtVisitor<WalkAST> { // Statement visitor methods. void VisitCallExpr(CallExpr *CE); + void VisitObjCMessageExpr(ObjCMessageExpr *CE); void VisitForStmt(ForStmt *S); void VisitCompoundStmt (CompoundStmt *S); void VisitStmt(Stmt *S) { VisitChildren(S); } @@ -93,6 +96,7 @@ class WalkAST : public StmtVisitor<WalkAST> { bool checkCall_strCommon(const CallExpr *CE, const FunctionDecl *FD); typedef void (WalkAST::*FnCheck)(const CallExpr *, const FunctionDecl *); + typedef void (WalkAST::*MsgCheck)(const ObjCMessageExpr *); // Checker-specific methods. void checkLoopConditionForFloat(const ForStmt *FS); @@ -110,6 +114,7 @@ class WalkAST : public StmtVisitor<WalkAST> { void checkCall_rand(const CallExpr *CE, const FunctionDecl *FD); void checkCall_random(const CallExpr *CE, const FunctionDecl *FD); void checkCall_vfork(const CallExpr *CE, const FunctionDecl *FD); + void checkMsg_decodeValueOfObjCType(const ObjCMessageExpr *ME); void checkUncheckedReturnValue(CallExpr *CE); }; } // end anonymous namespace @@ -182,6 +187,20 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { VisitChildren(CE); } +void WalkAST::VisitObjCMessageExpr(ObjCMessageExpr *ME) { + MsgCheck evalFunction = + llvm::StringSwitch<MsgCheck>(ME->getSelector().getAsString()) + .Case("decodeValueOfObjCType:at:", + &WalkAST::checkMsg_decodeValueOfObjCType) + .Default(nullptr); + + if (evalFunction) + (this->*evalFunction)(ME); + + // Recurse and check children. + VisitChildren(ME); +} + void WalkAST::VisitCompoundStmt(CompoundStmt *S) { for (Stmt *Child : S->children()) if (Child) { @@ -923,6 +942,54 @@ void WalkAST::checkCall_vfork(const CallExpr *CE, const FunctionDecl *FD) { CELoc, CE->getCallee()->getSourceRange()); } +//===----------------------------------------------------------------------===// +// Check: '-decodeValueOfObjCType:at:' should not be used. +// It is deprecated in favor of '-decodeValueOfObjCType:at:size:' due to +// likelihood of buffer overflows. +//===----------------------------------------------------------------------===// + +void WalkAST::checkMsg_decodeValueOfObjCType(const ObjCMessageExpr *ME) { + if (!filter.check_decodeValueOfObjCType) + return; + + // Check availability of the secure alternative: + // iOS 11+, macOS 10.13+, tvOS 11+, and watchOS 4.0+ + // FIXME: We probably shouldn't register the check if it's not available. + const TargetInfo &TI = AC->getASTContext().getTargetInfo(); + const llvm::Triple &T = TI.getTriple(); + const VersionTuple &VT = TI.getPlatformMinVersion(); + switch (T.getOS()) { + case llvm::Triple::IOS: + if (VT < VersionTuple(11, 0)) + return; + break; + case llvm::Triple::MacOSX: + if (VT < VersionTuple(10, 13)) + return; + break; + case llvm::Triple::WatchOS: + if (VT < VersionTuple(4, 0)) + return; + break; + case llvm::Triple::TvOS: + if (VT < VersionTuple(11, 0)) + return; + break; + default: + return; + } + + PathDiagnosticLocation MELoc = + PathDiagnosticLocation::createBegin(ME, BR.getSourceManager(), AC); + BR.EmitBasicReport( + AC->getDecl(), filter.checkName_decodeValueOfObjCType, + "Potential buffer overflow in '-decodeValueOfObjCType:at:'", "Security", + "Deprecated method '-decodeValueOfObjCType:at:' is insecure " + "as it can lead to potential buffer overflows. Use the safer " + "'-decodeValueOfObjCType:at:size:' method.", + MELoc, ME->getSourceRange()); +} + //===----------------------------------------------------------------------===// // Check: Should check whether privileges are dropped successfully. // Originally: <rdar://problem/6337132> @@ -1035,3 +1102,4 @@ REGISTER_CHECKER(vfork) REGISTER_CHECKER(FloatLoopCounter) REGISTER_CHECKER(UncheckedReturn) REGISTER_CHECKER(DeprecatedOrUnsafeBufferHandling) +REGISTER_CHECKER(decodeValueOfObjCType) diff --git a/clang/test/Analysis/security-syntax-checks-nscoder.m b/clang/test/Analysis/security-syntax-checks-nscoder.m new file mode 100644 index 000000000000..23aa95bedccd --- /dev/null +++ b/clang/test/Analysis/security-syntax-checks-nscoder.m @@ -0,0 +1,36 @@ +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-ios11.0 -verify=available \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-ios10.0 -verify=notavailable \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple x86_64-apple-macos10.13 -verify=available \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple x86_64-apple-macos10.12 -verify=notavailable \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-watchos4.0 -verify=available \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-watchos3.0 -verify=notavailable \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-tvos11.0 -verify=available \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s +// RUN: %clang_analyze_cc1 -triple thumbv7-apple-tvos10.0 -verify=notavailable \ +// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType %s + +// notavailable-no-diagnostics + +typedef unsigned long NSUInteger; + +@interface NSCoder +- (void)decodeValueOfObjCType:(const char *)type + at:(void *)data; +- (void)decodeValueOfObjCType:(const char *)type + at:(void *)data + size:(NSUInteger)size; +@end + +void test(NSCoder *decoder) { + // This would be a vulnerability on 64-bit platforms + // but not on 32-bit platforms. + NSUInteger x; + [decoder decodeValueOfObjCType:"I" at:&x]; // available-warning{{Deprecated method '-decodeValueOfObjCType:at:' is insecure as it can lead to potential buffer overflows. Use the safer '-decodeValueOfObjCType:at:size:' method}} + [decoder decodeValueOfObjCType:"I" at:&x size:sizeof(x)]; // no-warning +} diff --git a/clang/www/analyzer/available_checks.html b/clang/www/analyzer/available_checks.html index 12e66f11b118..501a2b306dfa 100644 --- a/clang/www/analyzer/available_checks.html +++ b/clang/www/analyzer/available_checks.html @@ -1446,6 +1446,22 @@ <h3 id="security_checkers">Security Checkers</h3> } </pre></div></div></td></tr> + +<tr><td><a id="security.insecureAPI.decodeValueOfObjCType"><div class="namedescr expandable"><span class="name"> +security.insecureAPI.decodeValueOfObjCType</span><span class="lang"> +(ObjC)</span><div class="descr"> +Warn on uses of the <code>-[NSCoder decodeValueOfObjCType:at:]</code> method. +The safe alternative is <code>-[NSCoder decodeValueOfObjCType:at:size:]</code>.</div></div></a></td> +<td><div class="exampleContainer expandable"> +<div class="example"><pre> +void test(NSCoder *decoder) { + // This would be a vulnerability on 64-bit platforms + // but not on 32-bit platforms. + NSUInteger x; + [decoder decodeValueOfObjCType:"I" at:&x]; // warn +} +</pre></div></div></td></tr> + </tbody></table> <!-- =========================== unix =========================== --> _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
