Fix config set handler (#5384)

- Return error when the config JSON has duplicate keys (fixes #5286)

- Limit size of configuration file provided to 256KiB - this prevents
  another form of DoS

cmd/admin-handlers.go

@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"encoding/xml"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net/http"
 	"net/url"
@@ -35,6 +36,8 @@ import (
 const (
 	minioAdminOpHeader   = "X-Minio-Operation"
 	minioConfigTmpFormat = "config-%s.json"
+
+	maxConfigJSONSize = 256 * 1024 // 256KiB
 )
 
 // Type-safe query params.
@@ -978,22 +981,39 @@ func (adminAPI adminAPIHandlers) SetConfigHandler(w http.ResponseWriter, r *http
 	}
 
 	// Read configuration bytes from request body.
-	configBytes, err := ioutil.ReadAll(r.Body)
-	if err != nil {
+	configBuf := make([]byte, maxConfigJSONSize+1)
+	n, err := io.ReadFull(r.Body, configBuf)
+	if err == nil {
+		// More than maxConfigSize bytes were available
+		writeErrorResponse(w, ErrAdminConfigTooLarge, r.URL)
+		return
+	}
+	if err != io.ErrUnexpectedEOF {
 		errorIf(err, "Failed to read config from request body.")
 		writeErrorResponse(w, toAPIErrorCode(err), r.URL)
 		return
 	}
 
+	configBytes := configBuf[:n]
+
+	// Validate JSON provided in the request body: check the
+	// client has not sent JSON objects with duplicate keys.
+	if err = checkDupJSONKeys(string(configBytes)); err != nil {
+		errorIf(err, "config contains duplicate JSON entries.")
+		writeErrorResponse(w, ErrAdminConfigBadJSON, r.URL)
+		return
+	}
+
 	var config serverConfig
 	err = json.Unmarshal(configBytes, &config)
-
 	if err != nil {
-		errorIf(err, "Failed to unmarshal config from request body.")
+		errorIf(err, "Failed to unmarshal JSON configuration", err)
 		writeErrorResponse(w, toAPIErrorCode(err), r.URL)
 		return
 	}
 
+	// If credentials for the server are provided via environment,
+	// then credentials in the provided configuration must match.
 	if globalIsEnvCreds {
 		creds := globalServerConfig.GetCredential()
 		if config.Credential.AccessKey != creds.AccessKey ||

cmd/admin-handlers_test.go

@@ -28,6 +28,7 @@ import (
 	"net/url"
 	"os"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -36,7 +37,8 @@ import (
 	"github.com/minio/minio/pkg/errors"
 )
 
-var configJSON = []byte(`{
+var (
+	configJSON = []byte(`{
 	"version": "13",
 	"credential": {
 		"accessKey": "minio",
@@ -131,6 +133,7 @@ var configJSON = []byte(`{
 		}
 	}
 }`)
+)
 
 // adminXLTestBed - encapsulates subsystems that need to be setup for
 // admin-handler unit tests.
@@ -1257,7 +1260,7 @@ func TestSetConfigHandler(t *testing.T) {
 	req, err := buildAdminRequest(queryVal, "set", http.MethodPut, int64(len(configJSON)),
 		bytes.NewReader(configJSON))
 	if err != nil {
-		t.Fatalf("Failed to construct get-config object request - %v", err)
+		t.Fatalf("Failed to construct set-config object request - %v", err)
 	}
 
 	rec := httptest.NewRecorder()
@@ -1275,6 +1278,44 @@ func TestSetConfigHandler(t *testing.T) {
 	if !result.Status {
 		t.Error("Expected set-config to succeed, but failed")
 	}
+
+	// Check that a very large config file returns an error.
+	{
+		// Make a large enough config string
+		invalidCfg := []byte(strings.Repeat("A", maxConfigJSONSize+1))
+		req, err := buildAdminRequest(queryVal, "set", http.MethodPut, int64(len(invalidCfg)),
+			bytes.NewReader(invalidCfg))
+		if err != nil {
+			t.Fatalf("Failed to construct set-config object request - %v", err)
+		}
+
+		rec := httptest.NewRecorder()
+		adminTestBed.mux.ServeHTTP(rec, req)
+		respBody := string(rec.Body.Bytes())
+		if rec.Code != http.StatusBadRequest ||
+			!strings.Contains(respBody, "Configuration data provided exceeds the allowed maximum of") {
+			t.Errorf("Got unexpected response code or body %d - %s", rec.Code, respBody)
+		}
+	}
+
+	// Check that a config with duplicate keys in an object return
+	// error.
+	{
+		invalidCfg := append(configJSON[:len(configJSON)-1], []byte(`, "version": "15"}`)...)
+		req, err := buildAdminRequest(queryVal, "set", http.MethodPut, int64(len(invalidCfg)),
+			bytes.NewReader(invalidCfg))
+		if err != nil {
+			t.Fatalf("Failed to construct set-config object request - %v", err)
+		}
+
+		rec := httptest.NewRecorder()
+		adminTestBed.mux.ServeHTTP(rec, req)
+		respBody := string(rec.Body.Bytes())
+		if rec.Code != http.StatusBadRequest ||
+			!strings.Contains(respBody, "JSON configuration provided has objects with duplicate keys") {
+			t.Errorf("Got unexpected response code or body %d - %s", rec.Code, respBody)
+		}
+	}
 }
 
 func TestAdminServerInfo(t *testing.T) {

cmd/api-errors.go

@@ -18,6 +18,7 @@ package cmd
 
 import (
 	"encoding/xml"
+	"fmt"
 	"net/http"
 
 	"github.com/minio/minio/pkg/auth"
@@ -175,6 +176,8 @@ const (
 	ErrAdminInvalidAccessKey
 	ErrAdminInvalidSecretKey
 	ErrAdminConfigNoQuorum
+	ErrAdminConfigTooLarge
+	ErrAdminConfigBadJSON
 	ErrAdminCredentialsMismatch
 	ErrInsecureClientRequest
 	ErrObjectTampered
@@ -712,6 +715,17 @@ var errorCodeResponse = map[APIErrorCode]APIError{
 		Description:    "Configuration update failed because server quorum was not met",
 		HTTPStatusCode: http.StatusServiceUnavailable,
 	},
+	ErrAdminConfigTooLarge: {
+		Code: "XMinioAdminConfigTooLarge",
+		Description: fmt.Sprintf("Configuration data provided exceeds the allowed maximum of %d bytes",
+			maxConfigJSONSize),
+		HTTPStatusCode: http.StatusBadRequest,
+	},
+	ErrAdminConfigBadJSON: {
+		Code:           "XMinioAdminConfigBadJSON",
+		Description:    "JSON configuration provided has objects with duplicate keys",
+		HTTPStatusCode: http.StatusBadRequest,
+	},
 	ErrAdminCredentialsMismatch: {
 		Code:           "XMinioAdminCredentialsMismatch",
 		Description:    "Credentials in config mismatch with server environment variables",