sec: fix s3 and gcs host checks #512

dduzgun-security · 2025-01-07T15:11:46Z

Resolves potential incorrect host checks for S3 and GCS.

Ref: SECVULN-14562, SECVULN-14561, SECVULN-14564

NodyHub

Hey @dduzgun-security, great contribution. I have added added some comments and thoughts around the implementation, nothing special.

Looking forward to see this PR landing!

NodyHub · 2025-01-07T15:16:22Z

detect_gcs.go

@@ -18,23 +21,39 @@ func (d *GCSDetector) Detect(src, _ string) (string, bool, error) {
 		return "", false, nil
 	}

-	if strings.Contains(src, "googleapis.com/") {
+	if strings.Contains(src, ".googleapis.com/") {


Can we be sure that src is always only the FQDN and not also the path? So that we can be sure that src is not something like evil.foo.bar/hi-hashi.googleapis.com/whatever/. I will take a look into the call path, but just having that as a first thought in mind.

If take a look into the call graph, the mentioned case ^ might ocour:

go-getter/detect.go

Lines 47 to 60 in 842d6c3

func Detect(src string, pwd string, ds []Detector) (string, error) {

getForce, getSrc := getForcedGetter(src)

// Separate out the subdir if there is one, we don't pass that to detect

getSrc, subDir := SourceDirSubdir(getSrc)

u, err := url.Parse(getSrc)

if err == nil && u.Scheme != "" {

// Valid URL

return src, nil

}

for _, d := range ds {

result, ok, err := d.Detect(getSrc, pwd)

Might be worse to parse the url and inspect only the host. :)

Also, what about various cases? 🙈

Ah, I see. You create your own url here later on:

go-getter/detect_gcs.go

Line 58 in 042c9f5

url, err := url.Parse(fmt.Sprintf("https://www.googleapis.com/storage/%s/%s/%s",

Good catch, it's still not a bad idea to url parse it here too and validate the sufix

Yeah, well ... as long as you check or for the suffix of the host name :)

NodyHub · 2025-01-07T16:15:48Z

detect_gcs.go

+	}
+
+	// Rule 6: Bucket name cannot contain "google" or common misspellings like "g00gle"
+	googlePattern := `google|g00gle`


What about go0gle? Is there somewhere a spec to that?

I wonder if there will be too many permutations to consider here. The documentation really only says:

Bucket names cannot contain "google" or close misspellings, such as "g00gle".
https://cloud.google.com/storage/docs/buckets#naming

We could go further with a regex like g[o0]{2}gle, but there's also 1s for l, etc. It's a rabbit hole I'm not sure we want to go down. 🕳️ 🐇

Since this will be enforced by Google Cloud when creating a bucket, maybe we don't really need to worry about it for the means of detecting a bucket. If we send an invalid bucket name in a request to GCS, it should just fail.

I suspect we can simply detect it likely being a bucket without checking these common misspellings.

💯
I don't mind if we leave it as @dduzgun-security proposed it and any further validation is done on the server site endpoint. My comment from above was mainly a thing that came through my mind while reading that check.

After talking with @picatz, we won't need this part as it might add more risk or blockers on specific use cases by people. Also, this doesn't allow any injection point and Google validates the call.

NodyHub · 2025-01-07T16:47:51Z

detect_gcs.go

+	}
+
+	// Rule 2: Bucket name cannot start or end with a hyphen, dot, or underscore
+	if bucket[0] == '-' || bucket[0] == '.' || bucket[len(bucket)-1] == '-' || bucket[len(bucket)-1] == '.' || bucket[len(bucket)-1] == '_' {


Suggested change

if bucket[0] == '-' || bucket[0] == '.' || bucket[len(bucket)-1] == '-' || bucket[len(bucket)-1] == '.' || bucket[len(bucket)-1] == '_' {

if bucket[0] == '-' || bucket[0] == '.' || bucket[0] == '_' || bucket[len(bucket)-1] == '-' || bucket[len(bucket)-1] == '.' || bucket[len(bucket)-1] == '_' {

NodyHub · 2025-01-07T16:49:05Z

detect_gcs.go

+	}
+
+	// Rule 4: Bucket name cannot be an IP address (only digits and dots, e.g., 192.168.5.4)
+	ipPattern := `^(\d{1,3}\.){3}\d{1,3}$`


999.999.999 is not a valid IP address, but is it still officially invalid?

Adding on to this maybe a more direct check should be used for example:

// IPv4 regex matches 0.0.0.0 to 255.255.255.255 simpleRegex := `^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])$`

🤔 Maybe it would be even better to avoid a regex, and use the standard library:

https://pkg.go.dev/net/netip#ParseAddr

addr, err := netip.ParseAddr(ipString) ...

Compared to the net.IP type, Addr type takes less memory, is immutable, and is comparable (supports == and being a map key).

Well, is it not a bit too much over-processing for a path-validation to try to convert parts of the path into a Addr object? I don't have a perfect solution in mind 🤷

NodyHub · 2025-01-07T16:50:51Z

detect_gcs.go

+		return false
+	}
+
+	// Rule 3: Object names cannot be exactly '.' or '..'


Cant be anyway, due to the fact that you perform the path.Clean(..) at the beginning 😬 however, the check is not wrong :)

NodyHub · 2025-01-08T09:33:59Z

get_gcs_test.go

@@ -233,3 +233,35 @@ func TestGCSGetter_GetFile_OAuthAccessToken(t *testing.T) {
 	}
 	assertContents(t, dst, "# Main\n")
 }
+
+func Test_GCSGetter_ParseUrl_Malformed(t *testing.T) {


It would be great to have a positive example for this test.

dduzgun-security added 2 commits January 6, 2025 18:22

sec: fix incorrect host checks for s3 and gcs

06a7b84

add more tests

042c9f5

NodyHub reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sec: fix s3 and gcs host checks #512

sec: fix s3 and gcs host checks #512

dduzgun-security commented Jan 7, 2025 •

edited by jira bot

Loading

NodyHub left a comment

NodyHub Jan 7, 2025

NodyHub Jan 7, 2025

NodyHub Jan 7, 2025

NodyHub Jan 7, 2025

dduzgun-security Jan 10, 2025

NodyHub Jan 10, 2025

NodyHub Jan 7, 2025

picatz Jan 8, 2025 •

edited

Loading

NodyHub Jan 8, 2025 •

edited

Loading

dduzgun-security Jan 10, 2025

NodyHub Jan 7, 2025

NodyHub Jan 7, 2025

alanpchua Jan 8, 2025

picatz Jan 8, 2025

NodyHub Jan 9, 2025

NodyHub Jan 7, 2025

NodyHub Jan 8, 2025

	func Detect(src string, pwd string, ds []Detector) (string, error) {
	getForce, getSrc := getForcedGetter(src)

	// Separate out the subdir if there is one, we don't pass that to detect
	getSrc, subDir := SourceDirSubdir(getSrc)

	u, err := url.Parse(getSrc)
	if err == nil && u.Scheme != "" {
	// Valid URL
	return src, nil
	}

	for _, d := range ds {
	result, ok, err := d.Detect(getSrc, pwd)

	if bucket[0] == '-' \|\| bucket[0] == '.' \|\| bucket[len(bucket)-1] == '-' \|\| bucket[len(bucket)-1] == '.' \|\| bucket[len(bucket)-1] == '_' {
	if bucket[0] == '-' \|\| bucket[0] == '.' \|\| bucket[0] == '_' \|\| bucket[len(bucket)-1] == '-' \|\| bucket[len(bucket)-1] == '.' \|\| bucket[len(bucket)-1] == '_' {

sec: fix s3 and gcs host checks #512

Are you sure you want to change the base?

sec: fix s3 and gcs host checks #512

Conversation

dduzgun-security commented Jan 7, 2025 • edited by jira bot Loading

NodyHub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picatz Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

NodyHub Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduzgun-security commented Jan 7, 2025 •

edited by jira bot

Loading

picatz Jan 8, 2025 •

edited

Loading

NodyHub Jan 8, 2025 •

edited

Loading