Repository: pkg/sftp
Pull request #389 (2020-10-28 11:24:37)

Don't crash when the packet length is zero
A packet with an invalid length causes a panic in recvPacket. Patch includes the fuzz test that found this out in a few seconds.

greatroar: Not an API, but a client panics when it connects to a broken server like the following one:

```go
func TestBrokenServer(t *testing.T) {
	cr, sw := io.Pipe() // client reads, server writes
	sr, cw := io.Pipe() // server reads, client writes

	// Broken server that doesn't speak proper SFTP.
	go func() {
		go io.Copy(ioutil.Discard, sr)
		sw.Write([]byte{0, 0, 0, 0})
		sw.Close()
	}()

	_, err := NewClientPipe(cr, cw)
	if err != nil {
		t.Fatal(err)
	}
}
```
greatroar: I can change the fuzz test to do something similar. It would be more expensive to run, obviously.
davecheney: Can you write that as a failing test case?
davecheney: Remember that new client pipe doesn't actually need an io.pipe, that should make the test simpler, and simpler to write a fuzzer
greatroar: New test and fuzzer now calls NewClientPipe.
greatroar: The test failure is the same as one master (fixed in #387).
greatroar: > I would like to see a test against the external API exposed by NewClientPipe.

Done, and also added a test that crashes the server on current master.
puellanivis: Probably fine from me as well, but I’d like to get https://github.com/pkg/sftp/pull/387 merged first. Then a rebase would be required here.
greatroar: Rebased on #387.

Patch:
diff --git a/client.go b/client.go
index e329ed5..37557c5 100644
--- a/client.go
+++ b/client.go
@@ -155,9 +155,6 @@ func NewClientPipe(rd io.Reader, wr io.WriteCloser, opts ...ClientOption) (*Clie
 			inflight: make(map[uint32]chan<- result),
 			closed:   make(chan struct{}),
 		},
-
-		ext: make(map[string]string),
-
 		maxPacket:             1 << 15,
 		maxConcurrentRequests: 64,
 	}
@@ -186,8 +183,6 @@ func NewClientPipe(rd io.Reader, wr io.WriteCloser, opts ...ClientOption) (*Clie
 type Client struct {
 	clientConn
 
-	ext map[string]string // Extensions (name -> data).
-
 	maxPacket             int // max packet size read or written.
 	maxConcurrentRequests int
 	nextid                uint32
@@ -228,32 +223,14 @@ func (c *Client) recvVersion() error {
 		return &unexpectedPacketErr{sshFxpVersion, typ}
 	}
 
-	version, data := unmarshalUint32(data)
+	version, _ := unmarshalUint32(data)
 	if version != sftpProtocolVersion {
 		return &unexpectedVersionErr{sftpProtocolVersion, version}
 	}
 
-	for len(data) > 0 {
-		var ext extensionPair
-		ext, data, err = unmarshalExtensionPair(data)
-		if err != nil {
-			return err
-		}
-		c.ext[ext.Name] = ext.Data
-	}
-
 	return nil
 }
 
-// HasExtension checks whether the server supports a named extension.
-//
-// The first return value is the extension data reported by the server
-// (typically a version number).
-func (c *Client) HasExtension(name string) (string, bool) {
-	data, ok := c.ext[name]
-	return data, ok
-}
-
 // Walk returns a new Walker rooted at root.
 func (c *Client) Walk(root string) *fs.Walker {
 	return fs.WalkFS(root, c)
diff --git a/fuzz.go b/fuzz.go
new file mode 100644
index 0000000..2a2cd76
--- /dev/null
+++ b/fuzz.go
@@ -0,0 +1,22 @@
+// +build gofuzz
+
+package sftp
+
+import "bytes"
+
+type sink struct{}
+
+func (*sink) Close() error                { return nil }
+func (*sink) Write(p []byte) (int, error) { return len(p), nil }
+
+var devnull = &sink{}
+
+// To run: go-fuzz-build && go-fuzz
+func Fuzz(data []byte) int {
+	c, err := NewClientPipe(bytes.NewReader(data), devnull)
+	if err != nil {
+		return 0
+	}
+	c.Close()
+	return 1
+}
diff --git a/packet.go b/packet.go
index 8393e31..71f0675 100644
--- a/packet.go
+++ b/packet.go
@@ -154,6 +154,10 @@ func recvPacket(r io.Reader, alloc *allocator, orderID uint32) (uint8, []byte, e
 		debug("recv packet %d bytes too long", length)
 		return 0, nil, errLongPacket
 	}
+	if length == 0 {
+		debug("recv packet of 0 bytes too short")
+		return 0, nil, errShortPacket
+	}
 	if alloc == nil {
 		b = make([]byte, length)
 	}


Test patch:
diff --git a/client_integration_linux_test.go b/client_integration_linux_test.go
index 58d4d84..b3a0dd2 100644
--- a/client_integration_linux_test.go
+++ b/client_integration_linux_test.go
@@ -3,8 +3,6 @@ package sftp
 import (
 	"syscall"
 	"testing"
-
-	"github.com/stretchr/testify/require"
 )
 
 func TestClientStatVFS(t *testing.T) {
@@ -15,9 +13,6 @@ func TestClientStatVFS(t *testing.T) {
 	defer cmd.Wait()
 	defer sftp.Close()
 
-	_, ok := sftp.HasExtension("statvfs@openssh.com")
-	require.True(t, ok, "server doesn't list statvfs extension")
-
 	vfs, err := sftp.StatVFS("/")
 	if err != nil {
 		t.Fatal(err)
diff --git a/client_test.go b/client_test.go
index 05107c5..b2eacbc 100644
--- a/client_test.go
+++ b/client_test.go
@@ -1,6 +1,7 @@
 package sftp
 
 import (
+	"bytes"
 	"errors"
 	"io"
 	"os"
@@ -208,3 +209,22 @@ func TestUseFstatChecked(t *testing.T) {
 	testFstatOption(t, UseFstat(true), true)
 	testFstatOption(t, UseFstat(false), false)
 }
+
+type sink struct{}
+
+func (*sink) Close() error                { return nil }
+func (*sink) Write(p []byte) (int, error) { return len(p), nil }
+
+func TestClientZeroLengthPacket(t *testing.T) {
+	// Packet length zero (never valid). This used to crash the client.
+	packet := []byte{0, 0, 0, 0}
+
+	r := bytes.NewReader(packet)
+	c, err := NewClientPipe(r, &sink{})
+	if err == nil {
+		t.Error("expected an error, got nil")
+	}
+	if c != nil {
+		c.Close()
+	}
+}
diff --git a/server_test.go b/server_test.go
index bf905c0..f93ba3f 100644
--- a/server_test.go
+++ b/server_test.go
@@ -1,6 +1,7 @@
 package sftp
 
 import (
+	"bytes"
 	"io"
 	"os"
 	"path"
@@ -13,6 +14,7 @@ import (
 
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 const (
@@ -365,3 +367,31 @@ func TestStatNonExistent(t *testing.T) {
 		}
 	}
 }
+
+func TestServerWithBrokenClient(t *testing.T) {
+	validInit := sp(sshFxInitPacket{Version: 3})
+	brokenOpen := sp(sshFxpOpenPacket{Path: "foo"})
+	brokenOpen = brokenOpen[:len(brokenOpen)-2]
+
+	for _, clientInput := range [][]byte{
+		// Packet length zero (never valid). This used to crash the server.
+		{0, 0, 0, 0},
+		append(validInit, 0, 0, 0, 0),
+
+		// Client hangs up mid-packet.
+		append(validInit, brokenOpen...),
+	} {
+		srv, err := NewServer(struct {
+			io.Reader
+			io.WriteCloser
+		}{
+			bytes.NewReader(clientInput),
+			&sink{},
+		})
+		require.NoError(t, err)
+
+		err = srv.Serve()
+		assert.Error(t, err)
+		srv.Close()
+	}
+}