From c710b7f8f2840d65a23c5ce10f439effb21ed2d7 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Sun, 30 Sep 2018 12:32:06 -0600 Subject: [PATCH] Fixup certificate parsing tests Skip failure when running tests when we're explicitly testing failure cases. Be more specific about failure conditions via NSError param --- Signal/test/util/CDSSigningCertificateTest.m | 35 ++++++++++----- .../src/Contacts/CDSSigningCertificate.h | 17 ++++++- .../src/Contacts/CDSSigningCertificate.m | 45 ++++++++++++++++--- .../src/Contacts/ContactDiscoveryService.m | 9 +++- 4 files changed, 88 insertions(+), 18 deletions(-) diff --git a/Signal/test/util/CDSSigningCertificateTest.m b/Signal/test/util/CDSSigningCertificateTest.m index db03dd751..7f0e6c014 100644 --- a/Signal/test/util/CDSSigningCertificateTest.m +++ b/Signal/test/util/CDSSigningCertificateTest.m @@ -17,15 +17,20 @@ - (void)testParsing_good { NSString *pem = [self certificatesPem_good]; - CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:pem]; + NSError *error; + CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:pem error:&error]; XCTAssertNotNil(certificate); + XCTAssertNil(error); } - (void)testParsing_bad { NSString *pem = [self certificatesPem_bad]; - - XCTAssertThrows([CDSSigningCertificate parseCertificateFromPem:pem]); + NSError *error; + CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:pem error:&error]; + XCTAssertNil(certificate); + XCTAssertNotNil(error); + XCTAssertEqual(error.code, CDSSigningCertificateError_InvalidDistinguishedName); } - (void)testVerification_good @@ -33,9 +38,11 @@ NSString *pem = [self certificatesPem_good]; NSData *signature = [self signature_good]; NSString *bodyString = [self bodyString_good]; - - CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:pem]; + + NSError *error; + CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:pem error:&error]; XCTAssertNotNil(certificate); + XCTAssertNil(error); BOOL result = [certificate verifySignatureOfBody:bodyString signature:signature]; XCTAssertTrue(result); @@ -46,11 +53,13 @@ NSString *pem = [self certificatesPem_good]; NSData *signature = [self signature_good]; NSString *bodyString = [self bodyString_bad]; - - CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:pem]; + + NSError *error; + CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:pem error:&error]; XCTAssertNotNil(certificate); + XCTAssertNil(error); - XCTAssertThrows([certificate verifySignatureOfBody:bodyString signature:signature]); + XCTAssertFalse([certificate verifySignatureOfBody:bodyString signature:signature]); } - (void)testVerification_badSignature @@ -58,13 +67,17 @@ NSString *pem = [self certificatesPem_good]; NSData *signature = [self signature_bad]; NSString *bodyString = [self bodyString_good]; - - CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:pem]; + + NSError *error; + CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:pem error:&error]; XCTAssertNotNil(certificate); + XCTAssertNil(error); - XCTAssertThrows([certificate verifySignatureOfBody:bodyString signature:signature]); + XCTAssertFalse([certificate verifySignatureOfBody:bodyString signature:signature]); } +#pragma mark - test values + - (NSString *)certificatesPem_good { return @"-----BEGIN CERTIFICATE----- \ MIIEoTCCAwmgAwIBAgIJANEHdl0yo7CWMA0GCSqGSIb3DQEBCwUAMH4xCzAJBgNV \ diff --git a/SignalServiceKit/src/Contacts/CDSSigningCertificate.h b/SignalServiceKit/src/Contacts/CDSSigningCertificate.h index 37067cb33..ef9721a4b 100644 --- a/SignalServiceKit/src/Contacts/CDSSigningCertificate.h +++ b/SignalServiceKit/src/Contacts/CDSSigningCertificate.h @@ -4,9 +4,24 @@ NS_ASSUME_NONNULL_BEGIN +typedef NS_ENUM(NSUInteger, CDSSigningCertificateErrorCode) { + // AssertionError's indicate either developer or some serious system error that should never happen. + // + // Do not use this for an "expected" error, e.g. something that could be induced by user input which + // we specifically need to handle gracefull. + CDSSigningCertificateError_AssertionError = 1, + + CDSSigningCertificateError_InvalidPEMSupplied, + CDSSigningCertificateError_CouldNotExtractLeafCertificate, + CDSSigningCertificateError_InvalidDistinguishedName, + CDSSigningCertificateError_UntrustedCertificate +}; + +NSError *CDSSigningCertificateErrorMake(CDSSigningCertificateErrorCode code, NSString *localizedDescription); + @interface CDSSigningCertificate : NSObject -+ (nullable CDSSigningCertificate *)parseCertificateFromPem:(NSString *)certificatePem; ++ (nullable CDSSigningCertificate *)parseCertificateFromPem:(NSString *)certificatePem error:(NSError **)error; - (BOOL)verifySignatureOfBody:(NSString *)body signature:(NSData *)theirSignature; diff --git a/SignalServiceKit/src/Contacts/CDSSigningCertificate.m b/SignalServiceKit/src/Contacts/CDSSigningCertificate.m index 448b4d92c..a7e5f1f0c 100644 --- a/SignalServiceKit/src/Contacts/CDSSigningCertificate.m +++ b/SignalServiceKit/src/Contacts/CDSSigningCertificate.m @@ -10,6 +10,13 @@ NS_ASSUME_NONNULL_BEGIN +NSError *CDSSigningCertificateErrorMake(CDSSigningCertificateErrorCode code, NSString *localizedDescription) +{ + return [NSError errorWithDomain:@"CDSSigningCertificate" + code:code + userInfo:@{ NSLocalizedDescriptionKey : localizedDescription }]; +} + @interface CDSSigningCertificate () @property (nonatomic) SecPolicyRef policy; @@ -49,15 +56,18 @@ NS_ASSUME_NONNULL_BEGIN } } -+ (nullable CDSSigningCertificate *)parseCertificateFromPem:(NSString *)certificatePem ++ (nullable CDSSigningCertificate *)parseCertificateFromPem:(NSString *)certificatePem error:(NSError **)error { OWSAssertDebug(certificatePem); + *error = nil; CDSSigningCertificate *signingCertificate = [CDSSigningCertificate new]; NSArray *_Nullable anchorCertificates = [self anchorCertificates]; if (anchorCertificates.count < 1) { OWSFailDebug(@"Could not load anchor certificates."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_AssertionError, @"Could not load anchor certificates."); return nil; } @@ -65,6 +75,7 @@ NS_ASSUME_NONNULL_BEGIN if (certificateDerDatas.count < 1) { OWSFailDebug(@"Could not parse PEM."); + *error = CDSSigningCertificateErrorMake(CDSSigningCertificateError_InvalidPEMSupplied, @"Could not parse PEM."); return nil; } @@ -72,10 +83,14 @@ NS_ASSUME_NONNULL_BEGIN NSData *_Nullable leafCertificateData = [certificateDerDatas firstObject]; if (!leafCertificateData) { OWSFailDebug(@"Could not extract leaf certificate data."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_CouldNotExtractLeafCertificate, @"Could not extract leaf certificate data."); return nil; } if (![self verifyDistinguishedNameOfCertificate:leafCertificateData]) { OWSFailDebug(@"Leaf certificate has invalid name."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_InvalidDistinguishedName, @"Could not extract leaf certificate data."); return nil; } @@ -83,7 +98,9 @@ NS_ASSUME_NONNULL_BEGIN for (NSData *certificateDerData in certificateDerDatas) { SecCertificateRef certificate = SecCertificateCreateWithData(NULL, (__bridge CFDataRef)(certificateDerData)); if (!certificate) { - OWSFailDebug(@"Could not load DER."); + OWSFailDebug(@"Could not create SecCertificate."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_AssertionError, @"Could not create SecCertificate."); return nil; } [certificates addObject:(__bridge_transfer id)certificate]; @@ -93,6 +110,7 @@ NS_ASSUME_NONNULL_BEGIN signingCertificate.policy = policy; if (!policy) { OWSFailDebug(@"Could not create policy."); + *error = CDSSigningCertificateErrorMake(CDSSigningCertificateError_AssertionError, @"Could not create policy."); return nil; } @@ -100,23 +118,30 @@ NS_ASSUME_NONNULL_BEGIN OSStatus status = SecTrustCreateWithCertificates((__bridge CFTypeRef)certificates, policy, &trust); signingCertificate.trust = trust; if (status != errSecSuccess) { - OWSFailDebug(@"trust could not be created."); + OWSFailDebug(@"Creating trust did not succeed."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_AssertionError, @"Creating trust did not succeed."); return nil; } if (!trust) { OWSFailDebug(@"Could not create trust."); + *error = CDSSigningCertificateErrorMake(CDSSigningCertificateError_AssertionError, @"Could not create trust."); return nil; } status = SecTrustSetNetworkFetchAllowed(trust, NO); if (status != errSecSuccess) { OWSFailDebug(@"trust fetch could not be configured."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_AssertionError, @"trust fetch could not be configured."); return nil; } status = SecTrustSetAnchorCertificatesOnly(trust, YES); if (status != errSecSuccess) { OWSFailDebug(@"trust anchor certs could not be configured."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_AssertionError, @"trust anchor certs could not be configured."); return nil; } @@ -124,7 +149,9 @@ NS_ASSUME_NONNULL_BEGIN for (NSData *certificateData in anchorCertificates) { SecCertificateRef certificate = SecCertificateCreateWithData(NULL, (__bridge CFDataRef)(certificateData)); if (!certificate) { - OWSFailDebug(@"Could not load DER."); + OWSFailDebug(@"Could not create pinned SecCertificate."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_AssertionError, @"Could not create pinned SecCertificate."); return nil; } @@ -133,6 +160,8 @@ NS_ASSUME_NONNULL_BEGIN status = SecTrustSetAnchorCertificates(trust, (__bridge CFArrayRef)pinnedCertificates); if (status != errSecSuccess) { OWSFailDebug(@"The anchor certificates couldn't be set."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_AssertionError, @"The anchor certificates couldn't be set."); return nil; } @@ -140,6 +169,8 @@ NS_ASSUME_NONNULL_BEGIN status = SecTrustEvaluate(trust, &result); if (status != errSecSuccess) { OWSFailDebug(@"Could not evaluate certificates."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_AssertionError, @"Could not evaluate certificates."); return nil; } @@ -147,7 +178,9 @@ NS_ASSUME_NONNULL_BEGIN // See the comments in the header where it is defined. BOOL isValid = (result == kSecTrustResultUnspecified || result == kSecTrustResultProceed); if (!isValid) { - OWSFailDebug(@"Certificate evaluation failed."); + OWSFailDebug(@"Certificate was not trusted."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_UntrustedCertificate, @"Certificate was not trusted."); return nil; } @@ -155,6 +188,8 @@ NS_ASSUME_NONNULL_BEGIN signingCertificate.publicKey = publicKey; if (!publicKey) { OWSFailDebug(@"Could not extract public key."); + *error = CDSSigningCertificateErrorMake( + CDSSigningCertificateError_AssertionError, @"Could not extract public key."); return nil; } diff --git a/SignalServiceKit/src/Contacts/ContactDiscoveryService.m b/SignalServiceKit/src/Contacts/ContactDiscoveryService.m index 757fb1dd1..a14920e26 100644 --- a/SignalServiceKit/src/Contacts/ContactDiscoveryService.m +++ b/SignalServiceKit/src/Contacts/ContactDiscoveryService.m @@ -509,7 +509,14 @@ NSErrorDomain const ContactDiscoveryServiceErrorDomain = @"SignalServiceKit.Cont OWSAssertDebug(signature.length > 0); OWSAssertDebug(quoteData); - CDSSigningCertificate *_Nullable certificate = [CDSSigningCertificate parseCertificateFromPem:certificates]; + NSError *error; + CDSSigningCertificate *_Nullable certificate = + [CDSSigningCertificate parseCertificateFromPem:certificates error:&error]; + if (error) { + OWSFailDebug(@"error when parsing signing certificate. %@", error.localizedDescription); + return NO; + } + if (!certificate) { OWSFailDebug(@"could not parse signing certificate."); return NO;