Using random.random() should not be used to generate randomness used for security reasons

[Zhang Yun]

In cinder code : /cinder/transfer/api.py . Below line of code used random.random() to generate a random number, Standard random number generators should not be used to generate randomness used for security reasons. Could we use a crytographic randomness generator to provide sufficient entropy to instead of it?

rndstr = ""
random.seed(datetime.datetime.now().microsecond)
while len(rndstr) < length:
 rndstr += hashlib.sha224(str(random.random())).hexdigest() ---------------> This line has described issues.

 return rndstr[0:length]

[Thierry Carrez (ttx)]

cinder coresec: could you comment on what that "slightly random" number is actually used for ?

[Mike Perez (thingee)]

This generates a secret which is shared with another party to transfer a volume to another user/project.

[Tristan Cacqueray (tristan-cacqueray)]

We might want to do something about this then, from python documentation https://docs.python.org/2/library/random.html:

The pseudo-random generators of this module should not be used for security purposes. Use os.urandom() or SystemRandom if you require a cryptographically secure pseudo-random number generator.

[John Griffith (john-griffith)]

This isn't really a "security" issue IMO. I'm fine with beefing it up via better library if it makes people more comfortable though.

[John Griffith (john-griffith)]

BTW, this is only used in generating the XFR key from the transfer API.

[Ollie Leahy (oliver-leahy-l)]

The rational for the design is in https://wiki.openstack.org/wiki/VolumeTransfer

The 'random' number generated is used as a 'password' that the volume donor sends to the intended recipient.

I also have no problem with this being hardened, but I don't think that any extra value will be added by doing so.

[Thierry Carrez (ttx)]

So the key is very temporary, I gather. If that's the case, then I'd remove the security tag on this one and let it be fixed (or not) without an advisory, unless someone complains.

[Thierry Carrez (ttx)]

That would be a welcome strengthening, but I don't think this is exploitable as is.

[Robert Clark (robert-clark)]

Where there's a defined security feature in OpenStack, such as here with the use of cryptographic functions in Cinder we should ensure that they are done correctly. I think failures to do this _are_ security issues and should be addressed and documented as such.

This probably isn't a big enough issue to warrant an OSSA and I don' think that an OSSN would be appropriate either if the plan is to improve this in the next release.

[Duncan Thomas (duncan-thomas)]

I think it's technically a security vulnerability, but of the most academic nature possible. We should (and will - fix on the way) fix it, but it isn't worthy of special treatment or an OSSA.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/96738

[Ollie Leahy (oliver-leahy-l)]

I just want to note here that implementing transfer expiry, as described in the blueprint, would be a much more valuable patch, in case anyone can take that on.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/96738
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=6791fa41e06beab23bc7832a3bfa9ab28adf1e34
Submitter: Jenkins
Branch: master

commit 6791fa41e06beab23bc7832a3bfa9ab28adf1e34
Author: Ollie Leahy <email address hidden>
Date: Fri May 30 11:57:02 2014 +0000

    Use os.urandom in volume transfer

    This patch replaces a call to random.random() with a call to
    os.urandom(), which generates a higher quality random number.

    Closes-Bug: #1319643

    Change-Id: Ifaa2216d4905f5286884629beac52b25249d621f

[Jeffrey Walton (noloader)]

Sorry about the late comment here.

I think the real problem is this line:

    random.seed(datetime.datetime.now().microsecond)

I think its a problem because there's no entropy in datetime. From Python's source code on seed() (available at http://svn.python.org/projects/python/branches/py3k/Lib/random.py):

    def seed(self, a=None, version=2):

        if a is None:
            try:
                a = int.from_bytes(_urandom(32), 'big')
            except NotImplementedError:
                import time
                a = int(time.time() * 256) # use fractional seconds

        if version == 2:
            if isinstance(a, (str, bytes, bytearray)):
                if isinstance(a, str):
                    a = a.encode("utf-8")
                a += _sha512(a).digest()
                a = int.from_bytes(a, 'big')

        super().seed(a)
        self.gauss_next = None

As can be seen above, the call to seed() replaces the current seed state with a hash of date/time. And its only hashed, and not mixed with bits from, for example, /dev/urandom. So an attacker can perform an exhaustive search of the keyspace based on the time he observes the transfer.

The current construction lacks the PRP notion of security. That is, an adversary can distinguish output from random. *If* the call to random.seed was performed using bits from /dev/urandom, then this would not be a problem. Even keying the hash with bits from /dev/urandom would suffice.

Wagner and Goldberg had a lot of fun with this sort of entropy collection in 1996. For completeness, Netscape did a little more than just date/time - they included the PID also. See "Randomness and the Netscape Browser", http://www.cs.berkeley.edu/~daw/papers/ddj-netscape.html.

[Robert Clark (robert-clark)]

Makes sense, I think most people understand the impact of using datetime to seed random.

However, looking at the change that was made in response to this issue: https://review.openstack.org/#/c/96738/2/cinder/transfer/api.py this is no longer the case and the code is much improved.

-Rob