From cb1556337d12d2ebf083e0350253a4ae7b561981 Mon Sep 17 00:00:00 2001
From: greatroar <@>
Date: Fri, 30 Oct 2020 09:36:20 +0100
Subject: [PATCH] Don't crash when the packet length is zero

---
 client_test.go | 20 ++++++++++++++++++++
 fuzz.go        | 22 ++++++++++++++++++++++
 packet.go      |  4 ++++
 server_test.go | 30 ++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+)
 create mode 100644 fuzz.go

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/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)
 	}
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()
+	}
+}