From 9375cacd65f812d604e0775d9891e69a2415ee1e Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Thu, 28 Sep 2023 11:02:02 +0000 Subject: [PATCH 1/5] Start on password related zipfs bugs --- generic/tclZipfs.c | 89 ++++++++++++++++++++++++++++++++++++++++--- tests/zipfiles/README | 2 +- tests/zipfs.test | 6 +-- 3 files changed, 87 insertions(+), 10 deletions(-) diff --git a/generic/tclZipfs.c b/generic/tclZipfs.c index 213ae23936e..b342c0516bc 100644 --- a/generic/tclZipfs.c +++ b/generic/tclZipfs.c @@ -229,10 +229,13 @@ typedef struct ZipEntry { int crc32; /* CRC-32 as stored in ZIP */ int timestamp; /* Modification time */ int isEncrypted; /* True if data is encrypted */ - int flags; -#define ZE_F_CRC_COMPARED 0x00000001 /* If 1, the CRC has been compared. */ -#define ZE_F_CRC_CORRECT 0x00000002 /* Only meaningful if ZE_F_CRC_COMPARED is 1 */ -#define ZE_F_VOLUME 0x00000004 /* Entry corresponds to //zipfs:/ */ + unsigned short flags; +#define ZE_F_CRC_COMPARED 0x0001 /* If 1, the CRC has been compared. */ +#define ZE_F_CRC_CORRECT 0x0002 /* Only meaningful if ZE_F_CRC_COMPARED is 1 */ +#define ZE_F_VOLUME 0x0004 /* Entry corresponds to //zipfs:/ */ +#define ZE_F_CRYPT_HDR_COMPARED 0x0008 /* If 1, the encryption header CRC has been compared. */ +#define ZE_F_CRYPT_HDR_CORRECT 0x0010 /* Only meaningful if ZE_F_CRYPT_HDR_COMPARED is 1 */ + unsigned char cryptHeaderCRC[2];/* May be one or two bytes depending on zip version */ unsigned char *data; /* File data if written */ struct ZipEntry *next; /* Next file in the same archive */ struct ZipEntry *tnext; /* Next top-level dir in archive */ @@ -328,6 +331,7 @@ static void SerializeLocalEntryHeader( const unsigned char *start, const unsigned char *end, unsigned char *buf, ZipEntry *z, int nameLength, int align); +static void ComputeCryptHeaderValidity(ZipEntry *z); #if !defined(STATIC_BUILD) static int ZipfsAppHookFindTclInit(const char *archive); #endif @@ -544,7 +548,7 @@ ZipWriteShort( ptr[0] = value & 0xff; ptr[1] = (value >> 8) & 0xff; } - + /* *------------------------------------------------------------------------- * @@ -728,6 +732,58 @@ CountSlashes( return count; } +/* + *------------------------------------------------------------------------ + * + * ComputeCryptHeaderValidity -- + * + * Computes the validity of the encryption header CRC for a ZipEntry. + * + * Results: + * None. + * + * Side effects: + * Sets the flags bits of the ZipEntry based on whether the validity + * could be computed and if so, whether the encryption header was valid. + * + *------------------------------------------------------------------------ + */ +static void ComputeCryptHeaderValidity(ZipEntry *z) +{ + if (z->flags & ZE_F_CRYPT_HDR_COMPARED) { + /* Already computed */ + return; + } + /* + * There are multiple possibilities. The last one or two bytes of the + * encryption header should match the last one or two bytes of the + * CRC of the file. Or the last byte of the encryption header should + * be the high order byte of the file time. Depending on the archiver + * and version, any of the might be in used. We follow libzip in checking + * only one byte against both the crc and the time. Note that by design + * the check generates high number of false positives in any case. + * Also, in case a check is passed when it should not, the final CRC + * calculation will (should) catch it. Only difference is it will be + * reported as a corruption error instead of incorrect password. + */ + int dosTime = ToDosTime(z->timestamp); + if (z->cryptHeaderCRC[1] == (unsigned char)(dosTime >> 8)) { + /* Infozip style - Tested with test-password.zip */ + z->flags |= ZE_F_CRYPT_HDR_COMPARED | ZE_F_CRYPT_HDR_CORRECT; + return; + } + /* DOS time did not match, may be CRC does */ + if (z->crc32) { + /* Pkware style - Tested with test-password2.zip */ + if (z->cryptHeaderCRC[1] == (unsigned char)(z->crc32 >> 24)) { + z->flags |= ZE_F_CRYPT_HDR_COMPARED | ZE_F_CRYPT_HDR_CORRECT; + } + else { + z->flags |= ZE_F_CRYPT_HDR_COMPARED; /* Compared and incorrect */ + } + } +} + /* *------------------------------------------------------------------------- * @@ -4930,9 +4986,30 @@ InitReadableChannel( 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]; - zdecode(info->keys, crc32tab, ch); + 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 + */ + z->cryptHeaderCRC[0] = encheader[10]; + z->cryptHeaderCRC[1] = encheader[11]; + ComputeCryptHeaderValidity(z); + if (z->flags & ZE_F_CRYPT_HDR_COMPARED) { + /* We had enough information to compare the encryption CRC */ + if (!(z->flags & ZE_F_CRYPT_HDR_CORRECT)) { + /* And it was found to be invalid */ + ZIPFS_ERROR(interp, "invalid password"); + ZIPFS_ERROR_CODE(interp, "PASSWORD"); + goto error_cleanup; + } } info->ubuf += i; } diff --git a/tests/zipfiles/README b/tests/zipfiles/README index 38ce998bfb6..a03663531ee 100644 --- a/tests/zipfiles/README +++ b/tests/zipfiles/README @@ -1,7 +1,7 @@ The files in this directory are used for testing zipfs file systems. They fall under the following licenses: -test-overlay.zip, test-password.zip, test-zip-in-zip.zip - Tcl's license +test-overlay.zip, test-password[2].zip, test-zip-in-zip.zip - Tcl's license All other files - test files from libzip (https://libzip.org) and are covered by the license in LICENSE-libzip. \ No newline at end of file diff --git a/tests/zipfs.test b/tests/zipfs.test index 705f26f95ee..972790fcfdd 100644 --- a/tests/zipfs.test +++ b/tests/zipfs.test @@ -1219,13 +1219,13 @@ namespace eval test_ns_zipfs { testConstraint bug-bbe7c6ff9e [expr {$::tcl_platform(os) ne "Darwin"}] testpassword plain plain.txt password plaintext testpassword plain-nopassword plain.txt "" plaintext - testpassword plain-badpassword plain.txt xxx plaintext + testpassword plain-badpassword plain.txt badpassword plaintext testpassword cipher cipher.bin password ciphertext -constraints bug-bbe7c6ff9e testpassword cipher-nopassword cipher.bin {} "decryption failed - no password provided" -returnCodes error - testpassword cipher-badpassword cipher.bin xxx "invalid CRC" -returnCodes error + testpassword cipher-badpassword cipher.bin badpassword "invalid password" -returnCodes error testpassword cipher-deflate cipher-deflate.bin password [lseq 100] -constraints bug-bbe7c6ff9e testpassword cipher-deflate-nopassword cipher-deflate.bin {} "decryption failed - no password provided" -returnCodes error - testpassword cipher-deflate-badpassword cipher-deflate.bin xxx "decompression error" -returnCodes error + testpassword cipher-deflate-badpassword cipher-deflate.bin badpassword "invalid password" -returnCodes error # # CRC errors From ebdc55ca4d872d922d6ebc846b9435d54bb02a2a Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Thu, 28 Sep 2023 11:32:16 +0000 Subject: [PATCH 2/5] Simplify password checks. No need to persist result --- generic/tclZipfs.c | 55 +++++++++++------------------- tests/zipfiles/test-password2.zip | Bin 0 -> 176 bytes tests/zipfs.test | 28 +++++++++------ 3 files changed, 37 insertions(+), 46 deletions(-) create mode 100644 tests/zipfiles/test-password2.zip diff --git a/generic/tclZipfs.c b/generic/tclZipfs.c index b342c0516bc..a8dff306711 100644 --- a/generic/tclZipfs.c +++ b/generic/tclZipfs.c @@ -229,13 +229,10 @@ typedef struct ZipEntry { int crc32; /* CRC-32 as stored in ZIP */ int timestamp; /* Modification time */ int isEncrypted; /* True if data is encrypted */ - unsigned short flags; + int flags; #define ZE_F_CRC_COMPARED 0x0001 /* If 1, the CRC has been compared. */ #define ZE_F_CRC_CORRECT 0x0002 /* Only meaningful if ZE_F_CRC_COMPARED is 1 */ #define ZE_F_VOLUME 0x0004 /* Entry corresponds to //zipfs:/ */ -#define ZE_F_CRYPT_HDR_COMPARED 0x0008 /* If 1, the encryption header CRC has been compared. */ -#define ZE_F_CRYPT_HDR_CORRECT 0x0010 /* Only meaningful if ZE_F_CRYPT_HDR_COMPARED is 1 */ - unsigned char cryptHeaderCRC[2];/* May be one or two bytes depending on zip version */ unsigned char *data; /* File data if written */ struct ZipEntry *next; /* Next file in the same archive */ struct ZipEntry *tnext; /* Next top-level dir in archive */ @@ -331,7 +328,7 @@ static void SerializeLocalEntryHeader( const unsigned char *start, const unsigned char *end, unsigned char *buf, ZipEntry *z, int nameLength, int align); -static void ComputeCryptHeaderValidity(ZipEntry *z); +static int IsCryptHeaderValid(ZipEntry *z, unsigned char cryptHeader[12]); #if !defined(STATIC_BUILD) static int ZipfsAppHookFindTclInit(const char *archive); #endif @@ -735,25 +732,23 @@ CountSlashes( /* *------------------------------------------------------------------------ * - * ComputeCryptHeaderValidity -- + * IsCryptHeaderValid -- * * Computes the validity of the encryption header CRC for a ZipEntry. * * Results: - * None. + * Returns 1 if the header is valid else 0. * * Side effects: - * Sets the flags bits of the ZipEntry based on whether the validity - * could be computed and if so, whether the encryption header was valid. + * None. * *------------------------------------------------------------------------ */ -static void ComputeCryptHeaderValidity(ZipEntry *z) +static int IsCryptHeaderValid( + ZipEntry *z, + unsigned char cryptHeader[12] + ) { - if (z->flags & ZE_F_CRYPT_HDR_COMPARED) { - /* Already computed */ - return; - } /* * There are multiple possibilities. The last one or two bytes of the * encryption header should match the last one or two bytes of the @@ -767,23 +762,20 @@ static void ComputeCryptHeaderValidity(ZipEntry *z) * reported as a corruption error instead of incorrect password. */ int dosTime = ToDosTime(z->timestamp); - if (z->cryptHeaderCRC[1] == (unsigned char)(dosTime >> 8)) { + if (cryptHeader[11] == (unsigned char)(dosTime >> 8)) { /* Infozip style - Tested with test-password.zip */ - z->flags |= ZE_F_CRYPT_HDR_COMPARED | ZE_F_CRYPT_HDR_CORRECT; - return; + return 1; } /* DOS time did not match, may be CRC does */ if (z->crc32) { /* Pkware style - Tested with test-password2.zip */ - if (z->cryptHeaderCRC[1] == (unsigned char)(z->crc32 >> 24)) { - z->flags |= ZE_F_CRYPT_HDR_COMPARED | ZE_F_CRYPT_HDR_CORRECT; - } - else { - z->flags |= ZE_F_CRYPT_HDR_COMPARED; /* Compared and incorrect */ - } + return (cryptHeader[11] == (unsigned char)(z->crc32 >> 24)); } + + /* No CRC, no way to verify. Assume valid */ + return 1; } - + /* *------------------------------------------------------------------------- * @@ -4999,17 +4991,10 @@ InitReadableChannel( * "standards" as to where the checksum for the encryption header * is */ - z->cryptHeaderCRC[0] = encheader[10]; - z->cryptHeaderCRC[1] = encheader[11]; - ComputeCryptHeaderValidity(z); - if (z->flags & ZE_F_CRYPT_HDR_COMPARED) { - /* We had enough information to compare the encryption CRC */ - if (!(z->flags & ZE_F_CRYPT_HDR_CORRECT)) { - /* And it was found to be invalid */ - ZIPFS_ERROR(interp, "invalid password"); - ZIPFS_ERROR_CODE(interp, "PASSWORD"); - goto error_cleanup; - } + if (!IsCryptHeaderValid(z, encheader)) { + ZIPFS_ERROR(interp, "invalid password"); + ZIPFS_ERROR_CODE(interp, "PASSWORD"); + goto error_cleanup; } info->ubuf += i; } diff --git a/tests/zipfiles/test-password2.zip b/tests/zipfiles/test-password2.zip new file mode 100644 index 0000000000000000000000000000000000000000..82bb73263ce04673435fdc6dc80c865ea6fe0708 GIT binary patch literal 176 zcmWIWW@Zs#U}RumSkqt=UaD69M-0g3g5u=Nf{fH6y`;>%gPQKUTi3cUpWJL9nzh5* zdHwgyoMff|Z$>7223-18fTny literal 0 HcmV?d00001 diff --git a/tests/zipfs.test b/tests/zipfs.test index 972790fcfdd..65f0ce765ad 100644 --- a/tests/zipfs.test +++ b/tests/zipfs.test @@ -1194,9 +1194,9 @@ namespace eval test_ns_zipfs { # # Password protected - proc testpassword {id filename password result args} { + proc testpassword {id zipfile filename password result args} { variable defaultMountPoint - set zippath [zippath test-password.zip] + set zippath [zippath $zipfile] test zipfs-password-read-$id "zipfs password read $id" -setup { unset -nocomplain fd if {$password ne ""} { @@ -1217,15 +1217,21 @@ namespace eval test_ns_zipfs { } # The bug bug-bbe7c6ff9e only manifests on macos testConstraint bug-bbe7c6ff9e [expr {$::tcl_platform(os) ne "Darwin"}] - testpassword plain plain.txt password plaintext - testpassword plain-nopassword plain.txt "" plaintext - testpassword plain-badpassword plain.txt badpassword plaintext - testpassword cipher cipher.bin password ciphertext -constraints bug-bbe7c6ff9e - testpassword cipher-nopassword cipher.bin {} "decryption failed - no password provided" -returnCodes error - testpassword cipher-badpassword cipher.bin badpassword "invalid password" -returnCodes error - testpassword cipher-deflate cipher-deflate.bin password [lseq 100] -constraints bug-bbe7c6ff9e - testpassword cipher-deflate-nopassword cipher-deflate.bin {} "decryption failed - no password provided" -returnCodes error - testpassword cipher-deflate-badpassword cipher-deflate.bin badpassword "invalid password" -returnCodes error + + # 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 # # CRC errors From 1cebc18c7595887b559fc17235acc0315299c9eb Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Thu, 28 Sep 2023 16:26:53 +0000 Subject: [PATCH 3/5] Proposed fix for [c315de9e44] --- generic/tclZipfs.c | 120 +++++++++++++++++++++++++++++---------------- tests/zipfs.test | 65 ++++++++++++++++++------ 2 files changed, 129 insertions(+), 56 deletions(-) diff --git a/generic/tclZipfs.c b/generic/tclZipfs.c index a8dff306711..80bcdc38d45 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 @@ -776,6 +778,57 @@ static int IsCryptHeaderValid( return 1; } +/* + *------------------------------------------------------------------------ + * + * 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; +} + /* *------------------------------------------------------------------------- * @@ -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 65f0ce765ad..f979f576e40 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 From 9fb03c8b99518f5210907247a9bdc5c397516012 Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Thu, 28 Sep 2023 17:11:39 +0000 Subject: [PATCH 4/5] Progress on [6d2ef441cc] --- generic/tclZipfs.c | 18 ++---------------- tests/zipfiles/test-password2.zip | Bin 176 -> 478 bytes tests/zipfs.test | 26 +++++++++++++++++++------- 3 files changed, 21 insertions(+), 23 deletions(-) diff --git a/generic/tclZipfs.c b/generic/tclZipfs.c index 80bcdc38d45..7616ad3f2d7 100644 --- a/generic/tclZipfs.c +++ b/generic/tclZipfs.c @@ -4873,21 +4873,7 @@ InitWritableChannel( unsigned char *zbuf = z->zipFilePtr->data + z->offset; if (z->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)); - for (i = 0; i < 12; i++) { - ch = info->ubuf[i]; - zdecode(info->keys, crc32tab, ch); - } - zbuf += i; + zbuf += 12; } if (z->compressMethod == ZIP_COMPMETH_DEFLATED) { @@ -4911,7 +4897,7 @@ InitWritableChannel( goto memoryError; } for (j = 0; j < stream.avail_in; j++) { - ch = info->ubuf[j]; + ch = zbuf[j]; cbuf[j] = zdecode(info->keys, crc32tab, ch); } stream.next_in = cbuf; diff --git a/tests/zipfiles/test-password2.zip b/tests/zipfiles/test-password2.zip index 82bb73263ce04673435fdc6dc80c865ea6fe0708..75a4d1c9946dc9a81ebcadc4720318c30c607321 100644 GIT binary patch literal 478 zcmWIWW@Zs#U}WH6IJ3bfd=b0z?O6;A42p~l3_?IUIkO-mwMaK5H7zHxBvmgdGw*I+ zzZCCZ!>EIbSARS>e3VPR@!39>b&2bf(x*MEKemxq?0Jraa%$46xHCI$=!6^CZn<{Y zVdCf8P0yN|;!mx-Azqq$P3!hy2II=yuDIRr3i|VSr#q|iKbrsN;l*#ZEc?SfgcWH#GOY@dDlrYpzyUMu>rdQ;Ym%&q%wZxg$ewcDm5GuwIZ-2_`DyZ9~^TRxe| zA^U6^1E4-(V7LzSNvT@-A2Fa0xS$x~6Oa$``k9R87oMIO%Iz8!dz>lz?DfspHIxFp z8JX-EaD{{l&=e3*0F&6lg$pXj$RNS6*w^K?=BJy$Fhm6iJFvMC*^W~{jR+SboBe{d V_d9m8S=m6gF#+KLAk6``9RS|&j$i-) literal 176 zcmWIWW@Zs#U}RumSkqt=UaD69M-0g3g5u=Nf{fH6y`;>%gPQKUTi3cUpWJL9nzh5* zdHwgyoMff|Z$>7223-18fTny diff --git a/tests/zipfs.test b/tests/zipfs.test index f979f576e40..e7f8de9a242 100644 --- a/tests/zipfs.test +++ b/tests/zipfs.test @@ -1246,7 +1246,7 @@ namespace eval test_ns_zipfs { fconfigure $fd -translation binary read $fd } - test zipfs-password-$id "zipfs write $id" -setup { + test zipfs-password-write-$id "zipfs write $id" -setup { unset -nocomplain fd if {$password ne ""} { zipfs mount $zippath $defaultMountPoint $password @@ -1261,12 +1261,24 @@ namespace eval test_ns_zipfs { 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 + # NOTE: test-password.zip is the DOS time based encryption header validity check (infozip style) + # test-password2.zip is the CRC based encryption header validity check (pkware style) + testpasswordw cipher-w-1 test-password.zip cipher.bin password w xyz + testpasswordw cipher-w-2 test-password2.zip cipher.bin password w xyz + testpasswordw cipher-deflate-w test-password2.zip cipher-deflate.bin password w xyz + testpasswordw cipher-badpass-w-1 test-password.zip cipher.bin badpass w {invalid password} -returnCodes error + testpasswordw cipher-badpass-w-2 test-password2.zip cipher.bin badpass w {invalid password} -returnCodes error + testpasswordw cipher-badpass-deflate-w test-password2.zip cipher-deflate.bin badpass w {invalid password} -returnCodes error + + testpasswordw cipher-w+ test-password.zip cipher.bin password w xyz + testpasswordw cipher-deflate-w+ test-password2.zip cipher-deflate.bin password w xyz + testpasswordw cipher-badpass-w+ test-password.zip cipher.bin badpass w {invalid password} -returnCodes error + testpasswordw cipher-badpass-deflate-w+ test-password2.zip cipher-deflate.bin badpass w {invalid password} -returnCodes error + + testpasswordw cipher-a+ test-password.zip cipher.bin password a+ ciphertextxyz + testpasswordw cipher-deflate-a+ test-password2.zip cipher-deflate.bin password a+ [lseq 100]xyz + testpasswordw cipher-badpass-a+ test-password.zip cipher.bin badpass a+ {invalid password} -returnCodes error + testpasswordw cipher-badpass-deflate-a+ test-password2.zip cipher-deflate.bin badpass a+ {invalid password} -returnCodes error # # CRC errors From bd10908764185f1cd2156ea68f00bab232761aca Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Fri, 29 Sep 2023 02:40:08 +0000 Subject: [PATCH 5/5] Minor edit --- generic/tclZipfs.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/generic/tclZipfs.c b/generic/tclZipfs.c index 7616ad3f2d7..7b3911caf5f 100644 --- a/generic/tclZipfs.c +++ b/generic/tclZipfs.c @@ -14,6 +14,10 @@ * generic/tclZipfs.c file in the TIP430-enabled Tcl cores. * compat/tclZipfs.c file in the tclconfig (TEA) file system, for pre-tip430 * projects. + * + * Helpful docs: + * https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.9.TXT + * https://libzip.org/specifications/appnote_iz.txt */ #include "tclInt.h" @@ -4839,8 +4843,7 @@ 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) { + if (z->isEncrypted) { assert(z->numCompressedBytes >= 12); /* caller should have checked*/ if (DecodeCryptHeader( interp, z, info->keys, z->zipFilePtr->data + z->offset) !=