stellar-expert / albedo

Security-centric, developer-friendly, easy-to-use delegated signer and keystore for Stellar Network
https://albedo.link
MIT License
64 stars 14 forks source link

`signed_message` is not the message that's signed #64

Closed kalepail closed 3 years ago

kalepail commented 3 years ago

The signed_message parameter prepends the account pubkey: onto the message but the signature is only for the actual token. This is odd and unexpected. Either sign the whole message including the prepended account pubkey or drop the prepend. Either way whatever signed_message is is what the signature should match against. Otherwise you've gotta do some wonky splitting on the signed_message string.

kalepail commented 3 years ago

Note I've only tested this with the publicKey method and not the signMessage method, though I would expect that method to work the same.

orbitlens commented 3 years ago

What method of the signature verification are you using? The @albedo-link/signature-verification package automatically prepends the signer's public key to the message. So everything works as expected.

const {verifyMessageSignature} = require("@albedo-link/signature-verification")

const isValid = verifyMessageSignature(
  'GAR5IK6AQHLCGUPQMPMAQ4LCFPHSKVJ7DJJYCY7TZ4FOXITS4LKH7N6D', //public key from response
  'LQxALoiq6PW3ekgG5a1EP9+MHRDUajpZr5ywuIv/XfY=', //original text to sign
  '0b6d0484184cac6be393b3f44421e28d3aeeafecb68aa270843049c9f5805d3c146f19854d6fa8539f24e3da26037b2b9d0af3162818c69216f4b32dcecb5407' //signature from response
)

If you build the verification code yourself, you can use signed_message parameter from response directly:

const encodedMessage = shajs('sha256').update(resp.signed_message).digest()
return nacl.sign.detached.verify(encodedMessage, signature, new Uint8Array(decodeBase32(publicKey).slice(1, -2)))

Please let me know if this works for you.

kalepail commented 3 years ago

You can see the issue in this demo here: https://stackblitz.com/edit/js-mquq3o?devtoolsheight=33&file=index.js

I would expect the output of verifyMessageSignature to be true in both cases of albedo.publicKey and albedo.signMessage.

orbitlens commented 3 years ago

Well, the verifyMessageSignature function expects the original message, not the concatenated version from the response. This will work:

const isValid = verifyMessageSignature(pubkey, signed_message.substr(signed_message.indexOf(':')+1), signature);

In your example, you didn't provide the auth token explicitly, so you don't have the original message unless you parse it from the response. I see how inconvenient this may be.

To eliminate the ambiguity I'm going to change two things:

Thanks for the clarification.

kalepail commented 3 years ago

Awesome! I am curious though why include the pubkey prefix at all? What purpose does it serve? Especially if in fact it is not part of the signed message for the signature?

orbitlens commented 3 years ago

It is a part of the signed payload. In the verification function it is prepended to the message before hash generation. The sole purpose of such format is to prevent the spoofing attack when an attacker provides a valid transaction hash instead of the random authentication token in the publicKey intent.

kalepail commented 3 years ago

Oooooooooohh. Okay so it's a feature of verifyMessageSignature to prepend for you off the raw message. So it's getting double appended. Geeze okay now I'm understanding. Yeah would be nice to not have that be enforced like you mentioned. Sorry for the misunderstanding.

orbitlens commented 3 years ago

Done