Wrong environment variable parsing with ANSI-C Quoting

[morxa]

We set secrets in an environment hook (following this guide).

One of the secrets is an ANSI-C quoted string, e.g., the following:

MYUSERNAME="user"
export MYUSERNAME
MYSECRETKEY=$'this\nis\na\nkey'
export MYSECRETKEY

The buildkite agent parses the environment variable MYSECRETKEY incorrectly. The output of the build (Running global environment hook to be more precise) shows:

# MYSECRETKEY=$'this\nis\na\nkey' added

As you can see, the string is treated as part of the environment variable name, rather than as its value. This causes secrets to be leaked to anyone who can access the build log.

Here is a full example. The environment hook is the following script:

#!/usr/bin/env bash

set -eu

HOST=
ORG=
REPO=

MYUSERNAME="user"
export MYUSERNAME
MYSECRETKEY=$'this\nis\na\nkey'
export MYSECRETKEY

export

If you open Running global environment hook, you can see the output and which variables are exported. I added an export to the end of the script to debug the environment variables. I also added another export to the script that uses the environment variable, which was set by the buildkite agent (IIUC). Here, the secret is incorrectly escaped:

declare -x MYSECRETKEY="\$'this\\nis\\na\\nkey'="

This is actually how we found the issue, because the secret was no longer working.

Looking at the agent code, the following assumption seems to be invalid:

agent/env/export.go

Lines 128 to 132 in 765795e

	// We now have a line that can either be one of these:
	//
	// 1. `FOO="bar"`
	// 2. `FOO="open quote for new lines`
	// 3. `FOO`

The buildkite agent version is v3.39.0.

[sj26]

Thanks for the detailed report, @morxa 🙏

We had trouble replicating this until we upgraded to bash 5.2, which was released about 16 days ago. Are you using bash 5.2?

[moskyb]

hey @morxa! thanks for reporting this, this seems like a pretty ugly bug to come across, and this description of the error is really well written.

for some context, the way we pull environment variables out of hooks/commands is pretty fragile, and has had to have some surgery a couple of times. we've been thinking about rewriting it for a while, and i think we've found a good reason here.

So: some background, both as a brain dump from me as i investigate this, and as info for future travellers.

Part of the cool thing about hooks is that they can modify the environment of the entire build chain. To get really deep into it, this is actually a holdover from older versions of the agent, where the bootstrap process was one (giant and janky) bash script. When we ran a hook from that, it would be sourced, so it would change the environment of the bootstrap process itself.

When we migrated the bootstrap to its current form as a Go binary that runs a bunch of processes and stuff, we couldn't include the sourcing behaviour that we previously got for free, so we came up with the concept of a scriptwrapper. The scriptwrapper remains the way that we run all hooks in the bootstrap process. What it does is basically:

• Before executing a hook, write the environment of the process to a file in a well-known location
• Run the hook
• Write the environment of the process again, to a different file this time

The scriptwrapper knows (or more correctly, env/export.go that you've linked above knows) how to compare the two environment files. so it collects the differences - additions, removals and changes - and tells the bootstrap about them, so that the bootstrap can change the environment for further processes it runs.

This scriptwrapper thingo is where we're running into the bug you've described here - it uses the output of the bash- (or zsh-, or fish-, or...) builtin export -p to output the contents of the current process' environment.

It seems like as of Bash 5.2, the behaviour of this builtin has changed, and it now exports ANSI-C-formatted strings when appropriate, which has broken our parsing of that kind of file. For reference, recent zsh versions also seem to exhibit this behaviour.

All of this is to say, we were relying on bash to not change something, otherwise our fragile parsing would break, and they rightfully changed it, and now our fragile parsing is broken. This sucks, but is kind of on us for relying on behaviour that wasn't stable.

We're investigating changing this process to be less fragile, which should be done soon - this is a high priority, as we don't want secrets leaking into the build logs!

Thank you again for your error report!

[moskyb]

Also, as a mitigation until we release a fix: Using Bash 5.1 as a shell should fix this issue, but please let us know if you continue to encounter this issue.

[morxa]

Thanks for the explanation! Indeed, we upgraded to bash 5.2 on Monday, which also explains why the error suddenly started showing up (this was something that puzzled me until now).

Indeed, downgrading to bash 5.1 fixed the issue! Thanks for the workaround!

[moskyb]

This is fixed as of v3.39.1. Thanks again for your report, @morxa!