Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SEP-24: Add callback signature requirement #1263

Merged
merged 7 commits into from
Aug 19, 2022
Merged

SEP-24: Add callback signature requirement #1263

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ccd5182
SEP-24: Add callback signature requirement
C0x41lch0x41 Jul 11, 2022
a61707e
SEP-24: Bump date and version
C0x41lch0x41 Jul 11, 2022
04d5cd2
SEP24: Correct markdown typo
C0x41lch0x41 Jul 12, 2022
6080754
SEP-24: Remove UI from callback signature
C0x41lch0x41 Jul 12, 2022
1f65d01
Merge branch 'master' into patch-5
C0x41lch0x41 Jul 14, 2022
8c1763f
SEP-24: Signature only for URL callbacks
C0x41lch0x41 Aug 19, 2022
54c9130
Merge branch 'master' into patch-5
C0x41lch0x41 Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 52 additions & 4 deletions ecosystem/sep-0024.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ Title: Hosted Deposit and Withdrawal
Author: SDF
Status: Active
Created: 2019-09-18
Updated: 2022-06-08
Version 2.5.0
Updated: 2022-07-11
Version 2.6.0
```

## Simple Summary
Expand Down Expand Up @@ -87,6 +87,50 @@ In order for browsers-based wallets to validate the CORS headers, as [specified

This protocol involves the transfer of value, and so HTTPS is required for all endpoints for security. Wallets and anchors should refuse to interact with any insecure HTTP endpoints.

## Callback signature
C0x41lch0x41 marked this conversation as resolved.
Show resolved Hide resolved

This protocol involves the optional use of callbacks that the Anchor can issue to update the wallet on ```status``` of a transaction.
In order to validate the integrity and provenance of the callback, the Anchor MUST include a signature in an additional HTTP Header `X-Stellar-Signature`.
This Header MUST follow the specification: `X-Stellar-Signature: t=<timestamp>, s=<base64 signature>` where:
* __timestamp__ is the current Unix timestamp (number of seconds since epoch) at the time the callback is sent. This is used to assure the freshness of the request and to prevent this request to be replayed in the future.
* __base64 signature__ is the base64 encoding of the request signature. We explain below how to compute and verify this signature. The signature is computed using the Stellar private key linked to the `SIGNING_KEY` field of the anchor's [`stellar.toml`](sep-0001.md). Note that the timestamp and the Wallet hostname will be part of the signature to prevent replay and relay attacks.

It is the wallet's responsability to:
* Verify the signature using the corresponding Stellar `SIGNING_KEY` field of the anchor's [`stellar.toml`](sep-0001.md).
* Verify the freshness of the request by comparing the `timestamp` in the request with the current timestamp at the time of the reception and discard every request above a threshold of few seconds (1 or 2 minute(s) maximum).
* Send a working callback URL (parameter ```callback``` or ```on_change_callback```) to the anchor.

### VERIFY signature

* Check that callback request has `X-Stellar-Signature` header
* Parse the header and extract:
* Key `t`: __timestamp__
* Key `s`: __base64 signature__
* Verify the request freshness: _current timestamp_ - __timestamp__ < few seconds (1-2 minute(s) max)
* Extract the __body__ of the request
* Base64 decode the __base64 signature__ to get the __signature__
* Prepare the payload to verify the signature:
* The __timestamp__ (as a string)
* The character `.`
* The wallet host to send the callback request to
* The character `.`
* The __body__
* Verify the signature using the correct `SIGNING_KEY`

### COMPUTE signature

* Prepare the callback
* Prepare the payload to sign:
* Current timestamp (as a string)
* The character `.`
* The wallet host to send the callback request to
* The character `.`
* The callback request body
* Sign the payload `<timestamp>.<host>.<body>` using the Anchor private key
* Base64 encode the signature
* Build the `X-Stellar-Signature` header:
* `X-Stellar-Signature: t=<current timestamp>, s=<base64 encoded signature>`

## Content Type

All endpoints accept in requests the following `Content-Type`s:
Expand Down Expand Up @@ -380,8 +424,8 @@ The basic parameters are summarized in the table below.

Name | Type | Description
-----|------|------------
`callback` | string | (optional) [`postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage) or a URL that the anchor should `POST` a JSON message to when the user successfully completes the interactive flow.
`on_change_callback` | string | (optional) [`postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage) or a URL that the anchor should `POST` a JSON message to when the `status` or `kyc_verified` properties change.
`callback` | string | (optional) [`postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage) or a URL that the anchor should `POST` a JSON message to when the user successfully completes the interactive flow. The callback needs to be signed by the anchor and the signature needs to be verified by the wallet according to the [callback signature specification](#callback-signature).
`on_change_callback` | string | (optional) [`postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage) or a URL that the anchor should `POST` a JSON message to when the `status` or `kyc_verified` properties change. The callback needs to be signed by the anchor and the signature needs to be verified by the wallet according to the [callback signature specification](#callback-signature).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to clarify that the added statement only applies to URLs, not postmessage callbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually don't you think we should add a signature requirement for the messages sent by the anchor if postMessage is used? That would solve some of the mentioned security concerns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postmessage is a JavaScript construct, its not made with a HTTPS request, so the description of how the callback should be made doesn't fit the mechanism by which the message is sent in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure we will need to come up with another way to compute the signature but don't you think that the messages should be signed somehow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we overlapping with guarantees the browser provides signing the postMessage callbacks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think its the same level of concern as the HTTPS callbacks, because the wallet verified the anchor's identity when it authenticated via SEP-10, which is required for requesting a webview URL.

So I would say the wallet has a higher level of confidence on the origin of the postmessage request because it comes from the webview, not a random client on the web.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree with you the risk is lower.

That being said when listening for events from postMessage all the messages are queued whatever the origin is.
For example with the code below:

<!-- wallet.html -->
<html>
    <h1>Wallet</h1>
    <body>
        <script type="text/Javascript">
            function OpenPopupWindow(url, name, where)
            {
                myRef = window.open(url , name,'left=20,top=20,width=500,height=500,toolbar=1,resizable=0');
                myRef.focus()

            }
            window.addEventListener("message", (event) => {
                console.log(event.data);
            }, false);
        </script>

    <form> 
        <input type=button value="Anchor" onClick="OpenPopupWindow('anchor.html', 'Anchor');"> 
        <input type=button value="Evil" onClick="OpenPopupWindow('evil.html', 'Evil');"> 
    </form> 
    </body>
</html>

<!-- anchor.html -->
<html>
    <h1>Anchor</h1>
    <body>
        <p>ANCHOR</p>
        <script type="text/Javascript">
            window.opener.postMessage("ANCHOR", "*");
        </script>
    </body>
</html>

<!-- evil.html -->
<html>
    <h1>Evil</h1>
    <body>
        <p>EVIL</p>
        <script type="text/Javascript">
            window.opener.postMessage("ANCHOR", "*");
        </script>
    </body>
</html>

The wallet will not be able to distinguish calls from the Anchor and calls from Evil.
This can happen if the wallet uses iframe for ads and integrate 3rd party JS/CSS/HTML libraries.

A simpler solution would be to add a requirement for the wallet to check that event.origin is set to the anchor host when listening for events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JakeUrban what do you think? Maybe I can specify this does not apply to postMessage callbacks to merge this and open another discussion about it? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm up for discussing how to handle postmessage callbacks but I agree it would be ideal if we didn't block our solution for HTTP callbacks. Lets merge this and start another thread.


The URL supplied by both callback parameters should receive the full transaction object.

Expand All @@ -402,6 +446,8 @@ Because `callback` is used to notify the wallet that the interactive flow is com
Wallet applications display content rendered by a third party anchor service. It is recommended to use the [`rel=noopener`](https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types/noopener) attribute on links or the `noopener` feature for [`Window.open()`](https://developer.mozilla.org/en-US/docs/Web/API/Window/open#window_functionality_features).

Note that using `noopener` prohibits the use of `postMessage` callbacks. If `postMessage` callbacks are required for your implementation, it recommended to only open URLs from anchors that you, the wallet developer, trust.

To validate the provenance and integrity of the callback, anchors [must sign it and the UI must validate the signature](#callback-signature) before taking any action.
C0x41lch0x41 marked this conversation as resolved.
Show resolved Hide resolved

**Differences between `callback` and `on_change_callback`**

Expand Down Expand Up @@ -939,6 +985,8 @@ There is a small set of changes when upgrading from SEP-6 to SEP-24.
* Solar wallet: https://solarwallet.io

## Changelog

* `v2.6.0`: Add callback signature and verification requirement. ([#1263](https://github.com/stellar/stellar-protocol/pull/1263))
* `v2.5.0`: Add `expired` transaction status. ([#1233](https://github.com/stellar/stellar-protocol/pull/1233))
* `v2.4.0`: Add `refunded` transaction status. ([#1195](https://github.com/stellar/stellar-protocol/pull/1195))
* `v2.3.0`: Change `lang` format from [ISO639-1](https://en.wikipedia.org/wiki/ISO_639-1) to [RFC4646](https://datatracker.ietf.org/doc/html/rfc4646) which is a superset of ISO639-1. Add `lang` field to GET `/transactions` and `/transaction`. ([#1191](https://github.com/stellar/stellar-protocol/pull/1191))
Expand Down