defer authentication/buffering to Read not Open

Author: alexmullins

README.txt

@@ -6,14 +6,12 @@ This package DOES NOT intend to implement the encryption methods
 mentioned in the original PKWARE spec (sections 6.0 and 7.0):
 https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT
 
-
 The process
-==============================================================================
+============
 hello.txt -> compressed -> encrypted -> .zip -> decrypted -> decompressed -> hello.txt
 
-
 Roadmap
-==============================================================================
+========
 Reading - Done.
     TODO:
     1. Change to streaming authentication and decryption. (Maybe not such a good
@@ -22,9 +20,8 @@ Reading - Done.
 Writing - Not started.
 Testing - Needs more.
 
-
 WinZip AES specifies
-==============================================================================
+=====================
 1. Encryption-Decryption w/ AES-CTR (128, 192, or 256 bits)
 2. Key generation with PBKDF2-HMAC-SHA1 (1000 iteration count) that
 generates a master key broken into the following:
@@ -71,8 +68,8 @@ used that was replaced by the encryption process mentioned in #8.
 15. AE-1 keeps the CRC and should be verified after decompression.
 AE-2 removes the CRC and shouldn't be verified after decompression.
 Refer to http://www.winzip.com/aes_info.htm#winzip11 for the reasoning.
-16. Storage Format (file data payload) totals CompressedSize64 bytes:
+16. Storage Format (file data payload totals CompressedSize64 bytes):
     a. Salt - 8, 12, or 16 bytes depending on keysize
     b. Password Verification Value - 2 bytes
-    c. Encrypted Data - compressed size - salt - pwv - auth lengths
+    c. Encrypted Data - compressed size - salt - pwv - auth code lengths
     d. Authentication code - 10 bytes

crypto.go

@@ -10,13 +10,20 @@ import (
 	"crypto/cipher"
 	"crypto/hmac"
 	"crypto/sha1"
+	"crypto/subtle"
+	"errors"
 	"io"
+	"io/ioutil"
 
 	"golang.org/x/crypto/pbkdf2"
 )
 
-// Counter (CTR) mode.
+// Decryption Errors
+var (
+	ErrDecryption = errors.New("zip: decryption error")
+)
 
+// Counter (CTR) mode.
 // CTR converts a block cipher into a stream cipher by
 // repeatedly encrypting an incrementing counter and
 // xoring the resulting stream of data with the input.
@@ -111,14 +118,75 @@ func xorBytes(dst, a, b []byte) int {
 	return n
 }
 
+type authReader struct {
+	data  io.Reader     // data to be authenticated
+	adata io.Reader     // the authentication code to read
+	akey  []byte        // authentication key
+	buf   *bytes.Buffer // buffer to store data to authenticate
+	err   error
+	auth  bool
+}
+
+func newAuthReader(akey []byte, data, adata io.Reader) io.Reader {
+	return &authReader{
+		data:  data,
+		adata: adata,
+		akey:  akey,
+		buf:   new(bytes.Buffer),
+		err:   nil,
+		auth:  false,
+	}
+}
+
+// Read will fully buffer the file data payload to authenticate first.
+// If authentication fails, returns ErrDecryption immediately.
+// Else, sends data along for decryption.
+func (a *authReader) Read(b []byte) (int, error) {
+	// check for sticky error
+	if a.err != nil {
+		return 0, a.err
+	}
+	// make sure we have auth'ed before we send any data
+	if !a.auth {
+		nn, err := io.Copy(a.buf, a.data)
+		if err != nil {
+			a.err = ErrDecryption
+			return 0, a.err
+		}
+		ab := new(bytes.Buffer)
+		nn, err = io.Copy(ab, a.adata)
+		if err != nil || nn != 10 {
+			a.err = ErrDecryption
+			return 0, a.err
+		}
+		a.auth = checkAuthentication(a.buf.Bytes(), ab.Bytes(), a.akey)
+		if !a.auth {
+			a.err = ErrDecryption
+			return 0, a.err
+		}
+	}
+	// so we've authenticated the data, now just pass it on.
+	n, err := a.buf.Read(b)
+	if err != nil {
+		a.err = err
+	}
+	return n, a.err
+}
+
 func checkAuthentication(message, authcode, key []byte) bool {
 	mac := hmac.New(sha1.New, key)
 	mac.Write(message)
 	expectedAuthCode := mac.Sum(nil)
 	// Truncate at the first 10 bytes
 	expectedAuthCode = expectedAuthCode[:10]
 	// Change to use crypto/subtle for constant time comparison
-	return bytes.Equal(expectedAuthCode, authcode)
+	b := subtle.ConstantTimeCompare(expectedAuthCode, authcode) > 0
+	return b
+}
+
+func checkPasswordVerification(pwvv, pwv []byte) bool {
+	b := subtle.ConstantTimeCompare(pwvv, pwv) > 0
+	return b
 }
 
 func generateKeys(password, salt []byte, keySize int) (encKey, authKey, pwv []byte) {
@@ -130,7 +198,7 @@ func generateKeys(password, salt []byte, keySize int) (encKey, authKey, pwv []by
 	return
 }
 
-func newDecryptionReader(r io.Reader, f *File) (io.Reader, error) {
+func newDecryptionReader(r *io.SectionReader, f *File) (io.ReadCloser, error) {
 	keyLen := aesKeyLen(f.aesStrength)
 	saltLen := keyLen / 2 // salt is half of key len
 	if saltLen == 0 {
@@ -141,38 +209,39 @@ func newDecryptionReader(r io.Reader, f *File) (io.Reader, error) {
 	// See:
 	// https://www.imperialviolet.org/2014/06/27/streamingencryption.html
 	// https://www.imperialviolet.org/2015/05/16/aeads.html
-	content := make([]byte, f.CompressedSize64)
-	if _, err := io.ReadFull(r, content); err != nil {
+	// grab the salt, pwvv, data, and authcode
+	saltpwvv := make([]byte, saltLen+2)
+	if _, err := r.Read(saltpwvv); err != nil {
 		return nil, ErrDecryption
 	}
-	// grab the salt, pwvv, data, and authcode
-	salt := content[:saltLen]
-	pwvv := content[saltLen : saltLen+2]
-	content = content[saltLen+2:]
-	size := f.CompressedSize64 - uint64(saltLen) - 2 - 10
-	data := content[:size]
-	authcode := content[size:]
+	salt := saltpwvv[:saltLen]
+	pwvv := saltpwvv[saltLen : saltLen+2]
+	dataOff := int64(saltLen + 2)
+	dataLen := int64(f.CompressedSize64 - uint64(saltLen) - 2 - 10)
+	data := io.NewSectionReader(r, dataOff, dataLen)
+	authOff := dataOff + dataLen
+	authcode := io.NewSectionReader(r, authOff, 10)
 	// generate keys
 	decKey, authKey, pwv := generateKeys(f.password, salt, keyLen)
 	// check password verifier (pwv)
 	// Change to use crypto/subtle for constant time comparison
-	if !bytes.Equal(pwv, pwvv) {
-		return nil, ErrDecryption
-	}
-	// check authentication
-	if !checkAuthentication(data, authcode, authKey) {
+	if !checkPasswordVerification(pwv, pwvv) {
 		return nil, ErrDecryption
 	}
-	return decryptStream(data, decKey), nil
+	// setup auth reader
+	ar := newAuthReader(authKey, data, authcode)
+	// return decryption reader
+	dr := decryptStream(decKey, ar)
+	return ioutil.NopCloser(dr), nil
 }
 
-func decryptStream(ciphertext, key []byte) io.Reader {
+func decryptStream(key []byte, ciphertext io.Reader) io.Reader {
 	block, err := aes.NewCipher(key)
 	if err != nil {
 		return nil
 	}
 	stream := newWinZipCTR(block)
-	reader := cipher.StreamReader{S: stream, R: bytes.NewReader(ciphertext)}
+	reader := &cipher.StreamReader{S: stream, R: ciphertext}
 	return reader
 }
 

reader.go

@@ -16,10 +16,9 @@ import (
 )
 
 var (
-	ErrFormat     = errors.New("zip: not a valid zip file")
-	ErrAlgorithm  = errors.New("zip: unsupported compression algorithm")
-	ErrChecksum   = errors.New("zip: checksum error")
-	ErrDecryption = errors.New("zip: decryption error")
+	ErrFormat    = errors.New("zip: not a valid zip file")
+	ErrAlgorithm = errors.New("zip: unsupported compression algorithm")
+	ErrChecksum  = errors.New("zip: checksum error")
 )
 
 type Reader struct {
@@ -53,6 +52,10 @@ func (f *File) IsEncrypted() bool {
 	return f.Flags&0x1 == 1
 }
 
+func (f *File) isAE2() bool {
+	return f.ae == 2
+}
+
 func (f *File) hasDataDescriptor() bool {
 	return f.Flags&0x8 != 0
 }
@@ -156,12 +159,14 @@ func (f *File) Open() (rc io.ReadCloser, err error) {
 	// and auth code lengths
 	size := int64(f.CompressedSize64)
 	var r io.Reader
-	r = io.NewSectionReader(f.zipr, f.headerOffset+bodyOffset, size)
+	rr := io.NewSectionReader(f.zipr, f.headerOffset+bodyOffset, size)
 	// check for encryption
 	if f.IsEncrypted() {
-		if r, err = newDecryptionReader(r, f); err != nil {
+		if r, err = newDecryptionReader(rr, f); err != nil {
 			return
 		}
+	} else {
+		r = rr
 	}
 	dcomp := decompressor(f.Method)
 	if dcomp == nil {
@@ -174,6 +179,14 @@ func (f *File) Open() (rc io.ReadCloser, err error) {
 	if f.hasDataDescriptor() {
 		desr = io.NewSectionReader(f.zipr, f.headerOffset+bodyOffset+size, dataDescriptorLen)
 	}
+	// if !f.isAE2() {
+	// 	rc = &checksumReader{
+	// 		rc:   rc,
+	// 		hash: crc32.NewIEEE(),
+	// 		f:    f,
+	// 		desr: desr,
+	// 	}
+	// }
 	rc = &checksumReader{
 		rc:   rc,
 		hash: crc32.NewIEEE(),

reader_test.go

@@ -605,97 +605,3 @@ func TestIssue11146(t *testing.T) {
 	}
 	r.Close()
 }
-
-func TestSimplePassword(t *testing.T) {
-	file := "hello-aes.zip"
-	var buf bytes.Buffer
-	r, err := OpenReader(filepath.Join("testdata", file))
-	if err != nil {
-		t.Errorf("Expected %s to open: %v.", file, err)
-	}
-	defer r.Close()
-	if len(r.File) != 1 {
-		t.Errorf("Expected %s to contain one file.", file)
-	}
-	f := r.File[0]
-	if f.FileInfo().Name() != "hello.txt" {
-		t.Errorf("Expected %s to have a file named hello.txt", file)
-	}
-	if f.Method != 0 {
-		t.Errorf("Expected %s to have its Method set to 0.", file)
-	}
-	f.SetPassword([]byte("golang"))
-	rc, err := f.Open()
-	if err != nil {
-		t.Errorf("Expected to open the readcloser: %v.", err)
-	}
-	_, err = io.Copy(&buf, rc)
-	if err != nil {
-		t.Errorf("Expected to copy bytes: %v.", err)
-	}
-	if !bytes.Contains(buf.Bytes(), []byte("Hello World\r\n")) {
-		t.Errorf("Expected contents were not found.")
-	}
-}
-
-func TestHelloWorldAes(t *testing.T) {
-	file := "world-aes.zip"
-	expecting := "helloworld"
-	r, err := OpenReader(filepath.Join("testdata", file))
-	if err != nil {
-		t.Errorf("Expected %s to open: %v", file, err)
-	}
-	defer r.Close()
-	if len(r.File) != 2 {
-		t.Errorf("Expected %s to contain two files.", file)
-	}
-	var b bytes.Buffer
-	for _, f := range r.File {
-		if !f.IsEncrypted() {
-			t.Errorf("Expected %s to be encrypted.", f.FileInfo().Name)
-		}
-		f.SetPassword([]byte("golang"))
-		rc, err := f.Open()
-		if err != nil {
-			t.Errorf("Expected to open readcloser: %v", err)
-		}
-		defer rc.Close()
-		if _, err := io.Copy(&b, rc); err != nil {
-			t.Errorf("Expected to copy bytes to buffer: %v", err)
-		}
-	}
-	if !bytes.Equal([]byte(expecting), b.Bytes()) {
-		t.Errorf("Expected ending content to be %s instead of %s", expecting, b.Bytes())
-	}
-}
-
-func TestMacbethAct1(t *testing.T) {
-	file := "macbeth-act1.zip"
-	expecting := "Exeunt"
-	var b bytes.Buffer
-	r, err := OpenReader(filepath.Join("testdata", file))
-	if err != nil {
-		t.Errorf("Expected %s to open: %v", file, err)
-	}
-	defer r.Close()
-	for _, f := range r.File {
-		if !f.IsEncrypted() {
-			t.Errorf("Expected %s to be encrypted.", f.Name)
-		}
-		f.SetPassword([]byte("golang"))
-		rc, err := f.Open()
-		if err != nil {
-			t.Errorf("Expected to open readcloser: %v", err)
-		}
-		defer rc.Close()
-		if _, err := io.Copy(&b, rc); err != nil {
-			t.Errorf("Expected to copy bytes to buffer: %v", err)
-		}
-	}
-	if !bytes.Contains(b.Bytes(), []byte(expecting)) {
-		t.Errorf("Expected to find %s in the buffer %v", expecting, b.Bytes())
-	}
-}
-
-// Test for AE-1 vs AE-2
-// Test for tampered data payload, use messWith