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.
Details
Details
- Reviewers
ashoat kamil tomek - Commits
- rCOMMe393f320797b: Introduce new header to siwe endpoint
For now keep testing that all SIWe features (log in, backup message and register) are not broken
Diff Detail
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 |
Comment Actions
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 |
| 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. |
Comment Actions
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:
- Decompose siweMessageToSign in siwe-panel before passing to headers and then re-construct it in siwe.react.js
- 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.
Comment Actions
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. |
Comment Actions
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. |
Comment Actions
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:
- Pass message on native to WebView and perform decomposition in landing-handler.
- 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.
Comment Actions
Why can't we just generate the message in landing/siwe.react.js, like we do in the existing code?
Comment Actions
Because we need the particular nonce and issuedAt from the backup message that was uploaded to backup service
Comment Actions
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?
Comment Actions
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
Comment Actions
Modify landing handler to take optional issuedAt property for SIWE. This will be needed for SIWE restore
Comment Actions
Thanks, this looks better! Some questions inline
| landing/siwe.react.js | ||
|---|---|---|
| 54 ↗ | (On Diff #40316) | Do we need to do anything to require the issuedAt field for siweMessageType === 'msg_backup_restore'? |
| 155 ↗ | (On Diff #40316) | 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 ↗ | (On Diff #40316) | Where is this type used? |
| lib/utils/siwe-utils.js | ||
| 93–96 ↗ | (On Diff #40316) | Do we still need this new function? |
| 152 ↗ | (On Diff #40316) | Typo |
| landing/siwe.react.js | ||
|---|---|---|
| 54 ↗ | (On Diff #40316) | New type in siwe-types requires the client to pass issuedAt if the type is msg_backup_restore. |
| 155 ↗ | (On Diff #40316) | It will only make sense to check issuedAt if the message type is msg_backup_restore. Otherwise we don't need issuedAt since it will be automatically resolved to Date.now() by siwe library. For msg_backup_restore we need issuedAt since we are not creating new message right now but recreating already existing one from smaller pieces. Types force the client to pass issuedAt if using msg_backup_restore type. |
| lib/types/siwe-types.js | ||
| 149–156 ↗ | (On Diff #40316) | It is used in the last diff in this stack. |
| lib/utils/siwe-utils.js | ||
| 93–96 ↗ | (On Diff #40316) | Should be removed |
| landing/siwe.react.js | ||
|---|---|---|
| 155 ↗ | (On Diff #40316) | I'm still a little confused here. We say "neither nonce nor complete message found", but what happens if a nonce is not provided, but a "complete message" is? Will this error appear? If the error does not appear when a nonce is not provided, but a "complete message" is, then it's not clear to me why we say "neither nonce nor complete message found". |
| lib/types/siwe-types.js | ||
| 149–156 ↗ | (On Diff #40316) | Looks like it's not used in the last diff in the stack, but rather in D12034. Seems like it would have been better to introduce it where it's used, but no need to change anything at this point |
| landing/siwe.react.js | ||
|---|---|---|
| 155 ↗ | (On Diff #40316) | It is probably my oversight. What we need is to fir check for nonce and say Unable to proceed: nonce not found.. Then check if message type is msg_backup_restore and in case it is check if issuedAt is provided. |
| landing/siwe.react.js | ||
|---|---|---|
| 168 ↗ | (On Diff #40620) | Is type needed here? Not sure what it's communicating |
| landing/siwe.react.js | ||
|---|---|---|
| 168 ↗ | (On Diff #40620) | We will require issuedAt only if the message is of type msg_backup_restore. I was thinking of adding sth like: message issuedAt not provided but backup message signature for requested but I thought it will be too verbose. |
| landing/siwe.react.js | ||
|---|---|---|
| 168 ↗ | (On Diff #40620) | I'd just say Unable to proceed: issuedAt not found or Unable to proceed: issuedAt not found for msg_backup_restore |