Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NewV4: non-random uuid #73

Closed
josselin-c opened this issue Mar 23, 2018 · 46 comments
Closed

NewV4: non-random uuid #73

josselin-c opened this issue Mar 23, 2018 · 46 comments

Comments

@josselin-c
Copy link

josselin-c commented Mar 23, 2018

I'm running this on my macbook pro.

I'm using uuid.Must(uuid.NewV4()).String() to generate random identifiers. I'm generating theses identifiers at a very low rate of a few dozen per days

Here are some non-random UUID that I got in the last weeks:

02524e6f-65a7-4cf5-8000-0000000000002
6e3ef1c8-0000-4000-8000-0000000000001
fa07f1e9-a8a0-427f-8103-e34e000000000
2cfa392b-71d5-4000-8000-000000000000
5f16db16-56ce-4000-8000-000000000000
bac73b37-aab3-498b-8000-000000000000
da5bd82d-4f98-4b51-8000-000000000000
eb245e52-535f-4274-8350-3a7200000000
f7510000-0000-4000-8000-000000000000
5936f955-286e-47ac-8000-000000000000
0a14da92-0000-4000-8000-000000000000
80730000-0000-4000-8000-000000000000
cd2b88a5-0000-4000-8000-000000000000

I think I shouldn't have theses.

@domino14
Copy link

domino14 commented Apr 3, 2018

humans are bad at telling randomness. While it is extraordinarily, astronomically unlikely that you would have so many uuids with that many zeros, it is still theoretically possible.

@josselin-c
Copy link
Author

In an IDs such as 80730000-0000-4000-8000-000000000000 there are ~14 consecutive bytes of zeros.. While a probability of something like 1/2**(14*8) isn't nil, it's still too low for my taste. Drawing theses IDs 5 times out of ~1000 isn't just bad luck. In real life, if it was possible, I would start mining bitcoins with my super powers!

6e3ef1c8-0000-4000-8000-0000000000001
f7510000-0000-4000-8000-000000000000
0a14da92-0000-4000-8000-000000000000
80730000-0000-4000-8000-000000000000
cd2b88a5-0000-4000-8000-000000000000

I don't know the root cause of this.

@peterstace
Copy link

These UUIDs seem suspiciously non-random to me as well.

NewV4 gets its randomness from crypto/rand.Reader (and is basically a passthrough to populate a UUID). The code in NewV4 is really simple, I don't see how it could go wrong.

The only possibilities I can think of:

  • There is a bug in crypto/rand (seems unlikely).
  • Since crypto/rand.Reader is just a variable, some malicious/accidental code could be swapping it out for a less random io.Reader implementation.

@josselin-c
Copy link
Author

josselin-c commented Apr 4, 2018

Okay, I can reproduce it!
On my machine I have two separate processes: one webserver (ServerA) that calls uuid.Must(uuid.NewV4()).String() while simultaneously doing http requests to another webserver (ServerB) used for image resizing (imgproxy).

I instrumented ServerA with this code:

	go func() {
		for {
			time.Sleep(1 * time.Second)
			for i := 0; i < 1000; i++ {
				u := uuid.Must(uuid.NewV4()).String()
				if strings.Contains(u, "000000000") {
					log.Fatal(u)
				}
			}
		}
	}()

Which runs without crashing if ServerA idle or handling standard requests. If ServerA is calling ServerB (to resize images), the ServerA immediately crash with a bad uuid.
I'll try to make a self-contained reproducer

@josselin-c
Copy link
Author

Okay, I found the problem.
Here is the current NewV4 code:

// NewV4 returns random generated UUID.
func (g *rfc4122Generator) NewV4() (UUID, error) {
	var err error
	var c int
	u := UUID{}
	if _, err = g.rand.Read(u[:]); err != nil {
		return Nil, err
	}
	u.SetVersion(V4)
	u.SetVariant(VariantRFC4122)

	return u, nil
}

Read() gives no guaranties about the number of bytes actually read. From https://golang.org/pkg/io/#Reader:

Read reads up to len(p) bytes into p. It returns the number of bytes read (0 <= n <= len(p)) and any error encountered

Turns out, on some occasions Read would read less than len(u) bytes. I thinks uuid should use ReadFull instead of plain Read().

@domino14
Copy link

domino14 commented Apr 4, 2018

@josselin-c Could you make a patch and see if you can reproduce the error with your script and ReadFull? I wonder if using ReadFull would outright return an error in this case, because there aren't enough bytes to read.

@domino14
Copy link

domino14 commented Apr 4, 2018

One more thing that is interesting to me is the fact that the UUIDs you posted are not of equal length.

josselin-c pushed a commit to josselin-c/go.uuid that referenced this issue Apr 4, 2018
Use ReadFull to fetch random bytes from crypto/rand instead of calling
Read directly as Read may read less bytes than asked.

Fix satori#73
@mikesun
Copy link

mikesun commented Jun 1, 2018

So it looks like this commit introduced the bug: 0ef6afb

Prior to that commit, rand.Read() was used, which is a wrapper that uses io.ReadFull(): https://golang.org/pkg/crypto/rand/#Read

@cameracker
Copy link

How is a critical bug like this still unresolved? Especially given that someone already did the work to fix it.

kmuriki pushed a commit to kmuriki/sif that referenced this issue Aug 10, 2021
The latest tagged version of the module contains the deficiency
described in satori/go.uuid#73. Advancing to
the commit where this issue was addressed.

Conflicts:
	go.mod
	internal/app/siftool/modif.go
	pkg/sif/create_test.go
@austinmhyatt
Copy link

What's the fix here? :)
Searching for upgrade option for CVE-2021-3538

@rkuris
Copy link

rkuris commented Aug 24, 2021

What's the fix here? :)
Searching for upgrade option for CVE-2021-3538

Switch from unmaintained github.com/satori/go.uuid to github.com/gofrs/uuid

@austinmhyatt
Copy link

Thanks! @rkuris

naveensrinivasan added a commit to naveensrinivasan/lnd that referenced this issue Oct 1, 2021
Fix the satori/go.uuid reference to avoid the CVE.
More information jackc/pgx#1052
satori/go.uuid#75
satori/go.uuid#73
naveensrinivasan added a commit to ossf/scorecard that referenced this issue Oct 4, 2021
naveensrinivasan added a commit to ossf/scorecard that referenced this issue Oct 4, 2021
tankbusta added a commit to mandiant/gocrack that referenced this issue Dec 14, 2021
arstercz pushed a commit to arstercz/telegraf that referenced this issue Mar 5, 2023
…uxdata#6768)

While there has been a workaround in place for some time, this change is
being made to reduce confusion around if Telegraf is affected by
satori/go.uuid#73
@nickdnk
Copy link

nickdnk commented Sep 18, 2024

So it looks like this commit introduced the bug: 0ef6afb

Prior to that commit, rand.Read() was used, which is a wrapper that uses io.ReadFull(): https://golang.org/pkg/crypto/rand/#Read

The code was already vulnerable in the released version 1.2.0, as the same call was made in the safeRandom function. See 0ef6afb#diff-0d0495ad16c6f603876bbf484c43b549f53d0b33d4cd74c908b0ee95a94369eaL200.

.I think this has incorrectly led some people to believe that 1.2.0 was not affected. This issue was created in March, and the latest release was in January same year, so unless @josselin-c used the code off the master branch and not 1.2.0, he would have experienced this issue on 1.2.0.

Currently, NIST lists 1.2.0 as excluded in the vulnerability scope of this project, which is causing us some compliance problems at the moment as the package is being used in a managed service we use at AWS. See https://nvd.nist.gov/vuln/detail/CVE-2021-3538

I have limited experience (alright, none) with Go, so please correct me if I am wrong. It seems various services/scanners out there do not agree if 1.2.0 is vulnerable or not.

If 1.2.0 is not vulnerable, there isn't any reason to panic at all, as the vulnerable code was then never released as a versioned dependency.

Edit: Okay, I read all of the comment I quoted. Since the replaced rand call is not vulnerable, 1.2.0 does not have any vulnerabilities. I have no idea why this vulnerability was ever published for 1.2.0 then.

Edit 2: I would like to stress that I am fully aware that this package is ancient and should not be used. However, I have run into compliance issues because it is still being used in code I don't control (AWS, thank you very much). I was/am trying to find out which way is up with this finding and how seriously to take it.

@nickdnk
Copy link

nickdnk commented Oct 21, 2024

Version 1.2.0 of this package is no longer listed as vulnerable. NIST have revised their publication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests