Proposed fix for [c315de9e44]

2 files changed, 129 insertions, 56 deletions

diff --git a/generic/tclZipfs.c b/generic/tclZipfs.c
index a8dff30..80bcdc3 100644
--- a/generic/tclZipfs.c
+++ b/generic/tclZipfs.c
@@ -328,7 +328,9 @@ static void		SerializeLocalEntryHeader(
 			    const unsigned char *start,
 			    const unsigned char *end, unsigned char *buf,
 			    ZipEntry *z, int nameLength, int align);
-static int		IsCryptHeaderValid(ZipEntry *z, unsigned char cryptHeader[12]);
+static int		IsCryptHeaderValid(ZipEntry *z, unsigned char cryptHdr[12]);
+static int		DecodeCryptHeader(Tcl_Interp *interp, ZipEntry *z,
+			    unsigned long keys[3], unsigned char cryptHdr[12]);
 #if !defined(STATIC_BUILD)
 static int		ZipfsAppHookFindTclInit(const char *archive);
 #endif
@@ -777,6 +779,57 @@ static int IsCryptHeaderValid(
 }
 
 /*
+ *------------------------------------------------------------------------
+ *
+ * DecodeCryptHeader --
+ *
+ *    Decodes the crypt header and validates it.
+ *
+ * Results:
+ *    TCL_OK on success, TCL_ERROR on failure.
+ *
+ * Side effects:
+ *    On success, keys[] are updated. On failure, an error message is
+ *    left in interp if not NULL.
+ *
+ *------------------------------------------------------------------------
+ */
+static int
+DecodeCryptHeader(Tcl_Interp *interp,
+		  ZipEntry *z,
+		  unsigned long keys[3],/* Updated on success. Must have been
+					   initialized by caller. */
+		  unsigned char cryptHeader[12]) /* From zip file content */
+{
+    int i;
+    int ch;
+    int len = z->zipFilePtr->passBuf[0] & 0xFF;
+    char passBuf[260];
+
+    for (i = 0; i < len; i++) {
+	ch = z->zipFilePtr->passBuf[len - i];
+	passBuf[i] = (ch & 0x0f) | pwrot[(ch >> 4) & 0x0f];
+    }
+    passBuf[i] = '\0';
+    init_keys(passBuf, keys, crc32tab);
+    memset(passBuf, 0, sizeof(passBuf));
+    unsigned char encheader[12];
+    memcpy(encheader, cryptHeader, 12);
+    for (i = 0; i < 12; i++) {
+	ch = cryptHeader[i];
+	ch ^= decrypt_byte(keys, crc32tab);
+	encheader[i] = ch;
+	update_keys(keys, crc32tab, ch);
+    }
+    if (!IsCryptHeaderValid(z, encheader)) {
+	ZIPFS_ERROR(interp, "invalid password");
+	ZIPFS_ERROR_CODE(interp, "PASSWORD");
+	return TCL_ERROR;
+    }
+    return TCL_OK;
+}
+
+/*
  *-------------------------------------------------------------------------
  *
  * DecodeZipEntryText --
@@ -4660,19 +4713,18 @@ ZipChannelOpen(
 	/* Read-only */
 	flags |= TCL_READABLE;
     }
-    if (flags & TCL_READABLE) {
-	if (z->isEncrypted) {
-	    if (z->numCompressedBytes < 12) {
-		ZIPFS_ERROR(interp, "decryption failed: truncated decryption header");
-		ZIPFS_ERROR_CODE(interp, "DECRYPT");
-		goto error;
 
-	    }
-	    if (z->zipFilePtr->passBuf[0] == 0) {
-		ZIPFS_ERROR(interp, "decryption failed - no password provided");
-		ZIPFS_ERROR_CODE(interp, "DECRYPT");
-		goto error;
-	    }
+    if (z->isEncrypted) {
+	if (z->numCompressedBytes < 12) {
+	    ZIPFS_ERROR(interp,
+			"decryption failed: truncated decryption header");
+	    ZIPFS_ERROR_CODE(interp, "DECRYPT");
+	    goto error;
+	}
+	if (z->zipFilePtr->passBuf[0] == 0) {
+	    ZIPFS_ERROR(interp, "decryption failed - no password provided");
+	    ZIPFS_ERROR_CODE(interp, "DECRYPT");
+	    goto error;
 	}
     }
 
@@ -4787,6 +4839,16 @@ InitWritableChannel(
     /* TODO - why is the memset necessary? Not cheap for default maxWrite. */
     memset(info->ubuf, 0, info->maxWrite);
 
+    info->isEncrypted = z->isEncrypted;
+    if (info->isEncrypted) {
+	assert(z->numCompressedBytes >= 12); /* caller should have checked*/
+	if (DecodeCryptHeader(
+		interp, z, info->keys, z->zipFilePtr->data + z->offset) !=
+	    TCL_OK) {
+	    goto error_cleanup;
+	}
+    }
+    
     if (trunc) {
 	/*
 	 * Truncate; nothing there.
@@ -4955,7 +5017,7 @@ InitReadableChannel(
 				 * from. */
 {
     unsigned char *ubuf = NULL;
-    int i, ch;
+    int ch;
 
     info->iscompr = (z->compressMethod == ZIP_COMPMETH_DEFLATED);
     info->ubuf = z->zipFilePtr->data + z->offset;
@@ -4968,35 +5030,11 @@ InitReadableChannel(
     info->numBytes = z->numBytes;
 
     if (info->isEncrypted) {
-	int len = z->zipFilePtr->passBuf[0] & 0xFF;
-	char passBuf[260];
-
-	for (i = 0; i < len; i++) {
-	    ch = z->zipFilePtr->passBuf[len - i];
-	    passBuf[i] = (ch & 0x0f) | pwrot[(ch >> 4) & 0x0f];
-	}
-	passBuf[i] = '\0';
-	init_keys(passBuf, info->keys, crc32tab);
-	memset(passBuf, 0, sizeof(passBuf));
-	unsigned char encheader[12];
-	memcpy(encheader, info->ubuf, 12);
-	for (i = 0; i < 12; i++) {
-	    ch = info->ubuf[i];
-	    ch ^= decrypt_byte(info->keys, crc32tab);
-	    encheader[i] = ch;
-	    update_keys(info->keys, crc32tab, ch);
-	}
-	/*
-	 * Validate the encryption key. This is tricky thanks to multiple
-	 * "standards" as to where the checksum for the encryption header
-	 * is 
-	 */
-	if (!IsCryptHeaderValid(z, encheader)) {
-	    ZIPFS_ERROR(interp, "invalid password");
-	    ZIPFS_ERROR_CODE(interp, "PASSWORD");
+	assert(z->numCompressedBytes >= 12); /* caller should have checked*/
+	if (DecodeCryptHeader(interp, z, info->keys, info->ubuf) != TCL_OK) {
 	    goto error_cleanup;
 	}
-	info->ubuf += i;
+	info->ubuf += 12;
     }
 
     if (info->iscompr) {
diff --git a/tests/zipfs.test b/tests/zipfs.test
index 65f0ce7..f979f57 100644
--- a/tests/zipfs.test
+++ b/tests/zipfs.test
@@ -1194,7 +1194,7 @@ namespace eval test_ns_zipfs {
 
     #
     # Password protected
-    proc testpassword {id zipfile filename password result args} {
+    proc testpasswordr {id zipfile filename password result args} {
         variable defaultMountPoint
         set zippath [zippath $zipfile]
         test zipfs-password-read-$id "zipfs password read $id" -setup {
@@ -1215,23 +1215,58 @@ namespace eval test_ns_zipfs {
             gets $fd
         } -result $result {*}$args
     }
-    # The bug bug-bbe7c6ff9e only manifests on macos
+    # The bug bbe7c6ff9e only manifests on macos
     testConstraint bug-bbe7c6ff9e [expr {$::tcl_platform(os) ne "Darwin"}]
 
     # NOTE: test-password.zip is the DOS time based encryption header validity check (infozip style)
-    # NOTE: test-password2.zip is the CRC based encryption header validity check (pkware style)
-    testpassword plain                  test-password.zip plain.txt password plaintext
-    testpassword plain-nopass           test-password.zip plain.txt "" plaintext
-    testpassword plain-badpass          test-password.zip plain.txt badpassword plaintext
-    testpassword cipher-1               test-password.zip cipher.bin password ciphertext -constraints bug-bbe7c6ff9e
-    testpassword cipher-2               test-password2.zip cipher.bin password ciphertext -constraints bug-bbe7c6ff9e
-    testpassword cipher-nopass-1        test-password.zip cipher.bin {} "decryption failed - no password provided" -returnCodes error
-    testpassword cipher-nopass-2        test-password2.zip cipher.bin {} "decryption failed - no password provided" -returnCodes error
-    testpassword cipher-badpass-1       test-password.zip cipher.bin badpassword "invalid password" -returnCodes error
-    testpassword cipher-badpass-2       test-password2.zip cipher.bin badpassword "invalid password" -returnCodes error
-    testpassword cipher-deflate         test-password.zip cipher-deflate.bin password [lseq 100] -constraints bug-bbe7c6ff9e
-    testpassword cipher-deflate-nopass  test-password.zip cipher-deflate.bin {} "decryption failed - no password provided" -returnCodes error
-    testpassword cipher-deflate-badpass test-password.zip cipher-deflate.bin badpassword "invalid password" -returnCodes error
+    #       test-password2.zip is the CRC based encryption header validity check (pkware style)
+    testpasswordr plain                  test-password.zip plain.txt password plaintext
+    testpasswordr plain-nopass           test-password.zip plain.txt "" plaintext
+    testpasswordr plain-badpass          test-password.zip plain.txt badpassword plaintext
+    testpasswordr cipher-1               test-password.zip cipher.bin password ciphertext -constraints bug-bbe7c6ff9e
+    testpasswordr cipher-2               test-password2.zip cipher.bin password ciphertext -constraints bug-bbe7c6ff9e
+    testpasswordr cipher-nopass-1        test-password.zip cipher.bin {} "decryption failed - no password provided" -returnCodes error
+    testpasswordr cipher-nopass-2        test-password2.zip cipher.bin {} "decryption failed - no password provided" -returnCodes error
+    testpasswordr cipher-badpass-1       test-password.zip cipher.bin badpassword "invalid password" -returnCodes error
+    testpasswordr cipher-badpass-2       test-password2.zip cipher.bin badpassword "invalid password" -returnCodes error
+    testpasswordr cipher-deflate         test-password.zip cipher-deflate.bin password [lseq 100] -constraints bug-bbe7c6ff9e
+    testpasswordr cipher-deflate-nopass  test-password.zip cipher-deflate.bin {} "decryption failed - no password provided" -returnCodes error
+    testpasswordr cipher-deflate-badpass test-password.zip cipher-deflate.bin badpassword "invalid password" -returnCodes error
+
+    proc testpasswordw {id zippath filename password mode result args} {
+        variable defaultMountPoint
+        set zippath [zippath $zippath]
+        set path [file join $defaultMountPoint $filename]
+        set body {
+            set fd [open $path $mode]
+            fconfigure $fd -translation binary
+            puts -nonewline $fd "xyz"
+            close $fd
+            set fd [open $path]
+            fconfigure $fd -translation binary
+            read $fd
+        }
+        test zipfs-password-$id "zipfs write $id" -setup {
+            unset -nocomplain fd
+            if {$password ne ""} {
+                zipfs mount $zippath $defaultMountPoint $password
+            } else {
+                zipfs mount $zippath $defaultMountPoint
+            }
+        } -cleanup {
+            # In case open succeeded when it should not
+            if {[info exists fd]} {
+                close $fd
+            }
+            cleanup
+        } -body $body -result $result {*}$args
+    }
+    testpasswordw write-w test-password.zip cipher.bin password w xyz
+    testpasswordw write-badpass-w test-password.zip cipher.bin badpass w {invalid password} -returnCodes error
+    testpasswordw write-w+ test-password.zip cipher.bin password w xyz
+    testpasswordw write-badpass-w+ test-password.zip cipher.bin badpass w {invalid password} -returnCodes error
+    testpasswordw write-a+ test-password.zip cipher.bin password a+ ciphertextxyz
+    testpasswordw write-badpass-a+ test-password.zip cipher.bin badpass a+ {invalid password} -returnCodes error
 
     #
     # CRC errors