diff options
author | Vladimir Shushlin <v.shushlin@gmail.com> | 2021-11-10 18:38:22 +0300 |
---|---|---|
committer | Vladimir Shushlin <v.shushlin@gmail.com> | 2021-11-11 11:42:42 +0300 |
commit | bf9c79a5477b61f375be659e2e16f377067d9c00 (patch) | |
tree | fbd7c2ceece4af9fc87e45c43679a725015e7588 | |
parent | aa897ce9849d35cd7ff1121351f1033e91d0c062 (diff) |
fix: reject requests with very long URIs
Some parts of the application may be vulnerable to very long URIs being passed.
E.g. Auth will try to save URI to session cookie, and it will fails, which will result in 500 error
Changelog: fixed
-rw-r--r-- | app.go | 7 | ||||
-rw-r--r-- | internal/config/config.go | 4 | ||||
-rw-r--r-- | internal/config/flags.go | 1 | ||||
-rw-r--r-- | internal/httperrors/httperrors.go | 13 | ||||
-rw-r--r-- | internal/httperrors/httperrors_test.go | 12 | ||||
-rw-r--r-- | internal/urilimiter/urilimiter.go | 23 | ||||
-rw-r--r-- | internal/urilimiter/urilimiter_test.go | 70 | ||||
-rw-r--r-- | test/acceptance/urilimiter_test.go | 56 |
8 files changed, 183 insertions, 3 deletions
@@ -39,6 +39,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/zip" "gitlab.com/gitlab-org/gitlab-pages/internal/source" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab" + "gitlab.com/gitlab-org/gitlab-pages/internal/urilimiter" "gitlab.com/gitlab-org/gitlab-pages/metrics" ) @@ -292,10 +293,10 @@ func (a *theApp) buildHandlerPipeline() (http.Handler, error) { handler = handlePanicMiddleware(handler) handler = correlation.InjectCorrelationID(handler, correlationOpts...) - // This MUST be the last handler! - // This handler blocks unknown HTTP methods, - // being the last means it will be evaluated first + // These middlewares MUST be added in the end. + // Being last means they will be evaluated first // preventing any operation on bogus requests. + handler = urilimiter.NewMiddleware(handler, a.config.General.MaxURILength) handler = rejectmethods.NewMiddleware(handler) return handler, nil diff --git a/internal/config/config.go b/internal/config/config.go index 3e03f7d6..c6f91db0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -47,6 +47,7 @@ type Config struct { type General struct { Domain string MaxConns int + MaxURILength int MetricsAddress string RedirectHTTP bool RootCertificate []byte @@ -181,6 +182,7 @@ func loadConfig() (*Config, error) { General: General{ Domain: strings.ToLower(*pagesDomain), MaxConns: *maxConns, + MaxURILength: *maxURILength, MetricsAddress: *metricsAddress, RedirectHTTP: *redirectHTTP, RootDir: *pagesRoot, @@ -307,6 +309,8 @@ func LogConfig(config *Config) { "enable-disk": config.GitLab.EnableDisk, "auth-redirect-uri": config.Authentication.RedirectURI, "auth-scope": config.Authentication.Scope, + "max-conns": config.General.MaxConns, + "max-uri-length": config.General.MaxURILength, "zip-cache-expiration": config.Zip.ExpirationInterval, "zip-cache-cleanup": config.Zip.CleanupInterval, "zip-cache-refresh": config.Zip.RefreshInterval, diff --git a/internal/config/flags.go b/internal/config/flags.go index c61447c7..6c9bd4a6 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -51,6 +51,7 @@ var ( redirectURI = flag.String("auth-redirect-uri", "", "GitLab application redirect URI") authScope = flag.String("auth-scope", "api", "Scope to be used for authentication (must match GitLab Pages OAuth application settings)") maxConns = flag.Int("max-conns", 0, "Limit on the number of concurrent connections to the HTTP, HTTPS or proxy listeners, 0 for no limit") + maxURILength = flag.Int("max-uri-length", 1024, "Limit the length of URI, 0 for unlimited.") insecureCiphers = flag.Bool("insecure-ciphers", false, "Use default list of cipher suites, may contain insecure ones like 3DES and RC4") tlsMinVersion = flag.String("tls-min-version", "tls1.2", tls.FlagUsage("min")) tlsMaxVersion = flag.String("tls-max-version", "", tls.FlagUsage("max")) diff --git a/internal/httperrors/httperrors.go b/internal/httperrors/httperrors.go index 8e61d590..a96e4d85 100644 --- a/internal/httperrors/httperrors.go +++ b/internal/httperrors/httperrors.go @@ -34,6 +34,14 @@ var ( <p>Make sure the address is correct and that the page hasn't moved.</p> <p>Please contact your GitLab administrator if you think this is a mistake.</p>`, } + content414 = content{ + status: http.StatusRequestURITooLong, + title: "Request URI Too Long (414)", + statusString: "414", + header: "Request URI Too Long.", + subHeader: `<p>The URI provided was too long for the server to process.</p> + <p>Try to make the request URI shorter.</p>`, + } content429 = content{ http.StatusTooManyRequests, @@ -184,6 +192,11 @@ func Serve404(w http.ResponseWriter) { serveErrorPage(w, content404) } +// Serve414 returns a 414 error response / HTML page to the http.ResponseWriter +func Serve414(w http.ResponseWriter) { + serveErrorPage(w, content414) +} + // Serve429 returns a 429 error response / HTML page to the http.ResponseWriter func Serve429(w http.ResponseWriter) { serveErrorPage(w, content429) diff --git a/internal/httperrors/httperrors_test.go b/internal/httperrors/httperrors_test.go index b003ce6f..671890f8 100644 --- a/internal/httperrors/httperrors_test.go +++ b/internal/httperrors/httperrors_test.go @@ -92,6 +92,18 @@ func TestServe404(t *testing.T) { require.Contains(t, w.Content(), content404.subHeader) } +func TestServe414(t *testing.T) { + w := newTestResponseWriter(httptest.NewRecorder()) + Serve414(w) + require.Equal(t, w.Header().Get("Content-Type"), "text/html; charset=utf-8") + require.Equal(t, w.Header().Get("X-Content-Type-Options"), "nosniff") + require.Equal(t, w.Status(), content414.status) + require.Contains(t, w.Content(), content414.title) + require.Contains(t, w.Content(), content414.statusString) + require.Contains(t, w.Content(), content414.header) + require.Contains(t, w.Content(), content414.subHeader) +} + func TestServe500(t *testing.T) { w := newTestResponseWriter(httptest.NewRecorder()) Serve500(w) diff --git a/internal/urilimiter/urilimiter.go b/internal/urilimiter/urilimiter.go new file mode 100644 index 00000000..65d664a5 --- /dev/null +++ b/internal/urilimiter/urilimiter.go @@ -0,0 +1,23 @@ +package urilimiter + +import ( + "net/http" + + "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" +) + +func NewMiddleware(handler http.Handler, limit int) http.Handler { + if limit == 0 { + return handler + } + + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if len(r.RequestURI) > limit { + httperrors.Serve414(w) + + return + } + + handler.ServeHTTP(w, r) + }) +} diff --git a/internal/urilimiter/urilimiter_test.go b/internal/urilimiter/urilimiter_test.go new file mode 100644 index 00000000..0ac89ea0 --- /dev/null +++ b/internal/urilimiter/urilimiter_test.go @@ -0,0 +1,70 @@ +package urilimiter + +import ( + "fmt" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestNewMiddleware(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, "hello") + }) + + tests := map[string]struct { + limit int + url string + expectedStatus int + }{ + "with_disabled_middleware": { + limit: 0, + url: "/index.html", + expectedStatus: http.StatusOK, + }, + "with_limit_set_to_request_length": { + limit: 17, + url: "/index.html?q=a#b", + expectedStatus: http.StatusOK, + }, + "with_uri_length_exceeding_the_limit": { + limit: 17, + url: "/index1.html?q=a#b", + expectedStatus: http.StatusRequestURITooLong, + }, + "with_uri_length_exceeding_the_limit_with_query": { + limit: 17, + url: "/index.html?q=aa#b", + expectedStatus: http.StatusRequestURITooLong, + }, + "with_uri_length_exceeding_the_limit_with_fragment": { + limit: 17, + url: "/index.html?q=a#bb", + expectedStatus: http.StatusRequestURITooLong, + }, + } + for tn, tt := range tests { + t.Run(tn, func(t *testing.T) { + middleware := NewMiddleware(handler, tt.limit) + + ww := httptest.NewRecorder() + rr := httptest.NewRequest(http.MethodGet, tt.url, nil) + + middleware.ServeHTTP(ww, rr) + + res := ww.Result() + defer res.Body.Close() + + require.Equal(t, tt.expectedStatus, res.StatusCode) + if tt.expectedStatus == http.StatusOK { + b, err := io.ReadAll(res.Body) + require.NoError(t, err) + + require.Equal(t, "hello", string(b)) + } + }) + } +} diff --git a/test/acceptance/urilimiter_test.go b/test/acceptance/urilimiter_test.go new file mode 100644 index 00000000..5e97921f --- /dev/null +++ b/test/acceptance/urilimiter_test.go @@ -0,0 +1,56 @@ +package acceptance_test + +import ( + "io" + "net/http" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestURILimits proves fix for https://gitlab.com/gitlab-org/gitlab-pages/-/issues/659 +func TestURILimits(t *testing.T) { + tests := map[string]struct { + limit string + path string + expectedStatus int + }{ + "with_disabled_limit": { + limit: "0", + path: "project/", + expectedStatus: http.StatusOK, + }, + "with_limit_set_to_request_length": { + limit: "19", + path: "/project/index.html", + expectedStatus: http.StatusOK, + }, + "with_uri_length_exceeding_the_limit": { + limit: "19", + path: "/project/index1.html", + expectedStatus: http.StatusRequestURITooLong, + }, + } + + for tn, tt := range tests { + t.Run(tn, func(t *testing.T) { + RunPagesProcess(t, withListeners([]ListenSpec{httpsListener}), withExtraArgument("max-uri-length", tt.limit)) + + rsp, err := GetPageFromListener(t, httpsListener, "group.gitlab-example.com", tt.path) + require.NoError(t, err) + defer func() { + require.NoError(t, rsp.Body.Close()) + }() + + require.Equal(t, tt.expectedStatus, rsp.StatusCode) + + b, err := io.ReadAll(rsp.Body) + require.NoError(t, err) + if tt.expectedStatus == http.StatusOK { + require.Equal(t, "project-subdir\n", string(b)) + } else { + require.Contains(t, string(b), "Request URI Too Long") + } + }) + } +} |