unlocking an encrypted wallet for a very long time fails silently #12100

issue dooglus openend this issue on January 6, 2018
  1. dooglus commented at 5:18 am on January 6, 2018: contributor

    If I unlock an encrypted wallet for 359711509437336256 or less seconds, I can sign transactions with it:

    $ bitcoin-cli walletpassphrase "$x" 359711509437336256
$ echo $?
0
$ bitcoin-cli signrawtransaction $tx | grep complete
  "complete": true

    But if I unlock it for 359711509437336257 or more seconds, it appears to have unlocked but I can’t sign any transactions:

    $ bitcoin-cli walletpassphrase "$x" 359711509437336257
$ echo $?
0
$ bitcoin-cli signrawtransaction $tx | grep complete
error code: -13
error message:
Error: Please enter the wallet passphrase with walletpassphrase first.

    I don’t know the significance of that number. It’s nowhere near a power of 2.

    I was trying to unlock the wallet effectively “forever”, so put a big number. If I use too big a number (2^63 or more) I get an error message JSON integer out of range but for numbers between 359711509437336257 and 2^63-1 inclusive I get no error, but no actual unlocking.

  2. achow101 commented at 5:28 am on January 6, 2018: member

    The field is an int64_t which is a signed integer. Any value 2^63 and greater and less than 2^64 will be interpreted as a negative value so it will unlock and then relock immediately.

    I suppose this should be a uint64_t or check that it isn’t negative.

  3. dooglus commented at 5:31 am on January 6, 2018: contributor

    Any value 2^63 and greater is already rejected.

    The bug I’m reporting is for numbers between 359711509437336257 and 2^63-1. ie.

     359711509437336257 and
9223372036854775807
  4. achow101 commented at 5:33 am on January 6, 2018: member
    Oh crap, I had a complete failure of math. My bad.
  5. dooglus commented at 5:35 am on January 6, 2018: contributor

    It turns out I’m wrong:

     359711509437336257 : fails
1000000000000000000 : succeeds

    So there are some bigger numbers of seconds I can unlock for. But I can’t unlock for exactly 359711509437336257 seconds.

  6. dooglus commented at 5:38 am on January 6, 2018: contributor 
    1000000000000000000 : ok
2000000000000000000 : ok
3000000000000000000 : fail
4000000000000000000 : fail
5000000000000000000 : ok
6000000000000000000 : ok
7000000000000000000 : ok
8000000000000000000 : fail
9000000000000000000 : fail
  7. achow101 commented at 5:55 am on January 6, 2018: member

    The problem is that it uses a timeval struct which has a seconds field of type time_t. Unfortunately this is not very well specified. The problem here is that there is integer roll over with each of these large values. Sometimes it is positive, other times its negative. I added some additional debugging output to my build and this is what I get:

    0entered: 359711509437336257 output: timeval seconds: -9223372036854775, microseconds: -320000
1entered: 1000000000000000000 output: timeval seconds: 3875820019684212, microseconds: 736000
2entered: 3000000000000000000 output: timeval seconds: -6819284014656913, microseconds: -408000
  8. fanquake added the label Wallet on Jan 6, 2018
  9. dooglus commented at 6:04 am on January 6, 2018: contributor

    I was thinking it might be because boost’s posix_time::seconds is a ’long'.

    I notice the unlocking for 2^31 seconds works, but unlocking for 2^31+1 seconds doesn’t.

    The RPCRunLater() callback which re-locks the wallet is running instantly when the 32 bit signed long number of seconds wraps around to a negative value.

    2147483648 : ok
2147483649 : fail

    2147483648 seconds is a little over 68 years which should be long enough. Maybe we should simply limit the unlock time to 2^31 seconds.

    Edit: or treat any number greater than 2^31 as meaning “forever”, and skip the RPCRunLater() call.

  10. achow101 commented at 6:08 am on January 6, 2018: member
    2^31+1 works find for me.
  11. dooglus commented at 6:09 am on January 6, 2018: contributor
    OK. I’ve been testing against a very old version of Bitcoin. I’ll try with the master branch.
  12. achow101 commented at 6:14 am on January 6, 2018: member
    Through my basic testing, I have found that time_t, at least on my system, changes sign somewhere between 2^53 and 2^54. This range also happens to be close to the range that doubles can represent integers plainly.
  13. dooglus commented at 6:30 am on January 6, 2018: contributor

    I found the same. It changes at 2^63 / 1000 - because class RPCTimerInterface works in milliseconds.

    9223372036854775 is ok
9223372036854776 isn't
9223372036854775.808 = 2^63 / 1000
  14. achow101 commented at 6:38 am on January 6, 2018: member

    Yup, looks like it to me:

    0timeval seconds: 9223372036854775, microseconds: 0
1timeval seconds: -9223372036854775, microseconds: -616000

    I guess the solution is to just put a bounds check at 9223372036854775 seconds.

  15. dooglus commented at 6:40 am on January 6, 2018: contributor

    9223372036854775 seconds is 292 million years.

    Seems pretty safe to skip over the RPCRunLater() call if the specified time offset is greater than that…

  16. achow101 commented at 6:43 am on January 6, 2018: member
    Actually it might be easier and make more sense when reading the code to make the input field an int32 rather than an int64. That caps it to only 136 years which also sounds pretty reasonable not to go past.
  17. dooglus commented at 7:23 am on January 6, 2018: contributor

    Wouldn’t doing that break any existing script that runs “walletpassphrase $pass 9999999999” for instance. That call currently works, but stops working if we require the time to fit within 32 bits.

    How about this?

    diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index f839a1861..a0b647195 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -2312,8 +2312,11 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
     pwallet->TopUpKeyPool();
 
     int64_t nSleepTime = request.params[1].get_int64();
+    int64_t nMaxSleepTime = (1LU << 63) / 1000;
     pwallet->nRelockTime = GetTime() + nSleepTime;
-    RPCRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), boost::bind(LockWallet, pwallet), nSleepTime);
+    if (nSleepTime <= nMaxSleepTime) {
+        RPCRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), boost::bind(LockWallet, pwallet), nSleepTime);
+    }
 
     return NullUniValue;
 }
  18. achow101 commented at 7:34 am on January 6, 2018: member

    Wouldn’t doing that break any existing script that runs “walletpassphrase $pass 9999999999” for instance. That call currently works, but stops working if we require the time to fit within 32 bits.

    I’m wary of allowing super long timeouts since it keeps the unencrypted keys in memory. With the recent news of the Spectre and Meltdown attacks, I don’t think allowing such large timeouts is a good idea at all as we should be trying to keep private keys in memory for as little time as possible.

  19. dooglus commented at 7:36 am on January 6, 2018: contributor
    I’m thinking of hot wallets for sites which offer automated payments. If there’s no way of keeping the private keys in memory constantly then I’ll have no choice but not to use encryption at all for those wallets. At least using encryption but having the wallet unlocked on load means that stolen backups don’t leak your private keys.
  20. achow101 commented at 7:37 am on January 6, 2018: member
    136 years is still sufficient for that use case.
  21. dooglus commented at 7:38 am on January 6, 2018: contributor

    This might be a cleaner patch:

    diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index f839a1861..83f82db6a 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -2312,6 +2312,10 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
     pwallet->TopUpKeyPool();
 
     int64_t nSleepTime = request.params[1].get_int64();
+    int64_t nMaxSleepTime = (1LU << 63) / 1000;
+    if (nSleepTime > nMaxSleepTime) {
+        nSleepTime = nMaxSleepTime;
+    }
     pwallet->nRelockTime = GetTime() + nSleepTime;
     RPCRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), boost::bind(LockWallet, pwallet), nSleepTime);
  22. dooglus commented at 7:39 am on January 6, 2018: contributor
    136 years is long enough, but existing scripts could be using a value like 9999999999 which currently works and which your proposed fix would break.
  23. achow101 commented at 7:47 am on January 6, 2018: member
    Yes, and the fix for it is trivial. Just make the value 999999. The change will be documented in the release notes. You can also make an alternative PR with your proposal.
  24. dooglus commented at 7:55 am on January 6, 2018: contributor
    Fair enough. I don’t see why you would want to break the existing scripts when there’s an equally good fix which doesn’t break them.
  25. laanwj closed this on Jan 17, 2018
  26. laanwj referenced this in commit c7978be899 on Jan 17, 2018
  27. PastaPastaPasta referenced this in commit 7d7f1211fb on Feb 25, 2020
  28. PastaPastaPasta referenced this in commit e5ae08aeef on Feb 27, 2020
  29. PastaPastaPasta referenced this in commit 1d630d3ba5 on Feb 27, 2020
  30. PastaPastaPasta referenced this in commit 2a8040cebb on Feb 27, 2020
  31. ckti referenced this in commit 52577137af on Mar 28, 2021
  32. MarcoFalke locked this on Sep 8, 2021
  33. gades referenced this in commit 0ac5a3b8b9 on Mar 27, 2022


dooglus achow101

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me