fix(security): harden access checks and migration validation (#38324) (#38400)

services/migrations/common.go

@@ -22,6 +22,9 @@ func WarnAndNotice(fmtStr string, args ...any) {
 }
 
 func hasBaseURL(toCheck, baseURL string) bool {
+	if baseURL == "" {
+		return false
+	}
 	if len(baseURL) > 0 && baseURL[len(baseURL)-1] != '/' {
 		baseURL += "/"
 	}

services/migrations/migrate.go

@@ -87,15 +87,14 @@ func IsMigrateURLAllowed(remoteURL string, doer *user_model.User) error {
 }
 
 func checkByAllowBlockList(hostName string, addrList []net.IP) error {
-	var ipAllowed bool
+	ipAllowed := len(addrList) > 0
 	var ipBlocked bool
 	for _, addr := range addrList {
-		ipAllowed = ipAllowed || allowList.MatchIPAddr(addr)
+		ipAllowed = ipAllowed && allowList.MatchIPAddr(addr)
 		ipBlocked = ipBlocked || blockList.MatchIPAddr(addr)
 	}
-	var blockedError error
 	if blockList.MatchHostName(hostName) || ipBlocked {
-		blockedError = &git.ErrInvalidCloneAddr{Host: hostName, IsPermissionDenied: true}
+		return &git.ErrInvalidCloneAddr{Host: hostName, IsPermissionDenied: true}
 	}
 	// if we have an allow-list, check the allow-list before return to get the more accurate error
 	if !allowList.IsEmpty() {
@@ -104,7 +103,7 @@ func checkByAllowBlockList(hostName string, addrList []net.IP) error {
 		}
 	}
 	// otherwise, we always follow the blocked list
-	return blockedError
+	return nil
 }
 
 // MigrateRepository migrate repository according MigrateOptions
@@ -524,9 +523,10 @@ func Init() error {
 	if setting.Migrations.AllowLocalNetworks {
 		allowList.AppendBuiltin(hostmatcher.MatchBuiltinPrivate)
 		allowList.AppendBuiltin(hostmatcher.MatchBuiltinLoopback)
+	} else {
+		blockList.AppendBuiltin(hostmatcher.MatchBuiltinPrivate)
+		blockList.AppendBuiltin(hostmatcher.MatchBuiltinLoopback)
 	}
-	// TODO: at the moment, if ALLOW_LOCALNETWORKS=false, ALLOWED_DOMAINS=domain.com, and domain.com has IP 127.0.0.1, then it's still allowed.
-	// if we want to block such case, the private&loopback should be added to the blockList when ALLOW_LOCALNETWORKS=false
 
 	return nil
 }

services/migrations/migrate_test.go

@@ -93,17 +93,19 @@ func TestAllowBlockList(t *testing.T) {
 	assert.NoError(t, checkByAllowBlockList("domain.com", []net.IP{net.ParseIP("1.2.3.4")}))
 	assert.NoError(t, checkByAllowBlockList("domain.com", []net.IP{net.ParseIP("127.0.0.1")}))
 
-	// allow wildcard, block some subdomains. if the domain name is allowed, then the local network check is skipped
+	// allow wildcard, block some subdomains. every resolved address must still be allowed.
 	init("*.domain.com", "blocked.domain.com", false)
 	assert.NoError(t, checkByAllowBlockList("sub.domain.com", []net.IP{net.ParseIP("1.2.3.4")}))
-	assert.NoError(t, checkByAllowBlockList("sub.domain.com", []net.IP{net.ParseIP("127.0.0.1")}))
+	assert.Error(t, checkByAllowBlockList("sub.domain.com", []net.IP{net.ParseIP("127.0.0.1")}))
+	assert.Error(t, checkByAllowBlockList("sub.domain.com", []net.IP{net.ParseIP("1.2.3.4"), net.ParseIP("127.0.0.1")}))
 	assert.Error(t, checkByAllowBlockList("blocked.domain.com", []net.IP{net.ParseIP("1.2.3.4")}))
 	assert.Error(t, checkByAllowBlockList("sub.other.com", []net.IP{net.ParseIP("1.2.3.4")}))
 
-	// allow wildcard (it could lead to SSRF in production)
+	// allow wildcard still follows the local network policy for resolved addresses.
 	init("*", "", false)
 	assert.NoError(t, checkByAllowBlockList("domain.com", []net.IP{net.ParseIP("1.2.3.4")}))
-	assert.NoError(t, checkByAllowBlockList("domain.com", []net.IP{net.ParseIP("127.0.0.1")}))
+	assert.Error(t, checkByAllowBlockList("domain.com", []net.IP{net.ParseIP("127.0.0.1")}))
+	assert.Error(t, checkByAllowBlockList("domain.com", []net.IP{net.ParseIP("1.2.3.4"), net.ParseIP("127.0.0.1")}))
 
 	// local network can still be blocked
 	init("*", "127.0.0.*", false)

services/migrations/restore.go

@@ -239,7 +239,7 @@ func (r *RepositoryRestorer) GetPullRequests(_ context.Context, page, perPage in
 		if pr.PatchURL != "" {
 			pr.PatchURL = "file://" + util.FilePathJoinAbs(r.baseDir, pr.PatchURL)
 		}
-		CheckAndEnsureSafePR(pr, "", r)
+		CheckAndEnsureSafePR(pr, "file://"+r.baseDir, r)
 	}
 	return pulls, true, nil
 }

services/migrations/restore_test.go

@@ -45,3 +45,30 @@ func TestRepositoryRestorer_GetReleases_LocalFileInclusion(t *testing.T) {
 	assert.Equal(t, "file://"+filepath.Join(baseDir, "good.txt"), optional.FromPtr(assets[0].DownloadURL).Value())
 	assert.Equal(t, "file://"+filepath.Join(baseDir, "etc/passwd"), optional.FromPtr(assets[1].DownloadURL).Value())
 }
+
+func TestRepositoryRestorer_GetPullRequestsStripsUnsafeCloneURL(t *testing.T) {
+	baseDir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(baseDir, "change.patch"), []byte("patch"), 0o644))
+
+	pullRequestYML := `
+- number: 1
+  patch_url: change.patch
+  head:
+    clone_url: http://127.0.0.1/private.git
+    ref: feature
+  base:
+    ref: main
+`
+	require.NoError(t, os.WriteFile(filepath.Join(baseDir, "pull_request.yml"), []byte(pullRequestYML), 0o644))
+
+	r, err := NewRepositoryRestorer(t.Context(), baseDir, "owner", "repo", false)
+	require.NoError(t, err)
+
+	pulls, _, err := r.GetPullRequests(t.Context(), 1, 10)
+	require.NoError(t, err)
+	require.Len(t, pulls, 1)
+
+	assert.Equal(t, "file://"+filepath.Join(baseDir, "change.patch"), pulls[0].PatchURL)
+	assert.Empty(t, pulls[0].Head.CloneURL)
+	assert.True(t, pulls[0].EnsuredSafe)
+}