[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

607 ↗

(On Diff #105452)

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

597–598 ↗

(On Diff #105452)

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.

632 ↗

(On Diff #105452)

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

670–675 ↗

(On Diff #105452)

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

622 ↗

(On Diff #105452)

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

632 ↗

(On Diff #105452)

Is the TODO still relevant in this line?

[koldaniel]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

597–598 ↗

(On Diff #105452)

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

670–675 ↗

(On Diff #105452)

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 ↗

(On Diff #117782)

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 ↗

(On Diff #117782)

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 ↗

(On Diff #117782)

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

597–598 ↗

(On Diff #105452)

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 ↗

(On Diff #117782)

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 ↗

(On Diff #117782)

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 ↗

(On Diff #117782)

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 ↗

(On Diff #117782)

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 ↗

(On Diff #117782)

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

100 ↗

(On Diff #136261)

80 chars

165 ↗

(On Diff #136261)

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

618 ↗

(On Diff #136261)

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

618 ↗

(On Diff #136261)

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

618 ↗

(On Diff #136261)

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

393 ↗

(On Diff #137162)

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

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

583 ↗

(On Diff #137162)

This is out of place.

591 ↗

(On Diff #137162)

Could you please add an extra newline?

598 ↗

(On Diff #137162)

I think this newline should stay.

603–610 ↗

(On Diff #137162)

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.

627–630 ↗

(On Diff #137162)

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

test/Analysis/security-syntax-checks.m

253 ↗

(On Diff #137162)

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

253 ↗

(On Diff #137162)

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

597–598 ↗

(On Diff #105452)

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

cfe/trunk/docs/analyzer/checkers.rst

  Warn on uses of the 'vfork' function.
 
 .. code-block:: c
  
  void test() {
    vfork(); // warn
  }
 
+security.insecureAPI.DeprecatedOrUnsafeBufferHandling (C) 
+""""""""""""""""""""""""""""""
+ Warn on occurrences of unsafe or deprecated buffer handling functions, which now have a secure variant: ``sprintf, vsprintf, scanf, wscanf, fscanf, fwscanf, vscanf, vwscanf, vfscanf, vfwscanf, sscanf, swscanf, vsscanf, vswscanf, swprintf, snprintf, vswprintf, vsnprintf, memcpy, memmove, strncpy, strncat, memset``
+
+.. code-block:: c
+ 
+ void test() {
+   char buf [5];
+   strncpy(buf, "a", 1); // warn
+ }
+
 .. _unix-checkers:
 
 unix
 ^^^^
 POSIX/Unix checkers.
 
 unix.API (C) 
 """"""""""""

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

def vfork : Checker<"vfork">,
   Documentation<HasDocumentation>;
 
 def UncheckedReturn : Checker<"UncheckedReturn">,
   HelpText<"Warn on uses of functions whose return values must be always "
            "checked">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation<HasDocumentation>;
 
+def DeprecatedOrUnsafeBufferHandling :
+  Checker<"DeprecatedOrUnsafeBufferHandling">,
+  HelpText<"Warn on uses of unsecure or deprecated buffer manipulating "
+           "functions">,
+  Dependencies<[SecuritySyntaxChecker]>,
+  Documentation<HasDocumentation>;
+
 } // end "security.insecureAPI"
 
 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)">,
   Dependencies<[SecuritySyntaxChecker]>,

cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

struct ChecksFilter {
   DefaultBool check_bcmp;
   DefaultBool check_bcopy;
   DefaultBool check_bzero;
   DefaultBool check_gets;
   DefaultBool check_getpw;
   DefaultBool check_mktemp;
   DefaultBool check_mkstemp;
   DefaultBool check_strcpy;
+  DefaultBool check_DeprecatedOrUnsafeBufferHandling;
   DefaultBool check_rand;
   DefaultBool check_vfork;
   DefaultBool check_FloatLoopCounter;
   DefaultBool check_UncheckedReturn;
 
   CheckName checkName_bcmp;
   CheckName checkName_bcopy;
   CheckName checkName_bzero;
   CheckName checkName_gets;
   CheckName checkName_getpw;
   CheckName checkName_mktemp;
   CheckName checkName_mkstemp;
   CheckName checkName_strcpy;
+  CheckName checkName_DeprecatedOrUnsafeBufferHandling;
   CheckName checkName_rand;
   CheckName checkName_vfork;
   CheckName checkName_FloatLoopCounter;
   CheckName checkName_UncheckedReturn;
 };
 
 class WalkAST : public StmtVisitor<WalkAST> {
   BugReporter &BR;
public:
   void checkCall_bcopy(const CallExpr *CE, const FunctionDecl *FD);
   void checkCall_bzero(const CallExpr *CE, const FunctionDecl *FD);
   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 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)
+    .Cases("sprintf", "vsprintf", "scanf", "wscanf", "fscanf", "fwscanf",
+           "vscanf", "vwscanf", "vfscanf", "vfwscanf",
+           &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Cases("sscanf", "swscanf", "vsscanf", "vswscanf", "swprintf",
+           "snprintf", "vswprintf", "vsnprintf", "memcpy", "memmove",
+           &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
+    .Cases("strncpy", "strncat", "memset",
+           &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
     .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_mktemp,
                      "Potential insecure temporary file in call 'mktemp'",
                      "Security",
                      "Call to function 'mktemp' is insecure as it always "
                      "creates or uses insecure temporary file.  Use 'mkstemp' "
                      "instead",
                      CELoc, CE->getCallee()->getSourceRange());
 }
 
-
 //===----------------------------------------------------------------------===//
 // Check: Use of 'mkstemp', 'mktemp', 'mkdtemp' should contain at least 6 X's.
 //===----------------------------------------------------------------------===//
 
 void WalkAST::checkCall_mkstemp(const CallExpr *CE, const FunctionDecl *FD) {
   if (!filter.check_mkstemp)
     return;
 

 }
 
 //===----------------------------------------------------------------------===//
 // Check: Any use of 'strcpy' is insecure.
 //
 // CWE-119: Improper Restriction of Operations within
 // the Bounds of a Memory Buffer
 //===----------------------------------------------------------------------===//
+
 void WalkAST::checkCall_strcpy(const CallExpr *CE, const FunctionDecl *FD) {
   if (!filter.check_strcpy)
     return;
 
   if (!checkCall_strCommon(CE, FD))
     return;
 
   const auto *Target = CE->getArg(0)->IgnoreImpCasts(),

 }
 
 //===----------------------------------------------------------------------===//
 // Check: Any use of 'strcat' is insecure.
 //
 // CWE-119: Improper Restriction of Operations within
 // the Bounds of a Memory Buffer
 //===----------------------------------------------------------------------===//
+
 void WalkAST::checkCall_strcat(const CallExpr *CE, const FunctionDecl *FD) {
   if (!filter.check_strcpy)
     return;
 
   if (!checkCall_strCommon(CE, FD))
     return;
 
   // Issue a warning.
   PathDiagnosticLocation CELoc =
     PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
   BR.EmitBasicReport(AC->getDecl(), filter.checkName_strcpy,
                      "Potential insecure memory buffer bounds restriction in "
                      "call 'strcat'",
                      "Security",
                      "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.
+//
+//        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;
+
+  if (!BR.getContext().getLangOpts().C11)
+    return;
+
+  // Issue a warning. ArgIndex == -1: Deprecated but not unsafe (has size
+  // restrictions).
+  enum { DEPR_ONLY = -1, UNKNOWN_CALL = -2 };
+  StringRef Name = FD->getIdentifier()->getName();
+  int ArgIndex =
+      llvm::StringSwitch<int>(Name)
+          .Cases("scanf", "wscanf", "vscanf", "vwscanf", 0)
+          .Cases("sprintf", "vsprintf", "fscanf", "fwscanf", "vfscanf",
+                 "vfwscanf", "sscanf", "swscanf", "vsscanf", "vswscanf", 1)
+          .Cases("swprintf", "snprintf", "vswprintf", "vsnprintf", "memcpy",
+                 "memmove", "memset", "strncpy", "strncat", DEPR_ONLY)
+          .Default(UNKNOWN_CALL);
+
+  assert(ArgIndex != UNKNOWN_CALL && "Unsupported function");
+  bool BoundsProvided = ArgIndex == DEPR_ONLY;
+
+  if (!BoundsProvided) {
+    // 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)
+      BoundsProvided = true;
+  }
+
+  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 ";
+
+  if (!BoundsProvided) {
+    Out2 << "bounding of the memory buffer or ";
+  }
+
+  Out2 << "security checks introduced "
+          "in the C11 standard. 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.
   int numArgs = FPT->getNumParams();
   if (numArgs != 2 && numArgs != 3)

 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)

cfe/trunk/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{{Call to function 'sprintf' is insecure}}
+  scanf("%d", &a); // expected-warning{{Call to function 'scanf' is insecure}}
+  scanf("%s", buf); // expected-warning{{Call to function 'scanf' is insecure}}
+  scanf("%4s", buf); // expected-warning{{Call to function 'scanf' is insecure}}
+  wscanf((const wchar_t*) L"%s", buf); // expected-warning{{Call to function 'wscanf' is insecure}}
+  fscanf(file, "%d", &a); // expected-warning{{Call to function 'fscanf' is insecure}}
+  fscanf(file, "%s", buf); // expected-warning{{Call to function 'fscanf' is insecure}}
+  fscanf(file, "%4s", buf); // expected-warning{{Call to function 'fscanf' is insecure}}
+  fwscanf(file, (const wchar_t*) L"%s", wbuf); // expected-warning{{Call to function 'fwscanf' is insecure}}
+  sscanf("5", "%d", &a); // expected-warning{{Call to function 'sscanf' is insecure}}
+  sscanf("5", "%s", buf); // expected-warning{{Call to function 'sscanf' is insecure}}
+  sscanf("5", "%4s", buf); // expected-warning{{Call to function 'sscanf' is insecure}}
+  swscanf(L"5", (const wchar_t*) L"%s", wbuf); // expected-warning{{Call to function 'swscanf' is insecure}}
+  swprintf(L"5", 1, (const wchar_t*) L"%s", wbuf); // expected-warning{{Call to function 'swprintf' is insecure}}
+  snprintf("5", 1, "%s", buf); // expected-warning{{Call to function 'snprintf' is insecure}}
+  memcpy(buf, wbuf, 1); // expected-warning{{Call to function 'memcpy' is insecure}}
+  memmove(buf, wbuf, 1); // expected-warning{{Call to function 'memmove' is insecure}}
+  strncpy(buf, "a", 1); // expected-warning{{Call to function 'strncpy' is insecure}}
+  strncat(buf, "a", 1); // expected-warning{{Call to function 'strncat' is insecure}}
+  memset(buf, 'a', 1); // expected-warning{{Call to function 'memset' is insecure}}
+}
+
+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{{Call to function 'vsprintf' is insecure}}
+  vscanf(format, args); // expected-warning{{Call to function 'vscanf' is insecure}}
+  vfscanf(file, format, args); // expected-warning{{Call to function 'vfscanf' is insecure}}
+  vsscanf("a", format, args); // expected-warning{{Call to function 'vsscanf' is insecure}}
+  vsnprintf("a", 1, format, args); // expected-warning{{Call to function 'vsnprintf' is insecure}}
+}
+
+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{{Call to function 'vwscanf' is insecure}}
+  vfwscanf(file, format, args); // expected-warning{{Call to function 'vfwscanf' is insecure}}
+  vswscanf(L"a", format, args); // expected-warning{{Call to function 'vswscanf' is insecure}}
+  vswprintf(L"a", 1, format, args); // expected-warning{{Call to function 'vswprintf' is insecure}}
+}