From 33af1692233c732291b175785e94e2ee022853e4 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 --- 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 ae168c07e6..939f236b9d 100644 --- a/templates/repo/migrate/migrating.tmpl +++ b/templates/repo/migrate/migrating.tmpl @@ -21,12 +21,12 @@
-

{{ctx.Locale.Tr "repo.migrate.migrating" .CloneAddr | Safe}}

+

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

{{if .CloneAddr}} -

{{ctx.Locale.Tr "repo.migrate.migrating_failed" .CloneAddr | Safe}}

+

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

{{else}}

{{ctx.Locale.Tr "repo.migrate.migrating_failed_no_addr" | Safe}}

{{end}} @@ -58,7 +58,7 @@
{{ctx.Locale.Tr "repo.settings.delete_notices_1" | Safe}}
- {{ctx.Locale.Tr "repo.settings.delete_notices_2" .Repository.FullName | Safe}} + {{ctx.Locale.Tr "repo.settings.delete_notices_2" (.Repository.FullName | Escape) | Safe}} {{if .Repository.NumForks}}
{{ctx.Locale.Tr "repo.settings.delete_notices_fork_1"}} {{end}} diff --git a/templates/repo/settings/options.tmpl b/templates/repo/settings/options.tmpl index b6ad3aacfa..65d4ac0a1a 100644 --- a/templates/repo/settings/options.tmpl +++ b/templates/repo/settings/options.tmpl @@ -906,7 +906,7 @@
{{ctx.Locale.Tr "repo.settings.delete_notices_1" | Safe}}
- {{ctx.Locale.Tr "repo.settings.delete_notices_2" .Repository.FullName | Safe}} + {{ctx.Locale.Tr "repo.settings.delete_notices_2" (.Repository.FullName | Escape) | Safe}} {{if .Repository.NumForks}}
{{ctx.Locale.Tr "repo.settings.delete_notices_fork_1"}} {{end}} @@ -941,7 +941,7 @@
{{ctx.Locale.Tr "repo.settings.delete_notices_1" | Safe}}
- {{ctx.Locale.Tr "repo.settings.wiki_delete_notices_1" .Repository.Name | Safe}} + {{ctx.Locale.Tr "repo.settings.wiki_delete_notices_1" (.Repository.Name | Escape) | Safe}}
{{.CsrfTokenHtml}} From d24c37e132a554cee499df416cf5123964564da8 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. --- 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 95b3cd0920..afd9c94c31 100644 --- a/templates/repo/wiki/revision.tmpl +++ b/templates/repo/wiki/revision.tmpl @@ -10,7 +10,7 @@ {{$title}}
{{$timeSince := TimeSince .Author.When ctx.Locale}} - {{ctx.Locale.Tr "repo.wiki.last_commit_info" .Author.Name $timeSince | Safe}} + {{ctx.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 039ff3f179..c6e47ee63e 100644 --- a/templates/repo/wiki/view.tmpl +++ b/templates/repo/wiki/view.tmpl @@ -40,7 +40,7 @@ {{$title}}
{{$timeSince := TimeSince .Author.When ctx.Locale}} - {{ctx.Locale.Tr "repo.wiki.last_commit_info" .Author.Name $timeSince | Safe}} + {{ctx.Locale.Tr "repo.wiki.last_commit_info" (.Author.Name | Escape) $timeSince | Safe}}
From 92dae3a387445fa0e9c939454981a231557b0736 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) --- 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 fe2df46d053b3a06c30c9221899707d3b26c3013 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`. --- 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 ad7b836e9c..d70561bfa3 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -615,7 +615,7 @@ {{else}} {{$reviewerName = .Review.OriginalAuthor}} {{end}} - {{ctx.Locale.Tr "repo.issues.review.dismissed" $reviewerName $createdStr | Safe}} + {{ctx.Locale.Tr "repo.issues.review.dismissed" ($reviewerName | Escape) $createdStr | Safe}}
{{if .Content}} From d3de80b9cc684d88ffa90f22af9f2dc912af6979 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) --- .../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`) +}