From f413224b2405067ab4d407af330001783dc7432f Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Wed, 10 Jan 2018 23:06:36 -0800 Subject: [PATCH] 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 | 28 ++++++++++++++++++++---- cmd/admin-handlers_test.go | 45 ++++++++++++++++++++++++++++++++++++-- cmd/api-errors.go | 14 ++++++++++++ 3 files changed, 81 insertions(+), 6 deletions(-) diff --git a/cmd/admin-handlers.go b/cmd/admin-handlers.go index 2c76a7ffc..1f5bb15bf 100644 --- a/cmd/admin-handlers.go +++ b/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 || diff --git a/cmd/admin-handlers_test.go b/cmd/admin-handlers_test.go index 719a6cd0b..17c3a62ce 100644 --- a/cmd/admin-handlers_test.go +++ b/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) { diff --git a/cmd/api-errors.go b/cmd/api-errors.go index a38682b07..3f2090436 100644 --- a/cmd/api-errors.go +++ b/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",