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: enable address sanitizer in Go #44853

Open
zhangfannie opened this issue Mar 8, 2021 · 33 comments
Open

proposal: enable address sanitizer in Go #44853

zhangfannie opened this issue Mar 8, 2021 · 33 comments
Labels
Projects
Milestone

Comments

@zhangfannie
Copy link
Contributor

@zhangfannie zhangfannie commented Mar 8, 2021

The address sanitizer (Asan) is a memory error detector, it finds a number of memory access errors that can occur in C, such as out-of-bounds accesses to heap, stack and global objects, and use-after-free. In general, these issues do not occur in the pure Go code, unless people intentionally use the unsafe package. However, people can use cgo to pass memory back and forth between C and Go, which will cause memory access problems.

Please refer to the following cases.
In this case (case-1) https://play.golang.org/p/eE_8k4poelj, C passes Go an invalid pointer, causing the Go code to have use-after-free errors. In this case (case-2) https://play.golang.org/p/wtHjV-4xuiL, Go passes C a pointer, the C code has out-of-bounds access to heap objects errors. In this case (case-3) https://play.golang.org/p/rOyEo7I7vTY, Go passes C a pointer, the C code has out-of-bounds access to global objects errors.

Integrating Asan with Go would detect the above memory errors. So it is useful. In addition, Asan can help to detect some memory errors that MSan cannot. For example, use-after-free memory error case https://play.golang.org/p/eE_8k4poelj, MSan cannot detect writes on uninitialized memory, while ASan can.

And we can instrument ASan in exactly the same way as MSan. As with the -msan option, if we do not run with -asan option, it has no effect on code execution, if you use it, it may find some memory errors.

Although the above cases are not real world cases. However, on the other hand, when debugging a memory error access in a Go program that calls C code, it is really good to be able to use Asan to ensure that code is correct. Obviously, this will require more code to be maintained in the toolchain, but since almost all of the code is next to the existing MSan code, this is quite limited.

Besides, Arm has a hardware feature MTE (Memory Tagging Extension), it aims to further mitigate these memory safety bugs by enabling us to detect them with low overhead. If ASan will be proven to be useful, we may consider enabling MTE for ASan. In this way, ASan even be enabled in the production environment to help to detect memory errors with low overhead. I saw a blog that introduces enable MTE in Android to detect the most common classes of memory safety bugs. Please refer to the document https://security.googleblog.com/2019/08/adopting-arm-memory-tagging-extension.html.

Thank you.

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Mar 8, 2021

I have updated the CLs for address sanitizer support, The implementation is divided into some smaller CLs 298610, 298611, 298612, 298613, 298614 for review. And with the current implementation, the -asan option can check for error memory access to heap objects. Thank you.

@oiooj oiooj changed the title proposal:enable address sanitizer in Go proposal: enable address sanitizer in Go Mar 8, 2021
@gopherbot gopherbot added this to the Proposal milestone Mar 8, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Mar 8, 2021
@AZ-X
Copy link

@AZ-X AZ-X commented Mar 13, 2021

I care about size of binary. Can you compare both size including small samples(<5M) and big samples(>10M)?

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Mar 15, 2021

@AZ-X If you run the go program without the -asan option, the size of binary will not change. Generally, we only enable the address sanitizer in the test environment to help detect memory access issues, so I think the size of binary is not important. Thank you.

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Mar 19, 2021

@AZ-X Regarding your question, my answer above seems too one-sided. I wrote a test case to compare the size of binary, and the size of binary with -asan is almost 10% larger than that without -asan. Please refer to the following results.

func main() {
        var overall [][]int
        var m runtime.MemStats
        s := 2*1024*1024                 //  >10M big samples
        //s := 10*1024                     // <1M small samples
        for i := 0; i < 10; i++ {
                a := make([]int, 0, 10*s)
                overall = append(overall, a)
        }
        runtime.ReadMemStats(&m)
        fmt.Printf("Alloc = %v MB", bToMb(m.Alloc))
}
func bToMb(b uint64) uint64 {
        return b / 1024 / 1024
}

// the binay size:

  size           binary
2154616   big_asan               // build with -asan and big samples
1950161   big_noasan           // build without -asan and big samples  
2154624   small_asan            // build with -asan and small samples
1950161   small_noasan        // build without -asan and small samples
1940941   big_main              // build with master go and big samples
1940949   small_main           // build with master go and small samples

From the above result, the size of binary is bigger. But as I mentioned above, we don't recommend opening the asan option in a production environment. The factors to consider are not only size, but also performance. The good news is that ARM64 has a new feature of MTE, which is designed to detect error memory accesses with lower overhead. If ASan with MTE is enabled in Go in the future, the impact on performance and the size of the binary will be relatively small. Thank you.

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Mar 19, 2021

If ASan with MTE is enabled in Go in the future

Do we need any support for this at all?
I would assume this will be done by just exporting some env var to enable MTE in the system malloc, e.g.:
https://sourceware.org/pipermail/libc-alpha/attachments/20200615/c8ef9e4e/attachment-0001.bin
https://patchwork.ozlabs.org/project/glibc/patch/8306e032-f980-a409-5239-74629e79d041@arm.com/
Or maybe in future it will be the default, so one doesn't need to do anything at all.

Though it can make sense to support MTE in Go runtime:
https://groups.google.com/g/golang-dev/c/ACKO07YeeW8/m/DX61CERlAwAJ

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Mar 19, 2021

Or maybe in future it will be the default, so one doesn't need to do anything at all.

In fact, we need to do something. Because Go has its own memory allocator, see https://golang.org/src/runtime/malloc.go, we may follow the implementaion in glic, refactor Go memory allcator (regarding the implementation details, I haven't done in-depth research. 🙂), setup memory tagging support if the hardware support it and if the user has requested it. Thank you.

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Mar 19, 2021

Yes, that's what I mean by "support MTE in Go runtime". But we don't need anything related to asan (linking asan, asan hooks, etc)

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Mar 19, 2021

Yes, that's what I mean by "support MTE in Go runtime". But we don't need anything related to asan (linking asan, asan hooks, etc)

Yes, you are right. With MTE, all load and store instructions, except for with SP base register and immediate offset, do check tags. Here https://github.com/google/sanitizers/wiki/Stack-instrumentation-with-ARM-Memory-Tagging-Extension-(MTE) introduces how MTE works for stack. Thank you.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2021

Change https://golang.org/cl/298610 mentions this issue: cmd/link: add -asan option

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2021

Change https://golang.org/cl/298611 mentions this issue: cmd/compile: add -asan option

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2021

Change https://golang.org/cl/298612 mentions this issue: cmd/go: add -asan option

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2021

Change https://golang.org/cl/298613 mentions this issue: runtime, runtime/asan: add asan runtime support

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2021

Change https://golang.org/cl/298614 mentions this issue: runtime, syscall: add calls to asan functions

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2021

Change https://golang.org/cl/298615 mentions this issue: cmd/dist: add asan tests in misc/cgo/testsanitizers package

@AZ-X
Copy link

@AZ-X AZ-X commented May 12, 2021

any updates?

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented May 13, 2021

any updates?

It has entered the code freeze for Go 1.17. 😟

@ianlancetaylor Any plans for it in 1.18? Thank you.

@andreybokhanko
Copy link
Contributor

@andreybokhanko andreybokhanko commented May 14, 2021

@AZ-X Regarding your question, my answer above seems too one-sided. I wrote a test case to compare the size of binary, and the size of binary with -asan is almost 10% larger than that without -asan. Please refer to the following results.

func main() {
        var overall [][]int
        var m runtime.MemStats
        s := 2*1024*1024                 //  >10M big samples
        //s := 10*1024                     // <1M small samples
        for i := 0; i < 10; i++ {
                a := make([]int, 0, 10*s)
                overall = append(overall, a)
        }
        runtime.ReadMemStats(&m)
        fmt.Printf("Alloc = %v MB", bToMb(m.Alloc))
}
func bToMb(b uint64) uint64 {
        return b / 1024 / 1024
}

// the binay size:

  size           binary
2154616   big_asan               // build with -asan and big samples
1950161   big_noasan           // build without -asan and big samples  
2154624   small_asan            // build with -asan and small samples
1950161   small_noasan        // build without -asan and small samples
1940941   big_main              // build with master go and big samples
1940949   small_main           // build with master go and small samples

From the above result, the size of binary is bigger. But as I mentioned above, we don't recommend opening the asan option in a production environment. The factors to consider are not only size, but also performance. The good news is that ARM64 has a new feature of MTE, which is designed to detect error memory accesses with lower overhead. If ASan with MTE is enabled in Go in the future, the impact on performance and the size of the binary will be relatively small. Thank you.

I assume all these measurements are done on ARM, right? What about x86?

Also, do you plan to implement first-class ASan support on x86-64? (Obviously, without MTE, as it's not available on x86)

Yours,
Andrey
===
Advanced Software Technology Lab
Huawei

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented May 17, 2021

I assume all these measurements are done on ARM, right? What about x86?

The size of binary with -asan is almost 11% larger than that without -asan. Please refer to the following results.

The binary size on x86-64.

      size                 binary
    1998048          big_asan
    1785270          big_noasan
    1785246          big_main
    1998048         small_asan
    1785270         small_noasan
    1785246         small_main

Also, do you plan to implement first-class ASan support on x86-64? (Obviously, without MTE, as it's not available on x86)

The current implementation also enables the ASan support on x86-64. Thank you.

@gopherbot
Copy link

@gopherbot gopherbot commented May 24, 2021

Change https://golang.org/cl/321715 mentions this issue: cmd/compile: enable Asan check for global variables

@gopherbot
Copy link

@gopherbot gopherbot commented May 24, 2021

Change https://golang.org/cl/321716 mentions this issue: cmd/dist: add asan tests for global objects in testsanitizers package

@ph1048
Copy link

@ph1048 ph1048 commented May 24, 2021

Hello!

What are your plans on supporting ASan for stack-based objects? The problem is that usage of unsafe.Pointer does not affect escape analysis, and there are lots of cases with potentially insecure dereference of unsafe.Pointer to stack objects. AFAIK go does not generate any stack canary code, so those vulnerabilities should be easily exploitable.
Simple example:
https://play.golang.org/p/5BLnDJtleF4
Here, due to misuse of unsafe.Pointer we have easy IP control.

Also, if you add MTE support, which runtime are you going to use, and will it be part of Go Asan implementation, or another feature?

Best wishes, Alex

@thepudds
Copy link

@thepudds thepudds commented May 24, 2021

For reference, there was some discussion of some different aspects of this proposal in this golang-dev thread:

https://groups.google.com/g/golang-dev/c/ACKO07YeeW8

@ph1048, FYI, there was some discussion there around stack objects, though perhaps @zhangfannie will have more to say here.

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented May 27, 2021

Also, if you add MTE support, which runtime are you going to use, and will it be part of Go Asan implementation, or another feature?

@ph1048 Add MTE support is another feature, as @dvyukov commented above, we do not need to call any compiler-rt address sanitizer runtime functions. Thank you.

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Jun 1, 2021

With the implementation of CL 321715 , ASan in Go can detect the error memory access to heap objects and global objects. For stack objects, as we discussed in the golang-dev mailing list https://groups.google.com/g/golang-dev/c/ACKO07YeeW8/m/B93kNorLBgAJ, we currently have no plans to add support for it. In this way, support ASan in Go is complete.

Welcome to review these patches. Thank you.

@andreybokhanko
Copy link
Contributor

@andreybokhanko andreybokhanko commented Jun 1, 2021

Hi @zhangfannie

With all respect, I find these two statements:

For stack objects, as we discussed in the golang-dev mailing list https://groups.google.com/g/golang-dev/c/ACKO07YeeW8/m/B93kNorLBgAJ, we currently have no plans to add support for it.

In this way, support ASan in Go is complete.

to be mutually contradictory.

Without stack objects being supported, ASan for Go is definitely not "complete". "ASan for heap and global objects" can be called as complete, sure -- but I still maintain that users shouldn't care on where their objects are allocated, especially giving that escape analysis can move objects from heap to stack easily.

As for plans to add support for stack objects, here is what @dvyukov wrote in the mailing list:

Either way, I would consider stack only after heap is implemented and
deployed. There is little point in planning/deciding re stack before
that.

I read this as "let's do heap objects first, then decide on stack objects support".

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Jun 2, 2021

@andreybokhanko

Thank you for correct my expression and "ASan for heap and global objects is commplete" is more accurate.

As for plans to add support for stack objects, here is what I wrote in the mailing list:

To support for heap objects, we change the go memeory allocator and insert redzone around the heap objects. Support for stack objects also need to change the frame layout and insert redzone around the stack objects, this refactor may cause some problems.

One problem is the compatibility of abi. Go passes a value, it is also equal to pass the memory, we can use unsafe.Pointer to access the parameter adress and return adress. So far, I am not very clear about the definition of Go calling convention. I am not sure it is passing by value, or passing a memory region shared by caller and callee.

For example, in the following case, if ASan in Go wants to detect this error memory acces to stack objects, we need to insert redzone around the pass variables(a, b) and return variable (ret), which will break the current abi.

package main

import "fmt"
import "unsafe"

//go:noinline
func add(x int, y int) (ret int) {
	*(* int)(unsafe.Pointer((uintptr(unsafe.Pointer(&ret)) - 1*unsafe.Sizeof(ret)))) = 123    // Is it an error?
	*(* int)(unsafe.Pointer((uintptr(unsafe.Pointer(&ret)) - 2*unsafe.Sizeof(ret)))) = 543    // Is it an error?

	ret = x + y
	return
}

func main() {
	fmt.Println(add(42, 13))
}

At present, we do not have a good idea to support it. There should be 3 kinds of stack memory need to be considered, including local variables, argument variables and return variables.

If there is no clear calling conversion definition at the moment, maybe this can be a part of the discussion in regabi. @cherrymui

@ph1048
Copy link

@ph1048 ph1048 commented Jun 2, 2021

Hello!

I reviewed your design, and this is a great solution. So, I would like to make some suggestions.

First, your approach of unpoisoning on the mallocgc side may not cover some cases. If we modify your example in a way that "growslice" code is generated, it does not produce ASan output for me: https://play.golang.org/p/PgoKHVZzSYd
The problem is because of built-in object constructors allocate more memory than real size of data: e.g. slices allocate memory by their capacity, not length; in other cases allocation size is just being rounded. Here I suggest moving poisoning handling inside the object constructors, and take into account real data length.

Second, typical ASan report is not very convenient to read, and it does produce very brief error information, with limited stack trace information. As you implement poison checking by yourself, there is may be a point to implement pretty-printing of ASan error with Go stacktrace, similar to the one generated by panic/throw.

Best wishes,
Alexander

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Jun 4, 2021

@ph1048 Thank you for the review and suggestion.

First, your approach of unpoisoning on the mallocgc side may not cover some cases. If we modify your example in a way that "growslice" code is generated, it does not produce ASan output for me: https://play.golang.org/p/PgoKHVZzSYd

First, we need to discuss whether the access in your case is an out-of-bounds access error.

For example, in the following case, the slice variables sl1 and sl2 both point to the same underlying array as the slice sa. Because slicing does not copy the slice's data, it just creates a new slice value that points to the original array. Only when the index is greater than the capacity of the slice instead of length, it will cause a runtime panic.

Therefore, the access in your case is valid.

package main

/*
#include <stdlib.h>
#include <stdio.h>

void test(int *a) {
        int c = a[5];           // the access is valid.
        printf("c=%d\n", c);
}
*/
import "C"

func main() {
        sa := []C.int{1, 2, 3, 4}
        sa = append(sa, 5) 

        var sl1 []C.int
        sl1 = sa[4:7] 
        sl1[1] = 6          // &sl1[1] = &sa[5],
        C.test(&sa[0])
}

there is may be a point to implement pretty-printing of ASan error with Go stacktrace, similar to the one generated by panic/throw.

This is a good suggestion. I will spend some time looking into whether if it can be implemented.

Thank you.
Fannie

@ph1048
Copy link

@ph1048 ph1048 commented Jun 4, 2021

@zhangfannie Thanks for your explanation. I agree that slicing, as documented method of reusing underlying memory, refers the capacity of the slice, so the access is valid in your example. Here we create Go object that permits to access this memory region.
But, in previous case (https://play.golang.org/p/PgoKHVZzSYd) we did not create a slice from the original slice: that memory is not accessible in terms of Go. So, do you mean that the access is still valid, despite indexing operator would have panicked on the cIntSlice[5]?

Best wishes,
Alexander

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Jun 7, 2021

@ph1048

But, in previous case (https://play.golang.org/p/PgoKHVZzSYd) we did not create a slice from the original slice: that memory is not accessible in terms of Go. So, do you mean that the access is still valid,

Yes, the access is valid. The poisoning handling should consider the capacity of the slice. If we take into account real data length (equl to 5 in your case), the cIntSlice[5] is unaddressable, then slicing cIntSlice[5:6] will fail, which is incorrect behavior.

Thank you.
Fannie

@ph1048
Copy link

@ph1048 ph1048 commented Jun 7, 2021

@zhangfannie sorry, but I do not fully understand why it is valid. I suggest not to mix taking the index and slicing operators. Slicing operation is totally valid in this case, but indexing is seems to be not valid (as Go issues panic). In CGO we are by design unable to use Go slicing, as it is supposed to create another Go slice.

If we take into account real data length (equl to 5 in your case), the cIntSlice[5] is unaddressable, then slicing cIntSlice[5:6] will fail, which is incorrect behavior.

Slicing like this: "cIntSlice[5:6]", by itself, does not imply reading of this range, and this is the point where we are able to make this region unpoisoned, as go entity is being created.

Moreover, the operation of slicing by capacity is a less common case than growing go slices. Therefore, in most cases, we'll be able to find more bugs/vulnerabilities if we disallow unsafe reading of data behind slice len().

Best wishes,
Alexander

@zhangfannie
Copy link
Contributor Author

@zhangfannie zhangfannie commented Jun 8, 2021

@ph1048 Your explanation is also reasonable. What I said above that this accss is valid may be inaccurate. What I want to say is that it is difficult for the current address sanitizer to mark the same memory region as different states. For ASan, a memory region is either allocated or unallocated.

In the following case, in Go code, as you said, indexing sa[4] is out of index, which will cause Go runtime panic "index out of range [4] with length 4", but indexing arr[4] is valid. In C code, the access to arr[4] is valid, the &sa[4] points to the same memory as &arr[4], for address sanitizer, the &arr[4] and the &sa[4] are in the same memory state, both are addressable, so the access of sa[4] is also valid.

package main

/*
#include <stdlib.h>

void test(int *arr, int *sa) {
        int c = arr[4];           // the access is valid.
        int d = sa[4];            // if the access is valid?
        printf("c=%d, d=%d\n", c, d);
}
*/
import "C"
import "fmt"

func main() {
        arr := [7]C.int{1, 2, 3, 4, 5, 6, 7}
        sa := []C.int(arr[:4])

        fmt.Println(arr[4]) // index is not out of range
        fmt.Println(sa[4])  // index out of range

        C.test(&arr[0], &sa[0])
}

Thank you,
Fannie

@ph1048
Copy link

@ph1048 ph1048 commented Jun 9, 2021

@zhangfannie I understand what you mean, thanks for the explanation. But anyway, I need to point out, that in common case of growing slices (when we do not reslice them again, like in your example), we have a trail of unknown size after buffer (after len(), before cap()). So, with current implementation, we will catch overflows only if they touch redzones, i.e. with cap(slice)==len(slice). I wrote simple test code that outputs probability of this case, and it is not very high (https://play.golang.org/p/hGvir8XNAPI).
Even though you are totally right about validity of C code accessing memory behind the slice, in common case it would just mean not detecting a small overflow (as we can't predict how large is memory region after len()).

Best wishes,
Alexander

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

Successfully merging a pull request may close this issue.

None yet
7 participants