Hey Joe Chen, I couldn't find out what versions were affected, is it matter for you to specify the affected version precisely?

And also, would you validate the occurrences if they are correct?

Hey @amammad, I think all versions are affected.

Regarding the occurrences, I think returning IP from isLocalHostname doesn't really help for mitigating this vulnerability (i.e. requesting to an IP can still end up in redirection). Thus, I agree disallow redirection in repo migrate and webhook make sense as the fix.

Besides, I can't seem to produce the vulnerability on my DO droplet anymore. The endpoint provided by the docs (https://docs.digitalocean.com/products/droplets/how-to/retrieve-droplet-metadata/) only accepts HEAD and GET requests, but webhooks are POST requests, could you help here? Thanks!

Hey Joe, with 308 Status code, The request method, and the body will not be altered, so if you use 308 then a POST request will be sent to the target endpoint. Differentially, the attacker can send a GET with 301 request because this status code will change any HTTP request to a GET request.

Can you please explain more about thisi.e., requesting to an IP can still end up in redirection?

This is my fault, I should say that the main problem with this vulnerability is not an SSRF, the main problem is the TOCTOU vulnerability. you validate an URL and its IP address when you want to store it for the first time, but when you want to send a request next time to webhook you make another DNS query, and here is the problem, you shouldn't send another DNS query, you should use the validated IP address as the destination, here is the standard procedure for resolving the TOCTOU + SSRF issues that I saw before in many repositories, If you have another better way please inform me whenever you reach it.

Hey @amammad, thanks for a quick response!

[...], with 308 Status code, The request method, and the body will not be altered, so if you use 308 then a POST request will be sent to the target endpoint. Differentially, the attacker can send a GET with 301 request because this status code will change any HTTP request to a GET request.

TIL!

Can you please explain more about thisi.e., requesting to an IP can still end up in redirection?

Even if the IP is returned and stored, making a request to a remote IP does not guarantee it won't a redirection to different IP in the future, does it? I think the part I'm unclear about this mitigation is that how is retuning the initially resolved IP address help mitigate this problem.

This is my fault, I should say that the main problem with this vulnerability is not an SSRF, the main problem is the TOCTOU vulnerability. you validate an URL and its IP address when you want to store it for the first time, but when you want to send a request next time to webhook you make another DNS query, and here is the problem, you shouldn't send another DNS query, you should use the validated IP address as the destination, here is the standard procedure for resolving the TOCTOU + SSRF issues that I saw before in many repositories, If you have another better way please inform me whenever you reach it.

I think the solution I'm going with is the combination of a) disallow redirection in repo migrate and webhook b) do isLocalHostname validation before every time of use, because I do not think it makes sense to "lock in" IP address for webhooks, which would be a very uncomfortable UX for the product (webhook suddenly failed because I use CDN for the receiving end and the edge nodes changed their IPs). What's your thoughts?

... the rendering is really broken compare to GitHub... It does not display "quotes" correctly...

Yeah, your welcome.

For CDN problems, every time you want to send a req to a webhook, you should validate and save new IP too.

Actually, in my opinion, there is no need for validating the webhook URL in the first step when you get it from the user because a webhook's IP will be changed each time, whether it will be resolved to a public IP by CDN or a private IP by an attacker ( as you said it before )

So we should get the webhook URL and do IsLocalHostname each time and return the IP address plus previous return values.

Also, we should send only one DNS query each time and not Two, this is a rule for whatever we should do to fix this issue.

yah seems not the same with GH, so we can use backquotes.

OK, I get the part that why IsLocalHostname needs to return its resolved IP, and seems we have alignment on doing the validation at every time of use.

Two remaining questions (gosh also no lists?):

A. By "store the IP address" you mentioned previously, did you mean return the resolved IP from IsLocalHostname is all we need, or do we need actually store the IP address in the database (for example)? I don't think the latter is worth it since we will validation every time.

B. Could clarify on what "two DNS queries" means? Is it the webhook should directly sending the request to the resolved IP (after getting it from the IsLocalHostname)?

Sorry for my delay! about A You are correct. The first one is what I mean, and storing IP is not a good option.

about B again You are correct.

Awesome! Thanks for the confirmation.

I'll start working on the patch as soon as my bandwidth permits.

Hey Joe, I think you should upgrade your bandwidth :)

I'm currently capped at 24 hours a day, how do I get more xD

A new patch release including the fix to this vulnerability has been published. One thing to note though, after some back and forth, I decided to not return resolved IP address from IsLocalHostname because making a request to an IP without a hostname doesn't work for CDN or any floating IP service that relies on the domain name for a correct reverse proxying.

I also think the double DNS queries is fine since most of time DNS queries are cached.

Thanks again for the report!