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

crypto: facility for key erasure #21865

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

Comments

@zx2c4
Copy link
Contributor

commented Sep 13, 2017

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

This comment has been minimized.

Copy link
Member

commented Sep 13, 2017

Related to #21374

@anitgandhi

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

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

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

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

This comment has been minimized.

Copy link

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 this to the Unplanned milestone Mar 26, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Feb 14, 2019

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
crypto/rc4: remove false guarantees from Reset 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>
@gopherbot

This comment has been minimized.

Copy link

commented Feb 22, 2019

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
[release-branch.go1.12] crypto/rc4: remove false guarantees from Rese…
…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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

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

@yahesh

This comment has been minimized.

Copy link

commented May 20, 2019

@randall77

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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.

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

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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.

@randall77

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

It might be the case that you'd like to redirect allocations of an entire module to some totally separate allocator or something wild.

Maybe not an entire module, but the secret parts of it, yes.

I see SetZeroOnGC as only half a solution. Maybe it would help with simple security improvements, but anything really hardened will need a special allocator. There's no evolutionary path between the two (i.e. the implementation of SetZeroOnGC can't be upgraded to change where the allocation happens, because by the time you get to the call site, it is too late).

And we can do the simple security improvements that SetZeroOnGC can do today, with SetFinalizer.

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

No, SetFinalizer is not sufficient, because of internal copies.

@randall77

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

What internal copies?

@FiloSottile

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Over time I grew more and more skeptical about this.

Even if we went and tagged every single GC allocation that holds secrets or derivatives in all packages (which is a lot of pollution), the heap is only half the story. What about values on the stack? What about every single variable in a field implementation? Moreover, the boundary between stack and heap allocation is not specified in Go, so you'd have to tag everything to be safe.

That quickly devolves into a lot of complexity, for elusive gains, as you need all your dependencies to strictly apply tagging.

Are there no studies about this? I would want to see proof that languages which support this actually succeeded in developing an ecosystem that delivers actual security properties, before we embark on it. All I know about is from back when Heartbleed happened, and we found out that no, secrets were still all over memory.

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Indeed it's tough. For the kernel, I'm working on a gcc plugin that will zero out the stack frame of a function down to the depth of its deepest leaf, upon return from the marked top level parent.

But even without something like that on Go, I suspect most of the stack for my use cases will be probably clean of most intermediate values within a matter of minutes. Hours? Some interval? Importantly, not forever, or overly long.

@tv42

This comment has been minimized.

Copy link

commented May 20, 2019

@randall77 I believe what was meant was internal copies that Go GC might in the future do. The internal copies the GC of another implementation of might have. There's never been a promise that the GC is not a moving GC. It'd be poor form to standardize an API that can't cope with that.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Presumably you know at allocation time that the value will hold something that should be scrubbed from memory. I think the only approach that is likely to work with the Go runtime is to use a separate allocator for those values, likely based directly on syscall.Mmap, and to implement the desired semantics directly. I think that writing a package that supports that would be more useful than trying to push the desired semantics into the Go runtime.

@awnumar

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@ianlancetaylor I think you're right but this leaves us with a choice:

  1. Rewrite libraries to use whatever custom data structure we end up with.
  2. Overload allocators in packages in some way. This way would make things easier but less flexible, as the allocators would need to return fixed data types or pointers since the packages have no "awareness" of the capabilities of the container objects.

Option one doesn't need the runtime to help it but option two probably would as I'm not sure if this can be accomplished with pure Go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.