Skip to content

Commit e6b3be4

Browse files
zeripathtechknowlogick
zeripath
and
techknowlogick
authored
Add more checks in migration code (#21011)
When migrating add several more important sanity checks: * SHAs must be SHAs * Refs must be valid Refs * URLs must be reasonable Signed-off-by: Andrew Thornton <[email protected]> Signed-off-by: Andrew Thornton <[email protected]> Co-authored-by: techknowlogick <[email protected]>
1 parent 93a610a commit e6b3be4

24 files changed

+693
-281
lines changed

‎models/activities/action.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ func (a *Action) GetRefLink() string {
267267
return a.GetRepoLink() + "/src/branch/" + util.PathEscapeSegments(strings.TrimPrefix(a.RefName, git.BranchPrefix))
268268
case strings.HasPrefix(a.RefName, git.TagPrefix):
269269
return a.GetRepoLink() + "/src/tag/" + util.PathEscapeSegments(strings.TrimPrefix(a.RefName, git.TagPrefix))
270-
case len(a.RefName) == 40 && git.SHAPattern.MatchString(a.RefName):
270+
case len(a.RefName) == 40 && git.IsValidSHAPattern(a.RefName):
271271
return a.GetRepoLink() + "/src/commit/" + a.RefName
272272
default:
273273
// FIXME: we will just assume it's a branch - this was the old way - at some point we may want to enforce that there is always a ref here.

‎modules/git/ref.go

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@
44

55
package git
66

7-
import "strings"
7+
import (
8+
"regexp"
9+
"strings"
10+
)
811

912
const (
1013
// RemotePrefix is the base directory of the remotes information of git.
@@ -15,6 +18,29 @@ const (
1518
pullLen = len(PullPrefix)
1619
)
1720

21+
// refNamePatternInvalid is regular expression with unallowed characters in git reference name
22+
// They cannot have ASCII control characters (i.e. bytes whose values are lower than \040, or \177 DEL), space, tilde ~, caret ^, or colon : anywhere.
23+
// They cannot have question-mark ?, asterisk *, or open bracket [ anywhere
24+
var refNamePatternInvalid = regexp.MustCompile(
25+
`[\000-\037\177 \\~^:?*[]|` + // No absolutely invalid characters
26+
`(?:^[/.])|` + // Not HasPrefix("/") or "."
27+
`(?:/\.)|` + // no "/."
28+
`(?:\.lock$)|(?:\.lock/)|` + // No ".lock/"" or ".lock" at the end
29+
`(?:\.\.)|` + // no ".." anywhere
30+
`(?://)|` + // no "//" anywhere
31+
`(?:@{)|` + // no "@{"
32+
`(?:[/.]$)|` + // no terminal '/' or '.'
33+
`(?:^@$)`) // Not "@"
34+
35+
// IsValidRefPattern ensures that the provided string could be a valid reference
36+
func IsValidRefPattern(name string) bool {
37+
return !refNamePatternInvalid.MatchString(name)
38+
}
39+
40+
func SanitizeRefPattern(name string) string {
41+
return refNamePatternInvalid.ReplaceAllString(name, "_")
42+
}
43+
1844
// Reference represents a Git ref.
1945
type Reference struct {
2046
Name string

‎modules/git/repo_commit_nogogit.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ func (repo *Repository) getCommitFromBatchReader(rd *bufio.Reader, id SHA1) (*Co
138138

139139
// ConvertToSHA1 returns a Hash object from a potential ID string
140140
func (repo *Repository) ConvertToSHA1(commitID string) (SHA1, error) {
141-
if len(commitID) == 40 && SHAPattern.MatchString(commitID) {
141+
if len(commitID) == 40 && IsValidSHAPattern(commitID) {
142142
sha1, err := NewIDFromString(commitID)
143143
if err == nil {
144144
return sha1, nil

‎modules/git/sha1.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,12 @@ const EmptySHA = "0000000000000000000000000000000000000000"
1919
const EmptyTreeSHA = "4b825dc642cb6eb9a060e54bf8d69288fbee4904"
2020

2121
// SHAPattern can be used to determine if a string is an valid sha
22-
var SHAPattern = regexp.MustCompile(`^[0-9a-f]{4,40}$`)
22+
var shaPattern = regexp.MustCompile(`^[0-9a-f]{4,40}$`)
23+
24+
// IsValidSHAPattern will check if the provided string matches the SHA Pattern
25+
func IsValidSHAPattern(sha string) bool {
26+
return shaPattern.MatchString(sha)
27+
}
2328

2429
// MustID always creates a new SHA1 from a [20]byte array with no validation of input.
2530
func MustID(b []byte) SHA1 {

‎modules/migration/pullrequest.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ type PullRequest struct {
2626
Updated time.Time
2727
Closed *time.Time
2828
Labels []*Label
29-
PatchURL string `yaml:"patch_url"`
29+
PatchURL string `yaml:"patch_url"` // SECURITY: This must be safe to download directly from
3030
Merged bool
3131
MergedTime *time.Time `yaml:"merged_time"`
3232
MergeCommitSHA string `yaml:"merge_commit_sha"`
@@ -37,6 +37,7 @@ type PullRequest struct {
3737
Reactions []*Reaction
3838
ForeignIndex int64
3939
Context DownloaderContext `yaml:"-"`
40+
EnsuredSafe bool `yaml:"ensured_safe"`
4041
}
4142

4243
func (p *PullRequest) GetLocalIndex() int64 { return p.Number }
@@ -55,9 +56,9 @@ func (p PullRequest) GetGitRefName() string {
5556

5657
// PullRequestBranch represents a pull request branch
5758
type PullRequestBranch struct {
58-
CloneURL string `yaml:"clone_url"`
59-
Ref string
60-
SHA string
59+
CloneURL string `yaml:"clone_url"` // SECURITY: This must be safe to download from
60+
Ref string // SECURITY: this must be a git.IsValidRefPattern
61+
SHA string // SECURITY: this must be a git.IsValidSHAPattern
6162
RepoName string `yaml:"repo_name"`
6263
OwnerName string `yaml:"owner_name"`
6364
}

‎modules/migration/release.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,16 @@ type ReleaseAsset struct {
1818
DownloadCount *int `yaml:"download_count"`
1919
Created time.Time
2020
Updated time.Time
21-
DownloadURL *string `yaml:"download_url"`
21+
22+
DownloadURL *string `yaml:"download_url"` // SECURITY: It is the responsibility of downloader to make sure this is safe
2223
// if DownloadURL is nil, the function should be invoked
23-
DownloadFunc func() (io.ReadCloser, error) `yaml:"-"`
24+
DownloadFunc func() (io.ReadCloser, error) `yaml:"-"` // SECURITY: It is the responsibility of downloader to make sure this is safe
2425
}
2526

2627
// Release represents a release
2728
type Release struct {
28-
TagName string `yaml:"tag_name"`
29-
TargetCommitish string `yaml:"target_commitish"`
29+
TagName string `yaml:"tag_name"` // SECURITY: This must pass git.IsValidRefPattern
30+
TargetCommitish string `yaml:"target_commitish"` // SECURITY: This must pass git.IsValidRefPattern
3031
Name string
3132
Body string
3233
Draft bool

‎modules/migration/repo.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ type Repository struct {
1212
IsPrivate bool `yaml:"is_private"`
1313
IsMirror bool `yaml:"is_mirror"`
1414
Description string
15-
CloneURL string `yaml:"clone_url"`
15+
CloneURL string `yaml:"clone_url"` // SECURITY: This must be checked to ensure that is safe to be used
1616
OriginalURL string `yaml:"original_url"`
1717
DefaultBranch string
1818
}

‎modules/validation/binding.go

Lines changed: 3 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import (
99
"regexp"
1010
"strings"
1111

12+
"code.gitea.io/gitea/modules/git"
13+
1214
"gitea.com/go-chi/binding"
1315
"github.com/gobwas/glob"
1416
)
@@ -24,30 +26,6 @@ const (
2426
ErrRegexPattern = "RegexPattern"
2527
)
2628

27-
// GitRefNamePatternInvalid is regular expression with unallowed characters in git reference name
28-
// They cannot have ASCII control characters (i.e. bytes whose values are lower than \040, or \177 DEL), space, tilde ~, caret ^, or colon : anywhere.
29-
// They cannot have question-mark ?, asterisk *, or open bracket [ anywhere
30-
var GitRefNamePatternInvalid = regexp.MustCompile(`[\000-\037\177 \\~^:?*[]+`)
31-
32-
// CheckGitRefAdditionalRulesValid check name is valid on additional rules
33-
func CheckGitRefAdditionalRulesValid(name string) bool {
34-
// Additional rules as described at https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
35-
if strings.HasPrefix(name, "/") || strings.HasSuffix(name, "/") ||
36-
strings.HasSuffix(name, ".") || strings.Contains(name, "..") ||
37-
strings.Contains(name, "//") || strings.Contains(name, "@{") ||
38-
name == "@" {
39-
return false
40-
}
41-
parts := strings.Split(name, "/")
42-
for _, part := range parts {
43-
if strings.HasSuffix(part, ".lock") || strings.HasPrefix(part, ".") {
44-
return false
45-
}
46-
}
47-
48-
return true
49-
}
50-
5129
// AddBindingRules adds additional binding rules
5230
func AddBindingRules() {
5331
addGitRefNameBindingRule()
@@ -67,16 +45,10 @@ func addGitRefNameBindingRule() {
6745
IsValid: func(errs binding.Errors, name string, val interface{}) (bool, binding.Errors) {
6846
str := fmt.Sprintf("%v", val)
6947

70-
if GitRefNamePatternInvalid.MatchString(str) {
48+
if !git.IsValidRefPattern(str) {
7149
errs.Add([]string{name}, ErrGitRefName, "GitRefName")
7250
return false, errs
7351
}
74-
75-
if !CheckGitRefAdditionalRulesValid(str) {
76-
errs.Add([]string{name}, ErrGitRefName, "GitRefName")
77-
return false, errs
78-
}
79-
8052
return true, errs
8153
},
8254
})

‎routers/api/v1/repo/commits.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import (
1717
"code.gitea.io/gitea/modules/git"
1818
"code.gitea.io/gitea/modules/setting"
1919
api "code.gitea.io/gitea/modules/structs"
20-
"code.gitea.io/gitea/modules/validation"
2120
"code.gitea.io/gitea/routers/api/v1/utils"
2221
)
2322

@@ -53,7 +52,7 @@ func GetSingleCommit(ctx *context.APIContext) {
5352
// "$ref": "#/responses/notFound"
5453

5554
sha := ctx.Params(":sha")
56-
if (validation.GitRefNamePatternInvalid.MatchString(sha) || !validation.CheckGitRefAdditionalRulesValid(sha)) && !git.SHAPattern.MatchString(sha) {
55+
if !git.IsValidRefPattern(sha) {
5756
ctx.Error(http.StatusUnprocessableEntity, "no valid ref or sha", fmt.Sprintf("no valid ref or sha: %s", sha))
5857
return
5958
}

‎routers/api/v1/repo/notes.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212
"code.gitea.io/gitea/modules/convert"
1313
"code.gitea.io/gitea/modules/git"
1414
api "code.gitea.io/gitea/modules/structs"
15-
"code.gitea.io/gitea/modules/validation"
1615
)
1716

1817
// GetNote Get a note corresponding to a single commit from a repository
@@ -47,7 +46,7 @@ func GetNote(ctx *context.APIContext) {
4746
// "$ref": "#/responses/notFound"
4847

4948
sha := ctx.Params(":sha")
50-
if (validation.GitRefNamePatternInvalid.MatchString(sha) || !validation.CheckGitRefAdditionalRulesValid(sha)) && !git.SHAPattern.MatchString(sha) {
49+
if !git.IsValidRefPattern(sha) {
5150
ctx.Error(http.StatusUnprocessableEntity, "no valid ref or sha", fmt.Sprintf("no valid ref or sha: %s", sha))
5251
return
5352
}

‎services/migrations/codebase.go

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,9 +107,24 @@ func NewCodebaseDownloader(ctx context.Context, projectURL *url.URL, project, re
107107
commitMap: make(map[string]string),
108108
}
109109

110+
log.Trace("Create Codebase downloader. BaseURL: %s Project: %s RepoName: %s", baseURL, project, repoName)
110111
return downloader
111112
}
112113

114+
// String implements Stringer
115+
func (d *CodebaseDownloader) String() string {
116+
return fmt.Sprintf("migration from codebase server %s %s/%s", d.baseURL, d.project, d.repoName)
117+
}
118+
119+
// ColorFormat provides a basic color format for a GogsDownloader
120+
func (d *CodebaseDownloader) ColorFormat(s fmt.State) {
121+
if d == nil {
122+
log.ColorFprintf(s, "<nil: CodebaseDownloader>")
123+
return
124+
}
125+
log.ColorFprintf(s, "migration from codebase server %s %s/%s", d.baseURL, d.project, d.repoName)
126+
}
127+
113128
// FormatCloneURL add authentication into remote URLs
114129
func (d *CodebaseDownloader) FormatCloneURL(opts base.MigrateOptions, remoteAddr string) (string, error) {
115130
return opts.CloneAddr, nil
@@ -451,8 +466,8 @@ func (d *CodebaseDownloader) GetPullRequests(page, perPage int) ([]*base.PullReq
451466
Value int64 `xml:",chardata"`
452467
Type string `xml:"type,attr"`
453468
} `xml:"id"`
454-
SourceRef string `xml:"source-ref"`
455-
TargetRef string `xml:"target-ref"`
469+
SourceRef string `xml:"source-ref"` // NOTE: from the documentation these are actually just branches NOT full refs
470+
TargetRef string `xml:"target-ref"` // NOTE: from the documentation these are actually just branches NOT full refs
456471
Subject string `xml:"subject"`
457472
Status string `xml:"status"`
458473
UserID struct {
@@ -564,6 +579,9 @@ func (d *CodebaseDownloader) GetPullRequests(page, perPage int) ([]*base.PullReq
564579
Comments: comments[1:],
565580
},
566581
})
582+
583+
// SECURITY: Ensure that the PR is safe
584+
_ = CheckAndEnsureSafePR(pullRequests[len(pullRequests)-1], d.baseURL.String(), d)
567585
}
568586

569587
return pullRequests, true, nil

‎services/migrations/common.go

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
// Copyright 2022 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package migrations
6+
7+
import (
8+
"fmt"
9+
"strings"
10+
11+
admin_model "code.gitea.io/gitea/models/admin"
12+
"code.gitea.io/gitea/modules/git"
13+
"code.gitea.io/gitea/modules/log"
14+
base "code.gitea.io/gitea/modules/migration"
15+
)
16+
17+
// WarnAndNotice will log the provided message and send a repository notice
18+
func WarnAndNotice(fmtStr string, args ...interface{}) {
19+
log.Warn(fmtStr, args...)
20+
if err := admin_model.CreateRepositoryNotice(fmt.Sprintf(fmtStr, args...)); err != nil {
21+
log.Error("create repository notice failed: ", err)
22+
}
23+
}
24+
25+
func hasBaseURL(toCheck, baseURL string) bool {
26+
if len(baseURL) > 0 && baseURL[len(baseURL)-1] != '/' {
27+
baseURL += "/"
28+
}
29+
return strings.HasPrefix(toCheck, baseURL)
30+
}
31+
32+
// CheckAndEnsureSafePR will check that a given PR is safe to download
33+
func CheckAndEnsureSafePR(pr *base.PullRequest, commonCloneBaseURL string, g base.Downloader) bool {
34+
valid := true
35+
// SECURITY: the patchURL must be checked to have the same baseURL as the current to prevent open redirect
36+
if pr.PatchURL != "" && !hasBaseURL(pr.PatchURL, commonCloneBaseURL) {
37+
// TODO: Should we check that this url has the expected format for a patch url?
38+
WarnAndNotice("PR #%d in %s has invalid PatchURL: %s baseURL: %s", pr.Number, g, pr.PatchURL, commonCloneBaseURL)
39+
pr.PatchURL = ""
40+
valid = false
41+
}
42+
43+
// SECURITY: the headCloneURL must be checked to have the same baseURL as the current to prevent open redirect
44+
if pr.Head.CloneURL != "" && !hasBaseURL(pr.Head.CloneURL, commonCloneBaseURL) {
45+
// TODO: Should we check that this url has the expected format for a patch url?
46+
WarnAndNotice("PR #%d in %s has invalid HeadCloneURL: %s baseURL: %s", pr.Number, g, pr.Head.CloneURL, commonCloneBaseURL)
47+
pr.Head.CloneURL = ""
48+
valid = false
49+
}
50+
51+
// SECURITY: SHAs Must be a SHA
52+
if pr.MergeCommitSHA != "" && !git.IsValidSHAPattern(pr.MergeCommitSHA) {
53+
WarnAndNotice("PR #%d in %s has invalid MergeCommitSHA: %s", pr.Number, g, pr.MergeCommitSHA)
54+
pr.MergeCommitSHA = ""
55+
}
56+
if pr.Head.SHA != "" && !git.IsValidSHAPattern(pr.Head.SHA) {
57+
WarnAndNotice("PR #%d in %s has invalid HeadSHA: %s", pr.Number, g, pr.Head.SHA)
58+
pr.Head.SHA = ""
59+
valid = false
60+
}
61+
if pr.Base.SHA != "" && !git.IsValidSHAPattern(pr.Base.SHA) {
62+
WarnAndNotice("PR #%d in %s has invalid BaseSHA: %s", pr.Number, g, pr.Base.SHA)
63+
pr.Base.SHA = ""
64+
valid = false
65+
}
66+
67+
// SECURITY: Refs must be valid refs or SHAs
68+
if pr.Head.Ref != "" && !git.IsValidRefPattern(pr.Head.Ref) {
69+
WarnAndNotice("PR #%d in %s has invalid HeadRef: %s", pr.Number, g, pr.Head.Ref)
70+
pr.Head.Ref = ""
71+
valid = false
72+
}
73+
if pr.Base.Ref != "" && !git.IsValidRefPattern(pr.Base.Ref) {
74+
WarnAndNotice("PR #%d in %s has invalid BaseRef: %s", pr.Number, g, pr.Base.Ref)
75+
pr.Base.Ref = ""
76+
valid = false
77+
}
78+
79+
pr.EnsuredSafe = true
80+
81+
return valid
82+
}

0 commit comments

Comments
 (0)