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

cmd/compile: depth limit reached in type formatting routine #29312

Closed
griesemer opened this issue Dec 17, 2018 · 13 comments
Closed

cmd/compile: depth limit reached in type formatting routine #29312

griesemer opened this issue Dec 17, 2018 · 13 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@griesemer
Copy link
Contributor

griesemer commented Dec 17, 2018

Place holder issue for cases of overly deep types or (invalid) recursive types. See #29264 for an example.

@griesemer griesemer added this to the Unplanned milestone Dec 17, 2018
@griesemer griesemer self-assigned this Dec 17, 2018
@gopherbot
Copy link

Change https://golang.org/cl/154583 mentions this issue: cmd/compile: increase nesting depth limit for type descriptors

@griesemer griesemer modified the milestones: Unplanned, Go1.13 Dec 17, 2018
@griesemer
Copy link
Contributor Author

A recursive type where we currently fail:

type T interface {
	M(interface {
		T
	})
}

(from #16369).

gopherbot pushed a commit that referenced this issue Dec 18, 2018
The formatting routines for types use a depth limit as primitive
mechanism to detect cycles. For now, increase the limit from 100
to 250 and file #29312 so we don't drop this on the floor.

Also, adjust some fatal error messages elsewhere to use
better formatting.

Fixes #29264.
Updates #29312.

Change-Id: Idd529f6682d478e0dcd2d469cb802192190602f6
Reviewed-on: https://go-review.googlesource.com/c/154583
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@griesemer griesemer modified the milestones: Go1.13, Go1.14 May 6, 2019
@ianlancetaylor
Copy link
Contributor

#32547 is a test case that fails due to the recursion limit, although the test itself does not use recursion. It just uses a very very very deep pointer type.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@empijei
Copy link
Contributor

empijei commented Dec 10, 2019

An update on this bug:

At Google CTF finals this year a team was able to exploit this behavior and obtain remote code execution without importing any package except for "fmt".

The issue is that some code that was supposedly safe had a memory corruption due to a very deep type.

Here you can find a write up of the challenge, and here is the exploit.

When there is a very deep type (253 in this case) the compiler generates an incorrect program. In my opinion this should not be the case and it should instead reject the invalid code (after all, the generated program will not work anyways).

/cc @mvdan @FiloSottile

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Dec 10, 2019
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Go1.14 Dec 10, 2019
@ianlancetaylor
Copy link
Contributor

Marking as a release blocker for 1.14.

I don't see a reason to block the beta release for this.

@josharian
Copy link
Contributor

Is there a threat model in which this attack is possible but https://research.swtch.com/gorace isn’t?

@randall77
Copy link
Contributor

This exploit doesn't require multiple threads, unlike Russ' example.
We've tried to keep Go strongly type safe in the absence of unsafe and data races. This exploit breaks that idea.

@empijei
Copy link
Contributor

empijei commented Dec 11, 2019

This bug has been there for quite a while and the threat model requires a lot of control on the attacker side (almost arbitrary code).

I don't know if this is exploitable by creating the type via reflection but even if it is it would still require the attacker to call arbitrary primitives, which is unlikely.

It would be nice to have this fixed for the next release but I would not say this is a blocker for 1.14.

@FiloSottile
Copy link
Contributor

This is a classic type confusion and definitely a mis-compilation (as well as an amazing CTF trick). It also introduces unsafety which can't be detected by looking for usages of unsafe or by the race detector. It sounds like something we should fix.

However, I believe we (rightfully) gave up on this threat model with the removal of "safe" mode in Go 1.12 (5185744), so I agree that this is not a security issue.

@dmitshur
Copy link
Contributor

dmitshur commented Jan 2, 2020

/cc @randall77 @griesemer Please let us know when there are updates on this issue, as this is currently marked a release blocker for 1.14.

@gopherbot
Copy link

Change https://golang.org/cl/213517 mentions this issue: cmd/compile: give every really deep type a unique name

gopherbot pushed a commit that referenced this issue Jan 8, 2020
This avoids the security problem in #29312 where two very deep, but
distinct, types are given the same name. They both make it to the
linker which chooses one, and the use of the other is now type unsafe.

Instead, give every very deep type its own name. This errs on the
other side, in that very deep types that should be convertible to each
other might now not be. But at least that's not a security hole.

Update #29312.

Change-Id: Iac0ebe73fdc50594fd6fbf7432eef65f9a053126
Reviewed-on: https://go-review.googlesource.com/c/go/+/213517
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
@randall77
Copy link
Contributor

The security hole aspects of this issue have been fixed. Removing release blocker and punting to 1.15.

@randall77 randall77 modified the milestones: Go1.14, Go1.15 Jan 8, 2020
@gopherbot
Copy link

Change https://golang.org/cl/214239 mentions this issue: cmd/compile: print recursive types correctly

@golang golang locked and limited conversation to collaborators Jan 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

9 participants