Fixes #12100
Makes the timeout be clamped to 2^30 seconds to avoid the issue with sign flipping with large timeout values and thus relocking the wallet instantly. Unlocking for at most ~34 years should be sufficient.
Also checks that the timeout is not negative to avoid instant relocks.
2297 | @@ -2298,6 +2298,14 @@ UniValue walletpassphrase(const JSONRPCRequest& request) 2298 | // Alternately, find a way to make request.params[0] mlock()'d to begin with. 2299 | strWalletPass = request.params[0].get_str().c_str(); 2300 | 2301 | + // Get the timeout 2302 | + int32_t nSleepTime = request.params[1].get_int(); 2303 | + pwallet->nRelockTime = GetTime() + nSleepTime;
Probably shouldn't set this until after the sanity checks...
Oops, fixed.
2297 | @@ -2298,6 +2298,14 @@ UniValue walletpassphrase(const JSONRPCRequest& request) 2298 | // Alternately, find a way to make request.params[0] mlock()'d to begin with. 2299 | strWalletPass = request.params[0].get_str().c_str(); 2300 | 2301 | + // Get the timeout 2302 | + int32_t nSleepTime = request.params[1].get_int();
Probably should use int, not int32_t (they're not guaranteed to be the same)
Done.
2302 | + int nSleepTime = request.params[1].get_int(); 2303 | + // Timeout cannot be negative, otherwise it will relock immediately 2304 | + if (nSleepTime < 0) { 2305 | + throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative."); 2306 | + } 2307 | + pwallet->nRelockTime = GetTime() + nSleepTime;
Still need to check the passphrase etc first... otherwise we've essentially tricked the wallet into lengthening its unlock time.
Fixed.
nRelockTime isn't actually used for anything except displaying stuff in getwalletinfo so this wouldn't actually be a problem.
Displaying the wrong time is kind of a problem, although maybe not a security one.
2297 | @@ -2298,6 +2298,13 @@ UniValue walletpassphrase(const JSONRPCRequest& request) 2298 | // Alternately, find a way to make request.params[0] mlock()'d to begin with. 2299 | strWalletPass = request.params[0].get_str().c_str(); 2300 | 2301 | + // Get the timeout 2302 | + int nSleepTime = request.params[1].get_int(); 2303 | + // Timeout cannot be negative, otherwise it will relock immediately 2304 | + if (nSleepTime < 0) {
Do we want to allow zero?
What would be the point of allowing 0?
Perhaps to just test if the passphrase is correct. (You're currently allowing zero here.)
(You're currently allowing zero here.)
Right.
It doesn't matter either way IMO.
Accepting 2147483647 (years long timeout) does not make too much sense in the time of Meltdown and Spectre vulnerabilities. Anyway having timeout on the level of single days can be too long for these type of attacks.
55 | @@ -56,6 +56,11 @@ def run_test(self): 56 | assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase, 10) 57 | self.nodes[0].walletpassphrase(passphrase2, 10) 58 | assert_equal(privkey, self.nodes[0].dumpprivkey(address)) 59 | + self.nodes[0].walletlock() 60 | + 61 | + # Test timeout bounds 62 | + assert_raises_rpc_error(-8, "Timeout cannot be negative.", self.nodes[0].walletpassphrase, passphrase, -10) 63 | + assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].walletpassphrase, passphrase, 2147483648)
2147483648 is not guaranteed to be out of range.
How can you guarantee that a number will be out of range?
Why don't you add test for 2147483647? It should work correctly for this value but we don't test it today. On the other hand waiting for timeout could be too long :) It should just accept the value.
I'd feel more comfortable using int64, but add a clamp (if value is over 100 years, set it to 100 years or so).
From user experience point of view when we have "seconds" defined timeout I would expect times maybe up to 1 day. Giving big number does not make sense.
If it is intended to allow days then the timeout should be specified in "days" and not "seconds".
Look at the usability aspect. Anyway I don't have a clue how it is used today in real life.
Giving big number does not make sense.
Yes it does. A common and recommended configuration on a system with an online wallet which would otherwise not be encrypted is to encrypt and then manually unlock any time the system is restarted. If we did not allow a setting which was effectively 'until restart' these systems would simply not use encryption.
This configuration saved localbitcoins loss of almost all their funds after a datacenter operator was social engineered, just as a single example. -- Facilities social engineering has historically been one of the most common compromise vectors, and it almost always results in a system reboot.
If it is intended to allow days then the timeout should be specified in "days" and not "seconds".
Different applications have different scales. More ordinary CLI user usage typical timeouts are just a few seconds or a few minutes.
What would be the point of allowing 0?
To trigger a keypool topup. (though I didn't look to see if that would be guaranteed to work, but we should make it do so by performing the topup on unlock while still holding the lock)
I'd feel more comfortable using int64, but add a clamp (if value is over 100 years, set it to 100 years or so).
That sounds fine, in particular we don't want people who have been happily using 999999999 to find it suddenly returning an error.
I have changed this to clamp at 2^31 seconds so the wallet will lock for up to 2^31 seconds which is approximately 68 years. This number was chosen because the time_t type that is used for the timer can be a 32 bit signed integer on some systems so this will avoid sign flipping with such systems.
I don't know why travis is failing.
Maybe add a limit to "help" printout?
2272: "2. timeout (numeric, required) The time to keep the decryption key in seconds.\n"
It looks like Travis test fails on 32bit architecture. There is something wrong as you get a lot of messages like:
JSONRPCException: Error: Please enter the wallet passphrase with walletpassphrase first.
I don't know yet why you get it yet.
Also you can see that in the log printout there is a wrong value:
node1 2018-01-08 18:52:07.849526 queue run of timer lockwallet(wallet.dat) in -2147483648 seconds (using HTTP)
The number is -2147483648 is 1L<<31 on 32bit architecture.
This faulty printout is probably caused by LogPrint string formatting in RPCRunLater:
LogPrint(BCLog::RPC, "queue run of timer %s in %i seconds (using %s)\n", name, nSeconds, timerInterface->Name());
63 | + # Check the timeout 64 | + # Check a time less than the limit 65 | + expected_time = int(time.time()) + (1 << 31) - 600 66 | + self.nodes[0].walletpassphrase(passphrase2, (1 << 31) - 600) 67 | + actual_time = self.nodes[0].getwalletinfo()['unlocked_until'] 68 | + assert actual_time < expected_time + 5 and actual_time >= expected_time # 5 second buffer
nit: Python allows chained comparisons: expected_time <= actual_time < expected_time + 5
Done
63 | + # Check the timeout 64 | + # Check a time less than the limit 65 | + expected_time = int(time.time()) + (1 << 31) - 600 66 | + self.nodes[0].walletpassphrase(passphrase2, (1 << 31) - 600) 67 | + actual_time = self.nodes[0].getwalletinfo()['unlocked_until'] 68 | + assert expected_time <= actual_time < expected_time + 5 # 5 second buffer
Might want to use the provided assert_greater... helper methods to give a more verbose output in case of failure.
Current error shows only:
File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet-encryption.py", line 68, in run_test
assert expected_time <= actual_time < expected_time + 5 # 5 second buffer
AssertionError
and not by how much it was off.
Done.
I changed the clamp to be at 2^31 - 1 seconds. Hopefully that fixes the travis problem with 32-bit linux.
2304 | + if (nSleepTime < 0) { 2305 | + throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative."); 2306 | + } 2307 | + // Clamp timeout to 2^31 seconds 2308 | + if (nSleepTime > ((int64_t)1 << 31) - 1) { 2309 | + nSleepTime = ((int64_t)1 << 31) - 1;
You could use the named constexpr std::numeric_limits<int32_t>::max().
Also, how does that take into account your observation that "the time_t type that is used for the timer can be a 32 bit signed integer on some systems". (Considering that nSleepTime is multiplied by 1000 somewhere down in the call stack)
You could use the named constexpr
std::numeric_limits<int32_t>::max().
I'll try that now.
Also, how does that take into account your observation that "the time_t type that is used for the timer can be a 32 bit signed integer on some systems". (Considering that nSleepTime is multiplied by 1000 somewhere down in the call stack)
It's multiplied by 1000 and then divided by 1000 before going into the time_t.
Ah, ok. But since nSleepTime represents a delta, that is added onto GetTime(), capping it at the max of int32_t is not enough?
The timer itself uses the delta. The only thing that's added is wallet->nRelockTime which is only used for display in getwalletinfo.
utACK, agree that clamping is marginally better in this specific case, as there is no standardized way to specify "forever" and unlocking for 68 years might was well be forever.
2268 | @@ -2269,7 +2269,7 @@ UniValue walletpassphrase(const JSONRPCRequest& request) 2269 | "This is needed prior to performing transactions related to private keys such as sending bitcoins\n" 2270 | "\nArguments:\n" 2271 | "1. \"passphrase\" (string, required) The wallet passphrase\n" 2272 | - "2. timeout (numeric, required) The time to keep the decryption key in seconds.\n" 2273 | + "2. timeout (numeric, required) The time to keep the decryption key in seconds. Limited to at most 2147483647 (2^31 - 1) seconds.\n"
The clamping semantics need to be in the documentation.
Done.
Maybe you can add some more observability from C++ code to debug.log It can help to find what is wrong with tests on 32bit.
It looks like you get similar fault as original one but only on 32 bits now. From the log you can see that actual_time is 0. This is taken from pwallet->nRelockTime. So if it is 0 then the wallet is locked.
I don't see how this can happen. It might be some other fault related to the timers handling?
I've tested a bit on the official release and I have found that the on 64bits it works fine for 9223372036854775 and lower timeout values and fails for 9223372036854776 and bigger timeout values (than again it works and fails in a cyclic way but I'have not found yet the cycle amplitude... maybe tomorrow but it is on the level of 2^53).
The point is that 9223372036854776 is nothing special when it comes to binary representation. This suggests that fault might be somewhere else.
@bolekC It has nothing to do with the 64 bit integer but rather overflowing on a 32-bit integer. Actually, this could be a problem related to single precision floats. I'm not sure, so I added debugging output so we can see what is wrong on travis.
This travis error continues to confuse me. It does not appear to be a sign issue in the timeval struct, yet the wallet is still locking immediately.
Clamps the timeout of walletpassphrase to 2^(30) seconds, which is
~34 years. Any number greater than that will be forced to be
2^(30). This avoids the sign flipping problem with large values which
can result in a negative time used.
Also perform bounds checks to ensure that the timeout is positive
to avoid immediate relocking of the wallet.
I got fed up with debugging this thing and just lowered the clamp to 2^30 seconds. I believe that should work and ~34 years is still long enough.
Please adjust the OP, since that ends up in the commit body of the merge commit.
@MarcoFalke adjusted
I still think we don't understand the fault's root cause very well. This 32bit failing doesn't give a rest. I have tested this functionality on 32bit version (0.15.1 without your fixes). What I have found is:
libevent: event.c:3117: Assertion tv->tv_usec >= 0 failed in timeout_nextI suspect QTimer implementation but I have not succeeded to troubleshoot it yet.
Still open questions: Why with high numbers wallet it re-locked immediately? What is causing this behaviour? Which code exactly? Is it QTimer?
I still think we don't understand the fault's root cause very well.
That's correct. I have no clue why it is failing, although I believe it has to do with floating point numbers. I'm pretty sure the issue has to do with the timeval struct, in particular tv.tv_sec's type of time_t.
I suspect QTimer implementation but I have not succeeded to troubleshoot it yet.
We don't use QTimers. Rather it uses libevent, so the issue lies in their implementation AFAICT.
Sure it's libevent. I've mixed it up. There must be something wrong somewhere :)
We don't use QTimers. Rather it uses libevent, so the issue lies in their implementation AFAICT.
In the most common case, yes. IIRC it does use QTimers when -server is false in the GUI: in that case libevent isn't initialized. Multiple timer implementations can be registered through RPCSetTimerInterface.
Not sure why I didn't think of this sooner: but another thing we can do instead of clamping the time when larger than a certain value, is to not use a timer at all in that case - interpret large numbers as 'infinite', unlocking forever (or until manually re-locked). Of course, this needs to be documented.
This avoids having to cope with timer edge-case behavior entirely.
2304 | + // Timeout cannot be negative, otherwise it will relock immediately 2305 | + if (nSleepTime < 0) { 2306 | + throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative."); 2307 | + } 2308 | + // Clamp timeout to 2^30 seconds 2309 | + if (nSleepTime > (int64_t)1 << 30) {
Could just write 1L instead of casting 1.
I was having some problems with using 1L on 32 bit systems so explicitly casting it like this seemed to be better.
utACK 134cdc7cee3da7c554e40ad947a9cdcbb3069f13
Since it seems infeasible to support large timeouts and clamping doesn't work very well on every platform, I'd prefer the approach by @laanwj to unlock and just return and not set any (potentially false) timers
clamping doesn't work very well on every platform
Where are you seeing this? It seems like current implementation should work on every platform. Agree that laanwj's suggestion would make code a little cleaner, but the behavior for all practical purposes would seem identical to what is implemented here.
Also, regardless of anything, current PR seems like an improvement over status the quo.
Where are you seeing this?
The "magic value" was determined by "ok, std::numeric_limits<int32_t>::max() doesn't work, so let's go a bit lower and try again". See
I got fed up with debugging this thing and just lowered the clamp to 2^30 seconds
Also, regardless of anything, current PR seems like an improvement over status the quo.
Agree.
utACK 134cdc7cee3da7c554e40ad947a9cdcbb3069f13
clamping doesn't work very well on every platform
We would still need some magic value where all of the values less than it are valid timeouts. Using std::numeric_limits<int32_t>::max() as the cutoff magic value for an infinite timeout would still be problematic as using certain values less than that would still result in invalid timeouts. Changing it from clamping to "infinite timeout cutoff" doesn't resolve that problem.
I think the suggestion is to simply not call RPCRunLater if there is an infinite timeout.
Just to remind that we don't know the root cause of this behaviour. Suspected is libevent library.
Described behaviour is that with big number wallet gets locked immediately so probably timeout is fired right after the command to set timer is given. I have observed also that sometimes this is few seconds delayed if bitcoind is heavily occupied with some tasks. This suggests event handling.
Find a root cause to have 100% safe solution. I have not debugged it to this level yet (lack of time)
0 <= nSleepTime <= 1,073,741,824 in wallet/rpcwallet.cpp is an int64_t, measured in seconds
pwallet->nRelockTime = GetTime()+nSleepTime in wallet/rpcwallet.cpp is an int64_t, so will only overflow when GetTime() returns 2**30*(2**33-1) which is about 292 million millenia away, and it's only used for printing in any event.
nSleepTime gets passed to RPCRunLater which multiplies it by 1000 to convert to milliseconds and passes it to RPCTimerInterface::NewTimer as an int64_t. 0 <= 1000*nSleepTime <= 1,073,741,824,000, which fits in about 40 bits, so still doesn't overflow.
RPCTimerInterface::NewTimer can be either QtRPCTimerInterface (using QTimers) or HTTPRPCTimerInterface (using libevent).
QtRPCTimerInterface passes the millisecond value to QTimer::start(), which in Qt5 accepts a std::chrono::milliseconds which is specced to be at least 45 bits, so is fine. In Qt4.8 it's specced as an int, and if int maxes out at 2**31-1 will overflow if nSleepTime is 2147484 or above, which is about 24 days.
HTTPRPCTimerInterface makes an HTTPRPCTimer, which populates a timeval with millis/1000 and (millis%1000)*1000. The latter should be fine, the former will overflow if time_t maxes out at 2**31-1 and millis is at least 2**31*1000, ie nSleepTime >= 2**31 which is already forbidden. The timeval is then passed to libevent's evtimer_add(), and libevent then turns the time into an absolute value using timeradd() from <sys/time.h> which will overflow if the final time overflows time_t, ie the Y2038 bug. This will happen if nSleepTime is greater than about 631,300,000 which is about 2**(29.2), decreasing every second as 2038 gets closer.
Trying on Debian/i386, I can confirm that libevent gets an event with a negative timeout in that scenario, which I would expect to behave badle. I can't actually get the event to trigger, however, so I'm not sure if this can be made to cause the wallet to actually be re-locked early. (Checking it's re-locked at the correct time doesn't seem worthwhile, since time_t will have wrapped by then, and who knows what will have broken...)
So if this needs to be improved further, maybe a check along the lines of nRelockTime < std::numeric_limits<time_t>::max() could be added.
But as is, I agree this makes things better than the status-quo, so ACK 134cdc7cee3da7c554e40ad947a9cdcbb3069f13 .
Okay, retrying with the limit being 2**31-1, I get a libevent timeout at time -631,298,485 which then gets handled immediately, causing the wallet to be relocked. I don't know why libevent handles that timeout immediately, but appears happy to leave a timeout corresponding to -2**32 or just after Y2038 pending indefinitely.
Also, regardless of anything, current PR seems like an improvement over status the quo.
OK, I agree. I was just a bit wary of setting a timer that we assume will never trigger at all, might as well not set it, but indeed this improves on the issue. Can be improved later. utACK 134cdc7
