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

runtime/secret: add new package #21865

Open
zx2c4 opened this issue Sep 13, 2017 · 191 comments
Open

runtime/secret: add new package #21865

zx2c4 opened this issue Sep 13, 2017 · 191 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted
Milestone

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Sep 13, 2017

Final API is here: #21865 (comment)


Forward secrecy is usually only a thing if it's possible to ensure keys aren't actually in memory anymore. Other security properties, too, often require the secure erasure of keys from memory.

The typical way of doing this is through a function such as explicit_bzero, memzero_explicit, or the various other functions that C library writers provide that ensure an optimization-free routine for zeroing buffers.

For the most part, the same is possible in Go application code. However, it is not easily possible to do so with crypto API interfaces that internally manage a key buffer, such as the AEAD interface.

In the Go implementation of WireGuard, @rot256 has been forced to resort to unholy hacks such as:

type safeAEAD struct {
	mutex sync.RWMutex
	aead  cipher.AEAD
}

func (con *safeAEAD) clear() {
	con.mutex.Lock()
	if con.aead != nil {
		val := reflect.ValueOf(con.aead)
		elm := val.Elem()
		typ := elm.Type()
		elm.Set(reflect.Zero(typ))
		con.aead = nil
	}
	con.mutex.Unlock()
}

func (con *safeAEAD) setKey(key *[chacha20poly1305.KeySize]byte) {
	con.aead, _ = chacha20poly1305.New(key[:])
}

Having to resort to this kind of reflection is a bit unsettling and something we'd much rather not do.

So, this issue is to request and track the addition of a consistent "Clear()" interface for parts of the Go crypto API that store keys in internal buffers.


Furthermore, even if real clearing is deemed to be an abject failure due to Go's GC, and one must instead mmap/mlock or use a library such as memguard, the AEAD interface still is inadequate, because it appears that SetKey winds up allocating its own buffer internally. So again, big problem.

cc: @agl

@dsnet
Copy link
Member

dsnet commented Sep 13, 2017

Related to #21374

@anitgandhi
Copy link
Contributor

anitgandhi commented Sep 13, 2017

Just as another example of what was mentioned at the end, even creating an AES block primitive causes an allocation right before key expansion, which shows wiping the key is inadequate, including how memguard does it.

https://github.com/golang/go/blob/master/src/crypto/aes/cipher.go#L46-L47

The key expansion is a deterministic process so once c.enc and c.dec are set, if the system was compromised such that memory could be scanned, even on a system where memguard is used, an attacker could get the contents of c.enc or c.dec, reverse the key expansion process, and now they have the original key. If I'm not mistaken, you could currently call aes.NewCipher(key) where key points to a memguard buffer, immediately wipe key, then continue to use the instantiated cipher "object" for calls to Encrypt and Decrypt calls, since now only the internally expanded key is required.

This comment explains that as well. Effectively, you'd have to create custom implementations of all the existing crypto code in the standard lib, supplementary x/crypto libs, even indirect packages like math/big would end up getting into scope.

In one sense, you'd have to replace make itself to using manually managed memory, like how memguard does it, but that of course is fundamental change to Golang itself, and would make the niceties of GC pointless

@FiloSottile
Copy link
Contributor

This is going to be extremely hard to obtain as a generic guarantee, for all the reasons mentioned here, at #21374 and at awnumar/memguard#3. In particular there is no way to retroactively enforce that an interface implementation does not copy key material on the heap (at least outside of the stdlib, and we don't want a security guarantee that breaks when you use external implementations).

But how about a smaller problem:

  • add a Wipe() method to the implementation (say chacha20poly1305); the method might exist or not based on the platform since implementations can differ, the application can decide to warn or bail out if Wipe() is unavailable by doing an interface-upgrade from AEAD, or not to compile at all by calling it directly on the concrete type
  • make an implementation with a Wipe() method guaranteed not to make copies of the key
  • if necessary, add a NewWithAllocator(makeSlice func(size int) []byte) function so that key material is placed in manually allocated memory exempt from GC and swapping (for example via memguard)
    • alternatively, unsafe can be used to instantiate the chacha20poly1305 object, but then you'd need a Init() method

Would that solve your problem enough?

@FiloSottile
Copy link
Contributor

Actually, forget Wipe(), it makes no sense if you have NewWithAllocator(makeSlice func(size int) []byte) since you can just use the allocator wiping feature (which memguard offers). Moreover, if you don't have neither NewWithAllocator nor a #21374 solution, Wipe() is useless because of GC copies and swapping.

@yahesh
Copy link

yahesh commented Mar 24, 2018

A possibility to reliably wipe secrets used by the crypto library would be highly appreciated. For example, gocryptfs has tried to solve this problem as far as is currently possible by wiping the memory locations it has access to.

However, as the crypto library creates copies of the encryption secrets by deriving encryption keys from the provided secrets it's currently not possible to completely wipe said secrets (and derived keys) from memory.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 26, 2018
@andybons andybons added this to the Unplanned milestone Mar 26, 2018
@gopherbot
Copy link

Change https://golang.org/cl/162297 mentions this issue: crypto/rc4: remove false guarantees from Reset docs and deprecate it

gopherbot pushed a commit that referenced this issue Feb 22, 2019
Nothing in Go can truly guarantee a key will be gone from memory (see
#21865), so remove that claim. That makes Reset useless, because
unlike most Reset methods it doesn't restore the original value state,
so deprecate it.

Change-Id: I6bb0f7f94c7e6dd4c5ac19761bc8e5df1f9ec618
Reviewed-on: https://go-review.googlesource.com/c/162297
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/163438 mentions this issue: [release-branch.go1.12] crypto/rc4: remove false guarantees from Reset docs and deprecate it

gopherbot pushed a commit that referenced this issue Feb 22, 2019
…t docs and deprecate it

Nothing in Go can truly guarantee a key will be gone from memory (see
#21865), so remove that claim. That makes Reset useless, because
unlike most Reset methods it doesn't restore the original value state,
so deprecate it.

Change-Id: I6bb0f7f94c7e6dd4c5ac19761bc8e5df1f9ec618
Reviewed-on: https://go-review.googlesource.com/c/162297
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit b35daca)
Reviewed-on: https://go-review.googlesource.com/c/163438
@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2019

Here's a somewhat different proposal that doesn't require adding anything to the language syntax itself and should deal with interesting behind-the-scenes copies:

runtime.SetZeroOnGC(obj)

In a similar way as SetFinalizer attaches a function to an object, SetZeroOnGC could mark that object's memory as needing to be zeroed whenever the runtime frees it or moves it to a new location. Overtime this could grow nice optimizations too that aren't as easy with manual code; for example, if an object is being GC'd in order to serve an immediate allocation request with a default initial value, then the zeroing could be skipped.

CC @mknyszek @aclements

@awnumar
Copy link
Contributor

awnumar commented May 20, 2019

@zx2c4 interesting solution, but since the garbage collector isn't guaranteed to run this wouldn't really provide any reasonable guarantee of secrets being erased.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2019

@awnumar People who must have the memory zeroed now can of course call runtime.GC(). People with more relaxed requirements, "memory should get zerored sometime in the future", can opt not to do that. Notably that's the same semantic that people are used to having with SetFinalizer.

@awnumar
Copy link
Contributor

awnumar commented May 20, 2019

@zx2c4 I understand yet it seems like considerable extra architectural effort to add this functionality to the runtime without giving it more power in the form of some kind of security guarantees.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2019

Is this talk of "considerable extra architectural effort" true? I was hoping one of the garbage collector maintainers authors would chime in saying something like, "oh that sounds very easy! we'll look into it." Maybe we can wait for them to weigh in on that first?

And "giving it more power in the form of ..." already evaluates to runtime.GC() as I already pointed out. So I'm not sure what you mean.

@awnumar
Copy link
Contributor

awnumar commented May 20, 2019

@zx2c4 Calling runtime.GC() when? Every so often? Or just before we exit? What if we terminate unexpectedly?

Having to deal with an additional function call to ensure some security property holds adds complexity to every code path, and dealing with it can be non-trivial.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2019

I guess it's up to the user and the use case. If the requirement is just that "at some point in the future, it's zeroed", then don't call runtime.GC(). If instead there are specific key erasure points, then call it then.

@awnumar
Copy link
Contributor

awnumar commented May 20, 2019

If the requirement is just that "at some point in the future, it's zeroed"...

Seems like a weak security requirement. I can't think of a use-case for it.

If a feature is going to be added to tackle the problem that this issue is about, it had better do it properly or not do it at all.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2019

Looks like you convenient ignored the following sentence, which mentions the case of a different security requirement.

@yahesh
Copy link

yahesh commented May 20, 2019 via email

@randall77
Copy link
Contributor

Well, you can zero on GC right now with a finalizer:

type Secret struct {
    key [16]byte
}

s := &Secret{key: ...}
runtime.SetFinalizer(s, func(s *Secret) { s.key = [16]byte{} })

If/when Go has a copying GC*, then we would need to think about whether to zero the original copy of an object when it is moved. Maybe we just do that for every object. Maybe we recommend using the finalizer above, and only zero the original copy of things that have finalizers. (Kinda hacky overloading of semantics, but might work.)

I think this issue is asking for something more. In particular, Go exerts no control over what the OS does with heap-backed pages. It could write them to a swap file, for example. I think in general key management is going to require separate allocations for secrets anyway (specially mapped, on secure physical memory, etc.), so adding a mechanism for Go objects is only solving part of the key management problem.

*Go does have copying for stack-allocated objects. We don't currently zero the old copy of those objects. But you can force objects to be allocated off the stack by, for example, putting a finalizer on them.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2019

Well, you can zero on GC right now with a finalizer:

Right, sort of. My proposal is to add something that formalizes the possibility of that, via a specific call to runtime.SetZeroOnGC(obj).

Go exerts no control over what the OS does with heap-backed pages.

Having a specific marking means various facets of it can improve over time with future pull requests. For example, SetZeroOnGC could imply a call to mlock at some point, or whatever else is deemed necessary to make this fit the bill.

In other words, everyone agrees that proper zeroing in Go requires some cooperation from the runtime. Let's start that by adding a marking to the runtime.

@awnumar
Copy link
Contributor

awnumar commented May 20, 2019

Speaking from experience, the ability to provide a custom allocator to the runtime or within well-defined scopes, such as within a function or within a package, would be incredibly helpful. Currently in order to use secure APIs like crypto code requires manually rewriting parts of them to use specially allocated memory.

This is an evolving landscape and we're learning new things all the time about what works best and what does not. I don't personally see the value in a wrapper function for the finalizer: it adds no real new functionality to the table and it gives the impression of greater security than it provides.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2019

Speaking from experience, the ability to provide a custom allocator to the runtime or within well-defined scopes

That sounds interesting. Maybe open a separate PR for that alongside some syntactical (Go 2) or function proposals?

@randall77
Copy link
Contributor

The problem I see with runtime.SetZeroOnGC is that it's too late. You really want to tell the runtime that this object is special at allocation time, not some time after the allocation has happened.

Maybe some reflect.New-style API in the crypto package.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2019

The problem I see with runtime.SetZeroOnGC is that it's too late. You really want to tell the runtime that this object is special at allocation time, not some time after the allocation has happened.

Why does it matter? If you are referencing the object, it's necessarily before it's been GC'd, and so there's a chance to call runtime.SetZeroOnGC. Of course it means that individual temporary or unexported buffers from various modules will overtime need to be marked correctly, but that's fine and probably an evolutionary thing that can happen, mostly in crypto and x/crypto, I assume. If doing this in various places is deemed an unnecessary performance hit by those who don't want it, it's easy enough to make the whole thing opt-in, whereby SetZeroOnGC is globally a no-op until some other function is called or the like.

@randall77
Copy link
Contributor

If it is just SetZeroOnGC, then yes, you can do it after allocation. But for the larger key management problem, you want to tell the runtime "this contains a secret" at allocation time, so a special allocator of some sort can be used.

@zx2c4
Copy link
Contributor Author

zx2c4 commented May 20, 2019

Not sure I follow. For a "larger key management" situation, you call SetZeroOnGC on each piece of key material. If you're allocating it yourself, then you call it right after the new or make keyword. If you're getting it from somewhere else, then you call it when you get it.

It might be the case that you'd like to redirect allocations of an entire module to some totally separate allocator or something wild. That's fine and interesting, and I hope @awnumar will open an issue with a proposal for introducing that kind of multi-allocator machinery to Golang or something. That seems like an interesting parallel effort that might benefit from being spec'd out.

My proposal here is to add a simple flag to the current allocator to solve this problem in an smaller and evolutionary way that can improve overtime.

@DmitriyMV
Copy link

So this was accepted, but no work was done (since most of the effort is currently focused on (runtime/generics/compiler). Is it still planned to be done at some time in the future?

@robaho
Copy link

robaho commented May 9, 2023

I don’t understand the concern here. You cannot read another processed memory without root access. If you have root access you can compromise the security of the entire server - including replacing the crypto libraries with compromised versions.

@nfisher
Copy link

nfisher commented May 9, 2023

@robaho not my forte but my understanding is that it’s to protect things like the key which is only required briefly and remove/zero it from memory as quickly as possible. I assume this would minimise damage by things like spectre attacks as well as more traditional buffer overruns. Commenting mostly to see if my understanding is correct.

@FiloSottile
Copy link
Contributor

I don’t understand the concern here. You cannot read another processed memory without root access. If you have root access you can compromise the security of the entire server - including replacing the crypto libraries with compromised versions.

The most compelling scenario here is a long-running process that is concerned with forward secrecy. If you have a Wireguard server that's been running for a year, and it gets compromised today, the protocol is designed such that it won't be possible to decrypt the past year of recorded traffic. However, if the ephemeral keys are still in memory, that property is lost.

crypto/tls also uses ephemeral key exchanges and rotates session ticket keys for the same reason, but its forward secrecy properties are also weakened by the risk of having old key material remain in memory.

@robaho
Copy link

robaho commented May 9, 2023

Hmm. That sounds suspect to me. If that machine is compromised at a later date, it is easier to install compromised libraries and use that to access the future - and past - data that any user accessing that server has access to. If the server has ephemeral keys in memory that are of any use then the sever is acting as a “free text” gateway and compromising a free text gateway is a critical security failure.

@FiloSottile
Copy link
Contributor

Forward secrecy is a widely desired, studied, and implemented property of cryptographic protocols, and such protocols should be safe to implement in Go. We are not re-debating whether forward secrecy has a reason to exist. I suggest looking up the literature on the matter.

@robaho

This comment was marked as off-topic.

@elagergren-spideroak

This comment was marked as off-topic.

@astrolox

This comment was marked as off-topic.

@FiloSottile

This comment was marked as off-topic.

@robaho

This comment was marked as off-topic.

@sjaconette-google
Copy link

This has come up recently for me related to coredumps. It's valuable to be able to ensure that a coredump does not contain sensitive key material, in case it is later accessed by a wider set of people or processes than had access to the process that generated the dump. IMO having the ability to automatically zero out memory that contained keys would be extremely useful. See https://www.cybersecuritydive.com/news/microsoft-crash-dump-cabinet-email-hacks/692995/ for a recent example of a real compromise that resulted from key material ending up in a coredump.

@tv42
Copy link

tv42 commented Dec 11, 2023

@sjaconette-google Your use of the word "ensure" is tricky here. Just clearing secrets after use does not give you "ensure". Zeroing memory is purely for decreasing the risk, and cannot eliminate it.

If the process dies with a signal that causes a core dump, it's too late for the process itself to do anything about the memory contents. The purpose of zeroing secrets after use is to minimize the window in which the secrets might leak into a core file, but it can't prevent that.

As I said before, for any absolute guarantees, you probably want memfd_secret (https://lwn.net/Articles/865256/).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted
Projects
Status: Accepted
Development

No branches or pull requests