Browse code

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>

Liron Levin authored on 2016/02/23 19:04:16
Showing 2 changed files
... ...
@@ -54,13 +54,11 @@ type Ctx struct {
54 54
 // AuthZRequest authorized the request to the docker daemon using authZ plugins
55 55
 func (ctx *Ctx) AuthZRequest(w http.ResponseWriter, r *http.Request) error {
56 56
 	var body []byte
57
-	if sendBody(ctx.requestURI, r.Header) {
58
-		if r.ContentLength < maxBodySize {
59
-			var err error
60
-			body, r.Body, err = drainBody(r.Body)
61
-			if err != nil {
62
-				return err
63
-			}
57
+	if sendBody(ctx.requestURI, r.Header) && r.ContentLength > 0 && r.ContentLength < maxBodySize {
58
+		var err error
59
+		body, r.Body, err = drainBody(r.Body)
60
+		if err != nil {
61
+			return err
64 62
 		}
65 63
 	}
66 64
 
... ...
@@ -121,23 +119,23 @@ func (ctx *Ctx) AuthZResponse(rm ResponseModifier, r *http.Request) error {
121 121
 	return nil
122 122
 }
123 123
 
124
-// drainBody dump the body, it reads the body data into memory and
125
-// see go sources /go/src/net/http/httputil/dump.go
124
+// drainBody dump the body (if it's length is less than 1MB) without modifying the request state
126 125
 func drainBody(body io.ReadCloser) ([]byte, io.ReadCloser, error) {
127 126
 	bufReader := bufio.NewReaderSize(body, maxBodySize)
128 127
 	newBody := ioutils.NewReadCloserWrapper(bufReader, func() error { return body.Close() })
129 128
 
130 129
 	data, err := bufReader.Peek(maxBodySize)
131
-	if err != io.EOF {
132
-		// This means the request is larger than our max
133
-		if err == bufio.ErrBufferFull {
134
-			return nil, newBody, nil
135
-		}
136
-		// This means we had an error reading
137
-		return nil, nil, err
130
+	// Body size exceeds max body size
131
+	if err == nil {
132
+		logrus.Warnf("Request body is larger than: '%d' skipping body", maxBodySize)
133
+		return nil, newBody, nil
138 134
 	}
139
-
140
-	return data, newBody, nil
135
+	// Body size is less than maximum size
136
+	if err == io.EOF {
137
+		return data, newBody, nil
138
+	}
139
+	// Unknown error
140
+	return nil, newBody, err
141 141
 }
142 142
 
143 143
 // sendBody returns true when request/response body should be sent to AuthZPlugin
... ...
@@ -148,8 +146,7 @@ func sendBody(url string, header http.Header) bool {
148 148
 	}
149 149
 
150 150
 	// body is sent only for text or json messages
151
-	v := header.Get("Content-Type")
152
-	return strings.HasPrefix(v, "text/") || v == "application/json"
151
+	return header.Get("Content-Type") == "application/json"
153 152
 }
154 153
 
155 154
 // headers returns flatten version of the http headers excluding authorization
... ...
@@ -17,9 +17,11 @@ import (
17 17
 	"reflect"
18 18
 	"testing"
19 19
 
20
+	"bytes"
20 21
 	"github.com/docker/docker/pkg/plugins"
21 22
 	"github.com/docker/go-connections/tlsconfig"
22 23
 	"github.com/gorilla/mux"
24
+	"strings"
23 25
 )
24 26
 
25 27
 const pluginAddress = "authzplugin.sock"
... ...
@@ -135,6 +137,41 @@ func TestResponseModifier(t *testing.T) {
135 135
 	}
136 136
 }
137 137
 
138
+func TestDrainBody(t *testing.T) {
139
+
140
+	tests := []struct {
141
+		length             int // length is the message length send to drainBody
142
+		expectedBodyLength int // expectedBodyLength is the expected body length after drainBody is called
143
+	}{
144
+		{10, 10}, // Small message size
145
+		{maxBodySize - 1, maxBodySize - 1}, // Max message size
146
+		{maxBodySize * 2, 0},               // Large message size (skip copying body)
147
+
148
+	}
149
+
150
+	for _, test := range tests {
151
+
152
+		msg := strings.Repeat("a", test.length)
153
+		body, closer, err := drainBody(ioutil.NopCloser(bytes.NewReader([]byte(msg))))
154
+		if len(body) != test.expectedBodyLength {
155
+			t.Fatalf("Body must be copied, actual length: '%d'", len(body))
156
+		}
157
+		if closer == nil {
158
+			t.Fatalf("Closer must not be nil")
159
+		}
160
+		if err != nil {
161
+			t.Fatalf("Error must not be nil: '%v'", err)
162
+		}
163
+		modified, err := ioutil.ReadAll(closer)
164
+		if err != nil {
165
+			t.Fatalf("Error must not be nil: '%v'", err)
166
+		}
167
+		if len(modified) != len(msg) {
168
+			t.Fatalf("Result should not be truncated. Original length: '%d', new length: '%d'", len(msg), len(modified))
169
+		}
170
+	}
171
+}
172
+
138 173
 func TestResponseModifierOverride(t *testing.T) {
139 174
 	r := httptest.NewRecorder()
140 175
 	m := NewResponseModifier(r)