Output hard to read

[boc-tothefuture]

I followed the instructions in the readme, it is working but the output is very hard to decipher:

For example: https://github.com/boc-tothefuture/openhab-jruby/runs/1770247963?check_suite_focus=true

Is there a way to have it be multiline? It also doesn't seem to render the special characters too well.

[micgro42]

Same. Would it be possible to get the colors and other styling as on the command line?

This single-line blob of text isn't easy to read 👇

[wagoid]

Yeah, this output is looking terrible! This was introduced on PR #58 when fixing a security issue. I'll have a look to see if there's a way to show the results formatted while keeping command processing disabled 🤞

[jdbruijn]

From @actions/core, the info function also doesn't seem secure in regards to command processing via stdout. One solution might be to keep workflow commands turned on, but parse the output before logging it to stdout. We could look for the command start syntax (ref) :: and only log the untrusted output if we know it doesn't contain any commands. If it does contain commands, log an error message or remove the commands.

• https://github.com/actions/toolkit/blob/main/packages/core/src/command.ts#L36

[wagoid]

@jdbruijn I believe it's a bit more complex than that. On the announcement of this issue, GitHub advised to disable command processing on any information that contains commit messages:

If you need to log untrusted information such as issue titles, bodies, or commit messages to STDOUT we recommend that you disable command processing prior to doing that.

[wagoid]

I understand that your idea is to just check if the messages contain the command start syntax (::), but we need to double check if just this simple check is enough. From what I've found about GitHub guidelines so far, this was not advised as an alternative solution 😃

[jdbruijn]

I agree that it is what they advised, I was merely suggesting an alternative approach. Based on what I quickly saw in the toolkit source code and documentation that is the defined syntax of commands.

I totally agree that if there is a solution that keeps the commands stopped, that would be preferred. I'm not an expert at that topic at all and only briefly looked into it before making my suggestion above. FYI: In that time I also have not found a generically used solution for this use case.

[jdbruijn]

Also, quickly based on the source code of the action runner commands are detected as follows.

1. Receive data to be outputted in OnDataReceived
2. Try to process a command in the output TryProcessCommand
3. Check whether the message is a command, with TryParseV2 or TryParse (legacy)
   a. In TryParseV2, a message is a command if it starts with _commandKey, which is :: ref
   b. In TryParse (legacy) a message that is wrapped in ##[ and ] ref. On this one I'm not 100% sure as the code is a bit more complex.

So this means a simple check on the current command start suffix :: won't suffice, given that the legacy syntax is not turned off in some way.

[wagoid]

Thanks for the in-depth check @jdbruijn! ❤️

Your investigation seems solid, ##[ and :: seem to be the only options that the runner will interpret.

But the problem is: will it be enough to just remove those sequences from the messages, even if those are the only available options? That's why I wanted to get an official alternative. We could definitely try a workaround, but we won't be sure whether the workaround is correct or not since we're not experts in the topic, just like you said. There might be other exploits we are missing by just replacing some strings.

I wish we could reach out to someone at GitHub to help us with this issue 😅

[wagoid]

I found out one thing. If we stop "trying" to issue commands, github actions will show the output in multiple lines again. I've replaced the core.setFailed calls with manual logs so that we only pass the direct error message instead of trying to issue the ::error:: command. It's not perfect, but at least we're sure the action is not exposed to the exploit 🙌

I also noticed that, due to the output generation logic, the action was still susceptible to the CVE-2020-15228 vulnerability.

I've added PR #71 to tackle these concerns. Once github removes add-path and set-env commands from runners, we can bring back those 2 features again.

[wagoid]

For reference, this is how the output looks like now (from this run):

[jdbruijn]

If we stop "trying" to issue commands, github actions will show the output in multiple lines again.

That's nice, the output looks good again!

I also noticed that, due to the output generation logic, the action was still susceptible to the CVE-2020-15228 vulnerability.

Could you please elaborate on this? I havent found reference for this in my quickly search, and would argue core.setOutput is safe to use.

I've added PR #71 to tackle these concerns. Once github removes add-path and set-env commands from runners, we can bring back those 2 features again.

They have removed these: GitHub Actions: Removing set-env and add-path commands on November 16.

[wagoid]

Could you please elaborate on this? I havent found reference for this in my quickly search, and would argue core.setOutput is safe to use.

Sure! The thing is not only that specific functions of the toolkit were unsafe, but the fact that all commands need to be disabled. Think of it as "actions injection", or something like that (in a refenrece to SQL injection). The reason why github advised to completely disable command execution whenever logging untrusted information is because anyone can add any command related chars like :: to the commit messages, and issue the set-env and add-path commands. The problem is not about sending a command directly to stdout without using the toolkit, the problem is that commands need to be disabled if we don't trust whether the commit message has any "actions injection". Quoting their guidance again:

If you need to log untrusted information such as issue titles, bodies, or commit messages to STDOUT we recommend that you disable command processing prior to doing that.

This is not a thing specific to the toolkit, is just that anytime you're logging any untrusted information to STDOUT you're open for other commands to be issued. I understand that the Command class applies some escaping to commands, but I don't believe this enough. Otherwise GitHub would be able to just advise "use this escaping logic to untrusted data" instead of advising to disable commands completely.

They have removed these: GitHub Actions: Removing set-env and add-path commands on November 16.

Oh that's interesting! I was searching for this but couldn't find it. I've checked the PR but it still seems a bit tricky. One could just set the ACTIONS_ALLOW_UNSECURE_COMMANDS at job level and still allow them. Seems like we are still susceptible to it?

[jdbruijn]

They have removed these: GitHub Actions: Removing set-env and add-path commands on November 16.

Oh that's interesting! I was searching for this but couldn't find it. I've checked the PR but it still seems a bit tricky. One could just set the ACTIONS_ALLOW_UNSECURE_COMMANDS at job level and still allow them. Seems like we are still susceptible to it?

True, a user could still set ACTIONS_ALLOW_UNSECURE_COMMANDS in its workflow, but IMO that is on them and not our responsibility. In this action we've done what we could, up to a certain level of course, so if someone sets ACTIONS_ALLOW_UNSECURE_COMMANDS explicitly, they're responsible for being vulnerable to unsecure commands IMO.

---

My point on the action output is that it does not print anything to the stdout explicitly, so is not vulnerable to that issue right? Sure the output could contain commands, but if it is not logged by this action it is no problem in relation to this action. If a user decides to log the output, they should do so in a safe matter themselves. I think it is then our responsibility to inform the user about potential commands in the output and reference the security issue, but the rest is on them.

Just my two cents.

[wagoid]

True, a user could still set ACTIONS_ALLOW_UNSECURE_COMMANDS in its workflow, but IMO that is on them and not our responsibility. In this action we've done what we could, up to a certain level of course, so if someone sets ACTIONS_ALLOW_UNSECURE_COMMANDS explicitly, they're responsible for being vulnerable to unsecure commands IMO.

It depends a bit, on forks we can edit the workflow and include the env. That's the part I was more concerned. But as you said, we can only do so much, if someone doesn't want to be exposed in any way they should disable running workflows from forks.

My point on the action output is that it does not print anything to the stdout explicitly, so is not vulnerable to that issue right? Sure the output could contain commands, but if it is not logged by this action it is no problem in relation to this action. If a user decides to log the output, they should do so in a safe matter themselves. I think it is then our responsibility to inform the user about potential commands in the output and reference the security issue, but the rest is on them.

In order to create an output, we need to log to stdout. Creating an output is just about adding a message to stdout that contains the ::set-output keyword on it (core.setOutput function ends up here). Since the output contains commit messages, the commit messages can be added in a way that a different command will be issued. That is, through the commit message someone can close the set-output command and issue a new one.

So considering all the scenarios that we've discussed here, I think we can summarize these concerns by doing 2 things in the end:

1. Enable command execution again on the action, use core.setFailed again for errors and return the output feature
2. Warn about running this action on forks, because fork PRs can add the ACTIONS_ALLOW_UNSECURE_COMMANDS env var at the job level.

What do you think?

[jdbruijn]

Creating an output is just about adding a message to stdout that contains the ::set-output keyword on it (core.setOutput function ends up here).

I didn't realise, so you're absolutely right. Setting an output could still be harmful.

---

Enable command execution again on the action, use core.setFailed again for errors and return the output feature

This would make this vulnerable to commands again, so is probably not what we want. I think we could create an issue in https://github.com/actions/toolkit/issues, to discuss how this topic should be handled with, I guess they can give good insight in what to do.

[wagoid]

That's true, maybe the toolkit collaborators can help us through this issue. Will do that!
Update: added the question actions/toolkit#702

[wagoid]

I've got an answer, looks like we're fine issuing those commands. Will add them again 🎉

[jdbruijn]

Sounds good, thanks for reaching out to the toolkit collaborators!