NewV4: non-random uuid about go.uuid HOT 44 CLOSED

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().

from go.uuid.

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.

from go.uuid.

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.

from go.uuid.

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

from go.uuid.

A few people in the Go community have banded together to adopt this package. We've just cut a v2.0.0 release of our fork: https://github.com/gofrs/uuid/releases/tag/v2.0.0

This is a GitHub organization with a few community members in it, so this package isn't being maintained by one individual. We're also available on the Gophers slack in #gofrs.

from go.uuid.

The CVE-2021-3538 has been assigned to this issue.

from go.uuid.

Migrating to this currently maintened fork would be a fair option.

from go.uuid.

@josselin-c could you update the issue description and link to the new https://github.com/gofrs/uuid project?

from go.uuid.

With BigQuery I tried to find github projects using the vulnerable version of this library and opened an issue on each of theses projects. At least the word is out and projects can act accordingly. I unfortunately don't have time to patch them all.
I hope there wasn't too many false positive 😄

from go.uuid.

@theckman @CameronAckermanSEL I fully agree with your comments.

That's why I think assigning an official CVE could be a good option to make more people aware that this repo shouldn't be use anymore and instead of this everyone should start using github.com/gofrs/uuid.

from go.uuid.

Thanks - my fork doesn't contain that commit, and still uses the crypto/rand reader, which errors on a non-full read, so it's not vulnerable to this issue. I'll add a note to the docs. I'm still committed to maintaining my fork.

from go.uuid.

This bug is a serious security issue, definitely a CVE should be assigned to this vulnerability.
Everyone should be aware about this vulnerability.
Additionally it would be good to release new version, because the last 1.2.0 is before the g.rand.Read function introduction.

@satori - can you request a CVE ID for this issue and release new version to make it visible that problem has been addressed?
If you need assistance with CVE ID please let me know (I can help and assign CVE ID from Red Hat side).

from go.uuid.

alternative: https://play.golang.org/p/flHC1sB2wMo

from go.uuid.

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.

from go.uuid.

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

from go.uuid.

@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.

from go.uuid.

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

from go.uuid.

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

from go.uuid.

@josselin-c Did you confirm that these project were using a vulnerable version, or did you just fire from the hip?

from go.uuid.

@josselin-c there's also talk in the Go community of taking over this project, and when we go this route I guess I'll need to go to every issue you opened and provide a correct the suggestions made to point to our version. Would you be willing to update all the issues you opened to mention that there may be a viable fork coming?

from go.uuid.

I used theses query:

SELECT * 
FROM [go.uuid] 
WHERE REGEXP_MATCH(content, r'Must\(\w+\.NewV4')

uuid.Must(uuid.NewV4()) has been introduced with the vulnerability so matching this expression should be correct.

SELECT id
FROM [go.uuid] 
WHERE REGEXP_MATCH(content, r'err := uuid\.NewV4')

Same thing, NewV4() that returns an error has been introduced with the vulnerability, matching this expression should be correct as well.

from go.uuid.

Here is the projects list: https://gist.github.com/josselin-c/c1bda90ba538c65a5b8062aa2db0c386

from go.uuid.

Thanks, I think this will get a lot of the people who upgraded, but some folks caught and handled the error without using Must. Not a huge deal though.

EDIT: Sorry, I misread the second query.

from go.uuid.

My second query is there to match the cases where Must wasn't used. In this case I assume they used the 'standard' convention for error variable naming & package name.

from go.uuid.

Thank you for opening the issues that you did. This combined with the versioning issue that was opened months ago has pushed me into transitioning over to the google uuid repo.

from go.uuid.

@yamamushi I checked that google/uuid repo and it's kind of lacking and has some code smells. I would advise against moving there long term. I have high hopes for a semi official go community solution that @theckman is trying to get set up.

from go.uuid.

ping @monkeydioude, @kevinburke, @markbates saw that you guys have some forks that other people were directed to for appropriate tagging and such. Thought you might want to be aware

from go.uuid.

from go.uuid.

@rkuris gofrs being an activists collaboration is open for any suggestions, as soon as we all can agree it adds value to the project

A required note: this is not an official position, just my personal understanding of the aim of the organisation.

from go.uuid.

@p-rog I've been trying to contact @satori for years with no success. Please see #103 and #84.

from go.uuid.

@p-rog The sad thing is given the amount of time this issue has existed, anybody using this repo is creating a self-inflicted problem. I fully realize that this is largely on @satori for not doing the bare minimum to tag the release, but there are multiple issues about this on the backlog and I've been personally going in and responding periodically to PRs and new issues telling people to stop using the repo. What we're fighting is people who don't do the due diligence to read anything about the dependencies they're pulling in, or will double down on using a dependency because they don't want to migrate.

It'd be nice if there was some way we could get github to intervene, at least in requesting an archival of a dangerous repo. But at some point we can't fix stupid.

from go.uuid.

@p-rog You can try reaching @satori through LinkedIn, he is active there. Considering this one has gone ignored for so many years I think that's your best bet for getting in contact.

from go.uuid.

@p-rog If you have the ability to file a CVE, that sounds great. Synk already has a "CVE" on their system so it's popping up there. If there's any help you need I would be willing to give a hand but I'm guessing you're quite savvy in this area :)

PS. Sorry for the rename timing hope it isnt confusing

from go.uuid.

@cameracker no worries, I caught the name change

I've requested new CVE id for this security bug. Once the CVE id will be issued I will post it here.

from go.uuid.

For those who end up here, this fork is maintained and includes the fix to this issue: https://github.com/gofrs/uuid

from go.uuid.

Am I missing something or tag 1.2 seem to be ok?

from go.uuid.

@vaiojarsad the commit where the issue was fixed is here: 75cca53 (April 4, 2018)

The v1.2.0 tag is here: https://github.com/satori/go.uuid/releases/tag/v1.2.0 (Jan 10, 2018) based on commit f58768c (Jan 3, 2018)

v1.2.0 is vulnerable to this issue, only the master branch is fixed: v1.2.0...master

from go.uuid.

Hi @theckman ! I'd already seen what you are saying but I took a look at https://github.com/satori/go.uuid/archive/refs/tags/v1.2.0.zip, more specifically at the generator.go file inside the zip, and it seems like the bug hasn't been introduced there.

Kind regards!

from go.uuid.

// NewV4 returns random generated UUID.
func (g *generator) NewV4() UUID {
	u := UUID{}
	g.safeRandom(u[:])
	u.SetVersion(V4)
	u.SetVariant(VariantRFC4122)

	return u
}

.......

func (g *generator) safeRandom(dest []byte) {
	if _, err := rand.Read(dest); err != nil {
		panic(err)
	}
}

And rand is crypto/rand here (crypto/rand uses ReadFull)

from go.uuid.

@vaiojarsad The issue was introduced by this commit:
0ef6afb#diff-0d0495ad16c6f603876bbf484c43b549f53d0b33d4cd74c908b0ee95a94369ea
When owner of that repo decided to change the return (UUID, error) instead of UUID.

That commit was after the 1.2.0 release.
In the 1.2.0 release the g.rand.Read function is not used.

Later with this commit owner changed the introduced g.rand.Read function to ReadFull:
d91630c

@vaiojarsad please only keep in mind that this repo is not maintained any more, it's much better and safer to start using this fork: https://github.com/gofrs/uuid

from go.uuid.

I am mistaken; apologies for the noise.

from go.uuid.

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

from go.uuid.

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

from go.uuid.

Thanks! @rkuris

from go.uuid.