Skip to content

Commit

Permalink
fix(vulnfeeds): validate the entire URL, not the repo part
Browse files Browse the repository at this point in the history
This commit adjusts link validation, so that it validates the *entire*
link *before* separating it into a repo and commit. Previously, the repo
just URL was validated. This was allowing GitHub commit URLs that were
404'ing to escape detection, which was not the intention.

This was discovered while investigating analysis failures for records
that had valid-looking GitHub commit ranges.
  • Loading branch information
andrewpollock committed Nov 6, 2024
1 parent 281ed7e commit b67f442
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 7 deletions.
19 changes: 12 additions & 7 deletions vulnfeeds/cves/versions.go
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,11 @@ func Repo(u string) (string, error) {
parsedURL.Hostname(), repo), nil
}

// Handle a Linux Kernel URL that is already cloneable and doesn't require remapping.
if parsedURL.Hostname() == "git.kernel.org" && strings.HasPrefix(parsedURL.Path, "/pub/scm/linux/kernel/git/torvalds/linux.git") {
return fmt.Sprintf("%s://%s%s", parsedURL.Scheme, parsedURL.Hostname(), "/pub/scm/linux/kernel/git/torvalds/linux.git"), nil
}

// GitWeb CGI URLs are structured very differently, and require significant translation to get a cloneable URL, e.g.
// https://git.gnupg.org/cgi-bin/gitweb.cgi?p=libksba.git;a=commit;h=f61a5ea4e0f6a80fd4b28ef0174bee77793cf070 -> git://git.gnupg.org/libksba.git
// https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=11d171f1910b508a81d21faa087ad1af573407d8 -> git://sourceware.org/git/binutils-gdb.git
Expand Down Expand Up @@ -647,21 +652,21 @@ func ValidateAndCanonicalizeLink(link string) (canonicalLink string, err error)

// For URLs referencing commits in supported Git repository hosts, return a cloneable AffectedCommit.
func extractGitCommit(link string, commitType CommitType) (ac AffectedCommit, err error) {
r, err := Repo(link)
// If URL doesn't validate, treat it as linkrot.
// Possible TODO(apollock): restart the entire extraction process when the
// repo changes (i.e. handle a redirect to a completely different host,
// instead of a redirect within GitHub)
link, err = ValidateAndCanonicalizeLink(link)
if err != nil {
return ac, err
}

c, err := Commit(link)
r, err := Repo(link)
if err != nil {
return ac, err
}

// If URL doesn't validate, treat it as linkrot.
// Possible TODO(apollock): restart the entire extraction process when the
// repo changes (i.e. handle a redirect to a completely different host,
// instead of a redirect within GitHub)
r, err = ValidateAndCanonicalizeLink(r)
c, err := Commit(link)
if err != nil {
return ac, err
}
Expand Down
54 changes: 54 additions & 0 deletions vulnfeeds/cves/versions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,12 @@ func TestRepo(t *testing.T) {
expectedRepoURL: "https://git.kernel.org/pub/scm/libs/libcap/libcap.git",
expectedOk: true,
},
{
description: "Linux kernel URL that doesn't require remapping to be cloneable",
inputLink: "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ee1fee900537b5d9560e9f937402de5ddc8412f3",
expectedRepoURL: "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git",
expectedOk: true,
},
}

for _, tc := range tests {
Expand Down Expand Up @@ -612,6 +618,13 @@ func TestExtractGitCommit(t *testing.T) {
Fixed: "aa332a59116c8398976434b57ea477c6823054f8",
},
},
{
description: "A GitHub commit link that is 404'ing (as seen on CVE-2019-8375)",
inputLink: "https://github.com/WebKit/webkit/commit/6f9b511a115311b13c06eb58038ddc2c78da5531",
inputCommitType: Fixed,
expectedAffectedCommit: AffectedCommit{},
expectFailure: true,
},
}

for _, tc := range tests {
Expand Down Expand Up @@ -1039,3 +1052,44 @@ func TestInvalidRangeDetection(t *testing.T) {
}
}
}

func TestValidateAndCanonicalizeLink(t *testing.T) {
type args struct {
link string
}
tests := []struct {
name string
args args
wantCanonicalLink string
wantErr bool
}{
{
name: "A link that 404's",
args: args{
link: "https://github.com/WebKit/webkit/commit/6f9b511a115311b13c06eb58038ddc2c78da5531",
},
wantCanonicalLink: "https://github.com/WebKit/webkit/commit/6f9b511a115311b13c06eb58038ddc2c78da5531",
wantErr: true,
},
{
name: "A functioning link",
args: args{
link: "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ee1fee900537b5d9560e9f937402de5ddc8412f3",
},
wantCanonicalLink: "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ee1fee900537b5d9560e9f937402de5ddc8412f3",
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
gotCanonicalLink, err := ValidateAndCanonicalizeLink(tt.args.link)
if (err != nil) != tt.wantErr {
t.Errorf("ValidateAndCanonicalizeLink() error = %v, wantErr %v", err, tt.wantErr)
return
}
if gotCanonicalLink != tt.wantCanonicalLink {
t.Errorf("ValidateAndCanonicalizeLink() = %v, want %v", gotCanonicalLink, tt.wantCanonicalLink)
}
})
}
}

0 comments on commit b67f442

Please sign in to comment.