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

Use after free in api/memory.go::receiveSlice #55

Closed
reuvenpo opened this issue Mar 18, 2020 · 3 comments
Closed

Use after free in api/memory.go::receiveSlice #55

reuvenpo opened this issue Mar 18, 2020 · 3 comments

Comments

@reuvenpo
Copy link
Contributor

reuvenpo commented Mar 18, 2020

In the receiveSlice method under api/memory.go the call to free_rust, followed by returning the slice at res immediately triggers a use-after-free in cGet and cSet.
Note that GoByes does not copy the slice contents

EDIT: I was wrong, GoBytes does perform a copy. The rest of this issue is still valid though.

This is a more fundamental issue than just removing the call to the free_rust function. The Buffer type which is used here probably represents a wrong approach to passing buffers across FFI between Rust and Go.

I personally don't know the capabilities of the Go runtime and type system that well, so I think that the best approach would be to pass equivalents to Rust's &[u8] type across the FFI boundaries, and copy the contents of the slices when receiving them on each side. Since Rust has a richer type system, we can probably also wrap go byte arrays in a type similar to Vec<u8> which implements AsRef<[u8]> and calls a go destructor across FFI on drop (but i don't know how to do that off the top of my head in Go).

Implementing this correctly will allow returning arbitrarily long buffers back in impl ReadonlyStorage for DB, impl Storage for DB, and impl Api for GoApi

P.S. the Buffer::read method should be marked unsafe, even today, for the same reason that consume is.

@reuvenpo
Copy link
Contributor Author

I checked with a friend that has more experience in Golang. Apparently i can't read, but the documentation linked above is quite vague. The GoBytes method DOES copy the contents of the slice. The linked paragraph mentions this method, and the explains how one would create a bytes object without copying it, using a different method.

@reuvenpo
Copy link
Contributor Author

This means that if the UAF is there, it's more subtle than the one i described, but the rest of my points still apply.

@reuvenpo
Copy link
Contributor Author

The points brought up in this Issue have been either addressed in PR #67 or better thought out in this comment. @ethanfrey @webmaster128 I think you can close this issue.

faddat pushed a commit to faddat/go-cosmwasm that referenced this issue Dec 10, 2021
faddat pushed a commit to faddat/go-cosmwasm that referenced this issue Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant