Add client domain corresponding signature when needed. #236

gianfra-t · 2024-10-28T19:13:28Z

Original issue: #225.

This PR also adds partially ARS integration: #172 and #173. Partially because given a UI particularity of Anclap, the ephemeral cannot perform the offramp because it doesn't have ARS token when created.

Main addition

Adds a new endpoint to the signer service, which upon request, will sign the corresponding challenge from the anchor. Also modifies the UI logic to add the extra client_domain parameter, and append the signature to the response when required.

Security

Although the challenge transaction is sent by the UI, we perform corresponding challenge operations checks.

netlify · 2024-10-28T19:13:44Z

✅ Deploy Preview for pendulum-pay ready!

Name	Link
🔨 Latest commit	`91e84e3`
🔍 Latest deploy log	https://app.netlify.com/sites/pendulum-pay/deploys/673359adb76f3900080591ce
😎 Deploy Preview	https://deploy-preview-236--pendulum-pay.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

gianfra-t · 2024-10-28T19:15:52Z

signer-service/src/api/services/stellar.service.js

+
+  const transactionSigned = new TransactionBuilder.fromXDR(challengeXDR, NETWORK_PASSPHRASE);
+  if (transactionSigned.source !== signingKey) {
+    throw new Error(`Invalid source account: ${transactionSigned.source}`);


We are fetching the signingKey of the anchor from our own .toml url. I see no vector from which someone could send a malicious transaction for us to sign.

This, in conjunction with the 0 sequence number check.

gianfra-t · 2024-10-28T19:34:23Z

@pendulum-chain/devs added to the security concern, we of course have this endpoint open, so although we will not sign any transaction that would withdraw funds, any person could request this signature and act in our regard as Client Application.

We discussed at some point for the subsidized endpoints that we could implement checks that the user has indeed transferred to the squidrouter contract, for it to be enabled.

This case will be harder to protect, since this operation is performed at the beginning and there is no guarantee that the user will go through with the off-ramp with us.

If we then reverse the order of operations and make the user first sign the commitment transactions to swap, before performing the sep-10, then we run the risk that the funds will get stuck because the user did not or could not complete the KYC.

Therefore the best option I see is to modify the code such that the UI first signs the sep-10 and later the backend sends the challenge response, potentially relaying back the jwt to the UI if we think this is safe, I believe that for a sep-24 should be, but we need to be sure that this jwt cannot be used for other endpoints (such as a sep-12) to identify users at will (by choosing the unique memo).

gianfra-t · 2024-10-28T20:03:04Z

Following on the above, I think the JWT is safe judging by the challenge, which has in the transaction operations the description of what account acts as client, client domain.

TorstenStueber

Let's think about the attack scenario and solutions here. But I don't think that we need to implement them yet (more below).

So what is the attack vector. I think it is just that someone could impersonate us by letting their own sep-10 challenge be signed by us. There is no way to prevent that with this endpoint as the endpoint can't know who created the challenge.

To make this secure, I think that the endpoint itself needs to request the sep-10 challenge. So the frontend does not request it directly but asks the service to request it. The backend can trust the anchor and the frontend can trust the backend.

However, this is still not safe as the backend can't trust the frontend. So I think that in order to make it secure, the backend should never return the resulting sep-10 token to the frontend. This means that the backend needs to initiate sep-24 as well. Would this then prevent the attack scenario? I think so.

Anyway, this could be too much effort for now. I think we should still keep it simple. Let's rather maintain a list of known weaknesses and solutions and prioritize them as required.

TorstenStueber · 2024-10-29T07:31:01Z

signer-service/src/api/helpers/anchors.js

@@ -0,0 +1,27 @@
+const fetchTomlValues = async (TOML_FILE_URL) => {


Since TOML_FILE_URL here is a function argument and not a global constant, I would rather use normal camelCase.

TorstenStueber · 2024-10-29T07:32:08Z

signer-service/src/api/routes/v1/stellar.route.js

@@ -8,4 +8,6 @@ router.route('/create').post(validateCreationInput, controller.createStellarTran

 router.route('/payment').post(validateChangeOpInput, controller.changeOpTransaction);

+router.route('/sep10').post(controller.signSep10Challenge);


Shall we also add a validation function that checks for a proper challengeXDR and outToken in the request body?

Yes, I will add checks now regarding the proper operations that the challenge needs to have. This way we can then fully trust the fronted in that it is not sending any incorrect challenge.

I think I misunderstood this comment, you mean the validator only to check if it's not empty right?

Yes, just the simple validator like the other endpoints.

TorstenStueber · 2024-10-29T07:52:26Z

src/services/anchor/index.ts

+  if (requiresClientDomain) {
+    urlParams = new URLSearchParams({
+      account: accountId,
+      client_domain: 'https://satoshipay.io/.well-known/stellar.toml',


Would be good to put this url to a configuration file instead of here in the code.

TorstenStueber · 2024-10-29T07:53:24Z

src/services/signingService.tsx

+  challengeXDR: string,
+  outToken: OutputTokenType,
+): Promise<ClientDomainSep10Response> => {
+  const response = await fetch(`http://localhost:3000/v1/stellar/sep10`, {


This domain needs to be changed of course.

Yes, it's for ease of testing.

gianfra-t · 2024-10-29T12:01:12Z

However, this is still not safe as the backend can't trust the frontend. So I think that in order to make it secure, the backend should never return the resulting sep-10 token to the frontend. This means that the backend needs to initiate sep-24 as well.

@TorstenStueber I thought the same, but then I proposed this hypothesis. If we add these extra checks I think we can trust the frontend, at least in terms of security. The first manage_data operation you sign specifies the account trying to do the sep-10. It is not stated (or I couldn't find) from the docs if the JWT obtained from such challenge can be used for something else, but it would not be logical. Still to be seen if this will also work with the 1 account + memo solution.

On the other hand, we could initiate the sep-24 but then for submitting the user needs the token, so it would have to be submitted by us as a proxy, seems quite complicated (although it would solve the iframe issue probably).

But there is the impersonating issue. Even with all those checks in place, a "malicious" application could request the authentication with this endpoint, and later just send the funds directly to the anchor's deposit account while benefiting from our special fees, because there are no funds commitment. Is there even a way to prevent this fundamentally?

gianfra-t · 2024-10-29T13:41:07Z

signer-service/src/api/services/stellar.service.js

+    throw new Error('The first operation should be manageData');
+  }
+  // We don't want to accept a challenge that would authorize as Application client account!
+  if (firstOp.source !== clientPublicKey || firstOp.source == signingKey) {


I believe this is the key check for security regarding this topic and subsequent comments.

We return the signature that should only be valid to authenticate the user public key.

gianfra-t · 2024-10-29T14:40:20Z

In relation with this issue which I think is very connected (it may also need the client_domain and signature from backend), I tested locally a sep-10 with a variant of this PR containing a memo field.

In that case we would need to add a check that the memo field on the challenge exists and is not null, since the operations themselves won't be able to distinguish between the master account and the derived ones using a memo. This returns a JWT where the sub field is account:memo as expected.

I also attempted a sep-24 flow with the JWT containing the memo field, but had no luck yet on both anchors.

TorstenStueber · 2024-10-29T18:57:25Z

Is there even a way to prevent this fundamentally?

The problem in my opinion is not that the backend receives an incorrect challenge – of course it is still reasonable to make all required test to check the validity of the challenge (I did not do when I wrote the sep-10 code initially).

The real problem is impersonating SatoshiPay/Vortex. Any malicious app could send their own SEP-10 challenge to use for cosigning. How can we detect that? I think only if we initiate SEP-10 from the backend itself. You are right, we don't need to execute SEP-24 on the backend, the resulting JWT would already contain only our own account (if we don't use the ephemeral account for SEP-10 anymore).

gianfra-t · 2024-10-29T19:22:07Z

When you say that an "app could send their own SEP-10 challenge" what do you mean? Because we will detect before signing all the signatures and memos. This is very connected also with the polygon verification and we should solve both at the same time, I think, at least the ideal solution for the future.

It doesn't matter who starts the sep-10 but as long as we verify the challenge's operations and that the requester is in control of the polygon address (and matches the memo), we would be secure. What we cannot verify is that the user or the bad actor app will make use of our runway after we cosigned, or will do it on it's own.

The only way I see to solve this (besides being in control of the funds) is to implement the contract we discussed way at the beginning of the prototype. User would first transfer the tokens to that contract, they will be paused until the backend gives the "green light" to proceed.

We can ensure that the polygon user is in control of the account given this first transfer happened in the first place(so no challenge), and use the account itself for memo. When the user finishes the KYC, we allow the contract to proceed with the swap, etc.
The only issue is, if the user doesn't want to go through with the KYC, we can return the funds, but this will have a network cost.

TorstenStueber · 2024-10-29T19:39:17Z

When you say that an "app could send their own SEP-10 challenge" what do you mean?

Another app could request a SEP-10 challenge from the anchor, providing our client_domain. Then they could just call our signing-service endpoint to add the SIGNING_KEY signature to it. Our signing-service endpoint has no way to determine whether this comes through our own app or another app (all the other checks you mention wouldn't help). The 3rd party app would then have a SEP-10 token containing our client_domain.

I don't think that we need a contract to solve it if our backend initiates the SEP-10 itself. Right now of course the backend does not control the rest of the flow and it would only start to be secure when the backend executes all the remaining flow, but we will implement it eventually that way.

gianfra-t · 2024-10-30T12:12:44Z

Yes totally, I think we are talking about the same potential issue, I also agree that is enough for now, I wanted to be sure that there exists a secure modification that we can do with some extra effort, even if we don't do it now.

So how should we proceed with this now? Once the anchor accepts our .toml and we test this properly, do we go with this implementation?

TorstenStueber · 2024-11-01T21:27:45Z

I am fine with using the implementation you did here. Instead of optimizing for the ideal solution now, let's rather move forward and improve this later.

gianfra-t · 2024-11-06T17:35:20Z

@pendulum-chain/devs I merged the changes from #201, the client_domain is working now with both Anchors.

gianfra-t · 2024-11-07T13:57:37Z

Update from my last comment. Solution in #201 is not viable anymore, so I removed the corresponding parts.

I kept ARS "vanilla" integration with ephemeral for now. It may be the case that they fix their UI such that this actually works.

gianfra-t · 2024-11-07T17:20:16Z

signer-service/src/api/services/sep10.service.js

+  }
+
+  // Only authorize a session that corresponds with the ephemeral client account
+  if (firstOp.source !== clientPublicKey) {


clientPublicKey is supplied by the user, so this doesn't add protection against the UI. It is mostly an extra check to ensure the challenge operation is consistent.

gianfra-t · 2024-11-07T17:24:38Z

signer-service/src/api/services/sep10.service.js

+    throw new Error('Transaction must contain a client_domain manageData operation');
+  }
+
+  let clientDomainSignature;


This is our barrier against signing for Anchors that we don't to sign client domain. Since above we also fetch the anchorSigningKey corresponding to the token type, the UI shouldn't be able to game this.

signer-service/src/api/middlewares/validators.js

signer-service/src/api/services/sep10.service.js

ebma · 2024-11-07T17:39:31Z

signer-service/src/api/services/sep10.service.js

+  if (!hasClientDomain && clientDomainEnabled) {
+    throw new Error('Transaction must contain a client_domain manageData operation');
+  }


Should we alleviate this restriction as it's marked as optional in the protocol?

It's mostly for us, and check what the UI client is doing. We can alleviate it, but the intention is that if the UI asks for a challenge corresponding to a specific token/anchor, let's say eurc, we check that we allow signing client_domain proof for it. Only if we allow it, and the operation correspond to what is expected, we sign.

It may be a bit of too defensive and redundant. We can analyze removing it if it doesn't add value. But it's not too much code.

I should mention that the client_domain operation is defined by the initial GET request to sep-10, if it is defined there, it will appear in the operations.
And right now we (the backend) don't control this.

Alright, well I guess if we always want to use the client_domain we can just keep the check.

This is also useful, I think, when we want to also disable some anchor. Yes I also understand we always want to send the client_domain. It's defensive, but for example right now we are experiencing a failure if we specify the client_domain with Anclap. So we disable it here and we can be sure it's not a misconfiguration on the frontend.

src/constants/tokenConfig.ts

TorstenStueber · 2024-11-07T17:55:22Z

signer-service/src/api/services/stellar.service.js

@@ -7,7 +7,7 @@ const {
  STELLAR_EPHEMERAL_STARTING_BALANCE_UNITS,
 } = require('../../constants/constants');
 const { TOKEN_CONFIG, getTokenConfigByAssetCode } = require('../../constants/tokenConfig');
-
+const { fetchTomlValues } = require('../helpers/anchors');


This is not used in this file.

TorstenStueber · 2024-11-07T23:22:27Z

signer-service/src/api/services/sep10.service.js

+  }
+
+  //  the web_auth_domain and client_domain operation must be present
+  if (!hasWebAuthDomain) {


According to this section, it does not seem to be mandatory that the challenge transaction has a web_auth_domain entry.

I understand from here that is not optional. You get that idea from step 7?

It sounds contradictory, I saw that. I thought that this is some backward compatible definition, i.e., an older version did not have this. Whoever creates the challenge should add this field in the current version. Whoever checks the challenge should accept also the older challenge without that field.

But who knows. We can just keep it as it is as long as it works with both anchors.

TorstenStueber · 2024-11-07T23:28:37Z

src/services/anchor/index.ts

-    amount: sessionParams.offrampAmount,
-  });
+  let sep24Params;
+  if (outputToken == 'ars') {


I would again avoid token specific hard coded logic here, but I understand that this is really specific logic that is only valid for Anclap and we might remove it again once they implement this compliantly (account is the SEP-24 instead of the SEP-10 account).

Yes, me too. But Anclap, therefore ars, seems to be the only one requiring the account on the sep-24. It seems from our current conversations that it will continue to be this way.

gianfra-t · 2024-11-08T20:47:13Z

@pendulum-chain/devs I believe I fixed and added the changes requested.

A word on Anclap:
I particularly set this supportsClientDomain flag since the combination of ephemeral + client_domain is not working with them. Example:

Sep-10 request:

api.anclap.com/auth?account=GBFQLD3D3HV2NKBQLDXRHFAIKMREBPSNVSQP5CQYXD6GZHRLMSL2FIOM&client_domain=satoshipay.io`

Sep-10 response:

LBL_ERROR_VALIDATING_CHALLENGE_TRANSACTION => There is more than one client signer on a challenge transaction for an account that doesn't exist"

I believe the error is clear, a client cannot sign a transaction when an account does not exists (ephemeral never exists at this stage of course).

Luckily the master + memo + client_domain PR works. The recurrent user identification is being fixed by Anclap.

TorstenStueber

👍

gianfra-t added 3 commits October 9, 2024 11:17

add ars icon and partial token config

820b2f7

test anclap, add extra params to sep24 request

a94272f

add client domain corresponding signature when needed

76a2335

gianfra-t requested a review from a team October 28, 2024 19:13

gianfra-t commented Oct 28, 2024

View reviewed changes

gianfra-t mentioned this pull request Oct 28, 2024

Add client_domain to the information sent to the anchor #225

Open

1 task

TorstenStueber reviewed Oct 29, 2024

View reviewed changes

add server side checkings of challenge

6cdf65a

gianfra-t commented Oct 29, 2024

View reviewed changes

gianfra-t force-pushed the sign-client-domain branch 2 times, most recently from 745dbbc to 6cdf65a Compare October 29, 2024 14:32

add potential memo support

5971583

gianfra-t mentioned this pull request Oct 29, 2024

Change the SEP24 flow to authenticate recurring users #237

Open

1 task

gianfra-t and others added 2 commits October 30, 2024 10:41

add conditional account for sep24 params

9a02746

Merge branch 'polygon-prototype-staging' into add-ars-offramp-support

0a16284

gianfra-t changed the title ~~Add client domain corresponding signature when needed.~~ [Blocked] - Add client domain corresponding signature when needed. Oct 31, 2024

gianfra-t added 4 commits November 4, 2024 11:48

Temporary fix for Ancalap UI issue. Ask master key signature.

c6f6c51

config fixes

f6c9e65

use stellar funding account as master key for sep10

b58747d

cleanup and fixes

8c6cd82

remove memo testing

8d84389

gianfra-t changed the title ~~[Blocked] - Add client domain corresponding signature when needed.~~ Add client domain corresponding signature when needed. Nov 6, 2024

gianfra-t added 4 commits November 6, 2024 09:30

remove comment

afe6ffc

fixes and improvements

29a9147

fixes, refactor fetchToml function

bf9b1a6

merged with ars anclap integration, unifying endpoints

b1bfc06

remove conditional client domain signature

519a349

gianfra-t requested a review from a team November 6, 2024 17:38

make client domain configurable

fa0da9b

gianfra-t changed the title ~~Add client domain corresponding signature when needed.~~ [DO NOT MERGE YET!] - Add client domain corresponding signature when needed. Nov 7, 2024

remove master account solution for Anclap

0b1474c

gianfra-t changed the title ~~[DO NOT MERGE YET!] - Add client domain corresponding signature when needed.~~ Add client domain corresponding signature when needed. Nov 7, 2024

gianfra-t mentioned this pull request Nov 7, 2024

[OUTDATED] - Add ARS offramping support. #201

Closed

gianfra-t added 2 commits November 7, 2024 14:06

add validator, cleanup

b04270f

more cleanup

bb1ad5a

gianfra-t commented Nov 7, 2024

View reviewed changes

ebma reviewed Nov 7, 2024

View reviewed changes

TorstenStueber approved these changes Nov 7, 2024

View reviewed changes

improvements and fixes

6d07848

gianfra-t added 2 commits November 8, 2024 17:50

rename stringRaw to string to avoid changes

7ff7874

rename asset code string to string raw

84eee6c

ebma approved these changes Nov 11, 2024

View reviewed changes

TorstenStueber approved these changes Nov 11, 2024

View reviewed changes

rename client secret env variable

91e84e3

gianfra-t merged commit 56110f4 into polygon-prototype-staging Nov 12, 2024
5 checks passed

gianfra-t deleted the sign-client-domain branch November 12, 2024 13:40

		@@ -0,0 +1,27 @@
		const fetchTomlValues = async (TOML_FILE_URL) => {

		@@ -8,4 +8,6 @@ router.route('/create').post(validateCreationInput, controller.createStellarTran

		router.route('/payment').post(validateChangeOpInput, controller.changeOpTransaction);

		router.route('/sep10').post(controller.signSep10Challenge);

Add client domain corresponding signature when needed. #236

Add client domain corresponding signature when needed. #236

Conversation

gianfra-t commented Oct 28, 2024 • edited Loading

Main addition

Security

netlify bot commented Oct 28, 2024 • edited Loading

✅ Deploy Preview for pendulum-pay ready!

Choose a reason for hiding this comment

gianfra-t commented Oct 28, 2024

gianfra-t commented Oct 28, 2024

TorstenStueber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianfra-t commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

gianfra-t commented Oct 29, 2024 • edited Loading

TorstenStueber commented Oct 29, 2024

gianfra-t commented Oct 29, 2024

TorstenStueber commented Oct 29, 2024

gianfra-t commented Oct 30, 2024

TorstenStueber commented Nov 1, 2024

gianfra-t commented Nov 6, 2024

gianfra-t commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianfra-t commented Nov 8, 2024

TorstenStueber left a comment

Choose a reason for hiding this comment

gianfra-t commented Oct 28, 2024 •

edited

Loading

netlify bot commented Oct 28, 2024 •

edited

Loading

gianfra-t commented Oct 29, 2024 •

edited

Loading

gianfra-t commented Oct 29, 2024 •

edited

Loading