:four_leaf_clover: `Proposal`: Change bad code implementation in exec

Esonhugh commented 2 years ago

If plugin dev trust framework func ExecInSystem will easily cause command injection even privilege escalation on Devs machine. Any params can be injected with evil command accidently.

func ExecInSystem(execPath string, params []string, logsBuffer *bytes.Buffer, print bool) error {
   paramStr := strings.Join(params, " ")
   fmt.Printf("Params : %s\n", paramStr)

   c := "-c"
   cmdName := "sh"

   cmd := exec.Command(cmdName, c, paramStr)
   cmd.Dir = execPath

   stdout, err := cmd.StdoutPipe()
   if err != nil {
      return err
   }

But so far thankfully no component ,plugin or anything else depends on this function.

The fix or improvement may need more discussion. Anyway, we still need label it as insecure in document to notice the devs be care,whatever the exec command function will be like.

I found kubectl execution in using the raw exec.Command pkg/util/kubectl/main.go.

Maybe we can change into framework exec in future.

That's all. Esonhugh.

algobot76 commented 2 years ago

Good point! There is a security risk in this code snippet: https://github.com/devstream-io/devstream/blob/ff3427bc2c0d634c86bf90951bc21e134ebb641a/pkg/util/kubectl/main.go#L23-L33

The filename should be validated using os.Stat. Otherwise, it could be anything, such as "foo.yaml && echo "you got hacked!", which may lead to disastrous results!

algobot76 commented 2 years ago

/assign

Esonhugh commented 2 years ago

Good point! There is a security risk in this code snippet:

https://github.com/devstream-io/devstream/blob/ff3427bc2c0d634c86bf90951bc21e134ebb641a/pkg/util/kubectl/main.go#L23-L33

The filename should be validated using os.Stat. Otherwise, it could be anything, such as "foo.yaml && echo "you got hacked!", which may lead to disastrous results!

Well ..... exec.Command will treat the filename as a param of 'kubectl'. if you add like "foo.yaml && echo "you got hacked!" will got error from shell like "file not found". It's secure but you still need care the bad filemame. Some guard code would be fine.Notice user this input problem.

daniel-hutao commented 2 years ago

First of all, thank you very much for your proposal @Esonhugh, you have looked at the DevStream code very carefully, it's very good! We look forward to your continued interest in the DevStream project and participation in our community!

Perhaps I have a different opinion about whether this function is a security problem. For a developer, he/she can just execute rm -rf / locally to finish the damage, so why go around through a function call? This function is a util, not a web page input box. So I don't really understand the "attack scenario" here.

Esonhugh commented 2 years ago

First of all, thank you very much for your proposal @Esonhugh, you have looked at the DevStream code very carefully, it's very good! We look forward to your continued interest in the DevStream project and participation in our community!

Perhaps I have a different opinion about whether this function is a security problem. For a developer, he/she can just execute rm -rf / locally to finish the damage, so why go around through a function call? This function is a util, not a web page input box. So I don't really understand the "attack scenario" here.

Yep.Good question.

the code there is really ugly and easy to exploit.(I'am not blame coder,but the code.)
We don't know where and what will call this func. As your pkg document said the The /pkg directory is still a good way to explicitly communicate that the code in that directory is safe for use by others.
For attack scenario, if some plugins use this in plugins or other place, some devs may cheated to download evil config (or some remote load) to fishing the user, or some one pollute them in cache.Even pollute this project (just provide a evil plugins use this func and if reviewer think func safe, that will escape the Code reviewer's eyes. attackers are always tricky.) And Your project is designed for devops, steal code or creds in config ,plant trojans will more easy.
for my frist comment in issue, Anyway, we still need label it as insecure in document to notice the devs be care,whatever the exec command function will be like. we need to let devs and user know that risk.
The env is also limited to Linux. windows don't have the sh.

So that's why i think i need issue this.

daniel-hutao commented 2 years ago

Thanks again for your enthusiastic work. About this issue, I still can't understand why I need to tell the caller that this function is unsafe. Compare with exec.Command() method, there is no additional "lifting" operation. The caller can choose to use ExecInSystem(), or exec.Command(). If ExecInSystem() needs to be treated specially, then why doesn't Golang itself mark exec.Command() as unsafe? It's just a tool, and it's up to the user to decide how to use it, just like a gun, where the bad guys can get their hands on it and kill people, but the gun itself doesn't need to be set to "refuse to fire a bullet if it's fired at a person". I just don't understand the need for the change, not trying to "argue" with you, I hope it won't affect your passion.

Esonhugh commented 2 years ago

Thanks again for your enthusiastic work. About this issue, I still can't understand why I need to tell the caller that this function is unsafe. Compare with exec.Command() method, there is no additional "lifting" operation. The caller can choose to use ExecInSystem(), or exec.Command(). If ExecInSystem() needs to be treated specially, then why doesn't Golang itself mark exec.Command() as unsafe? It's just a tool, and it's up to the user to decide how to use it, just like a gun, where the bad guys can get their hands on it and kill people, but the gun itself doesn't need to be set to "refuse to fire a bullet if it's fired at a person". I just don't understand the need for the change, not trying to "argue" with you, I hope it won't affect your passion.

Thanks. Write like 'sh -c "{expressions}"' allow us to inject shell commands into any params. as Tencent Go pointed out, code write like that is unsafe.

refer from the book.

1.3.1【必须】命令执行检查

使用exec.Command、exec.CommandContext、syscall.StartProcess、os.StartProcess等函数时，第一个参数（path）直接取外部输入值时，应使用白名单限定可执行的命令范围，不允许传入bash、cmd、sh等命令；

使用exec.Command、exec.CommandContext等函数时，通过bash、cmd、sh等创建shell，-c后的参数（arg）拼接外部输入，应过滤\n $ & ; | ' " ( ) `等潜在恶意字符； refer from code
func ExecInSystem(execPath string, params []string, logsBuffer *bytes.Buffer, print bool) error {
paramStr := strings.Join(params, " ")
fmt.Printf("Params : %s\n", paramStr)

c := "-c"
cmdName := "sh"

cmd := exec.Command(cmdName, c, paramStr)
cmd.Dir = execPath



There are no filter and frist param is hardcoded as  `sh`.

I mean that the unsafety is caused by the `exec.Command("sh","-c",.....) ` instead of `exec.Command` itself ,and my Changes is giving the `cmdName` to Devs and notice them the necessary information.

There is not a clear attack vector, just a gadget. So I understand your confuse and explain for you.

iyear commented 2 years ago

@Esonhugh In Tencent's guide, his scenario is external input, which requires filtering as a given. But in devstream, the CLI would have needed to execute a shell. sh is blocked how do we execute the command?

More simply, exec is only used in devstream for its own use and is not designed for external use.

Esonhugh commented 2 years ago

@Esonhugh In Tencent's guide, his scenario is external input, which requires filtering as a given. But in devstream, the CLI would have needed to execute a shell. sh is blocked how do we execute the command?

More simply, exec is only used in devstream for its own use and is not designed for external use.

I don't think so. The params is so easy to be controlled and everything depend on this will be vulnerable unless you completely hardcode you command.and That's no a security issue right now, I just want point out the coding problem.

By the way, What about delete this os package and use exec.Command.

IronCore864 commented 2 years ago

Very good discussions above. First things first: sorry for asking @Esonhugh to work on a fix prematurely before we discussed deep enough. My bad!

I understand the issue here: we followed the standard Go project layout which clearly states that the /pkg folder is public and should be safe, where we have unsafe functions there (no matter if it's easy to be exploited or not.)

We do not intend to fork the standard Go project layout and update it so that it fits our project's situation because that will bring in extra operational overhead. It's easier to stick with it. So we do not intend to update the doc part for that; I hope you understand @Esonhugh.

Building on top of what's said above, here I propose from another standpoint: how about we delete this function since it's dead code? If we need it in the future, we could recover it from history, anyway.

On the KubeApply, I don't see a problem, but we will refactor it anyway. At the moment, there are two plugins that depend on it, one is DevLake, and the other is ArgoCD App. I propose:

refactor the DevLake plugin so that it uses Helm (Go Helm client) to install instead of native YAML (issue https://github.com/devstream-io/devstream/issues/504, will probably be handled by someone from the DevLake community)
refactor the ArgoCD App plugin so that it uses k8s go client/ArgoCD Go API client to apply a CRD instead of kubectl apply (https://github.com/devstream-io/devstream/issues/515, haven't been enriched with more context)

What do you guys all think? @Esonhugh @algobot76 @daniel-hutao @iyear

Any input is welcome. Thanks!

Esonhugh commented 2 years ago

I can't agree more.

IronCore864 commented 2 years ago

See PR https://github.com/devstream-io/devstream/pull/631 which deleted the dead code.

devstream-io / devstream

:four_leaf_clover: `Proposal`: Change bad code implementation in exec #583