[Clang-tidy] CERT-MSC50-CPP (std:rand() )

[falho]

Summary

This checker warns fors the usage of std:rand() function

more info about the problem can be found here:
https://www.securecoding.cert.org/confluence/display/cplusplus/MSC50-CPP.+Do+not+use+std%3A%3Arand%28%29+for+generating+pseudorandom+numbers

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

docs/clang-tidy/checks/cert-msc50-cpp.rst

5

Should be same length as section name above.

7

Please use check and highlight std::rand() with ``.

[Prazek]

LGTM with the fixes of docs.

clang-tidy/cert/LimitedRandomnessCheck.cpp

32

I am not sure about the diagnostics convention, it is possible that you should replace comma with semicolon.

[aaron.ballman]

Thank you for working on this check!

clang-tidy/cert/CERTTidyModule.cpp

37

Please place this under a MSC heading rather than DCL.

This check should additionally be listed as cert-msc30-c (https://www.securecoding.cert.org/confluence/display/c/MSC30-C.+Do+not+use+the+rand%28%29+function+for+generating+pseudorandom+numbers).

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

This should be looking at a callExpr() rather than a declRefExpr(), should it not?

32

This diagnostic is incorrect for C code. Should only suggest C++ <random> in C++ mode. Also, it should be a semicolon rather than a comma in the diagnostic text.

clang-tidy/cert/LimitedRandomnessCheck.h

20

Both sentences are correct, but it doesn't clarify why std::rand() is bad. Should add a sentence between these two that says something about why std::rand() in particular is called out rather than the other pseudorandom number generators.

docs/clang-tidy/checks/cert-msc50-cpp.rst

7

The same argument could be used to disallow C++ <random> generators that aren't true truly random sources. Should specify more about why rand() is particularly bad.

docs/clang-tidy/checks/list.rst

21

Please also add a cert-msc30-c file with a redirect (like fio38-c from above).

test/clang-tidy/cert-limited-randomness.cpp

2

Please also add a test for C since the check works there as well, and will have different diagnostic text.

[Prazek]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

I was also thinking about this, but this is actually better, because it will also match with binding rand with function pointer.

[aaron.ballman]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

True, but a DeclRefExpr doesn't mean it's a function call. Binding the function is not contrary to the CERT rule, just calling it. For instance, the following pathological case will be caught by this check:

if (std::rand) {}

The behavior of this check should be consistent with cert-env33-c, which only looks at calls. (If we really care about bound functions, we'd need flow control analysis, and I think that's overkill for both of those checks, but wouldn't be opposed to someone writing the flow analysis if they really wanted to.)

[falho]

Hi! Thanks for the reviews! I will be off for a few days so I will start working on it when Im back. Greetz! Benedek

[Prazek]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

It would indeed fire on this pathological case, but I don't think we should care about cases like this, because no one is writing code like this (and if he would then it would probably be a bug).
I don't think that there is much code that binds pointer to std::rand either, but I think it would be good to display warning for this, because even if the function would be never called, then it means that this is a bug, and if it would be called then it would be nice to tell user that rand might be used here.

Anyway I don't oppose for changing it to callExpr, but I think it is better this way.

[aaron.ballman]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

It would indeed fire on this pathological case, but I don't think we should care about cases like this, because no one is writing code like this (and if he would then it would probably be a bug).

It would be a known false-positive for a check designed to conform to a particular coding standard. When deviations have come up in the past for various coding standards, we've added an option to enable the additional functionality, which I don't think would be reasonable in this case. Alternatively, the CERT guideline could be modified, but that is unlikely to occur because binding the function pointer is not a security concern (only calling the function).

[xazax.hun]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

In case you let binding to function pointer you introduce potential false negatives which is worse in this case in my opinion.

[aaron.ballman]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

Basically: this half-measure is sufficient for the CERT coding rule, but isn't ideal. The ideal check isn't likely to uncover many more cases than the half-measure, which is why it was not implemented in the past. If someone wants to implement the whole-measure, that's great! But implementing a half, half-measure that isn't consistent with other, similar checks is the wrong thing to do.

[xazax.hun]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

You can not implement an ideal checker. In general, it is undecidable whether std::rand is called or not. (You can easily create an example where you would need to solve the halting problem in order to decide whether std::rand is called.)

Since the ideal checker is infeasible the question is whether you are OK with false positives or false negatives. The approach you are suggesting result in false negatives. The current approach results in false positives. I think, for this security checker, a false positive is much less serious to have than a false negative. Moreover, I doubt that people write code where such false positives are intended and the code should not be changed. But in case you can think of an example, please let us know.

[xazax.hun]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

I think consisteny with other checks are not always a good argument. You might want to ask what is the expected false positive and false nagtive rate from a check, and what is the guarantee that a user expects from a check. And I think base on that it is a unique decision that should be considered independently for each check. In this case I think it is more valuable to have a guarantee that in case all of the code is covered, std::rand() is not invoked. Using a callExpr instead of declRefExpr we would loose this guarantee at the cost of not reporting some false positive cases that are unlikely to annoy anyone anyways.

[aaron.ballman]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

You can not implement an ideal checker. In general, it is undecidable whether std::rand is called or not. (You can easily create an example where you would need to solve the halting problem in order to decide whether std::rand is called.)

I said "ideal", not "perfect", but we're splitting hairs. You are correct, you're never going to get perfect clarity without runtime instrumentation. By "ideal", I meant "something that actually pays attention to the bound value from the point it is bound to the point the function pointer is called." Simply having the address of the function is not a security concern; calling it is.

I think, for this security checker, a false positive is much less serious to have than a false negative.

We'll have to agree to disagree. As the person who maintains the CERT rules (and their checkers), my preference for right now is to use callExpr().

[falho]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

Thank you for the reviews! So what is the conclusion? Should I change to callExpr()?

[aaron.ballman]

clang-tidy/cert/LimitedRandomnessCheck.cpp

23–24

My preference is that the check use callExpr() -- otherwise the check will trigger as a false positive for code that is allowed by MSC50-CPP and MSC30-C. If others feel strongly that declRefExpr() is the better approach, then I'd like CommandProcessorCheck::registerMatchers(), SetLongJmpCheck::registerMatchers(), and StrToNumCheck::registerMatchers() changed to behave similarly (though not as part of this patch).

[falho]

updated diff according to first reviews

[aaron.ballman]

Thank you for continuing your efforts on this, I have just a few minor nits remaining.

clang-tidy/cert/LimitedRandomnessCheck.cpp

36

For C code, this diagnostic will read strangely due to the trailing semicolon. You should move the semicolon into the msg above. Perhaps we can also drop "function" from the diagnostic as well.

docs/clang-tidy/checks/cert-msc50-cpp.rst

4

This should be cert-msc50-cpp instead.

docs/clang-tidy/checks/list.rst

21

This should be cert-msc30-c

[aaron.ballman]

I noticed you marked several comments as done, but the patch is not updated with changes. Have I missed something?

[falho]

changes according to 2nd review

[aaron.ballman]

clang-tidy/cert/.LimitedRandomnessCheck.cpp.swo was added and should not have been; also, there's one minor issue with the diagnostic wording that is still outstanding.

clang-tidy/cert/LimitedRandomnessCheck.cpp

36

This comment does not appear to have been handled -- the semicolon is still present even when msg is empty.

[falho]

removed semicolon, and replaced it with a comma that only appears in .cpp diagnostics
test cases corrected according to this

removed junk .swo file

[aaron.ballman]

In D22346#580242, @falho wrote:

removed semicolon, and replaced it with a comma that only appears in .cpp diagnostics

The semicolon was the correct punctuator to use, but thank you for moving it into the cpp message.

test cases corrected according to this

removed junk .swo file

Thanks!

clang-tidy/cert/LimitedRandomnessCheck.cpp

31

Switch the "," to ";", and remove the curly braces.

[falho]

in cpp diagnostics message: comma changed back to semicolon, + curly braces removed
testfiles corrected accordingly

[aaron.ballman]

LGTM, thank you for working on this!

[falho]

Cool! Thank you for the reviews!

[aaron.ballman]

In D22346#581329, @falho wrote:

Cool! Thank you for the reviews!

If you don't have commit privileges, let me know and I'm happy to commit on your behalf.

[falho]

Thanks but I think I will try it!

[falho]

I just figured out that I don't have right to commit to llvm so I would appreciate if you could commit this check for me. Do you need any info about me? Thank you!

[aaron.ballman]

Thanks! I've commit in r285809

[falho]

Thanks !

clang-tidy/cert/CERTTidyModule.cpp

 #include "../google/UnnamedNamespaceInHeaderCheck.h"
 #include "../misc/MoveConstructorInitCheck.h"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "CommandProcessorCheck.h"
 #include "FloatLoopCounter.h"
+#include "LimitedRandomnessCheck.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cert {
 
 class CERTModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     // C++ checkers
     // DCL
     CheckFactories.registerCheck<VariadicFunctionDefCheck>(
         "cert-dcl50-cpp");
     CheckFactories.registerCheck<misc::NewDeleteOverloadsCheck>(
         "cert-dcl54-cpp");
     CheckFactories.registerCheck<google::build::UnnamedNamespaceInHeaderCheck>(
         "cert-dcl59-cpp");
     // OOP
     CheckFactories.registerCheck<misc::MoveConstructorInitCheck>(
         "cert-oop11-cpp");
     // ERR
     CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
         "cert-err09-cpp");
     CheckFactories.registerCheck<SetLongJmpCheck>(
         "cert-err52-cpp");
     CheckFactories.registerCheck<StaticObjectExceptionCheck>(
         "cert-err58-cpp");
     CheckFactories.registerCheck<ThrownExceptionTypeCheck>(
         "cert-err60-cpp");
     CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
         "cert-err61-cpp");
+    // MSC
+    CheckFactories.registerCheck<LimitedRandomnessCheck>(
+        "cert-msc50-cpp");
 
     // C checkers
     // DCL
     CheckFactories.registerCheck<misc::StaticAssertCheck>(
         "cert-dcl03-c");
     // ENV
     CheckFactories.registerCheck<CommandProcessorCheck>(
         "cert-env33-c");
     // FLP
     CheckFactories.registerCheck<FloatLoopCounter>(
         "cert-flp30-c");
     // FIO
     CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>(
         "cert-fio38-c");
     // ERR
     CheckFactories.registerCheck<StrToNumCheck>(
         "cert-err34-c");
+    // MSC
+    CheckFactories.registerCheck<LimitedRandomnessCheck>(
+        "cert-msc30-c");
   }
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     Options.CheckOptions["cert-oop11-cpp.UseCERTSemantics"] = "1";
     return Options;
   }
 };
 

clang-tidy/cert/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCERTModule
   CERTTidyModule.cpp
   CommandProcessorCheck.cpp
   FloatLoopCounter.cpp
+  LimitedRandomnessCheck.cpp
   SetLongJmpCheck.cpp
   StaticObjectExceptionCheck.cpp
   StrToNumCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
 
   LINK_LIBS
   clangAnalysis

clang-tidy/cert/LimitedRandomnessCheck.h

+//===--- LimitedRandomnessCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Pseudorandom number generators are not genuinely random. The result of the
+/// std::rand() function makes no guarantees as to the quality of the random
+/// sequence produced.
+/// This check warns for the usage of std::rand() function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc50-cpp.html
+class LimitedRandomnessCheck : public ClangTidyCheck {
+public:
+  LimitedRandomnessCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_LIMITED_RANDOMNESS_H

clang-tidy/cert/LimitedRandomnessCheck.cpp

+//===--- LimitedRandomnessCheck.cpp - clang-tidy---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "LimitedRandomnessCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+void LimitedRandomnessCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(callExpr(callee(functionDecl(namedDecl(hasName("::rand")),
+                                                  parameterCountIs(0))))
+                         .bind("randomGenerator"),
+                     this);
+}
+
+void LimitedRandomnessCheck::check(const MatchFinder::MatchResult &Result) {
+  std::string msg = "";
+  if (getLangOpts().CPlusPlus) {
+    msg = " use C++11 random library instead";
+  }
+
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("randomGenerator");
+  diag(MatchedDecl->getLocStart(),
+       "rand() has limited randomness;" + msg);
+}
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+

docs/clang-tidy/checks/cert-msc30-c.rst

+.. title:: clang-tidy - cert-msc30-c
+
+cert-msc30-c
+============
+
+The cert-msc30-c check is an alias, please see
+`cert-msc50-cpp <cert-msc50-cpp.html>`_ for more information.

docs/clang-tidy/checks/cert-msc50-cpp.rst

+.. title:: clang-tidy - cert-msc50-cpp
+
+cert-msc50-cpp
+==============
+
+Pseudorandom number generators use mathematical algorithms to produce a sequence of numbers with good statistical properties, but the numbers produced are not genuinely random. The ``std::rand()`` function takes a seed (number), runs a mathematical operation on it and returns the result. By manipulating the seed the result can be predictible. This check warns for the usage of ``std::rand()``.

docs/clang-tidy/checks/list.rst

.. toctree::
    cert-err09-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err09-cpp>
    cert-err34-c
    cert-err52-cpp
    cert-err58-cpp
    cert-err60-cpp
    cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err61-cpp>
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
+   cert-msc30-c (redirects to cert-limited-randomness) <cert-msc30-c>
+   cert-msc50-cpp
    cert-oop11-cpp (redirects to misc-move-constructor-init) <cert-oop11-cpp>
    cppcoreguidelines-interfaces-global-init
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init

test/clang-tidy/cert-limited-randomness.c

+// RUN: %check_clang_tidy %s cert-msc30-c %t
+
+extern int rand(void);
+int nonrand();
+
+int cTest() {
+  int i = rand();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness; [cert-msc30-c]
+
+  int k = nonrand();
+
+  return 0;
+}

test/clang-tidy/cert-limited-randomness.cpp

+// RUN: %check_clang_tidy %s cert-msc50-cpp %t
+
+int rand();
+int rand(int);
+
+namespace std {
+using ::rand;
+}
+
+namespace nonstd {
+  int rand();
+}
+
+void testFunction1() {
+  int i = std::rand();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+
+  int j = ::rand();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+
+  int k = rand(i);
+
+  int l = nonstd::rand();
+
+  int m = rand();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: rand() has limited randomness, use C++11 random library instead [cert-msc50-cpp]
+}
+