Clamp walletpassphrase timeout to 2^30 seconds and check its bounds #12101

achow101 · 2018-01-06T07:11:52Z

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.

luke-jr · 2018-01-06T07:15:22Z

src/wallet/rpcwallet.cpp

@@ -2298,6 +2298,14 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
    // Alternately, find a way to make request.params[0] mlock()'d to begin with.
    strWalletPass = request.params[0].get_str().c_str();

+    // Get the timeout
+    int32_t nSleepTime = request.params[1].get_int();
+    pwallet->nRelockTime = GetTime() + nSleepTime;


Probably shouldn't set this until after the sanity checks...

Oops, fixed.

luke-jr · 2018-01-06T07:16:28Z

src/wallet/rpcwallet.cpp

@@ -2298,6 +2298,14 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
    // Alternately, find a way to make request.params[0] mlock()'d to begin with.
    strWalletPass = request.params[0].get_str().c_str();

+    // Get the timeout
+    int32_t nSleepTime = request.params[1].get_int();


Probably should use int, not int32_t (they're not guaranteed to be the same)

luke-jr · 2018-01-06T07:21:10Z

src/wallet/rpcwallet.cpp

+    if (nSleepTime < 0) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative.");
+    }
+    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.

luke-jr · 2018-01-06T07:24:58Z

src/wallet/rpcwallet.cpp

+    // Get the timeout
+    int nSleepTime = request.params[1].get_int();
+    // Timeout cannot be negative, otherwise it will relock immediately
+    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.

luke-jr · 2018-01-06T07:25:17Z

test/functional/wallet-encryption.py

+
+        # Test timeout bounds
+        assert_raises_rpc_error(-8, "Timeout cannot be negative.", self.nodes[0].walletpassphrase, passphrase, -10)
+        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.

sipa · 2018-01-06T10:34:21Z

I'd feel more comfortable using int64, but add a clamp (if value is over 100 years, set it to 100 years or so).

bolekC

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.

bolekC · 2018-01-06T22:55:58Z

test/functional/wallet-encryption.py

+
+        # Test timeout bounds
+        assert_raises_rpc_error(-8, "Timeout cannot be negative.", self.nodes[0].walletpassphrase, passphrase, -10)
+        assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].walletpassphrase, passphrase, 2147483648)


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.

bolekC · 2018-01-06T23:01:17Z

src/wallet/rpcwallet.cpp

+    // Get the timeout
+    int nSleepTime = request.params[1].get_int();
+    // Timeout cannot be negative, otherwise it will relock immediately
+    if (nSleepTime < 0) {


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.

gmaxwell · 2018-01-08T10:51:35Z

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.

achow101 · 2018-01-08T16:10:20Z

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.

achow101 · 2018-01-08T20:00:35Z

I don't know why travis is failing.

bolekC · 2018-01-08T21:35:52Z

Maybe add a limit to "help" printout?
2272: "2. timeout (numeric, required) The time to keep the decryption key in seconds.\n"

bolekC · 2018-01-09T21:14:15Z

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

achow101 · 2018-01-10T19:21:00Z

@bolekC Added some info about the limit to the help output.

I think I fixed the travis error. It has to do with the type of the value being shifted. The log output is correct.

jimpo · 2018-01-10T19:43:52Z

test/functional/wallet-encryption.py

+        expected_time = int(time.time()) + (1 << 31) - 600
+        self.nodes[0].walletpassphrase(passphrase2, (1 << 31) - 600)
+        actual_time = self.nodes[0].getwalletinfo()['unlocked_until']
+        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

maflcko · 2018-01-10T20:46:06Z

test/functional/wallet-encryption.py

+        expected_time = int(time.time()) + (1 << 31) - 600
+        self.nodes[0].walletpassphrase(passphrase2, (1 << 31) - 600)
+        actual_time = self.nodes[0].getwalletinfo()['unlocked_until']
+        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.

achow101 · 2018-01-10T21:24:51Z

I changed the clamp to be at 2^31 - 1 seconds. Hopefully that fixes the travis problem with 32-bit linux.

maflcko · 2018-01-10T21:42:28Z

src/wallet/rpcwallet.cpp

+    }
+    // Clamp timeout to 2^31 seconds
+    if (nSleepTime > ((int64_t)1 << 31) - 1) {
+        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.

achow101 · 2018-01-12T16:38:29Z

@MarcoFalke adjusted

bolekC · 2018-01-12T21:09:43Z

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:

with big timeout (e.g. famous 9223372036854776) bitcoind (sometimes!!!) crashes with
libevent: event.c:3117: Assertion tv->tv_usec >= 0 failed in timeout_next

this one should be prevented by your solution
anyway worth to test

when timeout + time() (given timeout + current system time in seconds) >= 3 663 264 070 then wallet is blocked immediately (like in original issue)

no idea why this happens
your solution may fail when timeout will be 2^30 and system time will be later than: GMT: Monday, 22 January 2052 07:44:06

I 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?

achow101 · 2018-01-12T21:29:53Z

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.

bolekC · 2018-01-12T22:59:00Z

Sure it's libevent. I've mixed it up. There must be something wrong somewhere :)

laanwj · 2018-01-15T08:39:54Z

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.

laanwj · 2018-01-16T11:57:46Z

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.

ryanofsky

utACK 134cdc7

ryanofsky · 2018-01-16T15:47:43Z

src/wallet/rpcwallet.cpp

+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative.");
+    }
+    // Clamp timeout to 2^30 seconds
+    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.

maflcko · 2018-01-16T16:22:50Z

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

ryanofsky · 2018-01-16T17:13:06Z

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.

maflcko · 2018-01-16T17:58:10Z

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.

maflcko · 2018-01-16T18:08:53Z

utACK 134cdc7

achow101 · 2018-01-16T18:13:25Z

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.

ryanofsky · 2018-01-16T18:26:33Z

I think the suggestion is to simply not call RPCRunLater if there is an infinite timeout.

bolekC · 2018-01-16T21:06:44Z

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)

ajtowns · 2018-01-17T10:28:56Z

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 134cdc7 .

ajtowns · 2018-01-17T10:36:37Z

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.

…k its bounds 134cdc7 Test walletpassphrase timeout bounds and clamping (Andrew Chow) 0b63e3c Clamp walletpassphrase timeout to 2^(30) seconds and check its bounds (Andrew Chow) Pull request description: 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. Tree-SHA512: 426922f08c54e323d259e25dcdbebc2cd560708a65111ce6051493a7e7c61e79d9da1ea4026cc0d68807d728f5d7c0d7c58168c6ef4167b94cf6c2877af88794

laanwj · 2018-01-17T11:17:54Z

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

Sjors · 2018-02-22T09:40:19Z

This change seems to break wallet_encryption.py on MacOS, see #12375 by @AkioNak.

…nd check its bounds 134cdc7 Test walletpassphrase timeout bounds and clamping (Andrew Chow) 0b63e3c Clamp walletpassphrase timeout to 2^(30) seconds and check its bounds (Andrew Chow) Pull request description: Fixes bitcoin#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. Tree-SHA512: 426922f08c54e323d259e25dcdbebc2cd560708a65111ce6051493a7e7c61e79d9da1ea4026cc0d68807d728f5d7c0d7c58168c6ef4167b94cf6c2877af88794

luke-jr reviewed Jan 6, 2018

View reviewed changes

achow101 force-pushed the wallet-unlock-time-int32 branch from 8600546 to 370f105 Compare January 6, 2018 07:19

luke-jr reviewed Jan 6, 2018

View reviewed changes

achow101 force-pushed the wallet-unlock-time-int32 branch 2 times, most recently from d64e014 to f94b423 Compare January 6, 2018 07:24

luke-jr reviewed Jan 6, 2018

View reviewed changes

fanquake added the Wallet label Jan 6, 2018

bolekC reviewed Jan 6, 2018

View reviewed changes

achow101 force-pushed the wallet-unlock-time-int32 branch from f94b423 to c8ca057 Compare January 8, 2018 16:08

achow101 changed the title ~~Change walletpassphrase timeout to be an int32 and check its bounds~~ Clamp walletpassphrase timeout to 2^31 seconds and check its bounds Jan 8, 2018

achow101 force-pushed the wallet-unlock-time-int32 branch 3 times, most recently from 77db820 to 09fbbfa Compare January 8, 2018 18:45

achow101 force-pushed the wallet-unlock-time-int32 branch 2 times, most recently from fed0254 to 71620b3 Compare January 10, 2018 19:18

jimpo reviewed Jan 10, 2018

View reviewed changes

achow101 force-pushed the wallet-unlock-time-int32 branch from 71620b3 to a36965c Compare January 10, 2018 20:08

maflcko reviewed Jan 10, 2018

View reviewed changes

achow101 force-pushed the wallet-unlock-time-int32 branch 2 times, most recently from aa16e2d to 7945bd0 Compare January 10, 2018 21:24

maflcko reviewed Jan 10, 2018

View reviewed changes

achow101 force-pushed the wallet-unlock-time-int32 branch from 7945bd0 to 6eeeb13 Compare January 10, 2018 21:58

ajtowns mentioned this pull request Jan 16, 2018

[tests] Rename functional tests #11774

Merged

maflcko added the Needs release note label Jan 16, 2018

ryanofsky reviewed Jan 16, 2018

View reviewed changes

laanwj merged commit 134cdc7 into bitcoin:master Jan 17, 2018

maflcko mentioned this pull request Jan 19, 2018

TODO for release notes 0.16.0 #11054

Closed

AkioNak mentioned this pull request Feb 7, 2018

wallet_encryption.py fails on macos #12375

Closed

maflcko removed the Needs release note label Apr 8, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Clamp walletpassphrase timeout to 2^30 seconds and check its bounds #12101

Clamp walletpassphrase timeout to 2^30 seconds and check its bounds #12101

Conversation

achow101 commented Jan 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Jan 6, 2018

bolekC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaxwell commented Jan 8, 2018 • edited

achow101 commented Jan 8, 2018

achow101 commented Jan 8, 2018

bolekC commented Jan 8, 2018

bolekC commented Jan 9, 2018

achow101 commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Jan 12, 2018

bolekC commented Jan 12, 2018

achow101 commented Jan 12, 2018

bolekC commented Jan 12, 2018

laanwj commented Jan 15, 2018

laanwj commented Jan 16, 2018 • edited

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Jan 16, 2018

ryanofsky commented Jan 16, 2018

maflcko commented Jan 16, 2018

maflcko commented Jan 16, 2018

achow101 commented Jan 16, 2018

ryanofsky commented Jan 16, 2018

bolekC commented Jan 16, 2018

ajtowns commented Jan 17, 2018

ajtowns commented Jan 17, 2018

laanwj commented Jan 17, 2018

Sjors commented Feb 22, 2018

achow101 commented Jan 6, 2018 •

edited

gmaxwell commented Jan 8, 2018 •

edited

laanwj commented Jan 16, 2018 •

edited