proposal: x/crypto/ssh: read single packets from SSH channels

[NHAS]

Proposal Details

I propose a new ssh.Channel function to read a single SSH packet.

This is required in to inter-opt with some channel types in use by OpenSSH.

For example to inter-opt with the tun@openssh.com channel type defined in section 2.3 of the openssh protocol standard:

283: Once established the client and server may exchange packet or frames 284: over the tunnel channel by encapsulating them in SSH protocol strings 285: and sending them as channel data. This ensures that packet boundaries 286: are kept intact. Specifically, packets are transmitted using normal 287: SSH_MSG_CHANNEL_DATA packets:

It is required that you are able to read each individual SSH packet as it defines the length of each Network packet.

Currently the Read(...) function defined by the ssh.Channel interface reads all buffered SSH packets. This requires the implementer of tun@openssh.com and other similar channel types to attempt to determine SSH packet size (and thus network packet size) from the data that is returned from Read(...).

This is error prone and in some cases not feasible.

The following are two snippets to show how this may be implemented.

Example addition to ssh.buffer:

func (b *buffer) ReadSingle() ([]byte, error) {

    sb.Cond.L.Lock()
    defer sb.Cond.L.Unlock()

    if sb.closed {
        return nil, io.EOF
    }

    if len(sb.head.buf) == 0 && sb.head == sb.tail {
        // If we have no messages right now, just wait until we do
        sb.Cond.Wait()
        if sb.closed {
            return nil, io.EOF
        }
    }

    result := make([]byte, len(sb.head.buf))
    n := copy(result, sb.head.buf)

    sb.head.buf = sb.head.buf[n:]

    if sb.head != sb.tail {
        sb.head = sb.head.next
    }

    return result, nil
}

Example addition to ssh.channel (and thus the ssh.Channel interface):

func (c *channel) ReadSSHPacket() ([]byte, error) {
    buff, err := m.pending.ReadSingle()
    if err != nil {
        return nil, err
    }

    if len(buff) > 0 {
        err = c.adjustWindow(uint32(len(buff)))
        if len(buff) > 0 && err == io.EOF {
            err = nil
        }
    }

    return buff, err
}

Additionally, here is a working example of how this must currently be done which uses incredibly brittle reflections.

https://github.com/NHAS/reverse_ssh/blob/f5d2a6cd8562e5f5ff33551aa37285651b90a309/internal/client/handlers/tun.go#L356

[gabyhelp]

Related Issues and Documentation

• proposal: x/crypto/ssh: new API to allow user to get channel close event #67672 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

note that we can't change the Channel interface definition for backwards compatibility reasons.

[ianlancetaylor]

CC @golang/security

[NHAS]

note that we can't change the Channel interface definition for backwards compatibility reasons.

Does that freeze include adding a method? As I don't want to change any existing function signatures.

Even adding a public ReadPacket method to the ssh.channel struct would make implementation easier as it wouldn't rely on getting an unsafe pointer to the ssh.buffer to interact with it directly.

[NHAS]

Two other alternatives could be:

1. Add a new ssh package function. Such as ssh.ReadPacket(ssh.Channel) []byte

That does an internal cast to ssh.channel and calls the public method ReadSSHPacket() as defined previously.

This would mean there wasn't a requirement to change the ssh.Channel interface

2. Add a transformer for ssh.Channel so that the Read() only reads a single buffer element then returns eof.

E.g

spr := ssh.SinglePacketReader(channel)

Which returns ssh.Channel with concrete implementation.

type singlePacketChannel struct {
    channel
}

func (spc *singlePacketChannel) Read([]byte) (int, error) {
  // Reads until single element in buffer is exhausted
}