335549 - [FIX]UUID generator is nonrandom on x86-64

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Benjamin Smedberg]

On my FC4 x86-64 box, I frequently see this error: Security Error: Content at moz-nullprincipal:{00000000-0000-4000-8000-000000000000} may not load or link to http://fxfeeds.mozilla.org/rss20.xml

This error is apparently pretty normal, but the ID is not: it should be random, and it's consistently the same number. bz thinks this is probably due to x86-64

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the "4000" comes from:

  155     id->m2 |= 0x4000;

and the "8000" comes from:

  159     id->m3[0] |= 0x80;

in the UUID generator.  So the actual "random" UUID we generate is 0 everywhere.

Benjamin, would you be willing to step through nsUUIDGenerator::GenerateUUIDInPlace and check the following:

1)  The value returned by the call to random() (as hex)
2)  The value of mRBytes
3)  The value of sizeof(rval)

Comment 2

[Benjamin Smedberg]

> 1)  The value returned by the call to random() (as hex)

(gdb) p/x rval
$36 = 0x4fa7a4f

> 2)  The value of mRBytes

(gdb) p mRBytes
$37 = 3 '\003'

> 3)  The value of sizeof(rval)

(gdb) p sizeof(rval)
$34 = 8

Comment 3

[Benjamin Smedberg]

So, it seems that random() only returns a 4-byte number, but we pretend it's an 8-byte number. Then, we only use the last three bytes, which are all zero?

Comment 4

[Boris Zbarsky [:bzbarsky]]

That's what it looks like, yes.  We should probably be using the mRBytes low bytes, not the mRBytes high bytes (the latter is what we do right now), no?

Comment 5

[Boris Zbarsky [:bzbarsky]]

So replace:

141         PRUint8 *src = (PRUint8*)&rval;
142 #ifdef IS_LITTLE_ENDIAN
143         src += sizeof(rval) - mRBytes;
144 #endif

with:

  // Read the least significant mRBytes bytes of rval
  PRUint8 *src = (PRUint8*)&rval;
#ifdef IS_BIG_ENDIAN
  src += sizeof(rval) - mRBytes;
#endif

Does doing that work?

And then before we do the "153     /* Put in the version */" thing, we should check whether the resulting ID is all zeros; if so, NS_ERROR and throw so these things are easier to catch.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Benjamin, could you maybe verify that this works?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

*** Bug 347300 has been marked as a duplicate of this bug. ***

Comment 9

[John Gaunt (redfive)]

This bug also exists on the 1.8.1 branch, are there plans to move the patch to the branch?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er.... I didn't realize this had gotten landed on branch.  Yeah, we should probably take this, as well as the various other fixes to the UUID generator on branch.  :(

Comment 11

[Mike Schroepfer]

Comment on attachment 220227 [details] [diff] [review]
Proposed patch

a=schrep for drivers.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.