[analyzer] Detect usages of unsafe I/O functions

[koldaniel]

Summary

There are certain unsafe or deprecated (since C11) buffer handling
functions which should be avoided in safety critical code. They
could cause buffer overflows. Two new checks had been implemented
which warn for every occurrence of such functions
(unsafe or deprecated printf, scanf family, and other buffer
handling functions, which now have a secure variant).

[lebedev.ri]

This does not do anything more than traversing the AST, shouldn't this be a clang-tidy check?
Also, i suspect CERT-MSC24-C might be relevant

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

606

depr*e*cated

[NoQ]

It'd look good in clang-tidy (especially if extended to provide fixits), but if Daniel is interested in having this feature in the analyzer (and picked by clang-tidy from there), i wouldn't mind.

I wonder how noisy this check is - did you test it on large codebases? Because these functions are popular, and in many cases it'd be fine to use insecure functions, i wonder if it's worth it to have this check on by default. Like, if it's relatively quiet - it's fine, but if it'd constitute 90% of the analyzer's warnings on popular projects, that'd probably not be fine.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

596–597

Note that you cannot easily figure out if the code is intended to get compiled only under C11 and above - maybe it's accidentally compiled under C11 for this user, but is otherwise intended to keep working under older standards.

631

Because it also checks deprecated buffer handling, i'd rename this function to checkDeprecatedOrUnsafeBufferHandling.

669–674

You'd probably also want to quit early if the format string is not a literal.

[xazax.hun]

In D35068#811437, @NoQ wrote:

It'd look good in clang-tidy (especially if extended to provide fixits), but if Daniel is interested in having this feature in the analyzer (and picked by clang-tidy from there), i wouldn't mind.

I wonder how noisy this check is - did you test it on large codebases? Because these functions are popular, and in many cases it'd be fine to use insecure functions, i wonder if it's worth it to have this check on by default. Like, if it's relatively quiet - it's fine, but if it'd constitute 90% of the analyzer's warnings on popular projects, that'd probably not be fine.

This patch basically extends an already existing static analyzer check. Even if tidy might be a better fit, I wonder what is the right thing to do in this case. We either end up overlapping functionality with the analyzer and tidy or have to come up with a policy what to do in this such cases.

[xazax.hun]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

621

I would put a new line above and remove one bellow.

631

Is the TODO still relevant in this line?

[koldaniel]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

596–597

It is a possible scenario, how should I check if the checks should warn (safe functions are available) if not by using this method?

669–674

If the format string is not a literal (i.e. a variable), currently we cannot determine if there were any restrictions regarding the size or not, so we want this check to warn.

[koldaniel]

Updated checker name, minor modifications

[koldaniel]

Renaming the unsafe checker, updating tests.

[xazax.hun]

include/clang/StaticAnalyzer/Checkers/Checkers.td

382

I do not like the naming of these two checks, It feels like one of them warns for a subset of the other, however, it is not the case.
What about removing the "deprecated" part from the first check?

lib/Driver/ToolChains/Clang.cpp

2256

Do we want to turn on both of these by default? Warning on every use of memset will be very noisy for example.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

147

This is the only print-related function in this group. Is this intended?

596–597

This is the same approach some check takes in clang tidy. I think it is still better to not warn for non C11 users than warn for everybody. If a project is not interested in this check but they are interested in having C11 builds, they can turn this check off. (Or it can be off by default in the first place).

[koldaniel]

include/clang/StaticAnalyzer/Checkers/Checkers.td

382

Both checker warns if a buffer handling function is deprecated (DeprecatedOrUnsafeBufferHandling calls DeprecatedBufferHandling), but the DeprecatedOrUnsafeBufferHandling checker also warns if a function is not only deprecated but unsafe (i.e. writes a buffer without size restrictions) too.

lib/Driver/ToolChains/Clang.cpp

2256

It's true that these checkers could be very noisy if they are enabled by default, I'm going to fix this.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

147

sprintf and vsprintf are buffer-writing functions without size restrictions, this is the reason why only these two are checked by DeprecatedOrUnsafeBufferHandling of the printf family. There are buffer-writing members of this family which are not unsafe (because of size restrictions) but have a secure version introduced in the C11 standard, and DeprecatedBufferHandling warns for them: swprintf, snprintf, vswprintf, vsnprintf. Other printf-related functions are either printing to a file or to the standard output.

[xazax.hun]

include/clang/StaticAnalyzer/Checkers/Checkers.td

382

I see. Maybe it would be better to make them disjoint? Also, I think it is not a good user experience to get two warnings for the same function call.

[koldaniel]

include/clang/StaticAnalyzer/Checkers/Checkers.td

382

Do you mean to separate them, so we would have one checker which warns for the unsafe and deprecated buffer handling functions, and one which warns for the deprecated functions which have some boundary restrictions?

[george.karpenkov]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

102

80 chars

168

That's a lot of duplicated WalkAST::checkDeprecatedOrUnsafeBufferHandling. Could that be simplified?

610

That's a lot of duplication of 1/0/-1.

And also 1/0/-1 are cryptic symbols, why not use an enum with a descriptive name?
Maybe use
.Cases("sprintf", "vsprintf", "vfscanf", WARN_UNSAFE) ?

[koldaniel]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

610

The duplications will be solved by using .Cases, but is using enum necessary? 1 and 0 refers to the index of the argument which could be a format string. -1 means there is no need to look for a string like this.

[george.karpenkov]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

610

Right, then I guess 1 and 0 are self-descriptive, but I would still add a separate constant for -1.

[koldaniel]

Do you have any other comments, or could this change be accepted?

[george.karpenkov]

@koldaniel Have you evaluated this checker? On which codebases? Were the warnings real security issues, or were they mostly spurious? The code seems fine, but I'm not sure whether it should be in security or in alpha.

[koldaniel]

In D35068#1049530, @george.karpenkov wrote:

@koldaniel Have you evaluated this checker? On which codebases? Were the warnings real security issues, or were they mostly spurious? The code seems fine, but I'm not sure whether it should be in security or in alpha.

I've evaluated this checker on LLVM+Clang, there were only a few (about 15) warnings, because of the C11 flag check at the beginning of the checker body. However, if this check was removed, number of the warnings would be increased significantly. I wouldn't say the findings were real security issues, most of the warnings were about usages of deprecated functions, which has not been considered unsecure (but which may cause problems if the code is modified in an improper way in the future).

[koldaniel]

Do you have any other comments? Could this checker be delivered into security?

[koldaniel]

Hi, could you please take a look at this issue?

[xazax.hun]

LGTM!

Any objections to commit this?
I think this is quiet coding guideline specific check which is useful for a set of security critical projects. As this is an opt in kind of check, I think it does no harm to have it upstream.

[Szelethus]

Overall I think this looks great, thanks! I left some inlines that would be nice to fix before commiting, but all of them are minor nits.

Would it be possible for you to commit the clang-formatting and the actual logic separately?

include/clang/StaticAnalyzer/Checkers/Checkers.td

380

Lately we started paying attention to the 80 column limit in Checkers.td. Could you please break this line?

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

587

I think this newline should stay.

591

This is out of place.

599

Could you please add an extra newline?

611–618

I wonder whether using CallDescription would be (way) more efficient here. Comparing IndentifierInfos would also be better I guess.

I'm a little unsure about performance implications here, it might not be worth the chore to refactor this.

635–638

Could you please start these variable names with a capital letter?

test/Analysis/security-syntax-checks.m

243

When using {{}}, you actually supply a regex as an argument, and the output of the analyzer is matched against it. My point is, could you instead just write

// expected-warning{{Call to function 'sprintf' is insecure}}

to improve readability?

[Szelethus]

test/Analysis/security-syntax-checks.m

243

Or whatever the shortest string is needed to know whether the expected output it there.

[Szelethus]

In D35068#811436, @NoQ wrote:

I wonder how noisy this check is - did you test it on large codebases? Because these functions are popular, and in many cases it'd be fine to use insecure functions, i wonder if it's worth it to have this check on by default. Like, if it's relatively quiet - it's fine, but if it'd constitute 90% of the analyzer's warnings on popular projects, that'd probably not be fine.

In D35068#1049530, @george.karpenkov wrote:

@koldaniel Have you evaluated this checker? On which codebases? Were the warnings real security issues, or were they mostly spurious? The code seems fine, but I'm not sure whether it should be in security or in alpha.

Sorry, didn't read the discussion, there are some fair points in the quoted comments.

In D35068#1069880, @koldaniel wrote:

I've evaluated this checker on LLVM+Clang, there were only a few (about 15) warnings, because of the C11 flag check at the beginning of the checker body. However, if this check was removed, number of the warnings would be increased significantly. I wouldn't say the findings were real security issues, most of the warnings were about usages of deprecated functions, which has not been considered unsecure (but which may cause problems if the code is modified in an improper way in the future).

My problem is that LLVM+Clang isn't really a C (nor a C11) project, and I think judging this checker on it is a little misleading. Could you please test it on some C11 projects? I think tmux uses C11.

Edit: it doesn't, but CMake is mostly a C project and it does!

In D35068#1361195, @xazax.hun wrote:

I think this is quiet coding guideline specific check which is useful for a set of security critical projects. As this is an opt in kind of check, I think it does no harm to have it upstream.

I do generally agree with this statement, but I'd be more comfortable either landing it in alpha or seeing some other results.

[whisperity]

In D35068#1361902, @Szelethus wrote:

Edit: it doesn't, but CMake is mostly a C project and it does!

CMake really isn't a C project if you look at what language it actually uses - the C files come from tests and system utilities such as ZIP being reimplemented.
And as far as I've seen with my recent endeavours, Clang has problems with parsing and building CMake (however all these errors relate to some very niche standard library memory size related stuff not being there where it wants them).

[xazax.hun]

In D35068#1361902, @Szelethus wrote:

In D35068#1069880, @koldaniel wrote:

I've evaluated this checker on LLVM+Clang, there were only a few (about 15) warnings, because of the C11 flag check at the beginning of the checker body. However, if this check was removed, number of the warnings would be increased significantly. I wouldn't say the findings were real security issues, most of the warnings were about usages of deprecated functions, which has not been considered unsecure (but which may cause problems if the code is modified in an improper way in the future).

My problem is that LLVM+Clang isn't really a C (nor a C11) project, and I think judging this checker on it is a little misleading. Could you please test it on some C11 projects? I think tmux uses C11.

Edit: it doesn't, but CMake is mostly a C project and it does!

What do we want to validate here? The lack of crashes? Or evaluate false positive ratio?

I have some doubts about evaluating this checker on open source projects. If a project does not care about the safe versions of these functions all of the results will be false positive (or a project might actually care but will not be able to comply due to portability constraints). If a project does care about using the safe variants, they are most likely already using another tool to verify this.
So I think the main value here is to subsume other tools.

[xazax.hun]

To add an analogy, Clang Tidy will not require C++ Core Guidelines related checks to be evaluated on projects that are not following the guidelines as the results are meaningless for those projects.

[Szelethus]

Yup, I'm sold on that.

[NoQ]

Ok! I hope that the C11 check would do the trick, let's see how it goes :)

In D35068#1364947, @xazax.hun wrote:

What do we want to validate here? The lack of crashes? Or evaluate false positive ratio?

This checker is opt-in, but it still has a notion of a useless positive, and it is something that can often be improved upon. Crashes are also a great thing to look for :)

In D35068#1364948, @xazax.hun wrote:

To add an analogy, Clang Tidy will not require C++ Core Guidelines related checks to be evaluated on projects that are not following the guidelines as the results are meaningless for those projects.

On the contrary, figuring out if the project follows C++ Core Guidelines is pretty hard. Well, it's often pretty easy to demonstrate that it doesn't (: But, i mean, the intention.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

596–597

Hmm, i guess in many cases such clients may suppress the warning through preprocessing. This will keep the checker warning in C11 builds but would also avoid miscompiles in builds for the older standards.

[Szelethus]

Could you rebase please? Many things have changed since you last update. After that I'll happily commit on your behalf if you don't have a commit access just yet.

[koldaniel]

Rebased.

[Szelethus]

Hmmm, DescFile was removed months ago from Checkers.td, are you sure you uploaded the correct diff?

[koldaniel]

In D35068#1391758, @Szelethus wrote:

Hmmm, DescFile was removed months ago from Checkers.td, are you sure you uploaded the correct diff?

No, you are absolutely right, an earlier diff had been uploaded, I will correct it.

[koldaniel]

Rebased. Documentation added.

[Szelethus]

This looks great, thanks! I'll commit sometime tomorrow.

[NoQ]

There seems to be a crash in this code. @koldaniel, would you like to take a look? https://bugs.llvm.org/show_bug.cgi?id=41185

[koldaniel]

In D35068#1438498, @NoQ wrote:

There seems to be a crash in this code. @koldaniel, would you like to take a look? https://bugs.llvm.org/show_bug.cgi?id=41185

Hi,

True, it is a faulty scenario, my question is what should be the way forward? I think in case of built-in functions there should be no warning, since they differ from the deprecated ones which come from the old standard. The only purpose of the assert was to help development and maintenance (if a new function had been added, it should be decided if it is deprecated or unsafe). Returning instead of asserting would solve the problem.

[NoQ]

In D35068#1440830, @koldaniel wrote:

In D35068#1438498, @NoQ wrote:

There seems to be a crash in this code. @koldaniel, would you like to take a look? https://bugs.llvm.org/show_bug.cgi?id=41185

Hi,

True, it is a faulty scenario, my question is what should be the way forward? I think in case of built-in functions there should be no warning, since they differ from the deprecated ones which come from the old standard. The only purpose of the assert was to help development and maintenance (if a new function had been added, it should be decided if it is deprecated or unsafe). Returning instead of asserting would solve the problem.

On many platforms such standard C functions are implemented as macros that expand to the respective builtins. I believe there should be no difference in behavior between the normal function and the builtin function. Cf. test/Analysis/bstring.c.

[koldaniel]

Bug fixing: faulty handling of built-in functions.

[lebedev.ri]

In D35068#1441581, @koldaniel wrote:

Bug fixing: faulty handling of built-in functions.

Please open a new differential, that is a completely new patch.

[koldaniel]

In D35068#1441601, @lebedev.ri wrote:

In D35068#1441581, @koldaniel wrote:

Bug fixing: faulty handling of built-in functions.

Please open a new differential, that is a completely new patch.

Hi,

Done: https://reviews.llvm.org/D59812

include/clang/StaticAnalyzer/Checkers/Checkers.td

def strcpy : Checker<"strcpy">,
     HelpText<"Warn on uses of the 'strcpy' and 'strcat' functions">,
     DescFile<"CheckSecuritySyntaxOnly.cpp">;
   def vfork : Checker<"vfork">,
     HelpText<"Warn on uses of the 'vfork' function">,
     DescFile<"CheckSecuritySyntaxOnly.cpp">;
   def UncheckedReturn : Checker<"UncheckedReturn">,
     HelpText<"Warn on uses of functions whose return values must be always checked">,
     DescFile<"CheckSecuritySyntaxOnly.cpp">;
+  def DeprecatedOrUnsafeBufferHandling : Checker<"DeprecatedOrUnsafeBufferHandling">,
+    HelpText<"Warn on uses of unsecure buffer manipulating functions">,
+    DescFile<"CheckSecuritySyntaxOnly.cpp">;
+  def DeprecatedBufferHandling : Checker<"DeprecatedBufferHandling">,
+    HelpText<"Warn on uses of deprecated buffer manipulating functions">,
+    DescFile<"CheckSecuritySyntaxOnly.cpp">;
 }
 let ParentPackage = Security in {
   def FloatLoopCounter : Checker<"FloatLoopCounter">,
     HelpText<"Warn on using a floating point value as a loop counter (CERT: FLP30-C, FLP30-CPP)">,
     DescFile<"CheckSecuritySyntaxOnly.cpp">;
 }
 
 let ParentPackage = SecurityAlpha in {

lib/Driver/ToolChains/Clang.cpp

if (!Args.hasArg(options::OPT__analyzer_no_default_checks)) {
 
     if (!Triple.isPS4CPU()) {
       CmdArgs.push_back("-analyzer-checker=security.insecureAPI.UncheckedReturn");
       CmdArgs.push_back("-analyzer-checker=security.insecureAPI.getpw");
       CmdArgs.push_back("-analyzer-checker=security.insecureAPI.gets");
       CmdArgs.push_back("-analyzer-checker=security.insecureAPI.mktemp");
       CmdArgs.push_back("-analyzer-checker=security.insecureAPI.mkstemp");
       CmdArgs.push_back("-analyzer-checker=security.insecureAPI.vfork");
+      CmdArgs.push_back("-analyzer-checker=security.insecureAPI.DeprecatedOrUnsafeBufferHandling");
+      CmdArgs.push_back("-analyzer-checker=security.insecureAPI.DeprecatedBufferHandling");
     }
 
     // Default nullability checks.
     CmdArgs.push_back("-analyzer-checker=nullability.NullPassedToNonnull");
     CmdArgs.push_back("-analyzer-checker=nullability.NullReturnedFromNonnull");
   }
 
   // Set the output format. The default is plist, for (lame) historical reasons.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

 
 namespace {
 struct ChecksFilter {
   DefaultBool check_gets;
   DefaultBool check_getpw;
   DefaultBool check_mktemp;
   DefaultBool check_mkstemp;
   DefaultBool check_strcpy;
+  DefaultBool check_DeprecatedOrUnsafeBufferHandling;
+  DefaultBool check_DeprecatedBufferHandling;
   DefaultBool check_rand;
   DefaultBool check_vfork;
   DefaultBool check_FloatLoopCounter;
   DefaultBool check_UncheckedReturn;
 
   CheckName checkName_gets;
   CheckName checkName_getpw;
   CheckName checkName_mktemp;
   CheckName checkName_mkstemp;
   CheckName checkName_strcpy;
+  CheckName checkName_DeprecatedOrUnsafeBufferHandling;
+  CheckName checkName_DeprecatedBufferHandling;
   CheckName checkName_rand;
   CheckName checkName_vfork;
   CheckName checkName_FloatLoopCounter;
   CheckName checkName_UncheckedReturn;
 };
 
 class WalkAST : public StmtVisitor<WalkAST> {
   BugReporter &BR;
public:
   // Checker-specific methods.
   void checkLoopConditionForFloat(const ForStmt *FS);
   void checkCall_gets(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_getpw(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_mktemp(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_mkstemp(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_strcpy(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_strcat(const CallExpr *CE, const FunctionDecl *FD);
+  void checkDeprecatedOrUnsafeBufferHandling(const CallExpr *CE, const FunctionDecl *FD);
+  void checkDeprecatedBufferHandling(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 checkCall_vfork(const CallExpr *CE, const FunctionDecl *FD);
   void checkUncheckedReturnValue(CallExpr *CE);
 };
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
FnCheck evalFunction = llvm::StringSwitch<FnCheck>(Name)
     .Case("gets", &WalkAST::checkCall_gets)
     .Case("getpw", &WalkAST::checkCall_getpw)
     .Case("mktemp", &WalkAST::checkCall_mktemp)
     .Case("mkstemp", &WalkAST::checkCall_mkstemp)
     .Case("mkdtemp", &WalkAST::checkCall_mkstemp)
     .Case("mkstemps", &WalkAST::checkCall_mkstemp)
     .Cases("strcpy", "__strcpy_chk", &WalkAST::checkCall_strcpy)
     .Cases("strcat", "__strcat_chk", &WalkAST::checkCall_strcat)
+    .Case("sprintf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("vsprintf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("scanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("wscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("fscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("fwscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("vscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("vwscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("vfscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("vfwscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("sscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("swscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("vsscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("vswscanf", &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Case("swprintf", &WalkAST::checkDeprecatedBufferHandling)
+    .Case("snprintf", &WalkAST::checkDeprecatedBufferHandling)
+    .Case("vswprintf", &WalkAST::checkDeprecatedBufferHandling)
+    .Case("vsnprintf", &WalkAST::checkDeprecatedBufferHandling)
+    .Case("memcpy", &WalkAST::checkDeprecatedBufferHandling)
+    .Case("memmove", &WalkAST::checkDeprecatedBufferHandling)
+    .Case("strncpy", &WalkAST::checkDeprecatedBufferHandling)
+    .Case("strncat", &WalkAST::checkDeprecatedBufferHandling)
+    .Case("memset", &WalkAST::checkDeprecatedBufferHandling)
     .Case("drand48", &WalkAST::checkCall_rand)
     .Case("erand48", &WalkAST::checkCall_rand)
     .Case("jrand48", &WalkAST::checkCall_rand)
     .Case("lrand48", &WalkAST::checkCall_rand)
     .Case("mrand48", &WalkAST::checkCall_rand)
     .Case("nrand48", &WalkAST::checkCall_rand)
     .Case("lcong48", &WalkAST::checkCall_rand)
     .Case("rand", &WalkAST::checkCall_rand)
BR.EmitBasicReport(AC->getDecl(), filter.checkName_strcpy,
                      "Call to function 'strcat' is insecure as it does not "
                      "provide bounding of the memory buffer. Replace "
                      "unbounded copy functions with analogous functions that "
                      "support length arguments such as 'strlcat'. CWE-119.",
                      CELoc, CE->getCallee()->getSourceRange());
 }
 
 //===----------------------------------------------------------------------===//
+// Check: Any use of 'sprintf', 'vsprintf', 'scanf', 'wscanf', 'fscanf',
+//        'fwscanf', 'vscanf', 'vwscanf', 'vfscanf', 'vfwscanf', 'sscanf',
+//        'swscanf', 'vsscanf', 'vswscanf', 'swprintf', 'snprintf', 'vswprintf',
+//        'vsnprintf', 'memcpy', 'memmove', 'strncpy', 'strncat', 'memset'
+//        is deprecated since C11.
+// CWE-119: Improper Restriction of Operations within
+// the Bounds of a Memory Buffer
+//===----------------------------------------------------------------------===//
+void WalkAST::checkDeprecatedBufferHandling(const CallExpr *CE, const FunctionDecl *FD) {
+  if (!filter.check_DeprecatedBufferHandling)
+    return;
+
+  if(!BR.getContext().getLangOpts().C11)
+    return;
+
+  StringRef Name = FD->getIdentifier()->getName();
+  SmallString<128> buf1;
+  SmallString<512> buf2;
+  llvm::raw_svector_ostream out1(buf1);
+  llvm::raw_svector_ostream out2(buf2);
+  out1 << "Potential insecure memory buffer bounds restriction in call '"
+       << Name << "'";
+  out2 << "Using '" << Name << "' is deprecated as it does not "
+                     "provide bounding of the memory buffer or security "
+                     "checks introduced in the C11 standard. Replace "
+                     "with analogous functions introduced in C11 standard that "
+                     "supports length arguments or provides boundary checks such as '"
+                     << Name << "_s'.";
+  
+  PathDiagnosticLocation CELoc =
+    PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
+  BR.EmitBasicReport(AC->getDecl(), filter.checkName_DeprecatedBufferHandling,
+                     out1.str(),
+                     "Security",
+                     out2.str(),
+                     CELoc, CE->getCallee()->getSourceRange());
+}
+
+//===----------------------------------------------------------------------===//
+// Check: Use of 'sprintf', 'vsprintf', 'scanf', 'wscanf', 'fscanf',
+//        'fwscanf', 'vscanf', 'vwscanf', 'vfscanf', 'vfwscanf', 'sscanf',
+//        'swscanf', 'vsscanf', 'vswscanf' without buffer limitations
+//        is insecure.
+//
+// CWE-119: Improper Restriction of Operations within
+// the Bounds of a Memory Buffer
+//===----------------------------------------------------------------------===//
+void WalkAST::checkDeprecatedOrUnsafeBufferHandling(const CallExpr *CE, const FunctionDecl *FD) {
+  if (!filter.check_DeprecatedOrUnsafeBufferHandling)
+    return;
+  checkDeprecatedBufferHandling(CE, FD);
+
+  // Issue a warning.
+  StringRef Name = FD->getIdentifier()->getName();
+  int ArgIndex =
+    llvm::StringSwitch<int>(Name)
+      .Case("sprintf", 1)
+      .Case("vsprintf", 1)
+      .Case("scanf", 0)
+      .Case("wscanf", 0)
+      .Case("fscanf", 1)
+      .Case("fwscanf", 1)
+      .Case("vscanf", 0)
+      .Case("vwscanf", 0)
+      .Case("vfscanf", 1)
+      .Case("vfwscanf", 1)
+      .Case("sscanf", 1)
+      .Case("swscanf", 1)
+      .Case("vsscanf", 1)
+      .Case("vswscanf", 1)
+      .Default(-1);
+      
+  assert(ArgIndex >= 0 && "Unsupported function");
+  
+  StringRef ReplaceName;
+  if(Name == "sprintf")
+    ReplaceName = "'snprintf' or 'sprintf";
+  else if(Name == "vsprintf")
+    ReplaceName = "'vsnprintf' or 'vsprintf";
+  else
+    ReplaceName = Name;
+  
+  // Currently we only handle (not wide) string literals. It is possible to do better,
+  // either by looking at references to const variables, or by doing real
+  // flow analysis. 
+  auto FormatString =
+    dyn_cast<StringLiteral>(CE->getArg(ArgIndex)->IgnoreParenImpCasts());
+  if(FormatString &&
+     FormatString->getString().find("%s") == StringRef::npos &&
+     FormatString->getString().find("%[") == StringRef::npos)
+    return;
+
+  SmallString<128> buf1;
+  SmallString<512> buf2;
+  llvm::raw_svector_ostream out1(buf1);
+  llvm::raw_svector_ostream out2(buf2);
+
+  out1 << "Potential insecure memory buffer bounds restriction in call '"
+       << Name << "'";
+  out2 << "Call to function '" << Name << "' is insecure as it does not "
+          "provide bounding of the memory buffer. Replace "
+          "with analogous functions that "
+          "support length arguments or provides boundary checks such as "
+       << "'" << Name << "_s' in case of C11.";
+  PathDiagnosticLocation CELoc =
+    PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
+  BR.EmitBasicReport(AC->getDecl(), filter.checkName_DeprecatedOrUnsafeBufferHandling,
+                     out1.str(),
+                     "Security",
+                     out2.str(),
+                     CELoc, CE->getCallee()->getSourceRange());
+}
+
+//===----------------------------------------------------------------------===//
 // Common check for str* functions with no bounds parameters.
 //===----------------------------------------------------------------------===//
 bool WalkAST::checkCall_strCommon(const CallExpr *CE, const FunctionDecl *FD) {
   const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>();
   if (!FPT)
     return false;
 
   // Verify the function takes two arguments, three in the _chk version.
bool WalkAST::checkCall_strCommon(const CallExpr *CE, const FunctionDecl *FD) {
   return true;
 }
 
 //===----------------------------------------------------------------------===//
 // Check: Linear congruent random number generators should not be used
 // Originally: <rdar://problem/63371000>
 // CWE-338: Use of cryptographically weak prng
 //===----------------------------------------------------------------------===//
-
 void WalkAST::checkCall_rand(const CallExpr *CE, const FunctionDecl *FD) {
   if (!filter.check_rand || !CheckRand)
     return;
 
   const FunctionProtoType *FTP = FD->getType()->getAs<FunctionProtoType>();
   if (!FTP)
     return;
 

 REGISTER_CHECKER(getpw)
 REGISTER_CHECKER(mkstemp)
 REGISTER_CHECKER(mktemp)
 REGISTER_CHECKER(strcpy)
 REGISTER_CHECKER(rand)
 REGISTER_CHECKER(vfork)
 REGISTER_CHECKER(FloatLoopCounter)
 REGISTER_CHECKER(UncheckedReturn)
-
-
+REGISTER_CHECKER(DeprecatedOrUnsafeBufferHandling)
+REGISTER_CHECKER(DeprecatedBufferHandling)

test/Analysis/security-syntax-checks.m

 // RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
 // RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DUSE_BUILTINS -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
 // RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DVARIANT -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
 // RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DUSE_BUILTINS -DVARIANT -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi -DUSE_BUILTINS -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi -DVARIANT -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi -DUSE_BUILTINS -DVARIANT -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
 
 #ifdef USE_BUILTINS
 # define BUILTIN(f) __builtin_ ## f
 #else /* USE_BUILTINS */
 # define BUILTIN(f) f
 #endif /* USE_BUILTINS */
 
+#include "Inputs/system-header-simulator-for-valist.h"
+#include "Inputs/system-header-simulator-for-simple-stream.h"
+
 typedef typeof(sizeof(int)) size_t;
 
 
 // <rdar://problem/6336718> rule request: floating point used as loop 
 //  condition (FLP30-C, FLP-30-CPP)
 //
 // For reference: https://www.securecoding.cert.org/confluence/display/seccode/FLP30-C.+Do+not+use+floating+point+variables+as+loop+counters
 //
void test_mkstemp() {
   mkstemps("XXXXXX", 0);
   mkstemps("XXXXXX", 1); // expected-warning {{5 'X's seen}}
   mkstemps("XXXXXX", 2); // expected-warning {{Call to 'mkstemps' should have at least 6 'X's in the format string to be secure (4 'X's seen, 2 characters used as a suffix)}}
   mkdtemp("XX"); // expected-warning {{2 'X's seen}}
   mkstemp("X"); // expected-warning {{Call to 'mkstemp' should have at least 6 'X's in the format string to be secure (1 'X' seen)}}
   mkdtemp("XXXXXX");
 }
 
+
+//===----------------------------------------------------------------------===
+// deprecated or unsafe buffer handling
+//===----------------------------------------------------------------------===
+typedef int wchar_t;
+
+int sprintf(char *str, const char *format, ...);
+//int vsprintf (char *s, const char *format, va_list arg);
+int scanf(const char *format, ...);
+int wscanf(const wchar_t *format, ...);
+int fscanf(FILE *stream, const char *format, ...);
+int fwscanf(FILE *stream, const wchar_t *format, ...);
+int vscanf(const char *format, va_list arg);
+int vwscanf(const wchar_t *format, va_list arg);
+int vfscanf(FILE *stream, const char *format, va_list arg);
+int vfwscanf(FILE *stream, const wchar_t *format, va_list arg);
+int sscanf(const char *s, const char *format, ...);
+int swscanf(const wchar_t *ws, const wchar_t *format, ...);
+int vsscanf(const char *s, const char *format, va_list arg);
+int vswscanf(const wchar_t *ws, const wchar_t *format, va_list arg);
+int swprintf(wchar_t *ws, size_t len, const wchar_t *format, ...);
+int snprintf(char *s, size_t n, const char *format, ...);
+int vswprintf(wchar_t *ws, size_t len, const wchar_t *format, va_list arg);
+int vsnprintf(char *s, size_t n, const char *format, va_list arg);
+void *memcpy(void *destination, const void *source, size_t num);
+void *memmove(void *destination, const void *source, size_t num);
+char *strncpy(char *destination, const char *source, size_t num);
+char *strncat(char *destination, const char *source, size_t num);
+void *memset(void *ptr, int value, size_t num);
+
+void test_deprecated_or_unsafe_buffer_handling_1() {
+  char buf [5];
+  wchar_t wbuf [5];
+  int a;
+  FILE *file;
+  sprintf(buf, "a"); // expected-warning{{Using 'sprintf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'sprintf_s'}}
+  scanf("%d", &a); // expected-warning{{Using 'scanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'scanf_s'}}
+  scanf("%s", buf); // expected-warning{{Call to function 'scanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'scanf_s' in case of C11}} expected-warning{{Using 'scanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'scanf_s'}}
+  scanf("%4s", buf); // expected-warning{{Using 'scanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'scanf_s'}}
+  wscanf((const wchar_t*) L"%s", buf); // expected-warning{{Call to function 'wscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'wscanf_s' in case of C11}} expected-warning{{Using 'wscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'wscanf_s'}}
+  fscanf(file, "%d", &a); // expected-warning{{Using 'fscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'fscanf_s'}}
+  fscanf(file, "%s", buf); // expected-warning{{Call to function 'fscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'fscanf_s' in case of C11}} expected-warning{{Using 'fscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'fscanf_s'}}
+  fscanf(file, "%4s", buf); // expected-warning{{Using 'fscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'fscanf_s'}}
+  fwscanf(file, (const wchar_t*) L"%s", wbuf); // expected-warning{{Call to function 'fwscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'fwscanf_s' in case of C11}} expected-warning{{Using 'fwscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'fwscanf_s'}}
+  sscanf("5", "%d", &a); // expected-warning{{Using 'sscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'sscanf_s'}}
+  sscanf("5", "%s", buf); // expected-warning{{Call to function 'sscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'sscanf_s' in case of C11}} expected-warning{{Using 'sscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'sscanf_s'}}
+  sscanf("5", "%4s", buf); // expected-warning{{Using 'sscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'sscanf_s'}}
+  swscanf(L"5", (const wchar_t*) L"%s", wbuf); // expected-warning{{Call to function 'swscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'swscanf_s' in case of C11}} expected-warning{{Using 'swscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'swscanf_s'}}
+  swprintf(L"5", 1, (const wchar_t*) L"%s", wbuf); // expected-warning{{Using 'swprintf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'swprintf_s'}}
+  snprintf("5", 1, "%s", buf); // expected-warning{{Using 'snprintf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'snprintf_s'}}
+  memcpy(buf, wbuf, 1); // expected-warning{{Using 'memcpy' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'memcpy_s'}}
+  memmove(buf, wbuf, 1); // expected-warning{{Using 'memmove' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'memmove_s'}}
+  strncpy(buf, "a", 1); // expected-warning{{Using 'strncpy' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'strncpy_s'}}
+  strncat(buf, "a", 1); // expected-warning{{Using 'strncat' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'strncat_s'}}
+  memset(buf, 'a', 1); // expected-warning{{Using 'memset' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'memset_s'}}
+}
+
+void test_deprecated_or_unsafe_buffer_handling_2(const char *format, ...) {
+  char buf [5];
+  FILE *file;
+  va_list args;
+  va_start(args, format);
+  vsprintf(buf, format, args); // expected-warning{{Using 'vsprintf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'vsprintf_s'}} expected-warning{{Call to function 'vsprintf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'vsprintf_s' in case of C11}}
+  vscanf(format, args); // expected-warning{{Using 'vscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'vscanf_s'}} expected-warning{{Call to function 'vscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'vscanf_s' in case of C11}}
+  vfscanf(file, format, args); // expected-warning{{Using 'vfscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'vfscanf_s'}} expected-warning{{Call to function 'vfscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'vfscanf_s' in case of C11}}
+  vsscanf("a", format, args); // expected-warning{{Using 'vsscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'vsscanf_s'}} expected-warning{{Call to function 'vsscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'vsscanf_s' in case of C11}}
+  vsnprintf("a", 1, format, args); // expected-warning{{Using 'vsnprintf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'vsnprintf_s'}}
+}
+
+void test_deprecated_or_unsafe_buffer_handling_3(const wchar_t *format, ...) {
+  wchar_t wbuf [5];
+  FILE *file;
+  va_list args;
+  va_start(args, format);
+  vwscanf(format, args); // expected-warning{{Using 'vwscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'vwscanf_s'}} expected-warning{{Call to function 'vwscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'vwscanf_s' in case of C11}}
+  vfwscanf(file, format, args); // expected-warning{{Using 'vfwscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'vfwscanf_s'}} expected-warning{{Call to function 'vfwscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'vfwscanf_s' in case of C11}}
+  vswscanf(L"a", format, args); // expected-warning{{Using 'vswscanf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'vswscanf_s'}} expected-warning{{Call to function 'vswscanf' is insecure as it does not provide bounding of the memory buffer. Replace with analogous functions that support length arguments or provides boundary checks such as 'vswscanf_s' in case of C11}}
+  vswprintf(L"a", 1, format, args); // expected-warning{{Using 'vswprintf' is deprecated as it does not provide bounding of the memory buffer or security checks introduced in the C11 standard. Replace with analogous functions introduced in C11 standard that supports length arguments or provides boundary checks such as 'vswprintf_s'}}
+}
+