Merge pull request #12491 from gyuho/better-exec

*: validate exec command args, remove unused "iptables" wrapper
2024-09-27 06:25:44 +00:00 · 2020-11-25 11:26:58 -08:00 · 2020-11-25 11:26:58 -08:00 · 64c409e124
commit 64c409e124
parent b5cefb5b3d b40b46f473
7 changed files with 45 additions and 117 deletions
--- a/CHANGELOG-3.4.md
+++ b/CHANGELOG-3.4.md
@ -13,10 +13,24 @@ The minimum recommended etcd versions to run in **production** are 3.2.28+, 3.3.

 See [code changes](https://github.com/etcd-io/etcd/compare/v3.4.13...v3.4.14) and [v3.4 upgrade guide](https://github.com/etcd-io/etcd/blob/master/Documentation/upgrades/upgrade_3_4.md) for any breaking changes.

+### Package `clientv3`
+
+- Fix [auth token invalid after watch reconnects](https://github.com/etcd-io/etcd/pull/12264). Get AuthToken automatically when clientConn is ready.
+
 ### etcd server

 - [Fix server panic](https://github.com/etcd-io/etcd/pull/12288) when force-new-cluster flag is enabled in a cluster which had learner node.

+### Package `netutil`
+
+- Remove [`netutil.DropPort/RecoverPort/SetLatency/RemoveLatency`](https://github.com/etcd-io/etcd/pull/12491).
+  - These are not used anymore. They were only used for older versions of functional testing.
+  - Removed to adhere to best security practices, minimize arbitrary shell invocation.
+
+### `tools/etcd-dump-metrics`
+
+- Implement [input validation to prevent arbitrary shell invocation](https://github.com/etcd-io/etcd/pull/12491).
+


 <hr>
--- a/CHANGELOG-3.5.md
+++ b/CHANGELOG-3.5.md
@ -221,6 +221,16 @@ Note that any `etcd_debugging_*` metrics are experimental and subject to change.
 - Add [`/v3/auth/status`](https://github.com/etcd-io/etcd/pull/11536) endpoint to check if authentication is enabled
 - [Add `Linearizable` field to `etcdserverpb.MemberListRequest`](https://github.com/etcd-io/etcd/pull/11639).

+### Package `netutil`
+
+- Remove [`netutil.DropPort/RecoverPort/SetLatency/RemoveLatency`](https://github.com/etcd-io/etcd/pull/12491).
+  - These are not used anymore. They were only used for older versions of functional testing.
+  - Removed to adhere to best security practices, minimize arbitrary shell invocation.
+
+### `tools/etcd-dump-metrics`
+
+- Implement [input validation to prevent arbitrary shell invocation](https://github.com/etcd-io/etcd/pull/12491).
+
 ### Dependency

 - Upgrade [`google.golang.org/grpc`](https://github.com/grpc/grpc-go/releases) from [**`v1.23.0`**](https://github.com/grpc/grpc-go/releases/tag/v1.23.0) to [**`v1.26.0`**](https://github.com/grpc/grpc-go/releases/tag/v1.26.0).
--- a/pkg/netutil/isolate_linux.go
+++ b/pkg/netutil/isolate_linux.go
@ -1,82 +0,0 @@
-// Copyright 2015 The etcd Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package netutil
-
-import (
-	"fmt"
-	"os/exec"
-)
-
-// DropPort drops all tcp packets that are received from the given port and sent to the given port.
-func DropPort(port int) error {
-	cmdStr := fmt.Sprintf("sudo iptables -A OUTPUT -p tcp --destination-port %d -j DROP", port)
-	if _, err := exec.Command("/bin/sh", "-c", cmdStr).Output(); err != nil {
-		return err
-	}
-	cmdStr = fmt.Sprintf("sudo iptables -A INPUT -p tcp --destination-port %d -j DROP", port)
-	_, err := exec.Command("/bin/sh", "-c", cmdStr).Output()
-	return err
-}
-
-// RecoverPort stops dropping tcp packets at given port.
-func RecoverPort(port int) error {
-	cmdStr := fmt.Sprintf("sudo iptables -D OUTPUT -p tcp --destination-port %d -j DROP", port)
-	if _, err := exec.Command("/bin/sh", "-c", cmdStr).Output(); err != nil {
-		return err
-	}
-	cmdStr = fmt.Sprintf("sudo iptables -D INPUT -p tcp --destination-port %d -j DROP", port)
-	_, err := exec.Command("/bin/sh", "-c", cmdStr).Output()
-	return err
-}
-
-// SetLatency adds latency in millisecond scale with random variations.
-func SetLatency(ms, rv int) error {
-	ifces, err := GetDefaultInterfaces()
-	if err != nil {
-		return err
-	}
-
-	if rv > ms {
-		rv = 1
-	}
-	for ifce := range ifces {
-		cmdStr := fmt.Sprintf("sudo tc qdisc add dev %s root netem delay %dms %dms distribution normal", ifce, ms, rv)
-		_, err = exec.Command("/bin/sh", "-c", cmdStr).Output()
-		if err != nil {
-			// the rule has already been added. Overwrite it.
-			cmdStr = fmt.Sprintf("sudo tc qdisc change dev %s root netem delay %dms %dms distribution normal", ifce, ms, rv)
-			_, err = exec.Command("/bin/sh", "-c", cmdStr).Output()
-			if err != nil {
-				return err
-			}
-		}
-	}
-	return nil
-}
-
-// RemoveLatency resets latency configurations.
-func RemoveLatency() error {
-	ifces, err := GetDefaultInterfaces()
-	if err != nil {
-		return err
-	}
-	for ifce := range ifces {
-		_, err = exec.Command("/bin/sh", "-c", fmt.Sprintf("sudo tc qdisc del dev %s root netem", ifce)).Output()
-		if err != nil {
-			return err
-		}
-	}
-	return nil
-}
--- a/pkg/netutil/isolate_stub.go
+++ b/pkg/netutil/isolate_stub.go
@ -1,25 +0,0 @@
-// Copyright 2015 The etcd Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-// +build !linux
-
-package netutil
-
-func DropPort(port int) error { return nil }
-
-func RecoverPort(port int) error { return nil }
-
-func SetLatency(ms, rv int) error { return nil }
-
-func RemoveLatency() error { return nil }
--- a/tools/etcd-dump-metrics/etcd.go
+++ b/tools/etcd-dump-metrics/etcd.go
@ -23,7 +23,7 @@ import (
 	"strings"
 	"time"

-	"go.etcd.io/etcd/client/v3"
+	clientv3 "go.etcd.io/etcd/client/v3"
 	"go.etcd.io/etcd/server/v3/embed"

 	"go.uber.org/zap"
@ -61,12 +61,22 @@ func setupEmbedCfg(cfg *embed.Config, curls, purls, ics []url.URL) {
 	cfg.InitialCluster = cfg.InitialCluster[1:]
 }

-func getCommand(exec, name, dir, cURL, pURL, cluster string) string {
-	s := fmt.Sprintf("%s --name %s --data-dir %s --listen-client-urls %s --advertise-client-urls %s ",
-		exec, name, dir, cURL, cURL)
-	s += fmt.Sprintf("--listen-peer-urls %s --initial-advertise-peer-urls %s ", pURL, pURL)
-	s += fmt.Sprintf("--initial-cluster %s ", cluster)
-	return s + "--initial-cluster-token tkn --initial-cluster-state new"
+func getCommand(exec, name, dir, cURL, pURL, cluster string) (args []string) {
+	if !strings.Contains(exec, "etcd") {
+		panic(fmt.Errorf("%q doesn't seem like etcd binary", exec))
+	}
+	return []string{
+		exec,
+		"--name", name,
+		"--data-dir", dir,
+		"--listen-client-urls", cURL,
+		"--advertise-client-urls", cURL,
+		"--listen-peer-urls", pURL,
+		"--initial-advertise-peer-urls", pURL,
+		"--initial-cluster", cluster,
+		"--initial-cluster-token=tkn",
+		"--initial-cluster-state=new",
+	}
 }

 func write(ep string) {
--- a/tools/etcd-dump-metrics/install_linux.go
+++ b/tools/etcd-dump-metrics/install_linux.go
@ -47,7 +47,8 @@ func install(ver, dir string) (string, error) {
 		return "", err
 	}

-	if err = exec.Command("bash", "-c", fmt.Sprintf("tar xzvf %s -C %s --strip-components=1", tarPath, dir)).Run(); err != nil {
+	// parametrizes to prevent attackers from adding arbitrary OS commands
+	if err = exec.Command("tar", "xzvf", tarPath, "-C", dir, "--strip-components=1").Run(); err != nil {
 		return "", err
 	}
 	return filepath.Join(dir, "etcd"), nil
--- a/tools/etcd-dump-metrics/main.go
+++ b/tools/etcd-dump-metrics/main.go
@ -87,7 +87,7 @@ func main() {
 			rc := make(chan run)

 			cs1 := getCommand(bp, "s1", d1, "http://localhost:2379", "http://localhost:2380", cluster)
-			cmd1 := exec.Command("bash", "-c", cs1)
+			cmd1 := exec.Command(cs1[0], cs1[1:]...)
 			go func() {
 				if *debug {
 					cmd1.Stderr = os.Stderr
@ -101,7 +101,7 @@ func main() {
 				rc <- run{cmd: cmd1}
 			}()
 			cs2 := getCommand(bp, "s2", d2, "http://localhost:22379", "http://localhost:22380", cluster)
-			cmd2 := exec.Command("bash", "-c", cs2)
+			cmd2 := exec.Command(cs2[0], cs2[1:]...)
 			go func() {
 				if *debug {
 					cmd2.Stderr = os.Stderr