Bug in Web Wallet's implementation of memo encryption/decryption! Please read!

[abit]

I pushed a fix to the GUI yesterday that makes it so the GUI falls back to the old shared secret with no padding in case the correct method fails. Dan tested it briefly yesterday and all his memos showed up, so it will be included in the next update.

Sent fra min MotoG3 via Tapatalk

Good 

[svk]

I pushed a fix to the GUI yesterday that makes it so the GUI falls back to the old shared secret with no padding in case the correct method fails. Dan tested it briefly yesterday and all his memos showed up, so it will be included in the next update.

Sent fra min MotoG3 via Tapatalk

[dannotestein]

The GUI does indeed try to decode all memos. The way the GUI handles memo is this: if a transaction includes a memo the GUI:

1. Checks if the wallet contains the private keys for either the "to" or the "from" keys of the memo
2. If yes, it attemps to decode the memo, if no it does nothing

I think the best thing to do is simply to include both methods, and apply the new method by default but then try the original if that fails. The main issue in implementing this is I need a test case to make sure I can catch the decode error. This would only have a performance impact in the very rare cases where the new, correct method fails to decode a memo.

When you've implemented the fallback to the old method, just let us know and we can pull the code from the repo and look thru all the openledger and blocktrades transactions to see if we can find any memos that aren't decodable. That should give us a decent confidence factor on the fallback.

[svk]

The GUI does indeed try to decode all memos. The way the GUI handles memo is this: if a transaction includes a memo the GUI:

1. Checks if the wallet contains the private keys for either the "to" or the "from" keys of the memo
2. If yes, it attemps to decode the memo, if no it does nothing

I think the best thing to do is simply to include both methods, and apply the new method by default but then try the original if that fails. The main issue in implementing this is I need a test case to make sure I can catch the decode error. This would only have a performance impact in the very rare cases where the new, correct method fails to decode a memo.

[dannotestein]

I would expect (but don't know) that some are also using my python code for decoding of the memo.
Could you give me some more details of the error so that I can check if my code decodes correctly? Do you have a breaking example?

We put up a pull request with the fix here to allow for easy viewing of the change: https://github.com/cryptonomex/graphene-ui/pull/849

[abit]

I would expect (but don't know) that some are also using my python code for decoding of the memo.
Could you give me some more details of the error so that I can check if my code decodes correctly? Do you have a breaking example?

At first glance, I think your python implementation is correct.  You pad the shared secret out to 256 bits here:
  https://github.com/xeroc/python-graphenelib/blob/master/graphenebase/memo.py#L29-L30
which is what the web wallet fails to do here:
  https://github.com/cryptonomex/graphene-ui/blob/master/dl/src/ecc/key_private.coffee#L81

We don't have a breaking example that we can publish (we'd have to reveal our private key to allow you to reproduce the error).

I think it's not hard to 'mine' or 'brute force' a breaking example.

[emf]

I would expect (but don't know) that some are also using my python code for decoding of the memo.
Could you give me some more details of the error so that I can check if my code decodes correctly? Do you have a breaking example?

At first glance, I think your python implementation is correct.  You pad the shared secret out to 256 bits here:
  https://github.com/xeroc/python-graphenelib/blob/master/graphenebase/memo.py#L29-L30
which is what the web wallet fails to do here:
  https://github.com/cryptonomex/graphene-ui/blob/master/dl/src/ecc/key_private.coffee#L81

We don't have a breaking example that we can publish (we'd have to reveal our private key to allow you to reproduce the error).

[xeroc]

I would expect (but don't know) that some are also using my python code for decoding of the memo.
Could you give me some more details of the error so that I can check if my code decodes correctly? Do you have a breaking example?

[abit]

I'd recommend patch cli_wallet to be compatible with the `buggy` but `working` web_wallet, since fewer people use/rely on cli_wallet, in addition the ones are likely / have more resources to fix it or make it work.

web wallet should be as quick as possible, so better not add additional computations onto it.

[cube]

I would suggest go with the fixed encryption/decryption function and let the end-users have a manual button  to decode old bugged encrypted string when they see a gibberish memo.

[Chronos]

Tough decision. As a fellow software dev, I hate having to manage backward-compatibility.

[dannotestein]

Why not include the flawed version of the algorithm as a backup in the web wallet (and maybe even the cli wallet) in case the correct version is unable to decrypt the memo? I know keeping this legacy cruft in the code would be annoying, but isn't it better than users being unable to view their old memos in a convenient manner?

We can do it if there's a significant interest, but I think this will effectively double the computational cost of decoding memos since the wallet tries to decode the memo in every transfer it sees and most of them won't be decodable. This still might not be that significant a cost, I really don't know, but also bear in mind only a very few old memos wouldn't be decryptable without this code.

I'm not sure why the wallet should attempt to decode the memo of every transfer it sees. It just needs to do so for transfers either to or from the accounts that are "My account". But even if that additional cost is too much, there could be an option in settings to only enable it for transfers dated prior to some specified date.

It does it because you can decode memos from transfers that aren't to or from your account as long as you have the memo key for one of those accounts (which is actually a very handy security feature).

I considered enabling it for transfers dated prior to the some date in the near future, but it's more work to figure out how to do that since the date will have to be piped into the decryption routines (I don't think it's readily accessible there now) and I'm not sure if it's worth spending time and money on to support a few old memos no one is looking at anyways, since there's a pretty hard limit on how many transactions you can see into the past in the web wallet anyways (a much more annoying limitation, from my point of view). As a practical matter, I suspect we use memos for automated processing more than anyone else except other exchanges and even we have no real use for those old memos. But if this rare case of backward compatibility is deemed an important enough feature, we can spend more time to get in, it's just a matter of cost (I'm charging half the cost of this work to the remaining balance in the blockchain maintenance worker that I've been saving for emergency work).

The biggest question I think is: Are any exchanges or other auto-processors of memos using the webwallet as their method for decoding memos? If so, I think we should add support for backwards compatibility for a little ways into the future. And if so, we could just add in the patch for the keys and leave it to SVK to handle the rest of it to keep the cost lower (he can probably do it faster than we can).

[arhag]

Why not include the flawed version of the algorithm as a backup in the web wallet (and maybe even the cli wallet) in case the correct version is unable to decrypt the memo? I know keeping this legacy cruft in the code would be annoying, but isn't it better than users being unable to view their old memos in a convenient manner?

We can do it if there's a significant interest, but I think this will effectively double the computational cost of decoding memos since the wallet tries to decode the memo in every transfer it sees and most of them won't be decodable. This still might not be that significant a cost, I really don't know, but also bear in mind only a very few old memos wouldn't be decryptable without this code.

I'm not sure why the wallet should attempt to decode the memo of every transfer it sees. It just needs to do so for transfers either to or from the accounts that are "My account". But even if that additional cost is too much, there could be an option in settings to only enable it for transfers dated prior to some specified date.

[dannotestein]

Why not include the flawed version of the algorithm as a backup in the web wallet (and maybe even the cli wallet) in case the correct version is unable to decrypt the memo? I know keeping this legacy cruft in the code would be annoying, but isn't it better than users being unable to view their old memos in a convenient manner?

We can do it if there's a significant interest, but I think this will effectively double the computational cost of decoding memos since the wallet tries to decode the memo in every transfer it sees and most of them won't be decodable. This still might not be that significant a cost, I really don't know, but also bear in mind only a very few old memos wouldn't be decryptable without this code.

[arhag]

Why not include the flawed version of the algorithm as a backup in the web wallet (and maybe even the cli wallet) in case the correct version is unable to decrypt the memo? I know keeping this legacy cruft in the code would be annoying, but isn't it better than users being unable to view their old memos in a convenient manner?