Merge pull request #1207 from sophieliu15/fix-issue-511043249

Fix Stackdriver Filter Injection vulnerability for In/NotIn operators

Author: sophieliu15

custom-metrics-stackdriver-adapter/pkg/adapter/translator/query_builder.go

@@ -741,7 +741,7 @@ func (t *Translator) filterForSelector(metricSelector labels.Selector, allowedLa
 		case selection.In:
 			if isAllowedLabelName(req.Key(), allowedLabelPrefixes, allowedFullLabelNames) {
 				if len(l) == 1 {
-					filters = append(filters, fmt.Sprintf("%s = %s", req.Key(), l[0]))
+					filters = append(filters, fmt.Sprintf("%s = %q", req.Key(), l[0]))
 				} else {
 					filters = append(filters, fmt.Sprintf("%s = one_of(%s)", req.Key(), strings.Join(quoteAll(l), ",")))
 				}
@@ -751,7 +751,7 @@ func (t *Translator) filterForSelector(metricSelector labels.Selector, allowedLa
 		case selection.NotIn:
 			if isAllowedLabelName(req.Key(), allowedLabelPrefixes, allowedFullLabelNames) {
 				if len(l) == 1 {
-					filters = append(filters, fmt.Sprintf("%s != %s", req.Key(), l[0]))
+					filters = append(filters, fmt.Sprintf("%s != %q", req.Key(), l[0]))
 				} else {
 					filters = append(filters, fmt.Sprintf("NOT %s = one_of(%s)", req.Key(), strings.Join(quoteAll(l), ",")))
 				}

custom-metrics-stackdriver-adapter/pkg/adapter/translator/query_builder_test.go

@@ -1585,3 +1585,41 @@ func TestTranslator_GetMetricKind_CacheKeyIsolation(t *testing.T) {
 		t.Errorf("metricKindCache.get(%v) = true, want false", keyDefault)
 	}
 }
+
+func TestTranslator_QueryBuilder_FilterInjection_Prevention(t *testing.T) {
+	translator, _ :=
+		NewFakeTranslator(2*time.Minute, time.Minute, "my-project", "my-cluster", "my-zone", time.Date(2017, 1, 2, 13, 2, 0, 0, time.UTC), true)
+
+	allowedLabelPrefixes := []string{"metric.labels"}
+	allowedFullLabelNames := []string{}
+
+	// 1. Test 'In' operator with single unquoted element containing injection payload.
+	reqIn, err := labels.NewRequirement("metric.labels.custom", selection.In, []string{"safedummy"})
+	if err != nil {
+		t.Fatalf("Failed to create requirement: %v", err)
+	}
+	selectorIn := labels.NewSelector().Add(*reqIn)
+	filterIn, _, err := translator.filterForSelector(selectorIn, allowedLabelPrefixes, allowedFullLabelNames)
+	if err != nil {
+		t.Fatalf("Failed to build filter: %v", err)
+	}
+	expectedFilterIn := "metric.labels.custom = \"safedummy\""
+	if filterIn != expectedFilterIn {
+		t.Errorf("Expected filter: %s, got: %s", expectedFilterIn, filterIn)
+	}
+
+	// 2. Test 'NotIn' operator with single unquoted element.
+	reqNotIn, err := labels.NewRequirement("metric.labels.custom", selection.NotIn, []string{"safedummy"})
+	if err != nil {
+		t.Fatalf("Failed to create requirement: %v", err)
+	}
+	selectorNotIn := labels.NewSelector().Add(*reqNotIn)
+	filterNotIn, _, err := translator.filterForSelector(selectorNotIn, allowedLabelPrefixes, allowedFullLabelNames)
+	if err != nil {
+		t.Fatalf("Failed to build filter: %v", err)
+	}
+	expectedFilterNotIn := "metric.labels.custom != \"safedummy\""
+	if filterNotIn != expectedFilterNotIn {
+		t.Errorf("Expected filter: %s, got: %s", expectedFilterNotIn, filterNotIn)
+	}
+}