[SECURITY] Toolkit setSecret does not protect against insecure uses

[JLLeitschuh]

Original report on H1 to the GitHub security team: https://hackerone.com/reports/1929487

---

Description:

From the GitHub actions toolkit documentation:

WARNING The add-mask and setSecret commands only support single line secrets. To register a multiline secrets you must register each line individually otherwise it will not be masked.
- https://github.com/actions/toolkit/blob/main/docs/commands.md?plain=1#LL53-L53C189

However, if you look at the code for the setSecret method doesn't protect against this vulnerability in any way.

toolkit/packages/core/src/core.ts

Lines 95 to 101 in 4573039

	/**
	* Registers a secret which will get masked from logs
	* @param secret value of the secret
	*/
	export function setSecret(secret: string): void {
	issueCommand('add-mask', {}, secret)
	}

Steps To Reproduce:

Run the following code in any action script:

core.setSecret('VeryComplexSecretThat\nThatIsMultiline\r\nOfBothTypes')

Prints the following, without erroring, when this mask will have, as documented, no effect.

::add-mask::VeryComplexSecretThat%0AThatIsMultiline%0D%0AOfBothTypes'

Problems

1. The setSecret method doesn't document this behaviour in the API docs, it's an external documentation line only found in the docs. AT MINIMUM this should be documented on the method itself.
2. The actions toolkit library is supposed to be an API to make interacting with GitHub actions easier/safer. By not incorporating any sort of protections on this API, this API leaves users vulnerable to leaking sensitive multi-line secrets to the console.

Fundamentally, this is a security issue because it fails open instead of failing closed.

Impact

Multi-line secrets passed to core.setSecret do not protect those secrets. But because the lack of protection is silent, instead of an error, the end user will not know they are not protected, although the toolkit provides no indication they are not.

---

Response from GitHub Hacker one Triage Team:

Hi @JLLeitschuh,

Thanks for the submission! This is an intentional design decision and is working as expected and as currently documented. We may make this functionality more strict in the future, but we don't have anything to announce right now. As a result, this is not eligible for reward under the Bug Bounty program.

However, because the Toolkit repository is open source, we encourage you to open an issue or a PR to help further clarify the documentation for this method.

Best regards and happy hacking!

---

In my professional opinion, as a Senior Software Security Researcher for the Open Source Security Foundation project Alpha Omega, and a GitHub Security Ambassador, we should avoid insecure-by-default method such as setSecret. We should instead aspire to prevent users from inadvertently shooting themselves in the foot by putting controls in place to protect against these sorts of vulnerabilities.

[rentziass]

Hey @JLLeitschuh, thanks for this! I've been looking at this for the last couple of days, I was able to reproduce this at first with a local (likely very old) version of toolkit and put #1427 together, but I haven't been able to reproduce ever since. Even running echo ::add-mask::VeryComplexSecretThat%0AThatIsMultiline%0D%0AOfBothTypes is working for me 😕

The runner code also seems to suggest this is the expected behaviour (perhaps we have a docs issue here?). Do you have a public example I can look at?

If you're interested, this is the action I am using for testing, and here's a workflow using it.

[JLLeitschuh]

Do you have a public example I can look at?

I do not. Did they fix the underlying vulnerability? If so, this actually may be completely resolved. I would, however, suggest adding integration tests to ensure there aren't regressions

[rentziass]

I've updated the docs to clarify what the current behaviour is and added a test to ensure escaping. Closing this now, thanks again for reporting this!