From b68a02d74f81be82564611a92ee432295944fa9f Mon Sep 17 00:00:00 2001 From: Dov Frankel Date: Thu, 26 Sep 2019 08:10:55 -0400 Subject: [PATCH] Made ignoreCRCMismatches property public, so it can be set by an API consumer, for purposes such as implementing a user preference to always ignore mismatches for a given file (or all files, etc.) (Issue #82) --- Classes/URKArchive.h | 27 +++++++++++++++++++-------- Classes/URKArchive.mm | 1 - Tests/CheckDataTests.m | 27 ++++++++++++++++++++++++++- Tests/ExtractBufferedDataTests.m | 7 +------ Tests/ExtractDataTests.m | 7 +------ Tests/ExtractFilesTests.m | 18 ++++-------------- Tests/IterateFileInfoTests.m | 7 +------ Tests/ListFileInfoTests.m | 9 ++------- Tests/ListFilenamesTests.m | 7 +------ Tests/ListVolumesTests.m | 9 ++------- Tests/PerformOnDataTests.m | 9 ++------- Tests/PerformOnFilesTests.m | 9 ++------- Tests/URKArchiveTests.m | 6 ++++-- 13 files changed, 65 insertions(+), 78 deletions(-) diff --git a/Classes/URKArchive.h b/Classes/URKArchive.h index eab6571..113c483 100644 --- a/Classes/URKArchive.h +++ b/Classes/URKArchive.h @@ -25,7 +25,7 @@ DllHppIgnore typedef NS_ENUM(NSInteger, URKErrorCode) { /** - * The archive's header is empty + * The last file of the archive has been read */ URKErrorCodeEndOfArchive = ERAR_END_ARCHIVE, @@ -35,7 +35,7 @@ typedef NS_ENUM(NSInteger, URKErrorCode) { URKErrorCodeNoMemory = ERAR_NO_MEMORY, /** - * The header is broken + * The header's CRC doesn't match the decompressed data's CRC */ URKErrorCodeBadData = ERAR_BAD_DATA, @@ -173,6 +173,17 @@ extern NSString *URKErrorDomain; */ @property(nullable, strong) NSProgress *progress; +/** + * When performing operations on a RAR archive, the contents of compressed files are checked + * against the record of what they were when the archive was created. If there's a mismatch, + * either the metadata (header) or archive contents have become corrupted. You can defeat this check by + * setting this property to YES, though there may be security implications to turning the + * warnings off, as it may indicate a maliciously crafted archive intended to exploit a vulnerability. + * + * It's recommended to leave the decision of how to treat archives with mismatched CRCs to the user + */ +@property (assign) BOOL ignoreCRCMismatches; + /** * **DEPRECATED:** Creates and returns an archive at the given path @@ -475,16 +486,16 @@ extern NSString *URKErrorDomain; - (BOOL)validatePassword; /** - Extract each file in the archive, checking whether the data matches the CRC checksum - stored at the time it was written + Iterate through the archive, checking for any errors, including CRC mismatches between + the archived file and its header - @return YES if the data is all correct, false if any check failed + @return YES if the data is all correct, false if any check failed (_even if ignoreCRCMismatches is YES_) */ - (BOOL)checkDataIntegrity; /** - Extract each file in the archive, checking whether the data matches the CRC checksum - stored at the time it was written. If any file doesn't match, run the given block + Iterate through the archive, checking for any errors, including CRC mismatches between + the archived file and its header. If any file's CRC doesn't match, run the given block to allow the API consumer to decide whether to ignore mismatches. NOTE: This may be a security risk. The block is intended to prompt the user, which is why it's forced onto the main thread, rather than making a design-time decision @@ -502,7 +513,7 @@ extern NSString *URKErrorDomain; - (BOOL)checkDataIntegrityIgnoringCRCMismatches:(BOOL(^)(void))ignoreCRCMismatches; /** - Extract a particular file, to determine if its data matches the CRC + Check a particular file, to determine if its data matches the CRC checksum stored at the time it written @param filePath The file in the archive to check diff --git a/Classes/URKArchive.mm b/Classes/URKArchive.mm index 9e87f49..8d38a1c 100644 --- a/Classes/URKArchive.mm +++ b/Classes/URKArchive.mm @@ -53,7 +53,6 @@ - (instancetype)initWithFile:(NSURL *)fileURL password:(NSString*)password error @property (assign) NSString *lastArchivePath; @property (assign) NSString *lastFilepath; -@property (assign) BOOL ignoreCRCMismatches; @end diff --git a/Tests/CheckDataTests.m b/Tests/CheckDataTests.m index ce46cfe..10a5da4 100644 --- a/Tests/CheckDataTests.m +++ b/Tests/CheckDataTests.m @@ -47,6 +47,16 @@ - (void)testCheckDataIntegrity_ModifiedCRC { XCTAssertFalse(success, @"Data integrity check passed for archive with a modified CRC"); } +- (void)testCheckDataIntegrity_ModifiedCRC_IgnoringMismatches { + NSURL *testArchiveURL = self.testFileURLs[@"Modified CRC Archive.rar"]; + URKArchive *archive = [[URKArchive alloc] initWithURL:testArchiveURL error:nil]; + archive.ignoreCRCMismatches = YES; + + // Still expect failure, so a consumer can still tell that data is mismatched + BOOL success = [archive checkDataIntegrity]; + XCTAssertFalse(success, @"Data integrity check passed for archive with a modified CRC"); +} + #pragma mark - checkDataIntegrityOfFile - (void)testCheckDataIntegrityForFile { @@ -91,6 +101,16 @@ - (void)testCheckDataIntegrityForFile_ModifiedCRC { XCTAssertFalse(success, @"Data integrity check passed for archive with modified CRC"); } +- (void)testCheckDataIntegrityForFile_ModifiedCRC_IgnoringMismatches { + NSURL *testArchiveURL = self.testFileURLs[@"Modified CRC Archive.rar"]; + URKArchive *archive = [[URKArchive alloc] initWithURL:testArchiveURL error:nil]; + archive.ignoreCRCMismatches = YES; + + // Still expect failure, so a consumer can still tell that data is mismatched + BOOL success = [archive checkDataIntegrityOfFile:@"README.md"]; + XCTAssertFalse(success, @"Data integrity check passed for archive with modified CRC"); +} + - (void)testCheckDataIntegrityIgnoringCRCMismatches { NSArray *testArchives = @[@"Test Archive.rar", @"Test Archive (Password).rar", @@ -112,7 +132,8 @@ - (void)testCheckDataIntegrityIgnoringCRCMismatches { XCTAssertTrue(success, @"Data integrity check failed for %@", testArchiveName); XCTAssertFalse(blockInvoked, @"Block prompting whether to ignore CRC mismatches should not have been called"); - } + XCTAssertFalse(archive.ignoreCRCMismatches); + } } - (void)testCheckDataIntegrityIgnoringCRCMismatches_InBackground { @@ -137,6 +158,7 @@ - (void)testCheckDataIntegrityIgnoringCRCMismatches_InBackground { [self waitForExpectationsWithTimeout:10 handler:nil]; XCTAssertTrue(blockInvoked, @"Block prompting whether to ignore CRC mismatches should have been called"); XCTAssertEqualObjects(blockThread, [NSThread mainThread]); + XCTAssertTrue(archive.ignoreCRCMismatches); } - (void)testCheckDataIntegrityIgnoringCRCMismatches_NotAnArchive { @@ -151,6 +173,7 @@ - (void)testCheckDataIntegrityIgnoringCRCMismatches_NotAnArchive { XCTAssertFalse(success, @"Data integrity check passed for non-archive"); XCTAssertFalse(blockInvoked, @"Block prompting whether to ignore CRC mismatches should not have been called"); + XCTAssertFalse(archive.ignoreCRCMismatches); } - (void)testCheckDataIntegrityIgnoringCRCMismatches_ModifiedCRC_Ignore { @@ -165,6 +188,7 @@ - (void)testCheckDataIntegrityIgnoringCRCMismatches_ModifiedCRC_Ignore { XCTAssertTrue(success, @"Data integrity check failed for archive with modified CRC, when instructed to ignore"); XCTAssertTrue(blockInvoked, @"Block prompting whether to ignore CRC mismatches should have been called"); + XCTAssertTrue(archive.ignoreCRCMismatches); } - (void)testCheckDataIntegrityIgnoringCRCMismatches_ModifiedCRC_DontIgnore { @@ -179,6 +203,7 @@ - (void)testCheckDataIntegrityIgnoringCRCMismatches_ModifiedCRC_DontIgnore { XCTAssertFalse(success, @"Data integrity check passed for archive with modified CRC"); XCTAssertTrue(blockInvoked, @"Block prompting whether to ignore CRC mismatches should have been called"); + XCTAssertFalse(archive.ignoreCRCMismatches); } @end diff --git a/Tests/ExtractBufferedDataTests.m b/Tests/ExtractBufferedDataTests.m index 634601b..a79a171 100644 --- a/Tests/ExtractBufferedDataTests.m +++ b/Tests/ExtractBufferedDataTests.m @@ -82,12 +82,7 @@ - (void)testExtractBufferedData_ModifiedCRC_IgnoringMismatches NSURL *archiveURL = self.testFileURLs[@"Modified CRC Archive.rar"]; NSString *extractedFile = @"README.md"; URKArchive *archive = [[URKArchive alloc] initWithURL:archiveURL error:nil]; - - BOOL checkIntegritySuccess = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return YES; - }]; - - XCTAssertTrue(checkIntegritySuccess, @"Data integrity check failed for archive with modified CRC, when instructed to ignore"); + archive.ignoreCRCMismatches = YES; NSError *error = nil; NSMutableData *reconstructedFile = [NSMutableData data]; diff --git a/Tests/ExtractDataTests.m b/Tests/ExtractDataTests.m index 88e0b7a..40c8cdc 100644 --- a/Tests/ExtractDataTests.m +++ b/Tests/ExtractDataTests.m @@ -136,12 +136,7 @@ - (void)testExtractData_ModifiedCRC - (void)testExtractData_ModifiedCRC_IgnoringMismatches { URKArchive *archive = [[URKArchive alloc] initWithURL:self.testFileURLs[@"Modified CRC Archive.rar"] error:nil]; - - BOOL checkIntegritySuccess = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return YES; - }]; - - XCTAssertTrue(checkIntegritySuccess, @"Data integrity check failed for archive with modified CRC, when instructed to ignore"); + archive.ignoreCRCMismatches = YES; NSError *error = nil; NSData *data = [archive extractDataFromFile:@"README.md" error:&error]; diff --git a/Tests/ExtractFilesTests.m b/Tests/ExtractFilesTests.m index 0845161..7881b6a 100644 --- a/Tests/ExtractFilesTests.m +++ b/Tests/ExtractFilesTests.m @@ -265,13 +265,8 @@ - (void)testExtractFiles_ModifiedCRC_IgnoreCRCMismatches NSFileManager *fm = [NSFileManager defaultManager]; URKArchive *archive = [[URKArchive alloc] initWithURL:self.testFileURLs[@"Modified CRC Archive.rar"] error:nil]; - - BOOL fileHasIntegrity = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return YES; - }]; - - XCTAssertTrue(fileHasIntegrity); - + archive.ignoreCRCMismatches = YES; + NSString *extractDirectory = [self randomDirectoryWithPrefix:@"ExtractInvalidArchive"]; NSURL *extractURL = [self.tempDirectory URLByAppendingPathComponent:extractDirectory]; @@ -295,13 +290,8 @@ - (void)testExtractFiles_ModifiedCRC_DontIgnoreCRCMismatches NSFileManager *fm = [NSFileManager defaultManager]; URKArchive *archive = [[URKArchive alloc] initWithURL:self.testFileURLs[@"Modified CRC Archive.rar"] error:nil]; - - BOOL fileHasIntegrity = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return NO; - }]; - - XCTAssertFalse(fileHasIntegrity); - + archive.ignoreCRCMismatches = YES; + NSString *extractDirectory = [self randomDirectoryWithPrefix:@"ExtractInvalidArchive"]; NSURL *extractURL = [self.tempDirectory URLByAppendingPathComponent:extractDirectory]; diff --git a/Tests/IterateFileInfoTests.m b/Tests/IterateFileInfoTests.m index 28f3cdb..826855a 100644 --- a/Tests/IterateFileInfoTests.m +++ b/Tests/IterateFileInfoTests.m @@ -230,12 +230,7 @@ - (void)testIterateFileInfo_ModifiedCRC - (void)testIterateFileInfo_ModifiedCRC_IgnoringMismatches { URKArchive *archive = [[URKArchive alloc] initWithURL:self.testFileURLs[@"Modified CRC Archive.rar"] error:nil]; - - BOOL checkIntegritySuccess = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return YES; - }]; - - XCTAssertTrue(checkIntegritySuccess, @"Data integrity check failed for archive with modified CRC, when instructed to ignore"); + archive.ignoreCRCMismatches = YES; NSError *error = nil; __block int calledCount = 0; diff --git a/Tests/ListFileInfoTests.m b/Tests/ListFileInfoTests.m index 139dd2d..215df79 100644 --- a/Tests/ListFileInfoTests.m +++ b/Tests/ListFileInfoTests.m @@ -199,13 +199,8 @@ - (void)testListFileInfo_ModifiedCRC - (void)testListFileInfo_ModifiedCRC_IgnoringMismatches { URKArchive *archive = [[URKArchive alloc] initWithURL:self.testFileURLs[@"Modified CRC Archive.rar"] error:nil]; - - BOOL checkIntegritySuccess = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return YES; - }]; - - XCTAssertTrue(checkIntegritySuccess, @"Data integrity check failed for archive with modified CRC, when instructed to ignore"); - + archive.ignoreCRCMismatches = YES; + NSError *error = nil; NSArray *files = [archive listFileInfo:&error]; diff --git a/Tests/ListFilenamesTests.m b/Tests/ListFilenamesTests.m index 6b87f31..059934a 100644 --- a/Tests/ListFilenamesTests.m +++ b/Tests/ListFilenamesTests.m @@ -196,12 +196,7 @@ - (void)testListFilenames_ModifiedCRC_IgnoringMismatch { NSURL *testArchiveURL = self.testFileURLs[@"Modified CRC Archive.rar"]; URKArchive *archive = [[URKArchive alloc] initWithURL:testArchiveURL error:nil]; - - BOOL checkIntegritySuccess = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return YES; - }]; - - XCTAssertTrue(checkIntegritySuccess, @"Data integrity check failed for archive with modified CRC, when instructed to ignore"); + archive.ignoreCRCMismatches = YES; NSError *error = nil; NSArray *filesInArchive = [archive listFilenames:&error]; diff --git a/Tests/ListVolumesTests.m b/Tests/ListVolumesTests.m index 4e97f89..91d4050 100644 --- a/Tests/ListVolumesTests.m +++ b/Tests/ListVolumesTests.m @@ -44,13 +44,8 @@ - (void)testSingleVolume_ModifiedCRC { - (void)testSingleVolume_ModifiedCRC_IgnoringMismatch { NSURL *testArchiveURL = self.testFileURLs[@"Modified CRC Archive.rar"]; URKArchive *archive = [[URKArchive alloc] initWithURL:testArchiveURL error:nil]; - - BOOL checkIntegritySuccess = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return YES; - }]; - - XCTAssertTrue(checkIntegritySuccess, @"Data integrity check failed for archive with modified CRC, when instructed to ignore"); - + archive.ignoreCRCMismatches = YES; + NSError *listVolumesError = nil; NSArray *volumeURLs = [archive listVolumeURLs:&listVolumesError]; diff --git a/Tests/PerformOnDataTests.m b/Tests/PerformOnDataTests.m index daaa2ab..72602d6 100644 --- a/Tests/PerformOnDataTests.m +++ b/Tests/PerformOnDataTests.m @@ -100,13 +100,8 @@ - (void)testPerformOnData_ModifiedCRC_IgnoringMismatches { NSURL *testArchiveURL = self.testFileURLs[@"Modified CRC Archive.rar"]; URKArchive *archive = [[URKArchive alloc] initWithURL:testArchiveURL error:nil]; - - BOOL checkIntegritySuccess = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return YES; - }]; - - XCTAssertTrue(checkIntegritySuccess, @"Data integrity check failed for archive with modified CRC, when instructed to ignore"); - + archive.ignoreCRCMismatches = YES; + __block NSUInteger fileIndex = 0; NSError *error = nil; diff --git a/Tests/PerformOnFilesTests.m b/Tests/PerformOnFilesTests.m index 52062da..a2f5c37 100644 --- a/Tests/PerformOnFilesTests.m +++ b/Tests/PerformOnFilesTests.m @@ -68,13 +68,8 @@ - (void)testPerformOnFiles_ModifiedCRC_MismatchesIgnored NSURL *testArchiveURL = self.testFileURLs[@"Modified CRC Archive.rar"]; NSString *password = nil; URKArchive *archive = [[URKArchive alloc] initWithURL:testArchiveURL password:password error:nil]; - - BOOL checkIntegritySuccess = [archive checkDataIntegrityIgnoringCRCMismatches:^BOOL{ - return YES; - }]; - - XCTAssertTrue(checkIntegritySuccess, @"Data integrity check failed for archive with modified CRC, when instructed to ignore"); - + archive.ignoreCRCMismatches = YES; + __block NSUInteger fileIndex = 0; NSError *error = nil; diff --git a/Tests/URKArchiveTests.m b/Tests/URKArchiveTests.m index 29e73c1..0f90563 100644 --- a/Tests/URKArchiveTests.m +++ b/Tests/URKArchiveTests.m @@ -34,7 +34,8 @@ - (void)testFileURL { : self.testFileURLs[testArchiveName]); URKArchive *archive = [[URKArchive alloc] initWithURL:testArchiveURL error:nil]; - + XCTAssertFalse(archive.ignoreCRCMismatches); + NSURL *resolvedURL = archive.fileURL.URLByResolvingSymlinksInPath; XCTAssertNotNil(resolvedURL, @"Nil URL returned for valid archive"); XCTAssertTrue([testArchiveURL isEqual:resolvedURL], @"Resolved URL doesn't match original"); @@ -56,7 +57,8 @@ - (void)testFilename { : self.testFileURLs[testArchiveName]); URKArchive *archive = [[URKArchive alloc] initWithURL:testArchiveURL error:nil]; - + XCTAssertFalse(archive.ignoreCRCMismatches); + NSString *resolvedFilename = archive.filename; XCTAssertNotNil(resolvedFilename, @"Nil filename returned for valid archive");