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

x/sys/unix: Capget/Capset writes past struct's memory #44312

Open
gfuchedzhy opened this issue Feb 16, 2021 · 7 comments
Open

x/sys/unix: Capget/Capset writes past struct's memory #44312

gfuchedzhy opened this issue Feb 16, 2021 · 7 comments

Comments

@gfuchedzhy
Copy link

@gfuchedzhy gfuchedzhy commented Feb 16, 2021

I have a go program that is using unix.Capget/Capset and was randomly crashing.
Thanks to @ianlancetaylor this code was identified as a culprit:

hdr := unix.CapUserHeader{Version: unix.LINUX_CAPABILITY_VERSION_3}
var data unix.CapUserData
unix.Capget(&hdr, &data) // this might write past &data.

Turns out that version 2 and version 3 linux capabilities are wider than version 1 capabilities.
Version 1 uses 32bit fields and fit perfectly into a single unix.CapUserData:

type CapUserData struct {
	Effective   uint32
	Permitted   uint32
	Inheritable uint32
}

However version 2 and version 3 capabilities are 64 bit and do not fit into a single unix.CapUserData.
Instead of using a struct with wider fields however linux capget syscall uses the same struct but writes into 2 instances of the struct. It writes lower bits of the capabilities into the first struct and the higher bits into the second one.

So, confusingly the correct way of writing the code above would be:

hdr := unix.CapUserHeader{Version: unix.LINUX_CAPABILITY_VERSION_3}
var data [2]unix.CapUserData
unix.Capget(&hdr, &data[0])

Which is normal practice in C, but doesn't look like a typical go to me.
This code would write lower bits into data[0] and higher bits into data[1]. Then the client would have to be careful when setting capabilities, choosing data[0] vs data[1] depending on the capability, for example:

data[0].Effective |= 1<<unix.CAP_SYS_ADMIN
data[1].Effective |= 1<<(unix.CAP_CHECKPOINT_RESTORE - 32)

Same applies to unix.Capset, except it would read past the first struct.

I'm wondering if the signature of unix.Capget/Capset should be changed into something like:

func Capget(hdr *CapUserHeader, data *[2]CapUserData) (err error)

to prevent issues like this. Such a signature would clearly indicate the amount of memory affected by the syscall.
Or maybe make the fields wider and repack during the call to account for a weird memory layout? That wouldn't work very well though if linux keeps expanding capabilities and would go even wider.
Thoughts?

@gopherbot gopherbot added this to the Unreleased milestone Feb 16, 2021
@AndrewGMorgan
Copy link
Contributor

@AndrewGMorgan AndrewGMorgan commented Feb 22, 2021

Have you considered using the cap package instead?

@gfuchedzhy
Copy link
Author

@gfuchedzhy gfuchedzhy commented Feb 22, 2021

Hey Andrew, thanks for the suggestion!
That looks like an awesome library, however after a bit of reading I realized that it modifies capabilities for the whole process. I thought it would also let me change capabilities of a single os thread, but it looks like it's not possible (or I might be missing something).

In my use case I'm doing the following:

go func() { 
  runtime.LockOSThread() // exclusively bind the goroutine to a single os thread.
  unix.Capget/Capset
  unix.Setns // change netns of the os thread
  exec.Start/Wait/Run/etc
  // exit goroutine without unlocking to kill the os thread
} () 
@AndrewGMorgan
Copy link
Contributor

@AndrewGMorgan AndrewGMorgan commented Feb 22, 2021

Take a look at the Launch() stuff. There is an example I've been playing with here: https://git.kernel.org/pub/scm/libs/libcap/libcap.git/tree/goapps/gowns/gowns.go

@gfuchedzhy
Copy link
Author

@gfuchedzhy gfuchedzhy commented Feb 22, 2021

Thanks for the pointer!
Wouldn't work for me though, since I need to raise capabilities first, then do setns. The callback seems to be called before capset (which makes total sense for the use case of dropping capabilities). Also ideally I would like to use a standard exec package to run programs in a net namespace (to handle stdin/stdout the standard way).

Are there any plans to expose cap API with cap.singlesc syscaller underneath? Or alternatively something similar to Launch, but with a callback only. Then I could exec.Start/Wait/etc in the callback instead of relying on ForkExec in the cap package.
Thanks!

@AndrewGMorgan
Copy link
Contributor

@AndrewGMorgan AndrewGMorgan commented Feb 23, 2021

You should feel free to make such a feature request. The cap.Launch() and callback mechanism are very much open for development.

[There seems to be a lot of confusion about the importance of POSIX semantics for privilege mechanisms in general though. I wrote this to capture how trivial it is for unprivileged threads to trick the other threads into doing privileged things, for C/C++ but it applies to Go/CGo too.]

@AndrewGMorgan
Copy link
Contributor

@AndrewGMorgan AndrewGMorgan commented Feb 24, 2021

Also, I forgot to mention, the Launch() stuff invokes the callback with the prevailing process capabilities, so if your callback needs something raised in pE, you can raise them prior to calling Launch(). I'd be happy to work through any issues you find.

@gfuchedzhy
Copy link
Author

@gfuchedzhy gfuchedzhy commented Feb 24, 2021

You should feel free to make such a feature request

Thanks! done: https://bugzilla.kernel.org/show_bug.cgi?id=211919

you can raise them prior to calling Launch()

But then I'm back to square one using naked syscalls instead of nice libcap API.

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

Successfully merging a pull request may close this issue.

None yet
4 participants