From 2cd03a5fdedcc5d347754df0bf662cc2e9433db1 Mon Sep 17 00:00:00 2001 From: Jameson Hyde Date: Mon, 26 Nov 2018 14:15:22 -0500 Subject: [PATCH 1/2] Authz plugin security fixes for 0-length content and path validation Signed-off-by: Jameson Hyde MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix comments (cherry picked from commit 9659c3a52bac57e615b5fb49b0652baca448643e) Signed-off-by: Paweł Gronowski (cherry picked from commit 2ac8a479c53d9b8e67c55f1e283da9d85d2b3415) Signed-off-by: Paweł Gronowski --- pkg/authorization/authz.go | 38 ++++++++++++++++++--- pkg/authorization/authz_unix_test.go | 49 ++++++++++++++++++++++++++-- 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/pkg/authorization/authz.go b/pkg/authorization/authz.go index a1edbcd89d..dafbc95ffb 100644 --- a/pkg/authorization/authz.go +++ b/pkg/authorization/authz.go @@ -7,6 +7,8 @@ import ( "io" "mime" "net/http" + "net/url" + "regexp" "strings" "github.com/docker/docker/pkg/ioutils" @@ -52,10 +54,23 @@ type Ctx struct { authReq *Request } +func isChunked(r *http.Request) bool { + // RFC 7230 specifies that content length is to be ignored if Transfer-Encoding is chunked + if strings.ToLower(r.Header.Get("Transfer-Encoding")) == "chunked" { + return true + } + for _, v := range r.TransferEncoding { + if 0 == strings.Compare(strings.ToLower(v), "chunked") { + return true + } + } + return false +} + // 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) && r.ContentLength > 0 && r.ContentLength < maxBodySize { + if sendBody(ctx.requestURI, r.Header) && (r.ContentLength > 0 || isChunked(r)) && r.ContentLength < maxBodySize { var err error body, r.Body, err = drainBody(r.Body) if err != nil { @@ -108,7 +123,6 @@ func (ctx *Ctx) AuthZResponse(rm ResponseModifier, r *http.Request) error { if sendBody(ctx.requestURI, rm.Header()) { ctx.authReq.ResponseBody = rm.RawBody() } - for _, plugin := range ctx.plugins { logrus.Debugf("AuthZ response using plugin %s", plugin.Name()) @@ -146,10 +160,26 @@ func drainBody(body io.ReadCloser) ([]byte, io.ReadCloser, error) { return nil, newBody, err } +func isAuthEndpoint(urlPath string) (bool, error) { + // eg www.test.com/v1.24/auth/optional?optional1=something&optional2=something (version optional) + matched, err := regexp.MatchString(`^[^\/]+\/(v\d[\d\.]*\/)?auth.*`, urlPath) + if err != nil { + return false, err + } + return matched, nil +} + // sendBody returns true when request/response body should be sent to AuthZPlugin -func sendBody(url string, header http.Header) bool { +func sendBody(inURL string, header http.Header) bool { + u, err := url.Parse(inURL) + // Assume no if the URL cannot be parsed - an empty request will still be forwarded to the plugin and should be rejected + if err != nil { + return false + } + // Skip body for auth endpoint - if strings.HasSuffix(url, "/auth") { + isAuth, err := isAuthEndpoint(u.Path) + if isAuth || err != nil { return false } diff --git a/pkg/authorization/authz_unix_test.go b/pkg/authorization/authz_unix_test.go index c38b5e1566..86ad29171b 100644 --- a/pkg/authorization/authz_unix_test.go +++ b/pkg/authorization/authz_unix_test.go @@ -174,8 +174,8 @@ func TestDrainBody(t *testing.T) { func TestSendBody(t *testing.T) { var ( - url = "nothing.com" testcases = []struct { + url string contentType string expected bool }{ @@ -219,15 +219,58 @@ func TestSendBody(t *testing.T) { contentType: "", expected: false, }, + { + url: "nothing.com/auth", + contentType: "", + expected: false, + }, + { + url: "nothing.com/auth", + contentType: "application/json;charset=UTF8", + expected: false, + }, + { + url: "nothing.com/auth?p1=test", + contentType: "application/json;charset=UTF8", + expected: false, + }, + { + url: "nothing.com/test?p1=/auth", + contentType: "application/json;charset=UTF8", + expected: true, + }, + { + url: "nothing.com/something/auth", + contentType: "application/json;charset=UTF8", + expected: true, + }, + { + url: "nothing.com/auth/test", + contentType: "application/json;charset=UTF8", + expected: false, + }, + { + url: "nothing.com/v1.24/auth/test", + contentType: "application/json;charset=UTF8", + expected: false, + }, + { + url: "nothing.com/v1/auth/test", + contentType: "application/json;charset=UTF8", + expected: false, + }, } ) for _, testcase := range testcases { header := http.Header{} header.Set("Content-Type", testcase.contentType) + if testcase.url == "" { + testcase.url = "nothing.com" + } - if b := sendBody(url, header); b != testcase.expected { - t.Fatalf("Unexpected Content-Type; Expected: %t, Actual: %t", testcase.expected, b) + if b := sendBody(testcase.url, header); b != testcase.expected { + t.Fatalf("sendBody failed: url: %s, content-type: %s; Expected: %t, Actual: %t", testcase.url, testcase.contentType, testcase.expected, b) } } } From eaa196855e1cec8f5e03919374e983ac412be668 Mon Sep 17 00:00:00 2001 From: Jameson Hyde Date: Sat, 1 Dec 2018 11:25:49 -0500 Subject: [PATCH 2/2] If url includes scheme, urlPath will drop hostname, which would not match the auth check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jameson Hyde (cherry picked from commit 754fb8d9d03895ae3ab60d2ad778152b0d835206) Signed-off-by: Paweł Gronowski (cherry picked from commit 5282cb25d09db924fa92fdb8fb7a9bd20bb845b7) Signed-off-by: Paweł Gronowski --- pkg/authorization/authz.go | 6 ++--- pkg/authorization/authz_unix_test.go | 35 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/pkg/authorization/authz.go b/pkg/authorization/authz.go index dafbc95ffb..85c3a86cd6 100644 --- a/pkg/authorization/authz.go +++ b/pkg/authorization/authz.go @@ -56,11 +56,11 @@ type Ctx struct { func isChunked(r *http.Request) bool { // RFC 7230 specifies that content length is to be ignored if Transfer-Encoding is chunked - if strings.ToLower(r.Header.Get("Transfer-Encoding")) == "chunked" { + if strings.EqualFold(r.Header.Get("Transfer-Encoding"), "chunked") { return true } for _, v := range r.TransferEncoding { - if 0 == strings.Compare(strings.ToLower(v), "chunked") { + if strings.EqualFold(v, "chunked") { return true } } @@ -162,7 +162,7 @@ func drainBody(body io.ReadCloser) ([]byte, io.ReadCloser, error) { func isAuthEndpoint(urlPath string) (bool, error) { // eg www.test.com/v1.24/auth/optional?optional1=something&optional2=something (version optional) - matched, err := regexp.MatchString(`^[^\/]+\/(v\d[\d\.]*\/)?auth.*`, urlPath) + matched, err := regexp.MatchString(`^[^\/]*\/(v\d[\d\.]*\/)?auth.*`, urlPath) if err != nil { return false, err } diff --git a/pkg/authorization/authz_unix_test.go b/pkg/authorization/authz_unix_test.go index 86ad29171b..647ad793d3 100644 --- a/pkg/authorization/authz_unix_test.go +++ b/pkg/authorization/authz_unix_test.go @@ -259,6 +259,41 @@ func TestSendBody(t *testing.T) { contentType: "application/json;charset=UTF8", expected: false, }, + { + url: "www.nothing.com/v1.24/auth/test", + contentType: "application/json;charset=UTF8", + expected: false, + }, + { + url: "https://www.nothing.com/v1.24/auth/test", + contentType: "application/json;charset=UTF8", + expected: false, + }, + { + url: "http://nothing.com/v1.24/auth/test", + contentType: "application/json;charset=UTF8", + expected: false, + }, + { + url: "www.nothing.com/test?p1=/auth", + contentType: "application/json;charset=UTF8", + expected: true, + }, + { + url: "http://www.nothing.com/test?p1=/auth", + contentType: "application/json;charset=UTF8", + expected: true, + }, + { + url: "www.nothing.com/something/auth", + contentType: "application/json;charset=UTF8", + expected: true, + }, + { + url: "https://www.nothing.com/something/auth", + contentType: "application/json;charset=UTF8", + expected: true, + }, } )