[PATCH] D44143: Create properly seeded random generator check

[Borsik Gábor via Phabricator via cfe-commits]

boga95 updated this revision to Diff 137915.
boga95 marked an inline comment as done.
boga95 added a comment.

Add capability to provide a user-configurable list of disallowed types which 
will trigger warning. Now it also detects C srand function. Rename check to 
cert-msc51-cpp. Add cert-msc32-c check. Change warning messages. Other minor 
fixes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44143

Files:
  clang-tidy/cert/CERTTidyModule.cpp
  clang-tidy/cert/CMakeLists.txt
  clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp
  clang-tidy/cert/ProperlySeededRandomGeneratorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cert-msc32-c.rst
  docs/clang-tidy/checks/cert-msc51-cpp.rst
  test/clang-tidy/cert-msc32-c.c
  test/clang-tidy/cert-msc51-cpp.cpp

Index: test/clang-tidy/cert-msc51-cpp.cpp
===
--- /dev/null
+++ test/clang-tidy/cert-msc51-cpp.cpp
@@ -0,0 +1,201 @@
+// RUN: %check_clang_tidy %s cert-msc51-cpp %t -- -config="{CheckOptions: [{key: cert-msc51-cpp.DisallowedSeedTypes, value: 'some_type,time_t'}]}" -- -std=c++11
+
+namespace std {
+
+template 
+struct linear_congruential_engine {
+  linear_congruential_engine(int _ = 0);
+  void seed(int _ = 0);
+};
+using default_random_engine = linear_congruential_engine;
+
+using size_t = int;
+template 
+struct mersenne_twister_engine {
+  mersenne_twister_engine(int _ = 0);
+  void seed(int _ = 0);
+};
+using mt19937 = mersenne_twister_engine;
+
+template 
+struct subtract_with_carry_engine {
+  subtract_with_carry_engine(int _ = 0);
+  void seed(int _ = 0);
+};
+using ranlux24_base = subtract_with_carry_engine;
+
+template 
+struct discard_block_engine {
+  discard_block_engine();
+  discard_block_engine(int _);
+  void seed();
+  void seed(int _);
+};
+using ranlux24 = discard_block_engine;
+
+template 
+struct independent_bits_engine {
+  independent_bits_engine();
+  independent_bits_engine(int _);
+  void seed();
+  void seed(int _);
+};
+using independent_bits = independent_bits_engine;
+
+template 
+struct shuffle_order_engine {
+  shuffle_order_engine();
+  shuffle_order_engine(int _);
+  void seed();
+  void seed(int _);
+};
+using shuffle_order = shuffle_order_engine;
+
+struct random_device {
+  random_device();
+  int operator()();
+};
+} // namespace std
+
+using time_t = unsigned int;
+time_t time(time_t *t);
+
+void f() {
+  const int seed = 2;
+  time_t t;
+
+  // One instantiation for every engine
+  std::default_random_engine engine1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: random number generator seeded with a default argument will generate a predictable sequence of values [cert-msc51-cpp]
+  std::default_random_engine engine2(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: random number generator seeded with a constant value will generate a predictable sequence of values [cert-msc51-cpp]
+  std::default_random_engine engine3(seed);
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: random number generator seeded with a constant value will generate a predictable sequence of values [cert-msc51-cpp]
+  std::default_random_engine engine4(time(&t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: random number generator seeded with a disallowed source of seed value will generate a predictable sequence of values [cert-msc51-cpp]
+  engine1.seed();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator seeded with a default argument will generate a predictable sequence of values [cert-msc51-cpp]
+  engine1.seed(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator seeded with a constant value will generate a predictable sequence of values [cert-msc51-cpp]
+  engine1.seed(seed);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator seeded with a constant value will generate a predictable sequence of values [cert-msc51-cpp]
+  engine1.seed(time(&t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator seeded with a disallowed source of seed value will generate a predictable sequence of values [cert-msc51-cpp]
+
+  std::mt19937 engine5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: random number generator seeded with a default argument will generate a predictable sequence of values [cert-msc51-cpp]
+  std::mt19937 engine6(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: random number generator seeded with a constant value will generate a predictable sequence of values [cert-msc51-cpp]
+  std::mt19937 engine7(seed);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: random number generator seeded with a constant value will generate a predictable sequence of values [cert-msc51-cpp]
+  std::mt19937 engine8(time(&t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: random number generator seeded with a disallowed source of seed value will generate a predictable sequence of values [cert-msc51-cpp]
+  engine5.seed();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random

[PATCH] D44143: Create properly seeded random generator check

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:26
+std::random_device dev;
+std::mt19937 engine3(dev()); // Good
+  }

Seeding MT19937 with a single 32-bit integer is //not// "Good". It makes the 
seed super easy to brute-force; and for example, `engine3` will never produce 7 
or 13 as its first output.
http://www.pcg-random.org/posts/cpp-seeding-surprises.html

This doesn't affect the implementation or usefulness of this clang-tidy check, 
which is pretty nifty. I merely object to marking this sample code with the 
comment "Good" in official documentation. It should be marked "Will not warn" 
at best. Or replace it with something slightly more realistic, e.g.

int x = atoi(argv[1]);
std::mt19937 engine3(x);  // Will not warn

As Aaron said above, seeding with the current time is approximately as good an 
idea, and "will not warn" with the current diagnostic either.

The correct way to seed a PRNG is to initialize the //entire state// with 
random bits, not just 32 bits of the state. This can be done, but not yet in 
standard C++: 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0205r0.html



Comment at: test/clang-tidy/cert-properly-seeded-random-generator.cpp:76
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be 
seeded with a random_device instead of a constant 
[cert-properly-seeded-random-generator]
+  engine1.seed(seed);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be 
seeded with a random_device instead of a constant 
[cert-properly-seeded-random-generator]

Is the diagnostic suppressed if `seed` is a template parameter? (Not that I'd 
do this. It's just a corner case I thought of.)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44143: Create properly seeded random generator check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this check!

Do you think it might be possible to also add a check for `cert-msc32-c` that 
handles the C side of things, and use a common check to implement both? C won't 
ever have the C++'isms, but C++ can certainly abuse `std::srand()` so it seems 
like there's at least some overlap.




Comment at: clang-tidy/cert/CERTTidyModule.cpp:43
+CheckFactories.registerCheck(
+"cert-properly-seeded-random-generator");
 CheckFactories.registerCheck("cert-dcl50-cpp");

The name should be `cert-msc51-cpp` (and categorized with the other msc checks).



Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:22-24
+  hasAnyName("linear_congruential_engine", "mersenne_twister_engine",
+ "subtract_with_carry_engine", "discard_block_engine",
+ "independent_bits_engine", "shuffle_order_engine"));

These should be using fully-qualified names, like 
`::std::mersenne_twister_engine`. Also, I think this list should be 
configurable so that users can add their own engines if needed.



Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:66
+
+  template
+void ProperlySeededRandomGeneratorCheck::checkSeed(

Formatting looks off here.



Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:71
+diag(Func->getExprLoc(), "random number generator must be seeded with "
+ "a random_device instead of default argument");
+return;

The diagnostics here aren't quite correct -- a `random_device` isn't 
*required*. For instance, it's also fine to open up /dev/random and read a 
seed's worth of bytes. I think a better phrasing might be: `random number 
generator seeded with a default argument will generate a predictable sequence 
of values`.



Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp:77-78
+  if (Func->getArg(0)->EvaluateAsInt(Value, *Result.Context)) {
+diag(Func->getExprLoc(), "random number generator must be seeded with "
+ "a random_device instead of a constant");
+return;

I'd probably use similar wording here: `random number generator seeded with a 
constant value will generate a predictable sequence of values`.

You can combine these diagnostics and use %select{}0 to choose between the 
variants.



Comment at: clang-tidy/cert/ProperlySeededRandomGeneratorCheck.h:33-34
+  template
+  void checkSeed(const ast_matchers::MatchFinder::MatchResult &Result,
+ const T *Func);
+};

It seems like this should be a private function rather than public.



Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:3
+
+cert-properly-seeded-random-generator
+=

When renaming the check, be sure to update titles, file names, etc.



Comment at: docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:7
+This check flags all pseudo-random number engines and engine adaptors
+instantiations when it initialized or seeded with default argument or constant
+expression. Pseudo-random number engines seeded with a predictable value may

Remove "it".



Comment at: 
docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst:22-23
+
+std::time_t t;
+engine1.seed(std::time(&t)); // Bad, system time might be controlled by 
user
+

There's no test case for this test, and I don't think this code would generate 
a diagnostic in the current check form. This might require a 
(user-configurable) list of disallowed sources of seed values. For instance, it 
should diagnose when called with a `time_t` value or a direct call to a time 
function, but it need not diagnose if the value is somehow obscured. e.g.,
```
unsigned get_seed() { std::time_t t; return std::time(&t); }
engine1.seed(get_seed()); // No diagnostic required
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44143



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44143: Create properly seeded random generator check

[Borsik Gábor via Phabricator via cfe-commits]

boga95 created this revision.
boga95 added a reviewer: clang-tools-extra.
boga95 added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, mgorny.

This check flags all pseudo-random number engines and engine adaptors 
instantiations when it initialized or seeded with default argument or a 
constant expression. Pseudo-random number engines seeded with a predictable 
value may cause vulnerabilities e.g. in security protocols.
This is a CERT security rule, see MSC51-CPP 
.

Example:

  void foo() {
  std::mt19937 engine1; // Bad, always generate the same sequence
  std::mt19937 engine2(1); // Bad
  engine1.seed(); // Bad
  engine2.seed(1); // Bad
  
  std::time_t t;
  engine1.seed(std::time(&t)); // Bad, system time might be controlled by 
user
  
  std::random_device dev;
  std::mt19937 engine3(dev()); // Good
  }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44143

Files:
  clang-tidy/cert/CERTTidyModule.cpp
  clang-tidy/cert/CMakeLists.txt
  clang-tidy/cert/ProperlySeededRandomGeneratorCheck.cpp
  clang-tidy/cert/ProperlySeededRandomGeneratorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cert-properly-seeded-random-generator.rst
  test/clang-tidy/cert-properly-seeded-random-generator.cpp

Index: test/clang-tidy/cert-properly-seeded-random-generator.cpp
===
--- /dev/null
+++ test/clang-tidy/cert-properly-seeded-random-generator.cpp
@@ -0,0 +1,172 @@
+// RUN: %check_clang_tidy %s cert-properly-seeded-random-generator %t -- -- -std=c++11
+
+namespace std {
+
+template 
+struct linear_congruential_engine {
+  linear_congruential_engine(int _ = 0);
+  void seed(int _ = 0);
+};
+using default_random_engine = linear_congruential_engine;
+
+using size_t = int;
+template 
+struct mersenne_twister_engine {
+  mersenne_twister_engine(int _ = 0);
+  void seed(int _ = 0);
+};
+using mt19937 = mersenne_twister_engine;
+
+template 
+struct subtract_with_carry_engine {
+  subtract_with_carry_engine(int _ = 0);
+  void seed(int _ = 0);
+};
+using ranlux24_base = subtract_with_carry_engine;
+
+template 
+struct discard_block_engine {
+  discard_block_engine();
+  discard_block_engine(int _);
+  void seed();
+  void seed(int _);
+};
+using ranlux24 = discard_block_engine;
+
+template 
+struct independent_bits_engine {
+  independent_bits_engine();
+  independent_bits_engine(int _);
+  void seed();
+  void seed(int _);
+};
+using independent_bits = independent_bits_engine;
+
+template 
+struct shuffle_order_engine {
+  shuffle_order_engine();
+  shuffle_order_engine(int _);
+  void seed();
+  void seed(int _);
+};
+using shuffle_order = shuffle_order_engine;
+
+struct random_device {
+  random_device();
+  int operator()();
+};
+} // namespace std
+
+void f() {
+  const int seed = 2;
+  // One instantiation for every engine
+  std::default_random_engine engine1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: random number generator must be seeded with a random_device instead of default argument
+  std::default_random_engine engine2(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator]
+  std::default_random_engine engine3(seed);
+  // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator]
+  engine1.seed();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be seeded with a random_device instead of default argument [cert-properly-seeded-random-generator]
+  engine1.seed(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator]
+  engine1.seed(seed);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator]
+
+  std::mt19937 engine4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: random number generator must be seeded with a random_device instead of default argument
+  std::mt19937 engine5(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator]
+  std::mt19937 engine6(seed);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: random number generator must be seeded with a random_device instead of a constant [cert-properly-seeded-random-generator]
+  engine4.seed();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: random number generator must be seeded with a random_device instead of default argument [cert-properly-seeded-random-generator]
+  engine4.seed(1);
+  // CHECK-MESSAGE