From 09635b6b1292312434ead7216ed0a6ca2e566705 Mon Sep 17 00:00:00 2001 From: jolheiser Date: Wed, 31 Jan 2024 21:43:52 -0600 Subject: [PATCH 1/5] [SECURITY] review(kn4ck3r): more template escapes Signed-off-by: jolheiser (cherry picked from commit 33af1692233c732291b175785e94e2ee022853e4) Conflicts: templates/repo/migrate/migrating.tmpl templates/repo/settings/options.tmpl trivial context conflict --- templates/repo/migrate/migrating.tmpl | 6 +++--- templates/repo/settings/options.tmpl | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/templates/repo/migrate/migrating.tmpl b/templates/repo/migrate/migrating.tmpl index 82ed660d92..98821ea7e1 100644 --- a/templates/repo/migrate/migrating.tmpl +++ b/templates/repo/migrate/migrating.tmpl @@ -21,12 +21,12 @@
-

{{.locale.Tr "repo.migrate.migrating" .CloneAddr | Safe}}

+

{{.locale.Tr "repo.migrate.migrating" (.CloneAddr | Escape) | Safe}}

{{if .CloneAddr}} -

{{.locale.Tr "repo.migrate.migrating_failed" .CloneAddr | Safe}}

+

{{.locale.Tr "repo.migrate.migrating_failed" (.CloneAddr | Escape) | Safe}}

{{else}}

{{.locale.Tr "repo.migrate.migrating_failed_no_addr" | Safe}}

{{end}} @@ -57,7 +57,7 @@
{{.locale.Tr "repo.settings.delete_notices_1" | Safe}}
- {{.locale.Tr "repo.settings.delete_notices_2" .Repository.FullName | Safe}} + {{.locale.Tr "repo.settings.delete_notices_2" (.Repository.FullName | Escape) | Safe}} {{if .Repository.NumForks}}
{{.locale.Tr "repo.settings.delete_notices_fork_1"}} {{end}} diff --git a/templates/repo/settings/options.tmpl b/templates/repo/settings/options.tmpl index a261ce95b5..9ee0dd127c 100644 --- a/templates/repo/settings/options.tmpl +++ b/templates/repo/settings/options.tmpl @@ -943,7 +943,7 @@
{{.locale.Tr "repo.settings.delete_notices_1" | Safe}}
- {{.locale.Tr "repo.settings.delete_notices_2" .Repository.FullName | Safe}} + {{.locale.Tr "repo.settings.delete_notices_2" (.Repository.FullName | Escape) | Safe}} {{if .Repository.NumForks}}
{{.locale.Tr "repo.settings.delete_notices_fork_1"}} {{end}} @@ -978,7 +978,7 @@
{{.locale.Tr "repo.settings.delete_notices_1" | Safe}}
- {{.locale.Tr "repo.settings.wiki_delete_notices_1" .Repository.Name | Safe}} + {{.locale.Tr "repo.settings.wiki_delete_notices_1" (.Repository.Name | Escape) | Safe}}
{{.CsrfTokenHtml}} From ef05332c3be415ddc8eb211a55151da12280698b Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 17 Jan 2024 16:16:46 +0100 Subject: [PATCH 2/5] [SECURITY] Fix XSS in wiki last commit information - On the wiki and revisions page, information is shown about the last commit that modified that wiki page. This includes the time it was last edited and by whom. That whole string is not being sanitized (passed trough `Safe` in the templates), because the last edited bit is formatted as an HTML element and thus shouldn't be sanitized. The problem with this is that now `.Author.Name` is not being sanitized. - This can be exploited, the names of authors and commiters on a Git commit is user controlled, they can be any value and thus also include HTML. It's not easy to actually exploit this, as you cannot use the official git binary to do use, as they actually strip `<` and `>` from user names (trivia: this behaviour was introduced in the initial commit of Git). In the integration testing, go-git actually has to generate this commit as they don't have such restrictions. - Pass `.Author.Name` trough `Escape` in order to be sanitized. (cherry picked from commit d24c37e132a554cee499df416cf5123964564da8) Conflicts: templates/repo/wiki/revision.tmpl templates/repo/wiki/view.tmpl trivial context conflict --- templates/repo/wiki/revision.tmpl | 2 +- templates/repo/wiki/view.tmpl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/templates/repo/wiki/revision.tmpl b/templates/repo/wiki/revision.tmpl index b2d6e63924..4d8fa25d31 100644 --- a/templates/repo/wiki/revision.tmpl +++ b/templates/repo/wiki/revision.tmpl @@ -10,7 +10,7 @@ {{$title}}
{{$timeSince := TimeSince .Author.When $.locale}} - {{.locale.Tr "repo.wiki.last_commit_info" .Author.Name $timeSince | Safe}} + {{.locale.Tr "repo.wiki.last_commit_info" (.Author.Name | Escape) $timeSince | Safe}}
diff --git a/templates/repo/wiki/view.tmpl b/templates/repo/wiki/view.tmpl index c294af3160..e64a106bcb 100644 --- a/templates/repo/wiki/view.tmpl +++ b/templates/repo/wiki/view.tmpl @@ -40,7 +40,7 @@ {{$title}}
{{$timeSince := TimeSince .Author.When $.locale}} - {{.locale.Tr "repo.wiki.last_commit_info" .Author.Name $timeSince | Safe}} + {{.locale.Tr "repo.wiki.last_commit_info" (.Author.Name | Escape) $timeSince | Safe}}
From 37f03e065a61164be5b7df660dd10c9803eb8469 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 17 Jan 2024 16:16:46 +0100 Subject: [PATCH 3/5] [SECURITY] Test XSS in wiki last commit information On the wiki and revisions page, information is shown about the last commit that modified that wiki page. This includes the time it was last edited and by whom. Verify it is sanitized. (cherry picked from commit 565e3312385d533f96c359979a3ae7cc14eba671) (cherry picked from commit 92dae3a387445fa0e9c939454981a231557b0736) --- tests/integration/xss_test.go | 75 +++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/integration/xss_test.go b/tests/integration/xss_test.go index e575ed3990..42ce35150c 100644 --- a/tests/integration/xss_test.go +++ b/tests/integration/xss_test.go @@ -4,14 +4,24 @@ package integration import ( + "context" + "fmt" "net/http" + "net/url" + "os" + "path/filepath" "testing" + "time" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/tests" + gogit "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing/object" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestXSSUserFullName(t *testing.T) { @@ -37,3 +47,68 @@ func TestXSSUserFullName(t *testing.T) { htmlDoc.doc.Find("div.content").Find(".header.text.center").Text(), ) } + +func TestXSSWikiLastCommitInfo(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + // Prepare the environment. + dstPath := t.TempDir() + r := fmt.Sprintf("%suser2/repo1.wiki.git", u.String()) + u, err := url.Parse(r) + assert.NoError(t, err) + u.User = url.UserPassword("user2", userPassword) + assert.NoError(t, git.CloneWithArgs(context.Background(), git.AllowLFSFiltersArgs(), u.String(), dstPath, git.CloneRepoOptions{})) + + // Use go-git here, because using git wouldn't work, it has code to remove + // `<`, `>` and `\n` in user names. Even though this is permitted and + // wouldn't result in a error by a Git server. + gitRepo, err := gogit.PlainOpen(dstPath) + require.NoError(t, err) + + w, err := gitRepo.Worktree() + require.NoError(t, err) + + filename := filepath.Join(dstPath, "Home.md") + err = os.WriteFile(filename, []byte("Oh, a XSS attack?"), 0o644) + require.NoError(t, err) + + _, err = w.Add("Home.md") + require.NoError(t, err) + + _, err = w.Commit("Yay XSS", &gogit.CommitOptions{ + Author: &object.Signature{ + Name: `Gusted`, + Email: "valid@example.org", + When: time.Date(2024, time.January, 31, 0, 0, 0, 0, time.UTC), + }, + }) + require.NoError(t, err) + + // Push. + _, _, err = git.NewCommand(git.DefaultContext, "push").AddArguments(git.ToTrustedCmdArgs([]string{"origin", "master"})...).RunStdString(&git.RunOpts{Dir: dstPath}) + require.NoError(t, err) + + // Check on page view. + t.Run("Page view", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, http.MethodGet, "/user2/repo1/wiki/Home") + resp := MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + htmlDoc.AssertElement(t, "script.evil", false) + assert.Contains(t, htmlDoc.Find(".ui.sub.header").Text(), `Gusted edited this page 2024-01-31`) + }) + + // Check on revisions page. + t.Run("Revision page", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, http.MethodGet, "/user2/repo1/wiki/Home?action=_revision") + resp := MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + htmlDoc.AssertElement(t, "script.evil", false) + assert.Contains(t, htmlDoc.Find(".ui.sub.header").Text(), `Gusted edited this page 2024-01-31`) + }) + }) +} From 4fdd0ed7282406cf245c922a7962ee75f8e74486 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 18 Jan 2024 00:18:39 +0100 Subject: [PATCH 4/5] [SECURITY] Fix XSS in dismissed review - It's possible for reviews to not be assiocated with users, when they were migrated from another forge instance. In the migration code, there's no sanitization check for author names, so they could contain HTML tags and thus needs to be properely escaped. - Pass `$reviewerName` trough `Escape`. (cherry picked from commit fe2df46d053b3a06c30c9221899707d3b26c3013) Conflicts: templates/repo/issue/view_content/comments.tmpl trivial context conflict --- templates/repo/issue/view_content/comments.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index e371df14dc..77ab603fff 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -775,7 +775,7 @@ {{else}} {{$reviewerName = .Review.OriginalAuthor}} {{end}} - {{$.locale.Tr "repo.issues.review.dismissed" $reviewerName $createdStr | Safe}} + {{$.locale.Tr "repo.issues.review.dismissed" ($reviewerName | Escape) $createdStr | Safe}}
{{if .Content}} From 672caa68138acf85e681e54242a8c95160869737 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 18 Jan 2024 00:18:39 +0100 Subject: [PATCH 5/5] [SECURITY] Test XSS in dismissed review It's possible for reviews to not be assiocated with users, when they were migrated from another forge instance. In the migration code, there's no sanitization check for author names, so they could contain HTML tags and thus needs to be properely escaped. (cherry picked from commit ca798e4cc2a8c6e3d1f2cfed01f47d8b3da9361f) (cherry picked from commit d3de80b9cc684d88ffa90f22af9f2dc912af6979) --- .../fixtures/TestXSSReviewDismissed/comment.yml | 9 +++++++++ .../fixtures/TestXSSReviewDismissed/review.yml | 8 ++++++++ tests/integration/xss_test.go | 15 +++++++++++++++ 3 files changed, 32 insertions(+) create mode 100644 tests/integration/fixtures/TestXSSReviewDismissed/comment.yml create mode 100644 tests/integration/fixtures/TestXSSReviewDismissed/review.yml diff --git a/tests/integration/fixtures/TestXSSReviewDismissed/comment.yml b/tests/integration/fixtures/TestXSSReviewDismissed/comment.yml new file mode 100644 index 0000000000..50162a4e7e --- /dev/null +++ b/tests/integration/fixtures/TestXSSReviewDismissed/comment.yml @@ -0,0 +1,9 @@ +- + id: 1000 + type: 32 # dismiss review + poster_id: 2 + issue_id: 2 # in repo_id 1 + content: "XSS time!" + review_id: 1000 + created_unix: 1700000000 + updated_unix: 1700000000 diff --git a/tests/integration/fixtures/TestXSSReviewDismissed/review.yml b/tests/integration/fixtures/TestXSSReviewDismissed/review.yml new file mode 100644 index 0000000000..56bc08d35f --- /dev/null +++ b/tests/integration/fixtures/TestXSSReviewDismissed/review.yml @@ -0,0 +1,8 @@ +- + id: 1000 + type: 1 + issue_id: 2 + original_author: "Otto " + content: "XSS time!" + updated_unix: 1700000000 + created_unix: 1700000000 diff --git a/tests/integration/xss_test.go b/tests/integration/xss_test.go index 42ce35150c..acd716c7c7 100644 --- a/tests/integration/xss_test.go +++ b/tests/integration/xss_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" @@ -112,3 +113,17 @@ func TestXSSWikiLastCommitInfo(t *testing.T) { }) }) } + +func TestXSSReviewDismissed(t *testing.T) { + defer tests.AddFixtures("tests/integration/fixtures/TestXSSReviewDismissed/")() + defer tests.PrepareTestEnv(t)() + + review := unittest.AssertExistsAndLoadBean(t, &issues_model.Review{ID: 1000}) + + req := NewRequest(t, http.MethodGet, fmt.Sprintf("/user2/repo1/pulls/%d", +review.IssueID)) + resp := MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + htmlDoc.AssertElement(t, "script.evil", false) + assert.Contains(t, htmlDoc.Find("#issuecomment-1000 .dismissed-message").Text(), `dismissed Otto ’s review`) +}