feat(utils) generate random strings via CSPRNG

[p0pr0ck5]

Summary

The current implementation of utils.random_string() leverages the LuaJIT math.random() under the hood to generate bytes; this is unsuitable for cases where cryptographically secure random data is needed, such as fallback values for authentication plugin secret values. To correct this, we introduce a wrapper around the kernel CSPRNG (via /dev/urandom) to read random bytes, and wrap utils.random_string around this. We also return these bytes in a modified Base64 encoding (replacing non-alphanumeric characters with random alphanumeric replacements); this serves to increase the size of the keyspace significantly while attempting to maintain some backwards compatibility with previous generated string
parameters (e.g. by generating a string of the same size and a somewhat matching pattern).

The underlying get_rand_bytes implementation is modified to read from /dev/urandom when explicitly requested, and falling back to OpenSSL's RAND_bytes when reading from urandom fails. The blocking read from urandom is acceptable when explicitly requested, as this is typically done in asynchronous environments (e.g. admin API requests), where the need for strong psuedorandomness outweighs the overhead of I/O and talking to the kernel.

Full changelog

• Update lua_system_constants to 0.1.2 (for O_RDONLY)
• Remove duplicate open(), close(), and strerror() cdefs from file-log plugin
• Implement urandom handler in get_rand_bytes
• Update random_string to use get_rand_bytes with urandom
• Update search pattern in default JWT secret spec test
• Update unrelated tests to use the proper util function

[Tieske]

@p0pr0ck5 this contains changes to kong/plugins/file-log/handler.lua? did you mix branches?

[p0pr0ck5]

@Tieske nope, it's noted in the PR summary, we remove duplicate and conflicting cdefs from this handler. Otherwise we attempt to redefine an already loaded symbol

[p0pr0ck5]

BTW the Travis failures here are due to a hiccup with using the wrong build cache and the existing lua_system_constants shared object not being used in the test as part of the luarocks install. It should be ignored, or we can purge the caches to see the build through.

[thibaultcha]

Since OpenSSL's RAND_bytes() is considered cryptographically secure, and itself gets its seed from /dev/urandom, what is the incentive for not using it instead?

I would go a little bit further, and would also like to ask why LuaJIT's math.random() isn't secure enough exactly, considering it is currently being seeded from OpenSSL (itself seeded from /dev/urandom)?

[p0pr0ck5]

Since OpenSSL's RAND_bytes() is considered cryptographically secure, and itself gets its seed from /dev/urandom, what is the incentive for not using it instead?

tl;dr: https://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/

Another good thread: https://twitter.com/ircmaxell/status/653950793830297603

Userspace CSPRNGs aren't the best solution. We have access to the raw kernel entropy device, so let's use it while we can. For use cases that don't need/care about the best possible solution (or don't want to block on the read), this is why we provide a flag to ignore urandom. And, if we want a buffered solution for reading urandom bytes, we can look at something like lua-resty-urandom, which is getting a much-needed update now that lua_system_constants is updated.

I would go a little bit further, and would also like to ask why LuaJIT's math.random() isn't secure enough exactly, considering it is currently being seeded from OpenSSL (itself seeded from /dev/urandom)?

Tausworthe is not a CSPRNG, and thus is certainly not an appropriate source of randomness for generating what we consider secret values. At a higher level, the small search space provided by using a UUID is a weakness IMO; we should absolutely expand the character set use to build these values.

[p0pr0ck5]

Another excellent resource I forgot to mention earlier: https://www.2uo.de/myths-about-urandom/

[thibaultcha]

Yeah, I knew those articles and all the fuss about random/urandom. I would have thought OpenSSL's RAND_bytes() to still be a good enough source though, tbh. The only benefit I see is the non-blocking nature they provide...

Tausworthe is not a CSPRNG

That I wasn't sure of, but couldn't find any source to assert that. Do you have one?

we should absolutely expand the character set use to build these values.

Oh yeah for sure, using UUIDs for secret values was a flawed approach since day 1, definitely.

[bungle]

https://wiki.openssl.org/index.php/Random_Numbers#Initialization

[p0pr0ck5]

Tausworthe is not a CSPRNG

That I wasn't sure of, but couldn't find any source to assert that. Do you have one?

The kernel source is a good one :)

https://github.com/torvalds/linux/blob/1001354ca34179f3db924eb66672442a173147dc/lib/random32.c#L71-L77

[p0pr0ck5]

@bungle yes the method by which OpenSSL seeds itself is not the issue, the problem is that userspace CSPRNGs are inherently flawed.

[bungle]

What do you think about this (it is a syscall but not file io):
https://github.com/justincormack/ljsyscall/blob/9a7b5843895d55cbc1b7ac06b9eafaae41ce947d/test/linux.lua#L344

The thing with userspace (like OpenSSL) is that they will certainly be faster.

[p0pr0ck5]

@bungle yeah this is the ideal approach. But getrandom is only available in more recently kernels (3.17+), which by default is not used by some LTS systems that we support (such as Ubuntu Trusty). So we need to handle both cases anyway (I wouldn't be opposed to doing both). Ideally we would just use the libc wrapper, but this was only very very recently introduced, and is pretty much unavailable anywhere :/

Yes, userspace would be "faster". In our use case (infrequent, asynchronous uses) we are not concerned with pure speed. We are concerned with providing the most secure solution. Trading off to a userspace CSPRNG purely for speed gains is not a valid argument here IMO.

Having said that, I'm not ready to die on a urandom hill. If we're all truly opposed to a kernel-based solution, we can circle back to the argument, but at bare minimum we need to make random_string far more robust, even if that means not touching get_rand_bytes.

[bungle]

It looks like the new random_string is not asking urandom?

[p0pr0ck5]

@bungle yep im a dummy and made a typo after doing some live testing. thanks!

[thibaultcha]

If we fail opening /dev/urandom, we still try to read from it, and later on, to close the file descriptor? Wouldn't that leave use with 3 WARN logs instead of one, potentially?

[p0pr0ck5]

Yeah, refactored approach to avoid this. let me know your thoughts.

[thibaultcha]

assert.is_string(first)

[p0pr0ck5]

done, thanks

[thibaultcha]

Something like assert.not_matches("^[^%a%d]+$", first) maybe

[p0pr0ck5]

done, thanks

[p0pr0ck5]

(same issue with test failures here, this requires cache purging in order to pass because it tries to use the old version of the lua_system_constants object while running the tests as part of the local install).

[p0pr0ck5]

(rebased following 0.10.3 release)

[thibaultcha]

LGTM 👍 Was lua_system_constants released yet? It seems CI is failing due to it currently.

[p0pr0ck5]

@thibaultcha yep lua_system_constants was deployed before this PR was pushed: http://luarocks.org:8080/modules/mashape/lua_system_constants/0.1.2-0

The CI is failing because, as a result of the travis cache, luarocks is using the 0.1.1 version of the shared object when trying to run the tests as part of the dep update process. With a clean cache this passes.