awnumar / memguard

Secure software enclave for storage of sensitive information in memory.
Apache License 2.0
2.49k stars 124 forks source link

LockedBuffers cause reflect.DeepEqual() to Segfault #129

Closed theory closed 4 years ago

theory commented 4 years ago

Describe the bug

I get a segfault when I try to compare two LockedBuffers with reflect.DeepEqual

To Reproduce

Run this:

package main

import (
    "reflect"

    "github.com/awnumar/memguard"
)

func main() {
    a := memguard.NewBufferFromBytes([]byte{1, 2})
    b := memguard.NewBufferFromBytes([]byte{1, 2})
    reflect.DeepEqual(a, b)
}

Output:

unexpected fault address 0x12bf000
fatal error: fault
[signal SIGBUS: bus error code=0x2 addr=0x12bf000 pc=0x1057ab2]

goroutine 1 [running]:
runtime.throw(0x10fe47d, 0x5)
    /usr/local/go/src/runtime/panic.go:774 +0x72 fp=0xc00006e560 sp=0xc00006e530 pc=0x102a3e2
runtime.sigpanic()
    /usr/local/go/src/runtime/signal_unix.go:391 +0x455 fp=0xc00006e590 sp=0xc00006e560 pc=0x103e855
runtime.memmove(0xc000088080, 0x12bf000, 0x1)
    /usr/local/go/src/runtime/memmove_amd64.s:144 +0x102 fp=0xc00006e598 sp=0xc00006e590 pc=0x1057ab2
runtime.typedmemmove(0x10d34e0, 0xc000088080, 0x12bf000)
    /usr/local/go/src/runtime/mbarrier.go:170 +0x43 fp=0xc00006e5d0 sp=0xc00006e598 pc=0x1011333
reflect.typedmemmove(0x10d34e0, 0xc000088080, 0x12bf000)
    /usr/local/go/src/runtime/mbarrier.go:186 +0x3f fp=0xc00006e5f8 sp=0xc00006e5d0 pc=0x10113ef
reflect.packEface(0x10d34e0, 0x12bf000, 0x1a8, 0x10d3401, 0xc00008807f)
    /usr/local/go/src/reflect/value.go:119 +0x9f fp=0xc00006e638 sp=0xc00006e5f8 pc=0x1078fff
reflect.valueInterface(0x10d34e0, 0x12bf000, 0x1a8, 0x111f400, 0x10d34e0, 0x6f963c56d8445bd0)
    /usr/local/go/src/reflect/value.go:1033 +0xe1 fp=0xc00006e688 sp=0xc00006e638 pc=0x107a5f1
reflect.deepValueEqual(0x10d34e0, 0x12bf000, 0x1a8, 0x10d34e0, 0x12c2000, 0x1a8, 0xc00006edc0, 0x5, 0x1)
    /usr/local/go/src/reflect/deepequal.go:132 +0x80d fp=0xc00006e7a0 sp=0xc00006e688 pc=0x106fb6d
reflect.deepValueEqual(0x10d1a20, 0xc000092248, 0x1b7, 0x10d1a20, 0xc0000922f8, 0x1b7, 0xc00006edc0, 0x4, 0x1)
    /usr/local/go/src/reflect/deepequal.go:84 +0xcb9 fp=0xc00006e8b8 sp=0xc00006e7a0 pc=0x1070019
reflect.deepValueEqual(0x10f2de0, 0xc000092210, 0x199, 0x10f2de0, 0xc0000922c0, 0x199, 0xc00006edc0, 0x3, 0x1000)
    /usr/local/go/src/reflect/deepequal.go:101 +0xa04 fp=0xc00006e9d0 sp=0xc00006e8b8 pc=0x106fd64
reflect.deepValueEqual(0x10f0fe0, 0xc0000623b0, 0x196, 0x10f0fe0, 0xc0000623d0, 0x196, 0xc00006edc0, 0x2, 0x1106630)
    /usr/local/go/src/reflect/deepequal.go:98 +0x1434 fp=0xc00006eae8 sp=0xc00006e9d0 pc=0x1070794
reflect.deepValueEqual(0x10f3560, 0xc0000623b0, 0x199, 0x10f3560, 0xc0000623d0, 0x199, 0xc00006edc0, 0x1, 0xc00006ec80)
    /usr/local/go/src/reflect/deepequal.go:101 +0xa04 fp=0xc00006ec00 sp=0xc00006eae8 pc=0x106fd64
reflect.deepValueEqual(0x10f7d40, 0xc0000623b0, 0x16, 0x10f7d40, 0xc0000623d0, 0x16, 0xc00006edc0, 0x0, 0x1058680)
    /usr/local/go/src/reflect/deepequal.go:98 +0x1434 fp=0xc00006ed18 sp=0xc00006ec00 pc=0x1070794
reflect.DeepEqual(0x10f7d40, 0xc0000623b0, 0x10f7d40, 0xc0000623d0, 0x0)
    /usr/local/go/src/reflect/deepequal.go:196 +0x29f fp=0xc00006ef18 sp=0xc00006ed18 pc=0x1070def
main.main()
    /go/src/try/main.go:12 +0xac fp=0xc00006ef60 sp=0xc00006ef18 pc=0x10bf6ec
runtime.main()
    /usr/local/go/src/runtime/proc.go:203 +0x21e fp=0xc00006efe0 sp=0xc00006ef60 pc=0x102bd6e
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:1357 +0x1 fp=0xc00006efe8 sp=0xc00006efe0 pc=0x1056821

goroutine 19 [sleep]:
runtime.goparkunlock(...)
    /usr/local/go/src/runtime/proc.go:310
time.Sleep(0x1dcd6500)
    /usr/local/go/src/runtime/time.go:105 +0x157
github.com/awnumar/memguard/core.NewCoffer.func1(0xc000064270)
    /go/src/github.com/awnumar/memguard/core/coffer.go:43 +0x2a
created by github.com/awnumar/memguard/core.NewCoffer
    /go/src/github.com/awnumar/memguard/core/coffer.go:40 +0xd6

goroutine 20 [syscall]:
os/signal.signal_recv(0x0)
    /usr/local/go/src/runtime/sigqueue.go:144 +0x96
os/signal.loop()
    /usr/local/go/src/os/signal/signal_unix.go:23 +0x22
created by os/signal.init.0
    /usr/local/go/src/os/signal/signal_unix.go:29 +0x41
exit status 2

Expected behaviour No error.

System (please complete the following information):

awnumar commented 4 years ago

This happens because DeepEqual recursively checks every sub-field of every field of the LockedBuffer struct, exported or not. Some of these refer to guard pages which are marked as NOACCESS and so the process triggers an access violation.

What are you trying to do? Comparing the values contained within them can be done with buffer.EqualTo(buffer2.Bytes()) and checking if two variables point to the same structure is a simple pointer comparison: bufferptr1 == bufferptr2 (where var bufferptri *LockedBuffer).

theory commented 4 years ago

I figured it was something like that. Would it be possible to get it to emit an error message to that effect? The segfault is a bit alarming.

I was updating a project with unit tests using Testify's Equal() function, which ultimately calls memguard.DeepEqual(). The code reads a secret from a config file into a struct with a LockedBuffer field, and I was comparing the structs. I can work around it by comparing the LockedBuffer field with EqualTo(), and think nil out the field to compare the rest of the struct, not a big deal, though a bit fussy.

awnumar commented 4 years ago

The kernel throws the error because the process reads from memory that it doesn't have permission to read from. I believe you can convert crashes to panics and then use recover to stop the crash if you want to, but there's no way to catch segmentation faults globally in Go.