From 2abfae283cd6128cf36bb31bffad1c818e82bbb4 Mon Sep 17 00:00:00 2001
From: Nick Craig-Wood <nick@craig-wood.com>
Date: Fri, 20 Jan 2017 16:00:55 +0000
Subject: [PATCH] crypt: fix crypt writer getting stuck in a loop #902

This happened when the underlying reader returned io.ErrUnexpectedEOF.
The error handling for the call to io.ReadFull failed to take this
into account.

io.ErrUnexpectedEOF is reasonably common when SSL connections go wrong
when communicating with ACD, so it manifested itself as transfers from
non-encrypted ACD to encrypted ACD getting stuck.
---
 crypt/cipher.go      | 30 +++++++++++++++---------------
 crypt/cipher_test.go | 34 ++++++++++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/crypt/cipher.go b/crypt/cipher.go
index 85ee101f1..cd3cd9d3a 100644
--- a/crypt/cipher.go
+++ b/crypt/cipher.go
@@ -449,14 +449,13 @@ func (fh *encrypter) Read(p []byte) (n int, err error) {
 		// FIXME should overlap the reads with a go-routine and 2 buffers?
 		readBuf := fh.readBuf[:blockDataSize]
 		n, err = io.ReadFull(fh.in, readBuf)
-		if err == io.EOF {
-			// ReadFull only returns n=0 and EOF
-			return fh.finish(io.EOF)
-		} else if err == io.ErrUnexpectedEOF {
-			// Next read will return EOF
-		} else if err != nil {
+		if n == 0 {
+			// err can't be nil since:
+			// n == len(buf) if and only if err == nil.
 			return fh.finish(err)
 		}
+		// possibly err != nil here, but we will process the
+		// data and the next call to ReadFull will return 0, err
 		// Write nonce to start of block
 		copy(fh.buf, fh.nonce[:])
 		// Encrypt the block using the nonce
@@ -563,26 +562,27 @@ func (fh *decrypter) fillBuffer() (err error) {
 	// FIXME should overlap the reads with a go-routine and 2 buffers?
 	readBuf := fh.readBuf
 	n, err := io.ReadFull(fh.rc, readBuf)
-	if err == io.EOF {
-		// ReadFull only returns n=0 and EOF
-		return io.EOF
-	} else if err == io.ErrUnexpectedEOF {
-		// Next read will return EOF
-	} else if err != nil {
+	if n == 0 {
+		// err can't be nil since:
+		// n == len(buf) if and only if err == nil.
 		return err
 	}
+	// possibly err != nil here, but we will process the data and
+	// the next call to ReadFull will return 0, err
+
 	// Check header + 1 byte exists
 	if n <= blockHeaderSize {
+		if err != nil {
+			return err // return pending error as it is likely more accurate
+		}
 		return ErrorEncryptedFileBadHeader
 	}
 	// Decrypt the block using the nonce
 	block := fh.buf
 	_, ok := secretbox.Open(block[:0], readBuf[:n], fh.nonce.pointer(), &fh.c.dataKey)
 	if !ok {
-		// if block wouldn't decode and got unexpected EOF
-		// then return that as it is probably a better error
 		if err != nil {
-			return err
+			return err // return pending error as it is likely more accurate
 		}
 		return ErrorEncryptedBadBlock
 	}
diff --git a/crypt/cipher_test.go b/crypt/cipher_test.go
index 0d4414bdd..2face09fa 100644
--- a/crypt/cipher_test.go
+++ b/crypt/cipher_test.go
@@ -788,6 +788,21 @@ func TestNewEncrypter(t *testing.T) {
 
 }
 
+// Test the stream returning 0, io.ErrUnexpectedEOF - this used to
+// cause a fatal loop
+func TestNewEncrypterErrUnexpectedEOF(t *testing.T) {
+	c, err := newCipher(NameEncryptionStandard, "", "")
+	assert.NoError(t, err)
+
+	in := &errorReader{io.ErrUnexpectedEOF}
+	fh, err := c.newEncrypter(in)
+	assert.NoError(t, err)
+
+	n, err := io.CopyN(ioutil.Discard, fh, 1E6)
+	assert.Equal(t, io.ErrUnexpectedEOF, err)
+	assert.Equal(t, int64(32), n)
+}
+
 type errorReader struct {
 	err error
 }
@@ -854,6 +869,23 @@ func TestNewDecrypter(t *testing.T) {
 	}
 }
 
+// Test the stream returning 0, io.ErrUnexpectedEOF
+func TestNewDecrypterErrUnexpectedEOF(t *testing.T) {
+	c, err := newCipher(NameEncryptionStandard, "", "")
+	assert.NoError(t, err)
+
+	in2 := &errorReader{io.ErrUnexpectedEOF}
+	in1 := bytes.NewBuffer(file16)
+	in := ioutil.NopCloser(io.MultiReader(in1, in2))
+
+	fh, err := c.newDecrypter(in)
+	assert.NoError(t, err)
+
+	n, err := io.CopyN(ioutil.Discard, fh, 1E6)
+	assert.Equal(t, io.ErrUnexpectedEOF, err)
+	assert.Equal(t, int64(16), n)
+}
+
 func TestNewDecrypterSeek(t *testing.T) {
 	c, err := newCipher(NameEncryptionStandard, "", "")
 	assert.NoError(t, err)
@@ -936,8 +968,6 @@ func TestDecrypterRead(t *testing.T) {
 		case i == fileHeaderSize:
 			// This would normally produce an error *except* on the first block
 			expectedErr = nil
-		case i <= fileHeaderSize+blockHeaderSize:
-			expectedErr = ErrorEncryptedFileBadHeader
 		default:
 			expectedErr = io.ErrUnexpectedEOF
 		}