-
-
Notifications
You must be signed in to change notification settings - Fork 11k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Server-Side Request Forgery Vulnerability (CVE-2024-39338) #6463
Comments
Hi. Trying to understand this vulnerability better. Mixed use of relative and absolute (including, protocol-relative) URLs Axios will see
Of course this may not align with application developer's expectation (ie. to be relative path). But it feels like that's not Axios' problem to solve. Axios could potentially warn this use pattern (ie. use of baseUrl with non relative path), but it seems too harsh for axios to dictate this. Impact of using protocol-relative URLs The following code kinda forces
Is this the main concern here? I guess an attacker could potentially move away from non-http protocol (eg. local file location, deemed safe) to http protocol to deliver remotely located malicious files into the target system. Expected behavior
If this is indeed a good solution, then
|
Guess most of us are here due to Snyk's action lately ;) I made a draft PR to materialize @SilverSting's idea above. Do lmk if that works out in the long-term, fellas Edit: regression test added |
Several auditing tools are reporting this issue as of today, including |
This vuln report seems like alerting.. all the alerts.. But I'm quite confused (maybe not enough caffeine today) Question: "using unsanitized input/data as string in URLs" made towards elsewhere is insanely dangerous in any way? on any library any place and any location. I somehow cannot thing that axios is wrong here. const unsanitizedVar = '/google.com'; // was expecting something like '12345'
const ANY_HTTP_LIBRARY = require(something, maybe axios);
const ANY_EXTERNAL_ENDPOINT = 'https://somewhere.innocent';
this.httpclient = ANY_HTTP_LIBRARY.create({
baseURL: ANY_EXTERNAL_ENDPOINT,
});
// Isn't >> THIS -the imaginary use case below- << the problem?
this.httpclient.request({ url: `/${unsanitizedVar}` }); even if you can prevent this value from diverting request to another target, you cannot prevent it from doing other kind of malicious things with the assumptions of this vuln report (and the fix).. with my current understanding of it. Axios's (current) baseURL behavior seems like a similar behavior of PS. (edit) Tested browser behavior, using
Same Vuln report would apply to (the design of?) |
|
Hi @MikeMcC399, Thanks for replying. |
The problem here isn't necessarily the behaviour of axios in the browser, it's the behaviour of it in a server-side context where the server is making requests from a private network. As the report says, in a server-side context, there is no base protocol for a protocol-relative URL to be relative to. Therefore, a protocol-relative URL should not be considered valid in that context. How the attack could be constructed is sort of secondary to that. If it doesn't make sense in that context, we should disallow it. |
Considering @furkanmustafa and @petewalker comments, wouldn't it be the case to update your PR to check if the code is running on server side? So the browser behavior would remain the same... |
I am looking into it |
Please check the note in the PR |
@hainenber I see that #6539 was merged, doesn't that make the behavior of axios is inconsistent with https://datatracker.ietf.org/doc/html/rfc3986#section-4.2 ? |
Yes, thats correct. Either me or Jay will need to come up with following PRs to enable protocol relative URLs on client-side only. Edit: updated with correct word. Thanks Cedric! |
This comment was marked as resolved.
This comment was marked as resolved.
Oh yeah, I accidentally a word there. Thanks for the correction! |
Updated the public advisory accordingly - github/advisory-database#4685 |
will audit tools automatically reference this new version as the fixed one? |
Yes. Snyk and GitHub both have it referenced. |
About handling of relative URLs, in respect of a predefined base URL, could be a design decision. As @levpachmanov shared above, the expected behavior has been defined in an RFC. It's of course OK to change the designed behavior of Axios. Axios does not state any guarantee towards RFC compliance (afaik). But, the vulnerability report is incorrect IMO, and making this change blindly just for this vulnerability report doesn't make sense. This vulnerability assumes that unsanitized input being passed around for external network access. And also has nothing to do with "protocol-relativeness".
The private-ness of the network does not matter if it is a "server" or "client"(browser), they BOTH reside in private networks. I don't think such a distinction would ever make sense.
A design change discussion, of course is up to library's designers. I don't have a comment on that. Although, all other mechanisms behave accordingly, in the context of predefined baseURL. When there is no predefined baseURL, it should be OK to reject as Invalid URL. Try Node JS's new URL('24791') // TypeError: Invalid URL
new URL('/24791') // TypeError: Invalid URL
new URL('//24791') // TypeError: Invalid URL
const userApiBase = 'https://my.api.local/users/';
new URL('24791', userApiBase) // https://my.api.local/users/24791
new URL('/24791', userApiBase) // https://my.api.local/24791
new URL('//24791', userApiBase) // https://0.0.96.215/
new URL('//www.google.com', userApiBase) // https://www.google.com/ if Axios uses standard On the security aspect of this,
It will not remotely matter if the protocol-relative behavior is carefully cherry-pick removed on a specific execution environment ("server context"), given other kind of inputs. Let me share some examples; the given example was this; this.axios = axios.create({
baseURL: 'https://userapi.example.com',
});
//userId = '12345';
userId = '/google.com'
this.axios.get(`/${userId}`) // <= buggy application code
// before MR #6539 = `https://google.com`;
// after MR #6539 = `https://userapi.example.com/google.com`; what if my code was; this.axios = axios.create({
baseURL: 'https://userapi.example.com/',
});
//userId = '12345';
userId = 'http://localhost:8080'
this.axios.get(userId) // <= buggy application code
// before MR #6539 = `http://localhost:8080`;
// after MR #6539 = `http://localhost:8080`; did the developer expect that this should have gone to as you can see above, if I omit a >> I << am, -as an application developer-, at fault, for not sanitizing my input. Not the library. Not the standard way of things work. Same thing can be demonstrated for all kinds of libraries, all network protocols, "connection strings", and so on. Do not put unsanitized input. It's not library's work to make it "safer", not not possible for library to make is "safe"/"safer" any way. I hope I made my point clear. In short, introducing this behavior inconsistency to Axios is not necessary, is contuary to standards, and is NOT improving any security. (Edit: formatting, example fix) |
If you re-run
|
@justinmchase what you could have is npm cache not knowing that |
Hey, I got the update thanks! I think I literally caught it between the release and the fix. I just got super unlucky, thank you for the super prompt release. |
Describe the bug
Axios is vulnerable to a Server-Side Request Forgery attack caused by unexpected behaviour where requests for path relative URLS gets processed as protocol relative URLs.
This could be leveraged by an attacker to perform arbitrary requests from the server, potentially accessing internal systems or exfiltrating sensitive data.
To Reproduce
In this vulnerable code snippet, the developer intended to invoke a path relative to the base URL, however it allows an attacker to craft a malicious protocol-relative URL which is then requested by the server. Given protocol-relative URLs are not relevant server-side as there is no protocol to be relative to, the expected result would be an error. Instead, a valid URL is produced and requested.
Example Vulnerable Code
Expected Output (Prior to axios 1.3.2):
Developer might also have expected:
Observed Output:
This behaviour is potentially unexpected and introduces the potential for attackers to request URLs on arbitrary hosts other than the host in the base URL.
The code related to parsing and preparing the URL for server-side requests, prior to version 1.3.2, only passed one argument to the Node.js URL class.
Version 1.3.2 introduced
http://localhost
as a base URL for relative paths (#5458)As protocol-relative URLs are considered to be absolute, the config.baseURL value is ignored so protocol-relative URLs are passed to the URL class without a protocol. The node.js URL class will then prepend the protocol from
'http://localhost'
to the protocol-relative URL.For example
Code snippet
No response
Expected behavior
An error could be raised when attempting to request protocol-relative URLs server-side as unlike in a client-side browser session, there is no established protocol to be relative to.
Axios Version
No response
Adapter Version
No response
Browser
No response
Browser Version
No response
Node.js Version
No response
OS
No response
Additional Library Versions
No response
Additional context/Screenshots
No response
The text was updated successfully, but these errors were encountered: