Page MenuHomePhabricator

Introduce new header to siwe endpoint
Needs RevisionPublic

Authored by marcin on Tue, May 14, 8:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 17, 6:20 PM
Unknown Object (File)
Tue, May 14, 9:46 AM
Unknown Object (File)
Tue, May 14, 9:46 AM
Unknown Object (File)
Tue, May 14, 9:46 AM
Subscribers
None

Details

Reviewers
ashoat
kamil
tomek
Summary

Signing already created backup message is new usage of siwe endpoints. Additionally it is the first time siwe endpoint is not supposed to generate message itself. This differential add additional header to the endpoint that can carry complete backup message. Invariant protects against sending complete message along with nonce in headers. Finally this diff introduces new copy that will be displayed to the user when signing backup message.

Test Plan

For now keep testing that all SIWe features (log in, backup message and register) are not broken

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

keyserver/src/responders/landing-handler.js
161 ↗(On Diff #40164)

siwe messages contain newline characters. I experimentally established that react native web view will skip headers with values containing newlines. client responsible for adding this header must first call encodeURIComponent

ashoat requested changes to this revision.Tue, May 14, 10:09 AM

This design seems very unsafe to me. Can we find a way forward that doesn't allow the client to pass in any message to be signed?

keyserver/src/responders/landing-handler.js
260 ↗(On Diff #40164)

This seems extremely dangerous to me. What if siweMessageToSignString contains the " character? You appear to be allowing somebody to inject arbitrary JavaScript

This revision now requires changes to proceed.Tue, May 14, 10:09 AM
keyserver/src/responders/landing-handler.js
260 ↗(On Diff #40164)

What is the difference between messageToSign and siweNonce? Both are arbitrary strings passed as endpoint headers. If isValidSIWENonce check above makes siweNonce safe then there is similar check for siweMessageToSign a couple of lines above that won't let the code reach this line if siweMessageToSign isn't exactly social proof or backup message.

This design seems very unsafe to me. Can we find a way forward that doesn't allow the client to pass in any message to be signed?

Two alternatives that I can think of are:

  1. Decompose siweMessageToSign in siwe-panel before passing to headers and then re-construct it in siwe.react.js
  2. Make direct HTTP request to backup service from siwe.react.js. - don't know if it will work.

Both are significant changes to entire stack so I would like to understand what exact dangers are we risking by passing full message to sign.

I think decomposing is the best bet. Curious for @tomek's perspective

keyserver/src/responders/landing-handler.js
161 ↗(On Diff #40164)

Why do we call decodeURIComponent here but not when we insert the <script> tag below?

260 ↗(On Diff #40164)

The difference between messageToSign and siweNonce is the validation that is performed. We can't pass a string that contains the " character as siweNonce, because isValidSIWENonce requires a RegEx match of ^[a-zA-Z0-9]{17}$.

isValidSIWEMessageString seems less strict. Looking at it more closely, it appears that we don't allow the " character in the statement. But it doesn't seem clear to me that the whole string will not contain the " character. I'm also not sure if there are other ways to inject arbitrary JavaScript here without including " character, or other ways to perform an attack.

I think decomposing is the best bet. Curious for @tomek's perspective

If we have a strict validation, the current approach might be ok - but I'm not sure if it is strict enough.

Decomposing sounds beneficial, but I'm wondering if decomposed strings could also cause some injections.

I'm also wondering what could this injection cause - this script seems to be executed on a client side, where a user can modify the code and I'm not sure if increasing the safety here really protects us against some attacks.

keyserver/src/responders/landing-handler.js
161 ↗(On Diff #40164)

I had similar issues with this as when passing siweMessageToSign directly to WebView headers. The only way I could make it work was by decoding directly in siwe.react component just before getting signature.

I have refactored the entire stack locally to use decomposition of siwe message on native to learn that it is not possible on native. This is because siwe library uses Buffer object when parsing string into structured message object. There are two solutions now:

  1. Pass message on native to WebView and perform decomposition in landing-handler.
  2. Use siwe create from Rust to parse the message and expose it vie C++ to JS. - it should be entire day of work.

Proceeding with 1 for now to have something completed for today but @ashoat if you think that it still risks JS injections attacks I will complete 2 by the end of tomorrow.

EDIT:
Perhaps it won't take entire day - maybe 3 - 4 hours.

EDIT:
Actually I will proceed this way - just locally modified some Rust to get it done.

Why can't we just generate the message in landing/siwe.react.js, like we do in the existing code?

Why can't we just generate the message in landing/siwe.react.js, like we do in the existing code?

Because we need the particular nonce and issuedAt from the backup message that was uploaded to backup service

So is the new plan for keyserver to parse the message that is passed in, extract nonce and issuedAt, and then insert that into the JS?

So is the new plan for keyserver to parse the message that is passed in, extract nonce and issuedAt, and then insert that into the JS?

Actually the plan is to parse it on native and put necessary pieces (nonce, issuedAt and primary key) into webview. I managed to use Rust for parsing - got it my my stack locally and I will put it tomorrow morning to phabricator

Modify landing handler to take optional issuedAt property for SIWE. This will be needed for SIWE restore

ashoat requested changes to this revision.Fri, May 17, 6:30 AM

Thanks, this looks better! Some questions inline

landing/siwe.react.js
54

Do we need to do anything to require the issuedAt field for siweMessageType === 'msg_backup_restore'?

155

This update isn't necessary anymore, right? We're only checking !hasNonce here. Or should we also be checking for issuedAt?

lib/types/siwe-types.js
149–156

Where is this type used?

lib/utils/siwe-utils.js
93–96

Do we still need this new function?

152

Typo

This revision now requires changes to proceed.Fri, May 17, 6:30 AM