Raise if the root domain changed unexepectedly

[Wardormeur]

Due to the concatenation of domains, it's possible to spoof the information into something unexpected, leading essentially to a Server Side Request Forgery.

Instead of validating the api-key format, which could change and could be escaped by playing cat/mouse game around allowed characters/encoding, I've preferred relying on the Ruby parsing of URL post-creation to ensure that the end result is what we expect.

The error itself is not very self-explanatory, happy to change it if you have a better idea :)
Cheers

[amro]

Thank you!

…

On Fri, Feb 18, 2022 at 6:11 AM Wardormeur ***@***.***> wrote: Due to the concatenation of domains, it's possible to spoof the information into something unexpected, leading essentially to a Server Side Request Forgery. Instead of validating the api-key format, which could change and could be escaped by playing cat/mouse game around allowed characters/encoding, I've preferred relying on the Ruby parsing of URL post-creation to ensure that the end result is what we expect. The error itself is not very self-explanatory, happy to change it if you have a better idea :) Cheers ------------------------------ You can view, comment on, or merge this pull request online at: #321 Commit Summary - 5d5a61d <5d5a61d> Ensure we raise if the root domain changed and it was not an expected behavior - 4b4a038 <4b4a038> Fix the test File Changes (2 files <https://github.com/amro/gibbon/pull/321/files>) - *M* lib/gibbon/api_request.rb <https://github.com/amro/gibbon/pull/321/files#diff-e6de6089948823fe40d1318b6eb0e03495976eb490987e2933fdcd9f4968948c> (2) - *M* spec/gibbon/gibbon_spec.rb <https://github.com/amro/gibbon/pull/321/files#diff-5e98cb636e828bb2e266758df3b106d5ca8e9433707ec5352d4845d2df536782> (8) Patch Links: - https://github.com/amro/gibbon/pull/321.patch - https://github.com/amro/gibbon/pull/321.diff — Reply to this email directly, view it on GitHub <#321>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEEBDTWGPUNN6TOFIZBRLU3YSNNANCNFSM5OXSGOTA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[phosphore]

I don’t believe the fix completely resolves the issue, since the use of include (api_request.rb) is not sufficient to prevent the injection of arbitrary hostnames. An attacker could still inject a FQDN containing api.mailchimp.com as a subdomain. The end_with? String class method should be used instead.

2.6.3 :001 > computed_api_endpoint = "https://foe.api.mailchimp.com.attacker.com/?api.mailchimp.com"
 => "https://foe.api.mailchimp.com.attacker.com/?api.mailchimp.com" 
2.6.3 :002 > URI(computed_api_endpoint).host
 => "foe.api.mailchimp.com.attacker.com" 
2.6.3 :003 > URI(computed_api_endpoint).host.include?("api.mailchimp.com")
 => true 

If possible, issue an advisory using the GitHub Security Advisories feature on the repository.

[amro]

Makes sense -- thanks @phosphore! Are you up for drafting a PR? If not I can get to it soon

[amro]

I'm going to solve this a different way

[brendon]

Just wondering, how can we tell if we’re vulnerable to this? Is there a specific use of the gem to look out for?

[amro]

Latest version has a fix, but please note this would only be a problem if you accepted an API key via user input (eg via a web form)

[brendon]

Ah righty, we just use it to populate our own mail chimp account with subscribers when new users are created so the API key is hard coded.

[amro]

Then you’re good to go

…

On Wed, Apr 27, 2022 at 5:06 PM Brendon Muir ***@***.***> wrote: Ah righty, we just use it to populate our own mail chimp account with subscribers when new users are created so the API key is hard coded. — Reply to this email directly, view it on GitHub <#321 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEEBBERCZD2Z4KX5NUCMLVHGT6ZANCNFSM5OXSGOTA> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[olliebennett]

Thanks for this. Glad to hear it's not a threat when the API key is static.

I agree the end_with? option suggested should work. @amro I see you've implemented a check on the input value in cade20c so it seems that 3.4.4 should be protected entirely; is that right?

The current GitHub advisory states;

A partial fix has been introduced in version 3.4.4, however a complete fix has not yet been created

Thanks for all your efforts maintaining this gem! 👍

For reference, this vulnerability is tracked as;

• CVE-2022-27311
• GHSA-vx9g-377x-xwxq

[amro]

That's correct, I implemented what I believe is a complete fix in 3.4.4.

[enthusiasmus]

When the version 3.4.4. fixes the vulnerability, who is then closing the CVE? "bundler-audit" warns us still about this CVE
Thank you!

[amro]

Thanks, as I didn't open these CVEs I'm actually not sure how to close/resolve them. Forgive my ignorance there, but any pointers would be appreciated!

[Plsr]

Hi, at least for GHSA I was able to submit a PR so it now states 3.4.4 as the patched version GHSA-vx9g-377x-xwxq

Not sure what the process to get the same done for the CVE would be, unfortunately.

[amro]

Thank you

…

On Tue, May 3, 2022 at 2:08 AM Christian Poplawski ***@***.***> wrote: Hi, at least for GHSA I was able to submit a PR so it now states 3.4.4 as the patched version GHSA-vx9g-377x-xwxq <https://github.com/advisories/GHSA-vx9g-377x-xwxq> Not sure what the process to get the same done for the CVE would be, unfortunately. — Reply to this email directly, view it on GitHub <#321 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEEBGYPNXXHQ5336K65B3VIC7HRANCNFSM5OXSGOTA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[claudiovf]

Hi @amro I found this CVE form, that could maybe be a way to get the report updated.

also, thanks for all the work maintaining this 😎

[amro]

Thank you very much @claudiovf! I just filled out the form.

Again, for folks who run across this in the future and want to assess risk for their projects: the exploit would only be possible if you accepted unvalidated user-provided APIs (eg via form input)