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
ghost opened this issue Feb 16, 2021 · 7 comments
Open

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

ghost opened this issue Feb 16, 2021 · 7 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ghost
Copy link

ghost 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
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 17, 2021
@AndrewGMorgan
Copy link
Contributor

Have you considered using the cap package instead?

@ghost
Copy link
Author

ghost 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

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

@ghost
Copy link
Author

ghost 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

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

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.

@ghost
Copy link
Author

ghost 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.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: Triage Backlog
Development

No branches or pull requests

3 participants