Fix ChannelAgentForwarding: move rbuf.reset() and fix skip() in unkno…

[payne1982]

See #1025

Last post:
After a month of testing I found out the actual fix for the Agent Forwarding is not optimal in some cases.
There are now two bugs in the current write() method — one is the rbuf.skip() already described here, plus a second one introduced by the rbuf.reset() call at the top of the method (added in 2.27.3).

---

Bug 1 — rbuf.reset() at the top of write() breaks partial-message accumulation

The method is designed to handle SSH agent messages that arrive in multiple chunks: when the data received so far is shorter than the declared message length, it backs up the read pointer and returns:

  if (mlen > rbuf.getLength()) {
      rbuf.s -= 4;
      return;
  }

On the next write() call the new chunk is supposed to be appended after the existing partial data. However, rbuf.reset() at the very top of the method resets both s and index to 0 on every call, silently discarding whatever was accumulated in the previous call. The result is an infinite partial-message loop that never resolves — especially visible on high-latency links (mobile networks, VPN tunnels with jump hosts) where fragmentation is common.

The fix is remove rbuf.reset(). The buffer-resize allocation must also be corrected accordingly (rbuf.s + l → rbuf.index + l):

  -    rbuf.reset();
       if (rbuf.buffer.length < rbuf.index + l) {
  -      byte[] newbuf = new byte[rbuf.s + l];
  +      byte[] newbuf = new byte[rbuf.index + l];
         System.arraycopy(rbuf.buffer, 0, newbuf, 0, rbuf.buffer.length);
         rbuf.buffer = newbuf;
       }

---

Bug 2 — rbuf.skip() in the else branch advances the wrong pointer (already described in this issue)

Buffer.skip(n) increments index (the write pointer), not s (the read pointer). So after hitting the else branch for an unknown message type, s is left pointing one byte into the unread data. That orphaned byte is prepended to the next message's length field, corrupting all subsequent parsing.

The fix (as already proposed in this issue):

       } else {
  -      rbuf.skip(rbuf.getLength() - 1);
  +      rbuf.s += rbuf.getLength();
         mbuf.putByte(SSH_AGENT_FAILURE);
       }

---

Combined effect

On a high-latency connection to a modern OpenSSH server (which sends SSH2_AGENTC_EXTENSION, type 27) through a jump host, both bugs fire together: Bug 2 leaves an orphaned byte which, combined with Bug 1's reset-on-every-call behaviour, causes the agent-forwarding channel to stall indefinitely. The SSH session appears to hang on the first connection attempt; a retry succeeds because the session state is clean.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud