BitShares Forum
Main => General Discussion => Topic started by: dannotestein on May 18, 2016, 01:27:36 am
-
Both the BlockTrades bridge and the OpenLedger gateways depend on the command-line interface (cli) wallet to encrypt and decrypt transaction memos. We recently observed that very occassionally the cli wallet was unable to decrypt a memo that the web wallet was able to decrypt. This is potentially a very serious issue for automated processing of transactions as the platform scales to larger numbers of transactions, so we began analyzing the web wallet and the cli wallet to see where the difference lay.
Our initial suspicion was that there might be a hard-to-trigger bug in the relatively new secp256k1 library that replaced the traditional openssl library, but after some tests we were able to determine that both libraries produced the same results. Ultimately we were able to identify a bug that it was the loss of a leading zero in the shared secret calculation performed by the web wallet that caused the bug.
This bug was never apparent in the web wallet, because both the encryption and the decryption routines use the same flawed algorithm, and it only rarely showed up in the cli wallet in the fairly rare case where the first byte in the shared secret was 0x00 (probably 1 in every 256 memos on average, I suppose, assuming that byte value is randomly distributed).
We have a fix for the bug, but one side effect of the fix is that some small number of old memos will no longer be viewable in the web wallet. If someone needs to decrypt these old memos at some point, it would still be possible to view them using an older version of the web wallet. All future memos generated with the fixed code will be properly viewable in the web wallet and the cli wallet.
I believe this is the only reasonable solution to the problem, but of course it’s up to the community to make a final determination on this issue. If there’s anyone who objects to this change, please let us know as soon as possible.
-
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?
-
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.
-
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.
-
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).
-
Tough decision. As a fellow software dev, I hate having to manage backward-compatibility.
-
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.
-
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.
-
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?
-
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 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.
-
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
-
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.
-
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.
-
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
-
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 +5% +5%