SSH2: Decryption Failures During Read

[rposky]

Hello again!

In upgrading from 3.0.35 => 3.0.37, I've begun to see SSH2 data decryption failures using read(), the cause of which I believe are related to commit 2fb6f31.

• Received initialization vector of size 0, but size 16 is required
• Error decrypting ciphertext with libsodium

The failures are intermittent but consistent enough, particularly in high latency environments with "chatty" servers. Through added logging, I observed stream_set_timeout within _get_binary_packet invoked with smaller and smaller values, coinciding with the decreasing cur_timeout allotted to read(), <10,000 usec before erroring. Also just prior to erroring, I observed that the $tag value within the aes128-gcm@openssh.com|aes256-gcm@openssh.com decryption algorithm case was smaller than the requested block size of 16.

I suspect that the diminishing stream timeout can become prohibitive to retrieving further stream contents necessary for decryption, arising from the multiple stream_get_contents invocations within decrypt processing.

The intent behind commit 2fb6f31 seems sane though, which as best I can tell is to better honor the current timeout while processing packets.

I suppose a deciding factor here is whether the timeout represents a promise to return control to the caller within said time, or, more simply, a limit on time spent blocking. That'd inform whether the SSH2 client should abandon packet processing upon timeout if the entire payload cannot be retrieved within the time frame, probably buffering read stream contents in the process, or allowing for some additional time spent blocking beyond the timeout to fully retrieve the payload, the prior behavior.

Regardless, the decryption errors upon close of the timeout window should be avoided for well-formed packets.

[terrafrost]

Regardless, the decryption errors upon close of the timeout window should be avoided for well-formed packets.

It's tricky because what's the point of setting a hard limit if you're gonna be all wishy washy about it?

Consider the following scenario. You say "I'm gonna spend no more than $500 on food and that's it!" and then you find yourself at $498 for the month. If you're then like "I'm not at $500 yet so imma spend $30 at this restaurant!" then you're moving the goal post and it kinda calls into question your commitment.

If your goal, from the get go, is to stop spending once you exceed $500 that'd be one thing but that wasn't the original goal in this scenario.

In this case, however, we don't know how much money the restaurant is going to charge for the food. In-so-far as you know it's $30 or it could be $1,000 - you don't know.

phpseclib doesn't know how much time it's going to take to receive the packet so if it just ignores the timeout until it's received the entire packet it could, in-so-far as it knows, take 1m for the server to send the rest of the packet, which seems suboptimal.

Is there a reason you can't just increase the timeout? You couldn't set the timeout to 0 in phpseclib 3.0.36 but 3.0.37 fixed that.

[rposky]

Your response to the quote you gave didn't add up for me. Perhaps you were responding to the paragraph above w.r.t. definition of the timeout parameter? It may make the most intuitive sense as a hard limit on execution time for read() and other calls, but IJS there are other possibilities in its definition and that may also lend to less complexity in implementation. Like if you actually wanted to make it a perfect limit on execution time, you would also have to carry forward timeout concerns to data packet decryption, as an example - it gets complicated.

Yes, we have increased the timeout to workaround the issue, but I would still like to use some timeout value and not allow calls to read() to block indefinitely, as would be the case with timeout=0, no? The read() decryption failure issue remains so long as we are using timeout, albeit we are seeing it much less at larger values. Surely, the recommendation is not to circumvent timeout altogether though, i.e. with timeout=0?

As it stands, the service quality of SSH2 degrades with the close of the timeout window, regardless of any positive timeout value that is used. As initially stated, I think you were right to introduce some timeout value on these previously unlimited stream_get_contents invocations, but at the point that the timeout value is very small, it becomes absurd.

There also does not seem to be good handling for when those read() decrypt stream_get_contents invocations do time out. Presently, the decrypt process is attempted on partial data, resulting in error, and that part of the server response is lost.

If maintaining timeout as more of a promise of control from read() within said time, it would seem those partial responses need to be cached so they can be re-assembled upon subsequent call to read().

[terrafrost]

Like if you actually wanted to make it a perfect limit on execution time, you would also have to carry forward timeout concerns to data packet decryption, as an example - it gets complicated.

Sure, but I think there's a bit of a balancing act between perfection and good enough. The addition of stream_set_timeout actually fixed an issue someone was having (see #1977) - it wasn't done done as some sort of prophylactic measure whereas subtracting the time spent encrypting / decrypting would purely be prophylactic at this point.

I would still like to use some timeout value and not allow calls to read() to block indefinitely, as would be the case with timeout=0, no?

I guess I could add another setTimeout method specifically for individual binary packets. eg. setPerPacketTimeout() or some such. So like if you did setTimeout(0) but then setPerPacketTimeout(10) then an infinite number of packet could be read so long as none of those packets took > 10s to load.

[rposky]

Thanks for the continued replies.

Interesting idea about setPerPacketTimeout. Are you thinking that also implies more graceful handling of packet timeout events, i.e. yield from read() what has been decrypted thus far and buffer the rest, or continue to let chips fall where they may, likely erroring during subsequent packet decryption?

If the former, I'm not sure the per-packet timeout is necessary. If the latter, then it seems the error potential remains, though is practically removed with sufficient packet timeout, albeit may be to hard to know what "sufficient" is. And since packet retrieval is dispersed across multiple stream_get_contents invocations, it's still tempting to maintain a diminishing timeout window among them for a "packet" scoped timeout, and you're back to the same issue.

However, it occurs to me that during read(), if the remaining cur_timeout is approaching the packet timeout, then you might not even attempt the next packet retrieval, which would seem to mitigate more of the error potential.

Spitballing with this, but would it be possible/sane to maintain some sense of packet retrieval time across the ssh2 session and factor that metric into the packet timeout, thereby encapsulating the concern from the user and creating a dynamic behavior.

[terrafrost]

Apologies for the delay - I've been vacationing.

Anyway,

Are you thinking that also implies more graceful handling of packet timeout events, i.e. yield from read() what has been decrypted thus far and buffer the rest, or continue to let chips fall where they may, likely erroring during subsequent packet decryption?

Neither - you'd just have more fine tuned control over when it's going to error under my proposal. So like if you did setTimeout(0) but then setPerPacketTimeout(10) then an infinite number of packet could be read so long as none of those packets took > 10s to load. So let's say you're doing $ssh->read('#[prompt]#'). Let's also say that it takes 3x packets for you #[prompt]# to match and that each of those 3x packets takes 9s to be read in their entirety. With setTimeout(10) it'd timeout after the first packet. Like the first packet would be read in its entirety and then a portion of the second packet. But if you did setTimeout(0) with setPerPacketTimeout(10) then the whole thing could be read.

However, it occurs to me that during read(), if the remaining cur_timeout is approaching the packet timeout, then you might not even attempt the next packet retrieval, which would seem to mitigate more of the error potential.

You'd probably want a sliding scale for how close is too close. Like one packet took 9s and you want to timeout after 10s then it seems reasonable to assume that 1s won't be enough time to retrieve the second packet. BUT if you were getting a bunch of packets and each one took 0.1s to receive then having 1s left would still mean that you should, in theory, be able to receive 10 more packets.

But even then, packets can take variable amounts of time to be sent. Like maybe you do $ssh->write("echo hello; sleep 10; echo world\n"). The first packet would presumably take almost no time at all to be sent whereas the second would take a lot longer. So I'm not even sure it's appropriate to be making assumptions for how long it ought to take for the next packet to be received. At least not for phpseclib. phpseclib is supposed to be an OS agnostic SSH client - not a Linux command parser.

[rposky]

But even then, packets can take variable amounts of time to be sent. Like maybe you do $ssh->write("echo hello; sleep 10; echo world\n"). The first packet would presumably take almost no time at all to be sent whereas the second would take a lot longer. So I'm not even sure it's appropriate to be making assumptions for how long it ought to take for the next packet to be received.

True, though perhaps this scenario is less applicable to the decrypt phase of get_binary_packet (relevant snippet of SSH2::get_binary_packet copied below), since we go back to the wire for the remainder of the encrypted payload, which presumably is already in-transit. Tracking the average time spent in stream wait during subsequent read into $raw or during initialization of $tag would seem to mostly reflect latency, given that initial contents of the payload have already arrived.

$start = microtime(true);
if ($this->curTimeout) {
    $sec = (int) floor($this->curTimeout);
    $usec = (int) (1000000 * ($this->curTimeout - $sec));
    stream_set_timeout($this->fsock, $sec, $usec);
}
$raw = stream_get_contents($this->fsock, $this->decrypt_block_size);

if (!strlen($raw)) {
    $this->bitmap = 0;
    throw new ConnectionClosedException('No data received from server');
}

if ($this->decrypt) {
            switch ($this->decryptName) {
                case 'aes128-gcm@openssh.com':
                case 'aes256-gcm@openssh.com':
                    $this->decrypt->setNonce(
                        $this->decryptFixedPart .
                        $this->decryptInvocationCounter
                    );
                    Strings::increment_str($this->decryptInvocationCounter);
                    $this->decrypt->setAAD($temp = Strings::shift($raw, 4));
                    extract(unpack('Npacket_length', $temp));
                    /**
                     * @var integer $packet_length
                     */

                    $raw .= $this->read_remaining_bytes($packet_length - $this->decrypt_block_size + 4);
                    $stop = microtime(true);
                    $tag = stream_get_contents($this->fsock, $this->decrypt_block_size);
                    $this->decrypt->setTag($tag);
                    $raw = $this->decrypt->decrypt($raw);
                    $raw = $temp . $raw;
                    $remaining_length = 0;
                    break;

I mention all that in consideration still as to how to account for or avoid timeout errors which may arise during the multiple stream reads shown above, and which presently result in decrypt error and loss of the partially processed packet.

I think now that setPerPacketTimeout doesn't do enough to address the issue. What do you think about increasing the initial stream wait done within the $skip_channel_filter case of get_binary_packet() to account for the entire encrypted payload, i.e. do not begin decrypting packet contents until fully arrived? That way, subsequent stream_get_contents calls will just fetch from the buffer. The initial stream wait would be a sane point in the code to enforce a timeout without leaving a partial packet in the buffer.

[terrafrost]

I think your proposal could work but the behavior of setTimeout() isn't going to change. A new function could be created to manage this new behavior however. Submit a PR?

I already presented a philosophical argument with my "I'm gonna spend no more than $500 on food and that's it!" from earlier but another reason to keep it is...

You can set your alarm to (1) wake up you up at a specific time (eg. 8:00am or some such) or you could (2) have your phone monitor where you're at in your sleep cycle and wake up you up at the lightest point of your sleep cycle to minimize grogginess or whatever. (2) is probably superior to (1) for a narrow use case (eg. using an alarm to wake you up in the morning) but what about the use case where you're already awake and want to set an alarm because you you're walking your dog and need to wrap that up at 1:50pm so you can be ready for that 2:00pm Zoom meeting?

I'm open to having alternate timeout implementations living beside one another but I'm not open to replacing one with another.