-
Notifications
You must be signed in to change notification settings - Fork 621
fix(ui): server-side request forgery api x-endpoint #2920
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
base: main
Are you sure you want to change the base?
Conversation
// SSRF protection: Only allow URLs from trusted hostnames | ||
const allowedHostnames = [ | ||
'example.com', | ||
'cdn.example.com', | ||
// Add other trusted hostnames as needed | ||
]; | ||
let parsedUrl; | ||
try { | ||
parsedUrl = new URL(url); | ||
} catch (e) { | ||
httpLogger.logger.error({ message: 'Invalid URL', url }); | ||
res.status(400).json({ type: undefined, error: 'Invalid URL' }); | ||
return; | ||
} | ||
if (!allowedHostnames.includes(parsedUrl.hostname)) { | ||
httpLogger.logger.error({ message: 'Disallowed hostname', url, hostname: parsedUrl.hostname }); | ||
res.status(403).json({ type: undefined, error: 'Disallowed hostname' }); | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main characteristic of this resource is that we do not know the domain of the server that serves the asset in advance. Therefore, we cannot determine the allowed hostname array at all.
const ALLOWED_ENDPOINTS = [ | ||
appConfig.apis.general.endpoint, | ||
// Add other allowed endpoints here, e.g.: | ||
// "https://api.example.com", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This resource is not used in production, so the changes are not necessary. If we want to keep them, please add all available API endpoints to the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I don't see how this resource can be requested using the endpoint from user input.
// Simple Ethereum address validation (42 chars, starts with 0x, hex) | ||
const isValidAddress = /^0x[a-fA-F0-9]{40}$/.test(address); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that if the address is invalid, the page will throw a 422 error, so this code will never execute.
frontend/pages/api/media-type.ts
Line 15 in bb50fc6
frontend/ui/address/details/AddressSaveOnGas.tsx
Line 35 in bb50fc6
frontend/nextjs/utils/fetchProxy.ts
Lines 55 to 59 in bb50fc6
Directly incorporating user input in the URL of an outgoing HTTP request can enable a request forgery attack, in which the request is altered to target an unintended API endpoint or resource. If the server performing the request is connected to an internal network, this can give an attacker the means to bypass the network boundary and make requests against internal services. A forged request may perform an unintended action on behalf of the attacker, or cause information leak if redirected to an external server or if the request response is fed back to the user. It may also compromise the server making the request, if the request response is handled in an unsafe way.
fix this SSRF vulnerability, we must ensure that user input cannot arbitrarily control the destination of the outgoing HTTP request. The best way to do this is to restrict the possible values of the base endpoint (i.e., the host and protocol) to a fixed allow-list of trusted endpoints. Instead of using the value of the
x-endpoint
header directly, we should check it against a list of allowed endpoints (e.g., from config), and only use it if it matches. Otherwise, we should reject the request or fall back to a safe default. This validation should be performed inpages/api/proxy.ts
where the endpoint is selected, before constructing the URL. The rest of the code can remain unchanged, as the taint is stopped at the source.Steps:
pages/api/proxy.ts
.x-endpoint
, check if its value is in the allow-list.The best way to implement this is to add a validation step in
AddressSaveOnGas
before making the fetch request. If the address is invalid, the function should return early or throw an error. This change should be made inui/address/details/AddressSaveOnGas.tsx
, specifically in thequeryFn
function where the fetch is performed.The changes should be made in
pages/api/media-type.ts
:URL
from the standard library).References
SSRF
CWE-918
Checklist for PR author