CVE-2019-18817

@@ -19,14 +19,24 @@ import (
 	"github.com/gogo/protobuf/proto"
 
 	networking "istio.io/api/networking/v1alpha3"
+	"istio.io/pkg/log"
 
 	"istio.io/istio/pilot/pkg/model"
+	"istio.io/istio/pilot/pkg/networking/util"
 	"istio.io/istio/pkg/config/host"
 )
 
 // ApplyClusterPatches applies patches to CDS clusters
-func ApplyClusterPatches(patchContext networking.EnvoyFilter_PatchContext, proxy *model.Proxy,
-	push *model.PushContext, clusters []*xdsapi.Cluster) []*xdsapi.Cluster {
+func ApplyClusterPatches(
+	patchContext networking.EnvoyFilter_PatchContext,
+	proxy *model.Proxy,
+	push *model.PushContext,
+	clusters []*xdsapi.Cluster) (out []*xdsapi.Cluster) {
+	defer util.HandleCrash(func() {
+		log.Errorf("clusters patch caused panic, so the patches did not take effect")
+	})
+	// In case the patches cause panic, use the clusters generated before to reduce the influence.
+	out = clusters
 
 	envoyFilterWrappers := push.EnvoyFilters(proxy)
 	clustersRemoved := false

@@ -22,14 +22,25 @@ import (
 	"github.com/gogo/protobuf/proto"
 	"github.com/gogo/protobuf/types"
 
+	"istio.io/pkg/log"
+
 	networking "istio.io/api/networking/v1alpha3"
 	"istio.io/istio/pilot/pkg/model"
 	"istio.io/istio/pilot/pkg/networking/util"
 )
 
 // ApplyListenerPatches applies patches to LDS output
-func ApplyListenerPatches(patchContext networking.EnvoyFilter_PatchContext,
-	proxy *model.Proxy, push *model.PushContext, listeners []*xdsapi.Listener, skipAdds bool) []*xdsapi.Listener {
+func ApplyListenerPatches(
+	patchContext networking.EnvoyFilter_PatchContext,
+	proxy *model.Proxy,
+	push *model.PushContext,
+	listeners []*xdsapi.Listener,
+	skipAdds bool) (out []*xdsapi.Listener) {
+	defer util.HandleCrash(func() {
+		log.Errorf("listeners patch caused panic, so the patches did not take effect")
+	})
+	// In case the patches cause panic, use the listeners generated before to reduce the influence.
+	out = listeners
 
 	envoyFilterWrappers := push.EnvoyFilters(proxy)
 	return doListenerListOperation(proxy, patchContext, envoyFilterWrappers, listeners, skipAdds)

@@ -24,11 +24,20 @@ import (
 
 	networking "istio.io/api/networking/v1alpha3"
 	"istio.io/istio/pilot/pkg/model"
+	"istio.io/istio/pilot/pkg/networking/util"
+	"istio.io/pkg/log"
 )
 
-func ApplyRouteConfigurationPatches(patchContext networking.EnvoyFilter_PatchContext,
-	proxy *model.Proxy, push *model.PushContext,
-	routeConfiguration *xdsapi.RouteConfiguration) *xdsapi.RouteConfiguration {
+func ApplyRouteConfigurationPatches(
+	patchContext networking.EnvoyFilter_PatchContext,
+	proxy *model.Proxy,
+	push *model.PushContext,
+	routeConfiguration *xdsapi.RouteConfiguration) (out *xdsapi.RouteConfiguration) {
+	defer util.HandleCrash(func() {
+		log.Errorf("listeners patch caused panic, so the patches did not take effect")
+	})
+	// In case the patches cause panic, use the route generated before to reduce the influence.
+	out = routeConfiguration
 
 	envoyFilterWrappers := push.EnvoyFilters(proxy)
 	for _, efw := range envoyFilterWrappers {

@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"math"
 	"net"
+	"runtime"
 	"sort"
 	"strconv"
 	"strings"
@@ -520,3 +521,23 @@ func MergeAnyWithAny(dst *types.Any, src *types.Any) (*types.Any, error) {
 
 	return retVal, nil
 }
+
+// logPanic logs the caller tree when a panic occurs.
+func logPanic(r interface{}) {
+	// Same as stdlib http server code. Manually allocate stack trace buffer size
+	// to prevent excessively large logs
+	const size = 64 << 10
+	stacktrace := make([]byte, size)
+	stacktrace = stacktrace[:runtime.Stack(stacktrace, false)]
+	log.Errorf("Observed a panic: %#v (%v)\n%s", r, r, stacktrace)
+}
+
+// HandleCrash catches the crash and calls additional handlers.
+func HandleCrash(handlers ...func()) {
+	if r := recover(); r != nil {
+		logPanic(r)
+		for _, handler := range handlers {
+			handler()
+		}
+	}
+}

@@ -615,3 +615,32 @@ func TestMergeAnyWithStruct(t *testing.T) {
 		t.Errorf("Merged HCM does not match the expected output")
 	}
 }
+
+func TestHandleCrash(t *testing.T) {
+	defer func() {
+		if x := recover(); x != nil {
+			t.Errorf("Expected no panic ")
+		}
+	}()
+
+	defer HandleCrash()
+	panic("test")
+}
+
+func TestCustomHandleCrash(t *testing.T) {
+	ch := make(chan struct{}, 1)
+	defer func() {
+		select {
+		case <-ch:
+			t.Logf("crash handler called")
+		case <-time.After(1 * time.Second):
+			t.Errorf("Custom handler not called")
+		}
+	}()
+
+	defer HandleCrash(func() {
+		ch <- struct{}{}
+	})
+
+	panic("test")
+}

@@ -15,7 +15,6 @@
 package v2
 
 import (
-	"fmt"
 	"time"
 
 	xdsapi "github.com/envoyproxy/go-control-plane/envoy/api/v2"
@@ -75,14 +74,11 @@ func (s *DiscoveryServer) generateRawClusters(node *model.Proxy, push *model.Pus
 
 	for _, c := range rawClusters {
 		if err := c.Validate(); err != nil {
-			retErr := fmt.Errorf("CDS: Generated invalid cluster for node %v: %v", node, err)
 			adsLog.Errorf("CDS: Generated invalid cluster for node:%s: %v, %v", node.ID, err, c)
 			cdsBuildErrPushes.Increment()
 			totalXDSInternalErrors.Increment()
 			// Generating invalid clusters is a bug.
-			// Panic instead of trying to recover from that, since we can't
-			// assume anything about the state.
-			panic(retErr.Error())
+			// Instead of panic, which will break down the whole cluster. Just ignore it here, let envoy process it.
 		}
 	}
 	return rawClusters

@@ -15,7 +15,6 @@
 package v2
 
 import (
-	"fmt"
 	"time"
 
 	xdsapi "github.com/envoyproxy/go-control-plane/envoy/api/v2"
@@ -51,13 +50,10 @@ func (s *DiscoveryServer) generateRawListeners(con *XdsConnection, push *model.P
 
 	for _, l := range rawListeners {
 		if err := l.Validate(); err != nil {
-			retErr := fmt.Errorf("LDS: Generated invalid listener for node %v: %v", con.modelNode, err)
 			adsLog.Errorf("LDS: Generated invalid listener for node:%s: %v, %v", con.modelNode.ID, err, l)
 			ldsBuildErrPushes.Increment()
 			// Generating invalid listeners is a bug.
-			// Panic instead of trying to recover from that, since we can't
-			// assume anything about the state.
-			panic(retErr.Error())
+			// Instead of panic, which will break down the whole cluster. Just ignore it here, let envoy process it.
 		}
 	}
 	return rawListeners

@@ -15,7 +15,6 @@
 package v2
 
 import (
-	"fmt"
 	"time"
 
 	xdsapi "github.com/envoyproxy/go-control-plane/envoy/api/v2"
@@ -57,13 +56,10 @@ func (s *DiscoveryServer) generateRawRoutes(con *XdsConnection, push *model.Push
 	// Now validate each route
 	for _, r := range rawRoutes {
 		if err := r.Validate(); err != nil {
-			retErr := fmt.Errorf("RDS: Generated invalid route %s for node %v: %v", r.Name, con.modelNode, err)
 			adsLog.Errorf("RDS: Generated invalid routes for route:%s for node:%v: %v, %v", r.Name, con.modelNode.ID, err, r)
 			rdsBuildErrPushes.Increment()
 			// Generating invalid routes is a bug.
-			// Panic instead of trying to recover from that, since we can't
-			// assume anything about the state.
-			panic(retErr.Error())
+			// Instead of panic, which will break down the whole cluster. Just ignore it here, let envoy process it.
 		}
 	}
 	return rawRoutes