OpenSSH sftp cannot connect with v1.4.14-stable, whilst it could with v1.4.13-stable

[falemagn]

After compiling v1.4.14-stable and running the echo server

./echoserver -d / -P ciao:ciao

I tried running OpenSSH sftp for a test:

sftp -vvv -P 22222 ciao@localhost

But that doesn't work.

On the sftp side:

OpenSSH_8.9p1 Ubuntu-3ubuntu0.1, OpenSSL 3.0.2 15 Mar 2022
[...]
debug2: pubkey_prepare: done
debug3: send packet: type 5
debug3: receive packet: type 7
debug1: SSH2_MSG_EXT_INFO received
debug1: kex_input_ext_info: server-sig-algs=<rsa-sha2-256,ecdsa-sha2-nistp256>
Connection closed by 127.0.0.1 port 22222

On the echoserver side:

[DEBUG] accept state: KEYED
[DEBUG] PR1: size = 16
[INFO] GID: readjust size
[DEBUG] Receive: recvd = 16
[DEBUG] Decrypt aes128-ctr
[DEBUG] PR2: size = 64
[INFO] GID: readjust size
[DEBUG] Receive: recvd = 48
[DEBUG] Decrypt aes128-ctr
[DEBUG] VerifyMac hmac-sha2-256
[DEBUG] VM: inSz = 32
[DEBUG] VM: seq = 3
[DEBUG] VM: keyLen = 32
[DEBUG] PR: Decrypt fail
[DEBUG] accept error: KEYED, -1021
[DEBUG] Entering wolfSSH_get_error()
[DEBUG] Entering wolfSSH_ErrorToName()
[DEBUG] Entering wolfSSH_shutdown()

Decryption is failing.

Such a situation doesn't happen with v1.4.13-stable. In both cases, wolfssl/wolfcrypto v5.6.0 is being used.

The configure command line for wolfssh:

$ ../configure --enable-static --disable-shared --enable-all --enable-debug --enable-examples

The configure command line for wolfssl/wolfcrypto:

../configure --enable-keygen 'C_EXTRA_FLAGS=-DWOLFSSL_PUBLIC_MP -DWOLFSSL_RSA_KEY_CHECK' --enable-curve25519 --enable-ed25519 --enable-ed25519-stream --enable-aesctr --enable-aesni --enable-sp-math-all --enable-intelasm --enable-sp --enable-debug

[falemagn]

With git bisect I was able to identify the first commit that caused this precise symptom.

6a3e4856b488f3d8598ce3a587436997fd2f6d9b is the first bad commit
commit 6a3e4856b488f3d8598ce3a587436997fd2f6d9b
Author: John Safranek <[email protected]>
Date:   Thu Jun 15 13:25:33 2023 -0700

    I/O Buffer Refactor
    
    1. Started changing LENGTH_SZ to UINT32_SZ. The RFCs define the length
       of strings and mpints as uint32 values.
    2. Some whitespace changes.
    3. The getting data into the input buffer is done based on the goal size.
       The pattern is to get the first block, decrypt it if necessary, then read
       the size out, then get that much data plus the size of the length and the
       MAC.

Analyzing this commit, turns out there's a copy'n'paste error. The following patch fixes it:

diff --git a/src/internal.c b/src/internal.c
index 1cde473..de86ea2 100644
--- a/src/internal.c
+++ b/src/internal.c
@@ -7461,7 +7461,7 @@ int DoReceive(WOLFSSH* ssh)
                                     + peerBlockSz,
                                 ssh->inputBuffer.buffer + ssh->inputBuffer.idx
                                     + peerBlockSz,
-                                ssh->curSz - peerBlockSz);
+                                ssh->curSz + UINT32_SZ - peerBlockSz);
                     }
                     else {
                         /* Entire packet fit in one block, don't need

After applying that patch over the aforementioned commit, it all works again, but applying that patch over v1.4.14-stable makes another problem show up: sftp stalls waiting for some replies that never arrives.

debug3: send packet: type 50
debug2: we sent a publickey packet, wait for reply

If I then rerun sftp with the -oPreferredAuthentications=password option, it works.

[falemagn]

After further investigation, reverting the following two commits, and then applying the aforementioned patch, makes things work: 9d1996a, 169d2c6.

[anhu]

Hi @falemagn ,

Thank you so much for the report and the proposed fixes!! I think completely reverting 9d1996a, 169d2c6 might not be desirable. I will get this ticket assigned to a wolfSSL engineering team member so we can get to a more pin pointed solution.

Please stay tuned.

Warm regards, Anthony

[anhu]

@falemagn ,

I've assigned this to @bigbrett . He will be looking into this for you.

Warm regards, Anthony

[falemagn]

Hi @falemagn ,

Thank you so much for the report and the proposed fixes!! I think completely reverting 9d1996a, 169d2c6 might not be desirable.

I am sure it wouldn't, I just thought I'd point out that there lies the source of the issue, so to help you guys find a proper fix. :)

Thanks for the prompt response!

[JacobBarthelmeh]

When investigating another report I added this pull request (#559). I suspect it will resolve this issue too.

[falemagn]

When investigating another report I added this pull request (#559). I suspect it will resolve this issue too.

Only the first one, though, not the other caused by something happened because of 9d1996a and 169d2c6, as reported previously.

[JacobBarthelmeh]

For the second part of the issue I think this part of the pull request might be related https://github.com/wolfSSL/wolfssh/pull/559/files#diff-1c4e2f5adfa1ad30618e78ff459b2c0758ecf34278459ad0a8d58db4fec622eaR5845.

Before adding that I was seeing a hang in the example server after rejecting a peer's public key.

[falemagn]

You are right, that PR solves this specific issue. I was misled by the fact we're experiencing an issue in our code base which I was able to identify as stemming from this very issue we are discussing.

However, even with that PR, in our code base we still experience the hang. Has anything changed about what's expected from the user auth callback, due to 9d1996a and 169d2c6?

[falemagn]

Ok, I believe I have found it: there's a missing ret = WS_SUCCESS if the return code from the user auth callback is not SUCCESS and is not one of the errors being explicitly handled, which makes the function not return failure to the client.

I have added a review to PR #559.

[falemagn]

Given the merge of #559 in master, the issue can now be closed.