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

Go programs cannot access environment variables #292

Closed
alexg-axis opened this issue Aug 29, 2022 · 18 comments · Fixed by #299
Closed

Go programs cannot access environment variables #292

alexg-axis opened this issue Aug 29, 2022 · 18 comments · Fixed by #299
Labels
bug Something isn't working

Comments

@alexg-axis
Copy link

alexg-axis commented Aug 29, 2022

Bug Description

Go binaries cannot access environment variables.

Steps to Reproduce

// main.go
package main

import (
	"fmt"
	"os"
)

func main() {
	fmt.Println(os.Environ())
}
go build main.go
mirrord exec \
        --pod-namespace some-namespace \
        --pod-name some-pod \
        --override-env-vars-include "*" \
        ./main

Compare with

mirrord exec \
        --pod-namespace some-namespace \
        --pod-name some-pod \
        --override-env-vars-include "*" \
        env

Backtrace

No response

Relevant Logs

No response

Your operating system and version

macOS 12.5.1 Monterey

Local process

main: Mach-O 64-bit executable x86_64
go version go1.19 darwin/amd64

Local process version

No response

Additional Info

No response

@alexg-axis alexg-axis added the bug Something isn't working label Aug 29, 2022
@alexg-axis alexg-axis changed the title Golang does not get environment variables Golang cannot access environment variables Aug 29, 2022
@alexg-axis alexg-axis changed the title Golang cannot access environment variables Go programs cannot access environment variables Aug 29, 2022
@alexg-axis
Copy link
Author

alexg-axis commented Aug 29, 2022

I did some digging. Although there's a "get env syscall" in Go, it seems to just return the values that are expected to be linked immediately after argv, hence env cannot be intercepted with syscall hooks: https://cs.opensource.google/go/go/+/master:src/runtime/runtime1.go;l=82;drc=a6219737e3eb062282e6483a915c395affb30c69;bpv=1;bpt=1

@aviramha
Copy link
Member

aviramha commented Aug 29, 2022

Thanks for reporting this so detailed and informative. We'll look into that and deliver a fix ASAP :)
P.S - Env vars feature doesn't rely on hooks - we just modify the env on init

@aviramha
Copy link
Member

Okay, your source code reference helped a lot. We use libc way of setting environment variables, which doesn't necessarily edit it after argv, like Go expects it to be.
I'll check and see how we can solve it :)

@aviramha
Copy link
Member

Some updates. I dug a bit, and Go is being Go as per usual. On program start (main) it gets the argv/argc from the function argument and then copies it to it's own globals. libc does a similar thing so we actually have two globals in the memory - one of libc and one of Go.
I started thinking on how we can overcome this - problem is that if we get the argc/argv pointer before Go does and edit it, we can't re-alloc it so we have to manage with it's own size limits which are probably not good for us and is very edgy.
My current idea is to hook main, fix argv to have our environment variables then call Go's main. Problem is it requires assembly, which is ofc possible.. but then again, low level. blah.
I hope I'll have a better idea by tomorrow. 😮‍💨

@alexg-axis
Copy link
Author

Thanks for the awesome work! I look forward to see your solution.

@aviramha
Copy link
Member

aviramha commented Aug 30, 2022

Okay this was quite a journey. Yesterday I was afraid we needed to do assembly - luckily my small assembly PoC didn't work so I had to find another way of doing it which turned out to be better. Right now I checked it only on macOS and it's quite ugly. will soon check it on Linux.

Raw drafts in case someone finds it interesting - I hooked the function that is called for sorting out the env (on init) - luckily that function has no parameters or return value, meaning ABI is quite safe (besides registers clobbering..).
There I lookup the symbol of Go's argv and create our own version of it (which is like the operating system does) and replace it with ours - boom!
Code drafts:

fn make_argv() ->  Vec<*mut c_char> {
    let size = std::env::args().len();
    // Add arguments to our new argv
    let mut argv = Vec::with_capacity(size);
    for arg in std::env::args() {
        // let arg = ManuallyDrop::new(CString::new(arg).unwrap());
        // argv.push(arg.as_ptr());
        argv.push(CString::new(arg).unwrap().into_raw());
    }
    argv.push(std::ptr::null_mut());

    // Add environment variables to our new argv
    for (key, value) in std::env::vars() {
        // let arg = ManuallyDrop::new(CString::new(format!("{key}={value}")).unwrap());
        argv.push(CString::new(format!("{key}={value}")).unwrap().into_raw());
    }

    argv.push(std::ptr::null_mut());
    argv
}

#[hook_fn]
unsafe extern "C" fn goenvs_unix_detour() {
    debug!("hook goenvs_unix");
    let modules = frida_gum::Module::enumerate_modules();
    let binary = &modules.first().unwrap().name;
    if let (Some(argc), Some(argv)) = (frida_gum::Module::find_symbol_by_name(binary, "runtime.argc"), frida_gum::Module::find_symbol_by_name(binary, "runtime.argv")) {
        // let argc = argc.0.cast::<isize>();
        // let argv: *const *const *const i8 = argv.0.cast();
        // let mut argv: *const *const i8 = *argv;
        // let mut result = Vec::new();
        // argv = argv.add(*argc as usize + 1);
        // if !argv.is_null() {
        //     while !(*argv).is_null() {
        //         if let Some(key_value) = parse(CStr::from_ptr(*argv).to_bytes()) {
        //             result.push(key_value);
        //         }
        //         argv = argv.add(1);
        //     }
        // }
        // debug!("env: {:#?}", result);
        // debug!("argc: {:#?}", *argc);
        let mut new_argv = ManuallyDrop::new(make_argv());
        let argv_ptr: *mut *mut *mut i8 = argv.0.cast();
        // let mut argv: *const *const i8 = *argv_ptr;
        std::ptr::replace(argv_ptr, new_argv.as_mut_ptr());
    }
    FN_GOENVS_UNIX();
}

aviramha added a commit that referenced this issue Aug 30, 2022
* fix go environment variables injection, issue #292
@aviramha
Copy link
Member

Merged the fix. Please let me know if this works for you after updating (we should release a version today/tomorrow)

@alexg-axis
Copy link
Author

@aviramha Thanks for the quick fix! Unfortunately I get SIGSEGV.

...
2022-08-31T08:02:45.292386Z TRACE tokio_tungstenite: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/lib.rs:274 Stream.poll_next    
2022-08-31T08:02:45.292398Z TRACE tokio_tungstenite: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/lib.rs:229 WebSocketStream.with_context    
2022-08-31T08:02:45.292408Z TRACE tokio_tungstenite: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/lib.rs:284 Stream.with_context poll_next -> read_message()    
2022-08-31T08:02:45.292416Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:178 Write.flush    
2022-08-31T08:02:45.292424Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:126 AllowStd.with_context    
2022-08-31T08:02:45.292433Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:180 Write.with_context flush -> poll_flush    
2022-08-31T08:02:45.292442Z TRACE tungstenite::protocol: Frames still in queue: 0    
2022-08-31T08:02:45.292450Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:149 Read.read    
2022-08-31T08:02:45.292459Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:126 AllowStd.with_context    
2022-08-31T08:02:45.292467Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:152 Read.with_context read -> poll_read    
2022-08-31T08:02:45.292480Z TRACE tokio_tungstenite::compat: WouldBlock    
2022-08-31T08:02:45.292500Z TRACE tokio_tungstenite: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/lib.rs:274 Stream.poll_next    
2022-08-31T08:02:45.292510Z TRACE tokio_tungstenite: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/lib.rs:229 WebSocketStream.with_context    
2022-08-31T08:02:45.292519Z TRACE tokio_tungstenite: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/lib.rs:284 Stream.with_context poll_next -> read_message()    
2022-08-31T08:02:45.292528Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:178 Write.flush    
2022-08-31T08:02:45.292536Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:126 AllowStd.with_context    
2022-08-31T08:02:45.292542Z TRACE actix_codec::framed: attempting to decode a frame    
2022-08-31T08:02:45.292545Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:180 Write.with_context flush -> poll_flush    
2022-08-31T08:02:45.292602Z TRACE actix_codec::framed: frame decoded from buffer    
2022-08-31T08:02:45.292605Z TRACE tungstenite::protocol: Frames still in queue: 0    
2022-08-31T08:02:45.292646Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:149 Read.read    
2022-08-31T08:02:45.292661Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:126 AllowStd.with_context    
2022-08-31T08:02:45.292671Z TRACE tokio_tungstenite::compat: /Users/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/tokio-tungstenite-0.17.2/src/compat.rs:152 Read.with_context read -> poll_read    
2022-08-31T08:02:45.292685Z TRACE tokio_tungstenite::compat: WouldBlock    
2022-08-31T08:02:45.292620Z DEBUG mirrord_layer: DaemonMessage::GetEnvVarsResponse {
...
}!
...
2022-08-31T08:02:45.293315Z TRACE actix_codec::framed: attempting to decode a frame    
2022-08-31T08:02:45.293667Z TRACE mirrord_layer: close_detour -> fd 12
2022-08-31T08:02:45.293739Z TRACE mirrord_layer: close_detour -> fd 12
fish: Job 1, 'RUST_LOG=trace mirrord exec \…' terminated by signal … (SIGSEGV)
fish: Job Address boundary error, '' terminated by signal  ()

It happens with "hello world" as well, so there's surely some Golang peculiarity left to straighten out.

// main.go
package main

import (
	"fmt"
)

func main() {
	fmt.Println("Hello, World")
}
go build main.go
RUST_LOG=trace RUST_BACKTRACE=1 mirrord exec \
        --pod-namespace some-namespace \
        --pod-name some-pod \
        --override-env-vars-include "*" \
      ./main

@aviramha aviramha reopened this Aug 31, 2022
@aviramha
Copy link
Member

aviramha commented Aug 31, 2022

Bummer. What's your machine type? Apple Sillicon/Intel?
Also, how many env vars do you have remotely? I wonder if it's a size issue.

@aviramha
Copy link
Member

Okay we managed to reproduce - this happens on a release build. Probably some optimization causing UB.
If you want to test debug version feel free to compile mirrord locally, I hope we'll be able to deliver a fix soon ;)

@alexg-axis
Copy link
Author

I'm on Intel. It seems to happen on debug builds as well, given that I build things correctly.

git rev-parse HEAD
# 7d9624a29a265f17b88b0f628ead23278c17a025

cargo --version
# cargo 1.65.0-nightly (4ed54cecc 2022-08-27)

cargo +nightly build --workspace --exclude mirrord-agent

cp target/debug/mirrord /usr/local/bin

If it's removed by the compiler in release builds I assume the behavior could differ between rust versions as well.

@aviramha
Copy link
Member

aviramha commented Aug 31, 2022

@alexg-axis seems we can't really reproduce it on builds that are not in the release for some reason.
Can you try running with this branch? #309
I tried a solution that might help (running the go env hook on a different stack to avoid overflow). It does run on my end but it also runs without it so 🧇
I'll try to reproduce locally meanwhile so I can prove this will fix it.
Update:
Okay I managed to reproduce this! It happens only after copying the binary to /usr/local/bin. The plot thickens.
Update2:
tester bug, I actually ran the same binary downloaded and not the one I compiled so still can't reproduce.
Update3:
managed to "reproduce" I think - by following the CI steps of creating mixed binary. The PR I sent seems to fix it and we will release it soon.

@aviramha
Copy link
Member

aviramha commented Sep 1, 2022

We just released 2.11.0 which should solve this. Let us know that we can close the issue please ;)

@alexg-axis
Copy link
Author

It works! Awesome work @aviramha. Closing.

@aviramha
Copy link
Member

aviramha commented Sep 2, 2022

@alexg-axis Great! If you have any feedback for mirrord, we'd love to hear!

@manlm
Copy link

manlm commented Oct 24, 2022

We just released 2.11.0 which should solve this. Let us know that we can close the issue please ;)

I'm still facing this issue on M1 Mac

@aviramha
Copy link
Member

We just released 2.11.0 which should solve this. Let us know that we can close the issue please ;)

I'm still facing this issue on M1 Mac

Hey! We're sorry to hear that.
Can you open a new issue with more information please?
Specifically, which Go version, what mirrord version, example command line, etc.

@t4lz
Copy link
Member

t4lz commented Oct 24, 2022

Sounds a bit like #373.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants