From 4ad2a9c1fa2808b0bb9b33b5d6278271ed5c7698 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Thu, 22 Sep 2022 20:41:28 +0200 Subject: [PATCH] WebClient: validate PDF files before rendering Signed-off-by: Nicola Murino --- internal/httpd/httpd.go | 3 + internal/httpd/httpd_test.go | 97 +++++++++++++++++++++++++++++++++ internal/httpd/internal_test.go | 7 +++ internal/httpd/server.go | 3 +- internal/httpd/webclient.go | 79 +++++++++++++++++++++++++-- 5 files changed, 183 insertions(+), 6 deletions(-) diff --git a/internal/httpd/httpd.go b/internal/httpd/httpd.go index 27d4fc35..2ad26aea 100644 --- a/internal/httpd/httpd.go +++ b/internal/httpd/httpd.go @@ -159,6 +159,7 @@ const ( webClientForgotPwdPathDefault = "/web/client/forgot-password" webClientResetPwdPathDefault = "/web/client/reset-password" webClientViewPDFPathDefault = "/web/client/viewpdf" + webClientGetPDFPathDefault = "/web/client/getpdf" webStaticFilesPathDefault = "/static" webOpenAPIPathDefault = "/openapi" // MaxRestoreSize defines the max size for the loaddata input file @@ -244,6 +245,7 @@ var ( webClientForgotPwdPath string webClientResetPwdPath string webClientViewPDFPath string + webClientGetPDFPath string webStaticFilesPath string webOpenAPIPath string // max upload size for http clients, 1GB by default @@ -967,6 +969,7 @@ func updateWebClientURLs(baseURL string) { webClientForgotPwdPath = path.Join(baseURL, webClientForgotPwdPathDefault) webClientResetPwdPath = path.Join(baseURL, webClientResetPwdPathDefault) webClientViewPDFPath = path.Join(baseURL, webClientViewPDFPathDefault) + webClientGetPDFPath = path.Join(baseURL, webClientGetPDFPathDefault) } func updateWebAdminURLs(baseURL string) { diff --git a/internal/httpd/httpd_test.go b/internal/httpd/httpd_test.go index 2f9c8044..aae3c5af 100644 --- a/internal/httpd/httpd_test.go +++ b/internal/httpd/httpd_test.go @@ -174,6 +174,7 @@ const ( webClientForgotPwdPath = "/web/client/forgot-password" webClientResetPwdPath = "/web/client/reset-password" webClientViewPDFPath = "/web/client/viewpdf" + webClientGetPDFPath = "/web/client/getpdf" webAdminEventRulesPath = "/web/admin/eventrules" webAdminEventRulePath = "/web/admin/eventrule" webAdminEventActionsPath = "/web/admin/eventactions" @@ -10941,6 +10942,13 @@ func TestMaxSessions(t *testing.T) { checkResponseCode(t, http.StatusTooManyRequests, rr) assert.Contains(t, rr.Body.String(), "too many open sessions") + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath+"?path=file", nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusTooManyRequests, rr) + assert.Contains(t, rr.Body.String(), "too many open sessions") + // test reset password smtpCfg := smtp.Config{ Host: "127.0.0.1", @@ -12960,16 +12968,105 @@ func TestWebClientViewPDF(t *testing.T) { rr := executeRequest(req) checkResponseCode(t, http.StatusBadRequest, rr) + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath, nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusBadRequest, rr) + req, err = http.NewRequest(http.MethodGet, webClientViewPDFPath+"?path=test.pdf", nil) assert.NoError(t, err) setJWTCookieForReq(req, webToken) rr = executeRequest(req) checkResponseCode(t, http.StatusOK, rr) + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath+"?path=test.pdf", nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusBadRequest, rr) + assert.Contains(t, rr.Body.String(), "Unable to get file") + + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath+"?path=%2F", nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusBadRequest, rr) + assert.Contains(t, rr.Body.String(), "Invalid file") + + err = os.WriteFile(filepath.Join(user.GetHomeDir(), "test.pdf"), []byte("some text data"), 0666) + assert.NoError(t, err) + + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath+"?path=%2Ftest.pdf", nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusBadRequest, rr) + assert.Contains(t, rr.Body.String(), "Invalid PDF file") + + err = createTestFile(filepath.Join(user.GetHomeDir(), "test.pdf"), 1024) + assert.NoError(t, err) + + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath+"?path=%2Ftest.pdf", nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusBadRequest, rr) + assert.Contains(t, rr.Body.String(), "does not look like a PDF") + + fakePDF := []byte(`%PDF-1.6`) + for i := 0; i < 128; i++ { + fakePDF = append(fakePDF, []byte(fmt.Sprintf("%d", i))...) + } + err = os.WriteFile(filepath.Join(user.GetHomeDir(), "test.pdf"), fakePDF, 0666) + assert.NoError(t, err) + + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath+"?path=%2Ftest.pdf", nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusOK, rr) + + user.Filters.FilePatterns = []sdk.PatternsFilter{ + { + Path: "/", + DeniedPatterns: []string{"*.pdf"}, + }, + } + _, _, err = httpdtest.UpdateUser(user, http.StatusOK, "") + assert.NoError(t, err) + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath+"?path=%2Ftest.pdf", nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusForbidden, rr) + assert.Contains(t, rr.Body.String(), "Unable to get a reader for the file") + + user.Filters.FilePatterns = []sdk.PatternsFilter{ + { + Path: "/", + DeniedPatterns: []string{"*.txt"}, + }, + } + user.Filters.DeniedProtocols = []string{common.ProtocolHTTP} + _, _, err = httpdtest.UpdateUser(user, http.StatusOK, "") + assert.NoError(t, err) + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath+"?path=%2Ftest.pdf", nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusForbidden, rr) + _, err = httpdtest.RemoveUser(user, http.StatusOK) assert.NoError(t, err) err = os.RemoveAll(user.GetHomeDir()) assert.NoError(t, err) + + req, err = http.NewRequest(http.MethodGet, webClientGetPDFPath+"?path=%2Ftest.pdf", nil) + assert.NoError(t, err) + setJWTCookieForReq(req, webToken) + rr = executeRequest(req) + checkResponseCode(t, http.StatusNotFound, rr) } func TestWebEditFile(t *testing.T) { diff --git a/internal/httpd/internal_test.go b/internal/httpd/internal_test.go index 9052820d..4f5fff39 100644 --- a/internal/httpd/internal_test.go +++ b/internal/httpd/internal_test.go @@ -2231,6 +2231,13 @@ func TestWebUserInvalidClaims(t *testing.T) { server.handleClientGetShares(rr, req) assert.Equal(t, http.StatusForbidden, rr.Code) assert.Contains(t, rr.Body.String(), "Invalid token claims") + + rr = httptest.NewRecorder() + req, _ = http.NewRequest(http.MethodGet, webClientViewPDFPath, nil) + req.Header.Set("Cookie", fmt.Sprintf("jwt=%v", token["access_token"])) + server.handleClientGetPDF(rr, req) + assert.Equal(t, http.StatusForbidden, rr.Code) + assert.Contains(t, rr.Body.String(), "Invalid token claims") } func TestInvalidClaims(t *testing.T) { diff --git a/internal/httpd/server.go b/internal/httpd/server.go index f4d889d5..117501ab 100644 --- a/internal/httpd/server.go +++ b/internal/httpd/server.go @@ -1427,7 +1427,7 @@ func (s *httpdServer) setupWebClientRoutes() { s.jwtAuthenticatorPartial(tokenAudienceWebClientPartial)). Post(webClientTwoFactorRecoveryPath, s.handleWebClientTwoFactorRecoveryPost) } - // share API exposed to external users + // share routes exposed to external users s.router.Get(webClientPubSharesPath+"/{id}", s.downloadFromShare) s.router.Get(webClientPubSharesPath+"/{id}/partial", s.handleClientSharePartialDownload) s.router.Get(webClientPubSharesPath+"/{id}/browse", s.handleShareGetFiles) @@ -1446,6 +1446,7 @@ func (s *httpdServer) setupWebClientRoutes() { router.Get(webClientLogoutPath, s.handleWebClientLogout) router.With(s.checkSecondFactorRequirement, s.refreshCookie).Get(webClientFilesPath, s.handleClientGetFiles) router.With(s.checkSecondFactorRequirement, s.refreshCookie).Get(webClientViewPDFPath, s.handleClientViewPDF) + router.With(s.checkSecondFactorRequirement, s.refreshCookie).Get(webClientGetPDFPath, s.handleClientGetPDF) router.With(s.checkSecondFactorRequirement, s.refreshCookie, verifyCSRFHeader).Get(webClientFilePath, getUserFile) router.With(s.checkSecondFactorRequirement, s.checkHTTPUserPerm(sdk.WebClientWriteDisabled), verifyCSRFHeader). Post(webClientFilePath, uploadUserFile) diff --git a/internal/httpd/webclient.go b/internal/httpd/webclient.go index 263f95e6..56cb5687 100644 --- a/internal/httpd/webclient.go +++ b/internal/httpd/webclient.go @@ -803,9 +803,8 @@ func (s *httpdServer) handleShareGetFiles(w http.ResponseWriter, r *http.Request s.renderSharedFilesPage(w, r, share.GetRelativePath(name), "", share) return } - inline := r.URL.Query().Get("inline") != "" dataprovider.UpdateShareLastUse(&share, 1) //nolint:errcheck - if status, err := downloadFile(w, r, connection, name, info, inline, &share); err != nil { + if status, err := downloadFile(w, r, connection, name, info, false, &share); err != nil { dataprovider.UpdateShareLastUse(&share, -1) //nolint:errcheck if status > 0 { s.renderSharedFilesPage(w, r, path.Dir(share.GetRelativePath(name)), err.Error(), share) @@ -938,8 +937,7 @@ func (s *httpdServer) handleClientGetFiles(w http.ResponseWriter, r *http.Reques s.renderFilesPage(w, r, name, "", user, len(s.binding.WebClientIntegrations) > 0) return } - inline := r.URL.Query().Get("inline") != "" - if status, err := downloadFile(w, r, connection, name, info, inline, nil); err != nil && status != 0 { + if status, err := downloadFile(w, r, connection, name, info, false, nil); err != nil && status != 0 { if status > 0 { if status == http.StatusRequestedRangeNotSatisfiable { s.renderClientMessagePage(w, r, http.StatusText(status), "", status, err, "") @@ -1350,9 +1348,80 @@ func (s *httpdServer) handleClientViewPDF(w http.ResponseWriter, r *http.Request name = util.CleanPath(name) data := viewPDFPage{ Title: path.Base(name), - URL: fmt.Sprintf("%v?path=%v&inline=1", webClientFilesPath, url.QueryEscape(name)), + URL: fmt.Sprintf("%s?path=%s&_=%d", webClientGetPDFPath, url.QueryEscape(name), time.Now().UTC().Unix()), StaticURL: webStaticFilesPath, Branding: s.binding.Branding.WebClient, } renderClientTemplate(w, templateClientViewPDF, data) } + +func (s *httpdServer) handleClientGetPDF(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, maxLoginBodySize) + claims, err := getTokenClaims(r) + if err != nil || claims.Username == "" { + s.renderClientForbiddenPage(w, r, "Invalid token claims") + return + } + name := r.URL.Query().Get("path") + if name == "" { + s.renderClientBadRequestPage(w, r, errors.New("no file specified")) + return + } + name = util.CleanPath(name) + user, err := dataprovider.GetUserWithGroupSettings(claims.Username) + if err != nil { + s.renderClientMessagePage(w, r, "Unable to retrieve your user", "", getRespStatus(err), nil, "") + return + } + + connID := xid.New().String() + protocol := getProtocolFromRequest(r) + connectionID := fmt.Sprintf("%v_%v", protocol, connID) + if err := checkHTTPClientUser(&user, r, connectionID, false); err != nil { + s.renderClientForbiddenPage(w, r, err.Error()) + return + } + connection := &Connection{ + BaseConnection: common.NewBaseConnection(connID, protocol, util.GetHTTPLocalAddress(r), + r.RemoteAddr, user), + request: r, + } + if err = common.Connections.Add(connection); err != nil { + s.renderClientMessagePage(w, r, "Unable to add connection", "", http.StatusTooManyRequests, err, "") + return + } + defer common.Connections.Remove(connection.GetID()) + + info, err := connection.Stat(name, 0) + if err != nil { + s.renderClientMessagePage(w, r, "Unable to get file", "", getRespStatus(err), err, "") + return + } + if info.IsDir() { + s.renderClientMessagePage(w, r, "Invalid file", fmt.Sprintf("%q is not a file", name), + http.StatusBadRequest, nil, "") + return + } + connection.User.CheckFsRoot(connection.ID) //nolint:errcheck + reader, err := connection.getFileReader(name, 0, r.Method) + if err != nil { + s.renderClientMessagePage(w, r, fmt.Sprintf("Unable to get a reader for the file %q", name), "", + getRespStatus(err), err, "") + return + } + defer reader.Close() + + var b bytes.Buffer + _, err = io.CopyN(&b, reader, 128) + if err != nil { + s.renderClientMessagePage(w, r, "Invalid PDF file", fmt.Sprintf("Unable to validate the file %q as PDF", name), + http.StatusBadRequest, nil, "") + return + } + if ctype := http.DetectContentType(b.Bytes()); ctype != "application/pdf" { + connection.Log(logger.LevelDebug, "detected %q content type, expected PDF, file %q", ctype, name) + s.renderClientBadRequestPage(w, r, fmt.Errorf("the file %q does not look like a PDF", name)) + return + } + downloadFile(w, r, connection, name, info, true, nil) //nolint:errcheck +}