-
Notifications
You must be signed in to change notification settings - Fork 34
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
[ANCHOR-403] Prevent SSRF through SEP-1 TOML redirect #1043
Conversation
Reference Server Preview is available here: |
Reference Server Preview is available here: |
Reference Server Preview is available here: |
Reference Server Preview is available here: |
…ation step before the assertions
Reference Server Preview is available here: |
Reference Server Preview is available here: |
platform/src/main/resources/config/anchor-config-default-values.yaml
Outdated
Show resolved
Hide resolved
Reference Server Preview is available here: |
Reference Server Preview is available here: |
Reference Server Preview is available here: |
Something went wrong with PR preview build please check |
Something went wrong with PR preview build please check |
Reference Server Preview is available here: |
Reference Server Preview is available here: |
Reference Server Preview is available here: |
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.
LGTM
if (!response.isSuccessful()) { | ||
throw new IOException(String.format("Unable to fetch data from %s", url)); | ||
} |
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.
Are we sure we want to throw an exception in this case? We aren't interested in the response body of a 400 transaction?
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.
thanks! I will add the response code. will that satisfy ?
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.
making additional changes per discussion in slack.
try { | ||
return new TomlContent(tomlString); | ||
} catch (Exception e) { | ||
// obfuscate exception message to prevent metadata leaks |
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.
Should we still log it on ERR level? Otherwise it's hard to debug what went wrong
* ANCHOR-403-reece-SSRF-metadata * use assertThrows * additional cleanup * remove custom exception * make sure that the log event is actually captured by adding a verification step before the assertions * temporary setting toml to url to test * just testing. ignore * try other file * patch toml parser as well * replace comment * revert * lint * debugF * revert config changes * revert defaults
* [ANCHOR-403] Prevent SSRF through SEP-1 TOML redirect (#1043) * ANCHOR-403-reece-SSRF-metadata
This PR addresses SDP-001. If an error is encountered while fetching stellar.toml via http, the exception is obfuscated with an error message to avoid ability to use the method to leak aws instance metadata via ssrf.
PR Structure
otherwise).
Thoroughness
To test this I used the Anchor Platform e2e test. I updated the profile/config.env to use stellar toml type url and value to be localhost:9000
I ran the local flask code which performed redirect to itself and simulated a metadata endpoint.
First to reproduce the error I received the exception in the to local flask app. can see the redirect and then returns a 200 for the fake metadata.
the fake metadata sent looks like this
the sep service had exception in toml parse
Throwing an exception with a customer error message obfuscates the exception message, preventing the leak