uuid.uuid1() is not suitable as an unguessable identifier/token

Bug #1379826 reported by Gavin Panella on 2014-10-10
260
This bug affects 1 person
Affects Status Importance Assigned to Milestone
MAAS
Critical
Blake Rouse
1.2
Critical
Unassigned
1.5
Critical
Unassigned
1.6
Critical
Unassigned
1.7
Undecided
Unassigned
1.9
Critical
Blake Rouse
Trunk
Critical
Blake Rouse

Bug Description

uuid.uuid1() has been used for the FileStorage.key field. This field is
used to create unguessable URLs that we can hand out from MAAS to refer
back to that file. However, uuid1() is not random enough:

  def uuid1(node=None, clock_seq=None):
      """Generate a UUID from a host ID, sequence number, and the
      current time. If 'node' is not given, getnode() is used to obtain
      the hardware address. If 'clock_seq' is given, it is used as the
      sequence number; otherwise a random 14-bit sequence number is
      chosen."""

The host ID is, afaik, based on the machine's MAC address, so this is
easily discoverable. The current time is assumed to be nanoseconds, but
in practice appears to be microseconds; check time.time() output.

An attacker, with the knowledge of the machine's MAC address, would need
to search 58982400000000 UUIDs to discover every file created during a 1
hour timespan (3600e6 microseconds * 2^14). That's a lot of UUIDs, but I
doubt it's even close to "secure".

I think uuid.uuid4() would be a better choice here, as it is entirely
made from random data. Or we could go direct and encode (e.g. base
16/32/64) 16 bytes straight from /dev/urandom.

This follows on from discussion in bug 1212205.

CVE References

Gavin Panella (allenap) on 2014-10-10
summary: - uuid.uuid1() is not suitable as a "private" identifier/token
+ uuid.uuid1() is not suitable as an unguessable identifier/token
description: updated
Julian Edwards (julian-edwards) wrote :

I am assuming this is a trivial fix s/uuid1/uuid4/ ?

It needs landing on all existing releases as well, so targeting appropriately.

Changed in maas:
status: New → Triaged
importance: Undecided → Critical
milestone: none → next
Gavin Panella (allenap) wrote :

> I am assuming this is a trivial fix s/uuid1/uuid4/ ?

Yes. I don't think we can do anything about preexisting files without breaking things.

Raphaël Badin (rvb) wrote :

Well spotted. This is a trivial fix, let's land this as part of 1.7 if we can.

On Tuesday 14 Oct 2014 15:38:31 you wrote:
> Well spotted. This is a trivial fix, let's land this as part of 1.7 if
> we can.

Let's not. If it's in 1.7 it needs to be pushed to all other releases at the
same time because of disclosure.

We can safely SRU this as a security fix after the release.

Christian Reis (kiko) wrote :

Aren't we going to have to update all releases anyway because of the other security issue we are fixing? If so, shouldn't we do it all at once?

Julian Edwards (julian-edwards) wrote :

Yes. That's exactly what I suggested in an email I sent separately about the outstanding security bugs.

Christian Reis (kiko) on 2014-10-30
Changed in maas:
milestone: next → 1.7.2
Christian Reis (kiko) on 2015-01-28
Changed in maas:
status: Triaged → In Progress
Marc Deslauriers (mdeslaur) wrote :

This is CVE-2014-1428

Marc Deslauriers (mdeslaur) wrote :

What's the status on this issue? Did this get fixed? Does this bug need to remain private?

Marc Deslauriers (mdeslaur) wrote :

This security bug has been open for over a year now. I plan on making this bug public on 2016-05-01.

Changed in maas:
status: Fix Committed → Fix Released
information type: Private Security → Public Security
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers