From ebdc55ca4d872d922d6ebc846b9435d54bb02a2a Mon Sep 17 00:00:00 2001 From: apnadkarni Date: Thu, 28 Sep 2023 11:32:16 +0000 Subject: 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 b342c05..a8dff30 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 0000000..82bb732 Binary files /dev/null and b/tests/zipfiles/test-password2.zip differ diff --git a/tests/zipfs.test b/tests/zipfs.test index 972790f..65f0ce7 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 -- cgit v0.12