-
Notifications
You must be signed in to change notification settings - Fork 22
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
access-api: email validation is insecure, currently easy to associate any target email with a malicious identity #333
Comments
I'm waiting for more discussion/agreement here but if we proceed in this direction we should also open over at https://github.com/web3-storage/w3ui/issues a corresponding ticket to track the work that would be needed on the client side. But I think the improvements here could be started independently from the client side — splitting the approval link into the GET landing page vs. actual PUT/POST handler and making sure the approval request itself is tied to some sort of temporary nonce or server secret–derived that prevents forgery of the approval itself. |
Thanks for the great write up & careful analysis @natevw 👍 I agree that we need to close the loop here, which probably means adding a tiny bit of user friction. Personally I think it's probably fine to require entering an OTP-style code into the client - that feels like the simplest thing, and it doesn't feel surprising as a user since it's pretty similar to 2FA flows that people have probably seen before. |
@natevw I think the proposed solution is a good and sufficient first step to fix this. Are you interested in working on it? I'd like it to be resolved ASAP. Thanks for raising and thanks especially for proposing a solution. |
@alanshaw Yeah, I think this would be a great scope for me to tackle. I'll start proceeding as above but to re-iterate I'm thinking roughly:
|
@natevw thank you for the feedback and proposal. I wrote some notes about this here https://purrfect-tracker-45c.notion.site/Email-auth-flow-d33f0715024a47c18a8114ba284e9c07. I not 100% sure that adding a new step to do a POST is the way to go here but there's definitely improvements to be done. |
@hugomrdias re. your:
This is what you captured as "Avoid SafeLinks and other auto open email client stuff" in the notes, and is ultimately just an HTTP standards issue. Any
And:
And since afaict it doesn't require any changes but to the
is to me the "low hanging fruit" here and I'm hoping to submit a PR for it asap. |
The thing here is that clicking on the email link does not change any state, it works just like any other GET just return whats there already in this case the ucan delegation. Either using websockets or the http response that also includes the delegation as string and QRcode. |
the email validation approval process is now split into two stages: a GET request with no side effects except to load a page that then auto-submits a POST request to actually continue the flow. this fixes the API to follow proper API semantics and thus starts addressing some of storacha#333 and presumably fixes all of storacha#348
This is mostly covered by #348 (→ #398) and the "validation phrase" part of #347 (→ #399). The UI side has a placeholder issue storacha/w3ui#307 as well. |
the email validation approval process is now split into two stages: a GET request with no side effects except to load a page that then auto-submits a POST request to actually continue the flow. this fixes the API to follow proper API semantics and thus starts addressing some of #333 and presumably fixes all of #348
The email validation approval process is now split into two stages: a GET request with no side effects except to load a page, that then auto-submits a POST request to actually continue the flow. ## Summary of problem This fixes the API so as to follow [proper HTTP semantics](#333 (comment)): > The purpose of distinguishing between safe [i.e. like GET] and unsafe [like PUT/POST] methods is to allow automated retrieval processes (spiders) and cache performance optimization (pre-fetching) to work without fear of causing harm. In addition, it allows a user agent to apply appropriate constraints on the automated use of unsafe methods when processing potentially untrusted content. That is, a `PUT` or `POST` (rather than a `GET`) **must** be the method used in order to do things like * cause a message to be sent (forwarding a UCAN delegation via a separate connection's websocket) * cause an untrusted keypair to be associated with a billable email address (which is the outcome of that forwarding, in practice!) Fixing the HTTP semantics should address all of #348, and is the first step to addressing the security concerns in #333. ## Summary of solution Clicking (or scanning/pre-fetching/previewing/etc.) the link in the email no longer finishes the validation process. Instead, it loads a (harmless to scan/pre-fetch/preview) landing page which simply says "Validating Email" while using JavaScript to auto-complete the process. This patch is able to fix the core HTTP semantics in a very self-contained way: * no changes needed to the email templates * will not break any existing unexpired links at the moment it is deployed * is essentially the exact same UX from a user's perspective (they might notice just a little extra blink) * does degrade gracefully if user has JS disabled, and any non-browser clients could still trigger the `POST` ± just as easy as before * no changes needed on the `w3ui` side for this part of the email validation improvements --------- Co-authored-by: Nathan Vander Wilt <[email protected]>
The email validation approval process is now split into two stages: a GET request with no side effects except to load a page, that then auto-submits a POST request to actually continue the flow. ## Summary of problem This fixes the API so as to follow [proper HTTP semantics](#333 (comment)): > The purpose of distinguishing between safe [i.e. like GET] and unsafe [like PUT/POST] methods is to allow automated retrieval processes (spiders) and cache performance optimization (pre-fetching) to work without fear of causing harm. In addition, it allows a user agent to apply appropriate constraints on the automated use of unsafe methods when processing potentially untrusted content. That is, a `PUT` or `POST` (rather than a `GET`) **must** be the method used in order to do things like * cause a message to be sent (forwarding a UCAN delegation via a separate connection's websocket) * cause an untrusted keypair to be associated with a billable email address (which is the outcome of that forwarding, in practice!) Fixing the HTTP semantics should address all of #348, and is the first step to addressing the security concerns in #333. ## Summary of solution Clicking (or scanning/pre-fetching/previewing/etc.) the link in the email no longer finishes the validation process. Instead, it loads a (harmless to scan/pre-fetch/preview) landing page which simply says "Validating Email" while using JavaScript to auto-complete the process. This patch is able to fix the core HTTP semantics in a very self-contained way: * no changes needed to the email templates * will not break any existing unexpired links at the moment it is deployed * is essentially the exact same UX from a user's perspective (they might notice just a little extra blink) * does degrade gracefully if user has JS disabled, and any non-browser clients could still trigger the `POST` ± just as easy as before * no changes needed on the `w3ui` side for this part of the email validation improvements --------- Co-authored-by: Nathan Vander Wilt <[email protected]>
The
access-api
package's/validate-email
endpoint does not provide sufficient authentication or authorization for its anticipated purpose.Currently the flow works roughly as follows:
The problem
The flow as implemented is not secure/trustworthy. There are two significant flaws, closely related but kept separate to highlight the need for improvements on both sides:
GET
request — this breaks HTTP expectations generally and will be particularly problematic if the user has any sort of link preview enabled or if the email goes through a provider that itself scans links (e.g. the MS Outlook Safelink protection service). This is the "authentication" side: just because a URL was fetched does not mean that the intended recipient was the one that actually fetched it.In most cases where email login is used, whoever opens the link gains access to the underlying session/identity. But in this case opening the link gives someone else that access! A malicious actor Eve simply provides Alice/Bob's email address — now Eve just needs to wait for Alice's computer to generate a page preview for the link, or for Bob to click the link, to gain access to their account.
[Currently this is not a big deal because there's not really much of any "account" to access, and things like upload lists and such are siloed per-identity rather than per-email. But the anticipated purpose is to link actions done by that identity to the email, otherwise the apps wouldn't be attempting to validate email at all!]
Proposed solution
This came out of a discussion we had at https://filecoinproject.slack.com/archives/C02BZPRS9HP/p1673387411040389 around these concerns (which were also brought up in storacha/specs#26 earlier as well). There needs to be some way to "close the loop" and ensure that maliciously requested privileges associated with an email address aren't accidentally approved.
I would propose the following improved email validation flow:
PUT
/POST
request to the API, which can then broadcast the delegated token thing to the original requestor. [Note that this is actually the link/request data that can't be predictable by anyone without the original link; i.e. need to ensure that an attacker can't just skip straight to this step and approve the association without actually proving they have access to the claimed email address]This approval form both resolves the HTTP/"Safelinks" problem and provides the authenticated end-user a way to review the request more thoroughly before approving. It does not necessarily require any changes to the client library(ies) to just solve the
GET
vs.PUT
/POST
problem. But to really solve the authorization side the client library will need to be updated to show something that the user matches up with a copy on the approval form. It could actually be a combo of:Alternatives considered
The workflow above isn't 100% foolproof! A user could still somehow/someway plow through and approve something they shouldn't have if they were tricked or in a hurry or whatnot. Some potentially more robust solutions might be:
There might be some sort of crypto-ey out-of-the-box-thinking de-centralized solution lurking here too, but we might prefer loading the problem more on the business side to keep the UX simpler!
The text was updated successfully, but these errors were encountered: