Removes drop based finalizer (#157)

This PR addresses an issue in memguard where the go garbage collector
(GC) will trigger the finalizer on a LockedBuffer, zeroing out and
freeing this buffer while the running code may still have a pointer to
this buffer. This can result in the code being run against partially or
fully zero'd out memory.

This issue was originally privately disclosed to memguard and it was
agreed that due to the minor security impact, a public PR should be
opened to track one possible solution to this problem.
 

The Fix
====

We fix this issue by removing the finalizer from the LockedBuffer. To
wipe the LockedBuffer and free the associated memory the developer must
call `Destroy()`. This is a reasonable fix because memguard provides
additional security at the cost of requiring the developer managing this
memory. The finalizer on the LockedBuffer mixes the approach of placing
the responsibility on the developer with sometimes having the go GC also
handle this responsibility.

This fix simplifies this so that the developer is always responsible for
calling Destroy() on their LockedBuffer.

The Issue
====

Consider the following code:
```go
  dataToLock := []byte(`abcdefghijklmnopqrstuvwxyz`)
  lb := memguard.NewBufferFromBytes(dataToLock)
  lbBytes := lb.Bytes()
  for i := 1; i < 30000; i++ {
      fmt.Printf("i=%d, len(lbBytes)=%d, lbBytes=%d\n", i, len(lbBytes), lbBytes)
      if lbBytes[0] != byte(97) {
          fmt.Printf("error: i=%d, len(lbBytes)=%d, lbBytes=%d\n", i, len(lbBytes), lbBytes)
      }
   }
```

At some point in the for loop lbBytes will eventually no longer equal
'a' (97) but will have its value changed to 0x0. Soon afterwards
attempts to write to lbBytes will result in a fault.

i=9837, len(lbBytes)=26, lbBytes=[97 98 99 100 101 102 103 104 105 106
107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122]
unexpected fault address 0x29af8971fe6
fatal error: fault
[signal 0xc0000005 code=0x0 addr=0x29af8971fe6 pc=0x28fb0e]

Cause
====

When memguard creates a LockedBuffer, it associates a 16 byte value
named drop with the LockedBuffer. This drop value acts as a standin for
the data in the LockedBuffer so that when the go garbage collector (GC)
detects there are zero references to the drop it calls the custom
finalizer on the drop value and then the finalizer called Destroy on the
LockedBuffer which in turn deletes the data in the LockedBuffer and then
frees the page.

https://github.com/awnumar/memguard/blob/master/buffer.go#L23C1-L35C2

In the above code, once you enter the for loop, the LockedBuffer is out
of a scope and this means the drop is out of scope as well. When the
Garbage Collector does a sweep, it will call the custom finalizer on the
drop, which will in turn delete the buffer, this will zero out lbBytes
and then deallocate the memory for lbBytes.

Since the timing of this depends on the GC (garbage collector) this can
result in:
(a). no error (most of the time) :
(b). a memory fault error (sometimes) ,
(c). the buffer being fully or partially overwritten with zeros
(rarely).

(a) and (b) are likely not at all security critical, but the randomness
and rareness of this occurring makes it a tricky problem to debug. It is
case (c) that could represent a security issue.

(c) could be understood as working as intended since by zeroing out the
buffer when the LockedBuffer goes out of scope memguard achieves the
feature: "Accidental memory leaks are mitigated against by harnessing
the garbage-collector to automatically destroy containers that have
become unreachable". However it can have a limited security impact (see
below).

Security impact
====

If memguard LockedBuffers are used for keying material then encrypting a
message with a secret key which is partially or fully changed to zero
may result in a loss of security. It may not be clear to an implementer
the security difference between:

```go
// This is insecure, very rarely you might get an all zero key
key := lb.Bytes()
ct := Encrypt(key, msg)
```
```go
// This is secure
ct := Encrypt(lb.Bytes(), msg)
```

I consider the security impact here to be minimal because:
1. Most projects create a LockedBuffer that lives in a struct that
exists for the lifetime of the process so GC is never called until the
process ends. Looking at all the projects that use memguard on github I
was unable to find a single one that this behavior would impact.
2. If they are working with ephemeral secrets then they should be seting
`defer lb.Destroy()` this prevents the GC from eating the buffer. The
defer keeps a reference to lb and GC isn't called until it leaves the
function. I experimentally tested this.
3. If they are not working with ephemeral secrets, but instead using the
LockedBuffer to manage secrets needed through out the lifetime of the
process, then the LockedBuffer shouldn't be garbage collected at all.

buffer.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"io"
 	"os"
-	"runtime"
 	"unsafe"
 
 	"github.com/awnumar/memguard/core"
@@ -17,26 +16,16 @@ The number of LockedBuffers that you are able to create is limited by how much m
 */
 type LockedBuffer struct {
 	*core.Buffer
-	*drop
 }
 
-/*
-Value monitored by a finalizer so that we can clean up LockedBuffers that have gone out of scope.
-*/
-type drop [16]byte
-
 // Constructs a LockedBuffer object from a core.Buffer while also setting up the finalizer for it.
 func newBuffer(buf *core.Buffer) *LockedBuffer {
-	b := &LockedBuffer{buf, new(drop)}
-	runtime.SetFinalizer(b.drop, func(_ *drop) {
-		go buf.Destroy()
-	})
-	return b
+	return &LockedBuffer{buf}
 }
 
 // Constructs a quasi-destroyed LockedBuffer with size zero.
 func newNullBuffer() *LockedBuffer {
-	return &LockedBuffer{new(core.Buffer), new(drop)}
+	return &LockedBuffer{new(core.Buffer)}
 }
 
 /*

buffer_test.go

@@ -12,19 +12,26 @@ import (
 	"unsafe"
 )
 
-func TestFinalizer(t *testing.T) {
-	b := NewBuffer(32)
-	ib := b.Buffer
+func TestPtrSafetyWithGC(t *testing.T) {
+	dataToLock := []byte(`abcdefgh`)
+	b := NewBufferFromBytes(dataToLock)
+	dataPtr := b.Bytes()
 
-	runtime.KeepAlive(b)
+	finalizerCalledChan := make(chan bool)
+	runtime.SetFinalizer(b, func(_ *LockedBuffer) {
+		finalizerCalledChan <- true
+	})
 	// b is now unreachable
 
 	runtime.GC()
-	for {
-		if !ib.Alive() {
-			break
-		}
-		runtime.Gosched() // should collect b
+	finalizerCalled := <-finalizerCalledChan
+	if finalizerCalled == false {
+		t.Error("this should never occur")
+	}
+
+	// Check that data hasn't been garbage collected
+	if !bytes.Equal(dataPtr, []byte(`abcdefgh`)) {
+		t.Error("data does not have the value we set")
 	}
 }