NoQ created this revision.
NoQ added a reviewer: dcoughlin.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus,
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.
Method `-[NSCoder decodeValueOfObjCType:at:]` is not only deprecated but also a
security hazard, hence a loud check.
Repository:
rC Clang
https://reviews.llvm.org/D71728
Files:
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
clang/test/Analysis/security-syntax-checks-nscoder.m
clang/www/analyzer/available_checks.html
Index: clang/www/analyzer/available_checks.html
===================================================================
--- clang/www/analyzer/available_checks.html
+++ clang/www/analyzer/available_checks.html
@@ -1446,6 +1446,22 @@
}
</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 =========================== -->
Index: clang/test/Analysis/security-syntax-checks-nscoder.m
===================================================================
--- /dev/null
+++ clang/test/Analysis/security-syntax-checks-nscoder.m
@@ -0,0 +1,20 @@
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin12 -verify %s\
+// RUN: -analyzer-checker=security.insecureAPI.decodeValueOfObjCType
+
+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]; // expected-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
+}
Index: clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
@@ -49,6 +49,7 @@
DefaultBool check_vfork;
DefaultBool check_FloatLoopCounter;
DefaultBool check_UncheckedReturn;
+ DefaultBool check_decodeValueOfObjCType;
CheckerNameRef checkName_bcmp;
CheckerNameRef checkName_bcopy;
@@ -63,6 +64,7 @@
CheckerNameRef checkName_vfork;
CheckerNameRef checkName_FloatLoopCounter;
CheckerNameRef checkName_UncheckedReturn;
+ CheckerNameRef checkName_decodeValueOfObjCType;
};
class WalkAST : public StmtVisitor<WalkAST> {
@@ -83,6 +85,7 @@
// 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 @@
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 @@
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 @@
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,27 @@
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;
+
+ 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 +1075,4 @@
REGISTER_CHECKER(FloatLoopCounter)
REGISTER_CHECKER(UncheckedReturn)
REGISTER_CHECKER(DeprecatedOrUnsafeBufferHandling)
+REGISTER_CHECKER(decodeValueOfObjCType)
Index: clang/lib/Driver/ToolChains/Clang.cpp
===================================================================
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2789,8 +2789,11 @@
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");
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -773,6 +773,11 @@
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 {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
