[clang-tidy] Create properly seeded random generator check
Needs ReviewPublic

Authored by boga95 on Tue, Mar 6, 3:58 AM.



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.


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

Diff Detail

boga95 created this revision.Tue, Mar 6, 3:58 AM

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.


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


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.


Formatting looks off here.


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.


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.


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

3 ↗(On Diff #137151)

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

7 ↗(On Diff #137151)

Remove "it".

22–23 ↗(On Diff #137151)

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
aaron.ballman edited reviewers, added: aaron.ballman, alexfh; removed: Restricted Project.Tue, Mar 6, 5:52 AM
aaron.ballman removed a subscriber: aaron.ballman.
Quuxplusone added inline comments.
26 ↗(On Diff #137151)

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.

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

76 ↗(On Diff #137151)

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.)

boga95 updated this revision to Diff 137915.Sat, Mar 10, 10:24 AM
boga95 marked an inline comment as done.

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.

lebedev.ri retitled this revision from Create properly seeded random generator check to [clang-tidy] Create properly seeded random generator check.Sat, Mar 10, 10:27 AM
boga95 marked 4 inline comments as done.Mon, Mar 12, 12:42 PM
boga95 added a subscriber: szepet.
boga95 added inline comments.
76 ↗(On Diff #137151)

Hopefully, it is not suppressed.

aaron.ballman added inline comments.Mon, Mar 12, 5:20 PM

This change looks unrelated to the patch.


This change looks unrelated to the patch.


These should be full-qualified names.


I think that in C mode, this is fine, but in C++ mode it should register ::std::srand.


You can use llvm::find() instead.


Backticks around srand


Should link to the CERT rule, and also link to the C CERT rule.

Eugene.Zelenko added inline comments.

Please replace srand with `srand ()`.


Two of them and please add (). Will be good idea to make first statement same as in Release Notes.


Is there guideline documentation available online? If so, please add link. See other checks documentation as example.

alexfh removed a reviewer: alexfh.Wed, Mar 14, 6:08 AM