admin: reject non-canonical config array indices (#7592)

* admin: reject non-canonical config array indices

* admin: expand canonical array index test coverage

* Update admin.go

Co-authored-by: Matt Holt <mholt@users.noreply.github.com>

* Update admin.go

Co-authored-by: Matt Holt <mholt@users.noreply.github.com>

* admin: improve canonical array index test diagnostics

---------

Co-authored-by: Matt Holt <mholt@users.noreply.github.com>

admin.go

@@ -1161,6 +1161,20 @@ func handleStop(w http.ResponseWriter, r *http.Request) error {
 	return nil
 }
 
+func parseCanonicalArrayIndex(idx string) (int, error) {
+	if idx == "" {
+		return 0, fmt.Errorf("empty index")
+	}
+	i, err := strconv.Atoi(idx)
+	if err != nil {
+		return 0, err
+	}
+	if strconv.Itoa(i) != idx {
+		return 0, fmt.Errorf("non-canonical array index")
+	}
+	return i, nil
+}
+
 // unsyncedConfigAccess traverses into the current config and performs
 // the operation at path according to method, using body and out as
 // needed. This is a low-level, unsynchronized function; most callers
@@ -1222,11 +1236,12 @@ traverseLoop:
 				var idx int
 				if method != http.MethodPost {
 					idxStr := parts[len(parts)-1]
-					idx, err = strconv.Atoi(idxStr)
+					idx, err = parseCanonicalArrayIndex(idxStr)
 					if err != nil {
 						return fmt.Errorf("[%s] invalid array index '%s': %v",
 							path, idxStr, err)
 					}
+
 					if idx < 0 || (method != http.MethodPut && idx >= len(arr)) || idx > len(arr) {
 						return fmt.Errorf("[%s] array index out of bounds: %s", path, idxStr)
 					}
@@ -1326,7 +1341,7 @@ traverseLoop:
 			}
 
 		case []any:
-			partInt, err := strconv.Atoi(part)
+			partInt, err := parseCanonicalArrayIndex(part)
 			if err != nil {
 				return fmt.Errorf("[/%s] invalid array index '%s': %v",
 					strings.Join(parts[:i+1], "/"), part, err)

admin_test.go

@@ -15,6 +15,7 @@
 package caddy
 
 import (
+	"bytes"
 	"context"
 	"crypto"
 	"crypto/tls"
@@ -1106,3 +1107,47 @@ MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDRS0LmTwUT0iwP
 		})
 	}
 }
+
+func TestUnsyncedConfigAccessCanonicalArrayIndices(t *testing.T) {
+	rawCfg = map[string]any{
+		rawConfigKey: map[string]any{
+			"list": []any{"zero", "one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"},
+		},
+	}
+
+	tests := []struct {
+		name       string
+		path       string
+		wantOutput string
+		wantErr    bool
+	}{
+		{name: "allow zero", path: "/" + rawConfigKey + "/list/0", wantOutput: "\"zero\"\n"},
+		{name: "allow one", path: "/" + rawConfigKey + "/list/1", wantOutput: "\"one\"\n"},
+		{name: "allow ten", path: "/" + rawConfigKey + "/list/10", wantOutput: "\"ten\"\n"},
+		{name: "reject leading zero", path: "/" + rawConfigKey + "/list/01", wantErr: true},
+		{name: "reject multiple leading zeros", path: "/" + rawConfigKey + "/list/002", wantErr: true},
+		{name: "reject plus sign", path: "/" + rawConfigKey + "/list/+1", wantErr: true},
+		{name: "reject negative zero", path: "/" + rawConfigKey + "/list/-0", wantErr: true},
+	}
+
+	for i, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			var gotOutput bytes.Buffer
+			err := unsyncedConfigAccess(http.MethodGet, tc.path, nil, &gotOutput)
+
+			if tc.wantErr {
+				if err == nil {
+					t.Errorf("test %d (%s): input path %q: expected error, got nil with output %q", i, tc.name, tc.path, gotOutput.String())
+				}
+				return
+			}
+
+			if err != nil {
+				t.Errorf("test %d (%s): input path %q: expected no error with output %q, got error %v with output %q", i, tc.name, tc.path, tc.wantOutput, err, gotOutput.String())
+			}
+			if gotOutput.String() != tc.wantOutput {
+				t.Errorf("test %d (%s): input path %q: expected output %q, got %q", i, tc.name, tc.path, tc.wantOutput, gotOutput.String())
+			}
+		})
+	}
+}