Skip to content

API should never transparently interpret "account" as a seed (Version: 1.9.4) #4337

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

Closed
intelliot opened this issue Nov 9, 2022 · 3 comments · Fixed by #4404
Closed

API should never transparently interpret "account" as a seed (Version: 1.9.4) #4337

intelliot opened this issue Nov 9, 2022 · 3 comments · Fixed by #4404
Assignees
Milestone

Comments

@intelliot
Copy link
Collaborator

Issue Description

In the API, seeds can be used in place of accounts. For example, when calling account_info, you can pass "account": "foo". The string "foo" is treated like a seed, so the method returns actNotFound (instead of actMalformed, as most developers would intuitively expect).

In the early days, this was a convenience to make testing easier. However, it is no longer a good idea, and it allows for poor security practices. It is controlled by the strict option on some methods.

Note, Clio currently does not behave this way. This results in a subtle incompatibility between Clio and rippled, even though Clio is intended to support (and eventually replace) the rippled API. The current thinking from the Clio team is that Clio should not behave this way, i.e. it should not "support" the ability to provide a seed in the "account" field of API requests.

Steps to Reproduce

Call account_info with "account": "foo".

Expected Result

The method should return actMalformed since "foo" is not a valid account.

Actual Result

The string "foo" is treated like a seed, so the method returns actNotFound.

Notes

This behavior, since it is available on public-facing servers like s2, allows bad actors to use servers to try to brute-force account seeds, for "free". However, this is not a worry, as accounts are not brute-forceable regardless. While we should have spam prevention (e.g. rate limiting), trying many seeds (with no entropy-reducing "hints") is not a viable attack for attempting to compromise an account.

Removing this behavior from the API is a breaking change. We could argue that it shouldn't be done without bumping the API version, and we are open to that (depending on user demand). However, in this instance, we doubt that anyone is using the API in the "legacy" way, and it is a huge security hole. It allows/encourages users to send secrets to places where they are not needed, where they could end up in logs, error messages, etc. There's no reason to take such a risk with a seed/secret, since only the public address is needed.

We prefer to break the insecure behavior (by making it safer), even if it means that some implementations may experience a breaking change and need to update. (And even that is unlikely.)

@intelliot intelliot added API Change Good First Issue Great issue for a new contributor Documentation README changes, code comments, etc. Medium Priority labels Nov 9, 2022
@jatinsahijwani
Copy link

I want to work on this issue, please guide me, i am a beginner

@intelliot
Copy link
Collaborator Author

@jatinsahijwani welcome! What kind of help are you looking for?

@intelliot
Copy link
Collaborator Author

will be fixed by #4404

@intelliot intelliot moved this from 🏗 In progress to 👀 Needs review in Core Ledger Feb 14, 2023
@intelliot intelliot added this to the 1.10.1 milestone Feb 14, 2023
@intelliot intelliot modified the milestones: 1.10.1, 1.11.0 Mar 15, 2023
@intelliot intelliot moved this from 👀 Needs review to 🏗 In progress in Core Ledger Apr 10, 2023
@intelliot intelliot linked a pull request Apr 20, 2023 that will close this issue
7 tasks
intelliot pushed a commit that referenced this issue May 17, 2023
The API would allow seeds (and public keys) to be used in place of
accounts at several locations in the API. For example, when calling
account_info, you could pass `"account": "foo"`. The string "foo" is
treated like a seed, so the method returns `actNotFound` (instead of
`actMalformed`, as most developers would expect). In the early days,
this was a convenience to make testing easier. However, it allows for
poor security practices, so it is no longer a good idea. Allowing a
secret or passphrase is now considered a bug. Previously, it was
controlled by the `strict` option on some methods. With this commit,
since the API does not interpret `account` as `seed`, the option
`strict` is no longer needed and is removed.

Removing this behavior from the API is a [breaking
change](https://xrpl.org/request-formatting.html#breaking-changes). One
could argue that it shouldn't be done without bumping the API version;
however, in this instance, there is no evidence that anyone is using the
API in the "legacy" way. Furthermore, it is a potential security hole,
as it allows users to send secrets to places where they are not needed,
where they could end up in logs, error messages, etc. There's no reason
to take such a risk with a seed/secret, since only the public address is
needed.

Resolves: #3329, #3330, #4337

BREAKING CHANGE: Remove non-strict account parsing (#3330)
@intelliot intelliot moved this from 🏗 In progress to 🏎 Released in 1.12 in Core Ledger Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants