When SystemRandom is not available, we try to

  from random import random

and then use "random.random()" - but this fails as we have only 
imported the random function, not the module.

In password.py we use os.urandom (for SSHA) without checking if it is 
available.

The places where we reseed the random number generator seem to be a 
bit, well, random. We should probably only reseed after forking, i.e. 
in ForkingServer (roundup_server.py).

And finally, Python 3.6 provides a secrets module - we should probably 
use that, if available, and fall back to SystemRandom/os.urandom and 
finally just the random module.

patch as basis for discussion:

https://bitbucket.org/cmeerw/roundup/commits/3df442e3d72bb8c4755eacc280
9583d62aa813ce

Hi Christof:

In message <1533241514.61.0.56676864532.issue2550988@psf.upfronthosting.co.za>,
Christof Meerwald writes:
>New submission from Christof Meerwald:
>
>When SystemRandom is not available, we try to
>
>  from random import random
>
>and then use "random.random()" - but this fails as we have only 
>imported the random function, not the module.

That's probably code that never gets tested. Adding a test to generate
the needed exception would be good.

>In password.py we use os.urandom (for SSHA) without checking if it is 
>available.

Wow, yeah that would generate a nice exception at the very least.
Question, does the salt needs to be generated using os.urandom at all?
Everything else in that module seems to use just random. Is the
randomness of the salt that critical?

>The places where we reseed the random number generator seem to be a 
>bit, well, random. We should probably only reseed after forking, i.e. 
>in ForkingServer (roundup_server.py).

There are at least 5 deployment mechanisms for roundup (see "Configure
a Web Interface" in doc/installation.txt . Only one of which is the
server. What happens when roundup is used as a cgi or under
mod_python, zope or wsgi? Cgi is probably ok since we get a default
seed when the process is started that is thrown away when the process
exits. But mod_python, wsgi and zope are IIUC long lived processes
which do not go through scripts/roundup_server.py, but do go through
the cgi/client.py module which is where I did the seeding.

I think the seeding there isn't the best as in theory the seeding data
source could be known (IIRC it's the timestap when seed is
called). That could be known by an attacker as it is some small window
around the time of the http request. It would probably be better
to mix in some state derived from all prior interactions with the
server. But I didn't have the knowledge to make that happen.

>And finally, Python 3.6 provides a secrets module - we should probably 
>use that, if available, and fall back to SystemRandom/os.urandom and 
>finally just the random module.

That sounds like a good idea. Just make sure that every execution
path for all deployment methods runs through the module.

On Thu, 2 Aug 2018, John P. Rouillard wrote:

> >In password.py we use os.urandom (for SSHA) without checking if it is 
> >available.
> 
> Wow, yeah that would generate a nice exception at the very least.
> Question, does the salt needs to be generated using os.urandom at all?
> Everything else in that module seems to use just random. Is the
> randomness of the salt that critical?

Optimally for salts, no two passwords hashed with the same algorithm 
(ever, anywhere) would use the same salt, so there is no benefit to an 
attacker precomputing hashes / rainbow tables.  Typically that should mean 
salts of at least 64 bits.  The security model here is protecting against 
attackers who somehow acquire a large number of password hashes and 
corresponding salts.  Since the attacker is presumed to know the salt, its 
unpredictability should not be so critical, unless it results in the same 
salt being likely to be reused (including on different sites).

Things that need secure randomness include password generation 
(generatePassword), OTKs, session IDs for cookies, CSRF nonces.  It's less 
critical, I think, for salts and for message-ids.  (I don't think Roundup 
should do its own message-id generation anyway - use 
email.utils.make_msgid instead from the standard library.)

When secure randomness is needed, you can either use system random number 
generation directly, or use pseudorandom number generation having recently 
seeded from the system random number generator.  What's not safe is using 
pseudorandom number generation having not reseeded that generator for a 
long time (because if too many random numbers have been generated from the 
same generator, an attacker may be able to determine its state, and thus 
predict the random numbers used for other users).

Rather than trying to decide that particular places do not need secure 
randomness, it's certainly safer always to use the system random number 
generator.

> >The places where we reseed the random number generator seem to be a 
> >bit, well, random. We should probably only reseed after forking, i.e. 
> >in ForkingServer (roundup_server.py).
> 
> There are at least 5 deployment mechanisms for roundup (see "Configure
> a Web Interface" in doc/installation.txt . Only one of which is the

That's actually a list of six, despite saying five above the list.

> server. What happens when roundup is used as a cgi or under
> mod_python, zope or wsgi? Cgi is probably ok since we get a default
> seed when the process is started that is thrown away when the process
> exits. But mod_python, wsgi and zope are IIUC long lived processes
> which do not go through scripts/roundup_server.py, but do go through
> the cgi/client.py module which is where I did the seeding.
> 
> I think the seeding there isn't the best as in theory the seeding data
> source could be known (IIRC it's the timestap when seed is
> called). That could be known by an attacker as it is some small window

When os.urandom is available, it should be used by Python for reseeding by 
default.  Except that this code is using SystemRandom when available, so 
the only time when reseeding is not a no-op, it's also using a weak seed.

I'd suggest simply making the code fail if SystemRandom / os.urandom is 
not available, rather than trying to have fallbacks (without SystemRandom 
there is no good way to get a strong seed).  Once you're always using 
SystemRandom (directly or indirectly), you no longer need to deal with 
reseeding.

Hi Joseph:

In message <alpine.DEB.2.20.1808022303100.30418@digraph.polyomino.org.uk>,
Joseph Myers writes:
>On Thu, 2 Aug 2018, John P. Rouillard wrote:
>> Question, does the salt needs to be generated using os.urandom at all?
>> Everything else in that module seems to use just random. Is the
>> randomness of the salt that critical?
>
>Optimally for salts, no two passwords hashed with the same algorithm 
>(ever, anywhere) would use the same salt, so there is no benefit to an 
>attacker precomputing hashes / rainbow tables.  Typically that should mean 
>salts of at least 64 bits.  The security model here is protecting against 
>attackers who somehow acquire a large number of password hashes and 
>[...]
>seeded from the system random number generator.  What's not safe is using 
>pseudorandom number generation having not reseeded that generator for a 
>long time (because if too many random numbers have been generated from the 
>same generator, an attacker may be able to determine its state, and thus 
>predict the random numbers used for other users).

Correct. That's why I was trying to figure out some way to mix
existing state into the salt to make the reseed state depend on
something other than just the time.

>Rather than trying to decide that particular places do not need secure 
>randomness, it's certainly safer always to use the system random number 
>generator.

Agreed. I considered dropping the pseudorandom generator entirely, but
I couldn't rule out that there were platforms which run python that do
not have a working os.random. Roundup can be used in a mode that does
not require security (e.g. as a GTD tool with a single user on a stick
computer, (yeah I thought it sounded interesting)).

>> >The places where we reseed the random number generator seem to be a 
>> >bit, well, random. We should probably only reseed after forking, i.e. 
>> >in ForkingServer (roundup_server.py).
>> 
>> There are at least 5 deployment mechanisms for roundup (see "Configure
>> a Web Interface" in doc/installation.txt . Only one of which is the
>
>That's actually a list of six, despite saying five above the list.

Two of them are different ways of invoking the cgi mode so...

Also one other thing occurred to me, some of the code paths may be
used by roundup-admin and thus need to be seeded (assuming you don't
do away with the pseudo random generator) in those code paths as well.

Also since a user can write scripts that import roundup, the roundup
routines that need random data should make sure that the pseudo random
generator is properly randomized when required. It shouldn't require
the user to properly initialize the random generators.

On Thu, 2 Aug 2018, John P. Rouillard wrote:

> >> There are at least 5 deployment mechanisms for roundup (see "Configure
> >> a Web Interface" in doc/installation.txt . Only one of which is the
> >
> >That's actually a list of six, despite saying five above the list.
> 
> Two of them are different ways of invoking the cgi mode so...

I'd suggest saying "several web interfaces" rather than "five web 
interfaces" would avoid the question of what is or is not counted as a 
different web interface (and mean one fewer thing to update the next time 
someone adds a new web interface).

Here is another patch that keeps the re-seeding in place, just moves 
the selection of the preferred random number generation method into a 
single module:

https://bitbucket.org/cmeerw/roundup/commits/91c7933f5a95fc8f1f9867b5f7
cd45f4430424d8

(Note that I have removed some special "randomness" code for CSRF 
handling as it seems strange to handle that in a different way than 
gen_sid/gen_nonce in client.py; ideally, we would have a strong random 
number generator anyway if we care about these things).

Hi Christof:

In message
<1533420121.71.0.56676864532.issue2550988@psf.upfronthosting.co.za>,
Christof Meerwald writes:
>Christof Meerwald added the comment:
>
>Here is another patch that keeps the re-seeding in place, just moves 
>the selection of the preferred random number generation method into a 
>single module:
>
>https://bitbucket.org/cmeerw/roundup/commits/91c7933f5a95fc8f1f9867b5f7
>cd45f4430424d8

In your patch you seed a new random instance, not the default random
instance. You should also seed the default random instance on every
client session. The issue with the default pseudorandom number
generator not being seeded was detected by an extension on the
python.org bugtracker. The extension returned a random issue. That
stopped working because the default random generator was not seeded on
every client connection. While every extension writer could add their
own call to seed the ransom source, I think it is better if we seed it
with proper random info in the core.

So make sure you seed the random generator even if we don't use it in
the main roundup code. It is reasonable for tracker developers to
think that random is properly seeded on every client connection.

>(Note that I have removed some special "randomness" code for CSRF 
>handling as it seems strange to handle that in a different way than 
>gen_sid/gen_nonce in client.py; ideally, we would have a strong random 
>number generator anyway if we care about these things).

I'm ok with that if you believe that there is enough randomness in
roundup/anypy/random_.py when it takes the pseudorandom path.

Have a great week.

On Sat, Aug 04, 2018 at 10:56:49PM +0000, John Rouillard wrote:
> In your patch you seed a new random instance, not the default random
> instance. You should also seed the default random instance on every
> client session. The issue with the default pseudorandom number
> [...]
> 
> So make sure you seed the random generator even if we don't use it in
> the main roundup code. It is reasonable for tracker developers to
> think that random is properly seeded on every client connection.

But that's what seed_pseudorandom in client.py is for, isn't it? And I
haven't changed that in this version of the patch. Note that if
SystemRandom is available, this was so far the only instance where the
default random number generator was re-seeded as well - so no change
in that case.

Hello Christof:

In message <20180805062132.GP9331@edge.cmeerw.net>,
Christof Meerwald writes:
>Christof Meerwald added the comment:
>On Sat, Aug 04, 2018 at 10:56:49PM +0000, John Rouillard wrote:
>> So make sure you seed the random generator even if we don't use it in
>> the main roundup code. [...]
>
>But that's what seed_pseudorandom in client.py is for, isn't it? And I
>haven't changed that in this version of the patch. Note that if
>SystemRandom is available, this was so far the only instance where the
>default random number generator was re-seeded as well - so no change
>in that case.

I agree with your assessment. Thanks for the explanation.

changes pushed

Looking into this change, I wonder if there are any platforms
that we support that lack random.SystemRandom.

os.urandom() documentation talks about Linux, Unix-like systems and
Windows. While https://en.wikipedia.org/wiki//dev/random 
explains that we have all major operations systems covered.

As even the salt is a cryptographic purpose, my take on this is that
we should only use random.SystemRandom, which does not need to be seeded
and remove all seeding and fallback code. It should make our code even
simpler.

What do you think?

Hi Bern:

In message <1533822990.41.0.56676864532.issue2550988@psf.upfronthosting.co.za>,
Bernhard Reiter writes:
>Bernhard Reiter added the comment:
>Looking into this change, I wonder if there are any platforms
>that we support that lack random.SystemRandom.
>os.urandom() documentation talks about Linux, Unix-like systems and
>Windows. While https://en.wikipedia.org/wiki//dev/random 
>explains that we have all major operations systems covered.
>
>As even the salt is a cryptographic purpose, my take on this is that
>we should only use random.SystemRandom, which does not need to be seeded
>and remove all seeding and fallback code. It should make our code even
>simpler.
>
>What do you think?

For a good quality system random implementation, you need hardware
support.  Running entropy gathering daemons such as eged, haveged
etc. can help if there is no random data source located in the
hardware chip (e.g. on vm's I have run many of them out of random data
and suffered hangs as a result).

Running on a compute stick that has no hardware entropy and minimal
source of randomness (see my earlier comment about setting up a GTD
(getting things done) tracker) should be possible if the user's use
case for roundup supports it. Also consider embedded systems that may
want to use roundup for some purpose.

In any case, seeding the prng random generator obtained via
"import random" should be done in all cases by the core code.
This allows tracker developers to use "import random" to:

  * display a random open issue
  * assign task to random person in list
  * show option A 10% of the time, option B 60% of the time and
    option C 30% of the time
  * etc.

without having to figure out why they keep getting the same value from
random every time.

The changes Christof made simplified the code used for security
related randomness significantly. It is now in one module that is used
throughout the code.

Also there is no reason roundup shouldn't work on VMS or OS/390 or
z/OS or iOS. Although I suspect not many shops these days would run
python on these legacy os's. I wonder if roundup would run in Jython,
PyPy, IronPython or MicroPython variants? I suspect it could work with
anydbm support at the very least.

Have a great week.

> In any case, seeding the prng random generator obtained via
> "import random" should be done in all cases by the core code.

I fully agree for non-cryptographic purposes.

> The changes Christof made simplified the code used for security
> related randomness significantly. It is now in one module that is used
> throughout the code.

Talking about this cryptographic purposes random module:
My suggestion is to remove the variant that goes without SystemRandom.
This would simply it  even more, because the security properties
are more predictable. (And most interesting platforms will have it or if they 
don't they at least have a strong warning that they are lacking an important 
random source and a good solution is then to implement a good SystemRandom
for their platform.)

While I like the idea of simplifying the code I don't want to rule out special purpose use of roundup on embedded chips (e.g. micropython).

I am closing this since the problem is solved and shouldn't show up
on open issues list.