unlocking an encrypted wallet for a very long time fails silently

[dooglus]

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.

[achow101]

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.

[dooglus]

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

[achow101]

Oh crap, I had a complete failure of math. My bad.

[dooglus]

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.

[dooglus]

1000000000000000000 : ok
2000000000000000000 : ok
3000000000000000000 : fail
4000000000000000000 : fail
5000000000000000000 : ok
6000000000000000000 : ok
7000000000000000000 : ok
8000000000000000000 : fail
9000000000000000000 : fail

[achow101]

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:

entered: 359711509437336257 output: timeval seconds: -9223372036854775, microseconds: -320000
entered: 1000000000000000000 output: timeval seconds: 3875820019684212, microseconds: 736000
entered: 3000000000000000000 output: timeval seconds: -6819284014656913, microseconds: -408000

[dooglus]

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.

[achow101]

2^31+1 works find for me.

[dooglus]

OK. I've been testing against a very old version of Bitcoin. I'll try with the master branch.

[achow101]

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.

[dooglus]

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

[achow101]

Yup, looks like it to me:

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

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

[dooglus]

9223372036854775 seconds is 292 million years.

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

[achow101]

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.

[dooglus]

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;
 }

[achow101]

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.

[dooglus]

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.

[achow101]

136 years is still sufficient for that use case.

[dooglus]

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);

[dooglus]

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.

[achow101]

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.

[dooglus]

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.