Fix #20508 - Authz plugin enabled with large text/JSON POST payload corrupts body

Based on the discussion, we have changed the following:

1. Send body only if content-type is application/json (based on the
Docker official daemon REST specification, this is the provided for all
APIs that requires authorization.

2. Correctly verify that the msg body is smaller than max cap (this was
the actual bug). Fix includes UT.

3. Minor: Check content length > 0 (it was -1 for load, altough an
attacker can still modify this)

Signed-off-by: Liron Levin <liron@twistlock.com>

authorization/authz.go

@@ -54,13 +54,11 @@ type Ctx struct {
 // AuthZRequest authorized the request to the docker daemon using authZ plugins
 func (ctx *Ctx) AuthZRequest(w http.ResponseWriter, r *http.Request) error {
 	var body []byte
-	if sendBody(ctx.requestURI, r.Header) {
+	if sendBody(ctx.requestURI, r.Header) && r.ContentLength > 0 && r.ContentLength < maxBodySize {
-		if r.ContentLength < maxBodySize {
+		var err error
-			var err error
+		body, r.Body, err = drainBody(r.Body)
-			body, r.Body, err = drainBody(r.Body)
+		if err != nil {
-			if err != nil {
+			return err
-				return err
-			}
 		}
 	}
 
@@ -121,23 +119,23 @@ func (ctx *Ctx) AuthZResponse(rm ResponseModifier, r *http.Request) error {
 	return nil
 }
 
-// drainBody dump the body, it reads the body data into memory and
+// drainBody dump the body (if it's length is less than 1MB) without modifying the request state
-// see go sources /go/src/net/http/httputil/dump.go
 func drainBody(body io.ReadCloser) ([]byte, io.ReadCloser, error) {
 	bufReader := bufio.NewReaderSize(body, maxBodySize)
 	newBody := ioutils.NewReadCloserWrapper(bufReader, func() error { return body.Close() })
 
 	data, err := bufReader.Peek(maxBodySize)
-	if err != io.EOF {
+	// Body size exceeds max body size
-		// This means the request is larger than our max
+	if err == nil {
-		if err == bufio.ErrBufferFull {
+		logrus.Warnf("Request body is larger than: '%d' skipping body", maxBodySize)
-			return nil, newBody, nil
+		return nil, newBody, nil
-		}
-		// This means we had an error reading
-		return nil, nil, err
 	}
-
+	// Body size is less than maximum size
-	return data, newBody, nil
+	if err == io.EOF {
+		return data, newBody, nil
+	}
+	// Unknown error
+	return nil, newBody, err
 }
 
 // sendBody returns true when request/response body should be sent to AuthZPlugin
@@ -148,8 +146,7 @@ func sendBody(url string, header http.Header) bool {
 	}
 
 	// body is sent only for text or json messages
-	v := header.Get("Content-Type")
+	return header.Get("Content-Type") == "application/json"
-	return strings.HasPrefix(v, "text/") || v == "application/json"
 }
 
 // headers returns flatten version of the http headers excluding authorization

authorization/authz_unix_test.go

@@ -17,9 +17,11 @@ import (
 	"reflect"
 	"testing"
 
+	"bytes"
 	"github.com/docker/docker/pkg/plugins"
 	"github.com/docker/go-connections/tlsconfig"
 	"github.com/gorilla/mux"
+	"strings"
 )
 
 const pluginAddress = "authzplugin.sock"
@@ -135,6 +137,41 @@ func TestResponseModifier(t *testing.T) {
 	}
 }
 
+func TestDrainBody(t *testing.T) {
+
+	tests := []struct {
+		length             int // length is the message length send to drainBody
+		expectedBodyLength int // expectedBodyLength is the expected body length after drainBody is called
+	}{
+		{10, 10}, // Small message size
+		{maxBodySize - 1, maxBodySize - 1}, // Max message size
+		{maxBodySize * 2, 0},               // Large message size (skip copying body)
+
+	}
+
+	for _, test := range tests {
+
+		msg := strings.Repeat("a", test.length)
+		body, closer, err := drainBody(ioutil.NopCloser(bytes.NewReader([]byte(msg))))
+		if len(body) != test.expectedBodyLength {
+			t.Fatalf("Body must be copied, actual length: '%d'", len(body))
+		}
+		if closer == nil {
+			t.Fatalf("Closer must not be nil")
+		}
+		if err != nil {
+			t.Fatalf("Error must not be nil: '%v'", err)
+		}
+		modified, err := ioutil.ReadAll(closer)
+		if err != nil {
+			t.Fatalf("Error must not be nil: '%v'", err)
+		}
+		if len(modified) != len(msg) {
+			t.Fatalf("Result should not be truncated. Original length: '%d', new length: '%d'", len(msg), len(modified))
+		}
+	}
+}
+
 func TestResponseModifierOverride(t *testing.T) {
 	r := httptest.NewRecorder()
 	m := NewResponseModifier(r)