diff --git a/Autoupdate/SUSignatureVerifier.m b/Autoupdate/SUSignatureVerifier.m index 444473c3..5dc5aa9b 100644 --- a/Autoupdate/SUSignatureVerifier.m +++ b/Autoupdate/SUSignatureVerifier.m @@ -113,8 +113,8 @@ case SUSigningInputStatusPresent: switch (signatures.ed25519SignatureStatus) { case SUSigningInputStatusAbsent: - SULog(SULogLevelDefault, @"The update has an EdDSA signature, but it won't be used, because the old app doesn't have an EdDSA public key"); - break; + SULog(SULogLevelError, @"The app has an EdDSA public key, but there is no EdDSA signature in the update, so the update will be rejected."); + return NO; case SUSigningInputStatusInvalid: // We will have already logged an error for this failure when the signature was read in, so just do an informational log here. SULog(SULogLevelDefault, @"The update has an EdDSA signature, but it's invalid, so the update will automatically be rejected."); @@ -128,10 +128,10 @@ } if (ed25519_verify(signatures.ed25519Signature, data.bytes, data.length, self.pubKeys.ed25519PubKey)) { SULog(SULogLevelDefault, @"OK: EdDSA signature is correct"); - if (self.pubKeys.dsaPubKeyStatus == SUSigningInputStatusAbsent) { + // No need to check DSA when EdDSA verification succeeded, unless a DSA signature is provided and it's + // erroneously invalid + if (signatures.dsaSignatureStatus != SUSigningInputStatusInvalid) { return YES; - } else { - SULog(SULogLevelDefault, @"This app has a DSA public key, so a DSA signature is required too"); } } else { SULog(SULogLevelError, @"EdDSA signature does not match. Data of the update file being checked is different than data that has been signed, or the public key and the private key are not from the same set."); diff --git a/Tests/SUSignatureVerifierTest.m b/Tests/SUSignatureVerifierTest.m index 5da08f97..43312069 100644 --- a/Tests/SUSignatureVerifierTest.m +++ b/Tests/SUSignatureVerifierTest.m @@ -135,26 +135,27 @@ NSString *edSig = @"EIawm2YkDZ2gBfkEMF2+1VuuTeXnCGZOdnMdVgPPvDZioq7bvDayXqKkIIzSjKMmeFdcFJOHdnba5ZV60+gPBw=="; NSString *wrongEdSig = @"wTcpXCgWoa4NrJpsfzS61FXJIbv963//12U2ef9xstzVOLPHYK2N4/ojgpDV5N1/NGG1uWMBgK+kEWp0Z5zMDQ=="; - XCTAssertTrue([v verifyFileAtPath:self.testFile - signatures:[[SUSignatures alloc] initWithDsa:dsaSig ed:nil]], - @"Allow just a DSA signature if that's all that's available"); XCTAssertFalse([v verifyFileAtPath:self.testFile + signatures:[[SUSignatures alloc] initWithDsa:dsaSig ed:nil]], + @"EdDSA signature must be present if app has EdDSA key"); + + XCTAssertTrue([v verifyFileAtPath:self.testFile signatures:[[SUSignatures alloc] initWithDsa:nil ed:edSig]], - @"Require the DSA signature to match because there's a DSA public key"); + @"Allow just an EdDSA signature if that's all that's available"); XCTAssertFalse([v verifyFileAtPath:self.testFile signatures:[[SUSignatures alloc] initWithDsa:dsaSig ed:wrongEdSig]], @"Fail on a bad Ed25519 signature regardless"); - XCTAssertFalse([v verifyFileAtPath:self.testFile + XCTAssertTrue([v verifyFileAtPath:self.testFile signatures:[[SUSignatures alloc] initWithDsa:wrongDSASig ed:edSig]], - @"Fail on a bad DSA signature if provided"); + @"Allow bad DSA signature if EdDSA signature is good"); XCTAssertFalse([v verifyFileAtPath:self.testFile signatures:[[SUSignatures alloc] initWithDsa:dsaSig ed:@"lol"]], @"Fail if the Ed25519 signature is invalid."); XCTAssertFalse([v verifyFileAtPath:self.testFile signatures:[[SUSignatures alloc] initWithDsa:@"lol" ed:edSig]], - @"Fail if the DSA signature is invalid."); + @"Fail if invalid DSA signature is used even if EdDSA signature is good."); XCTAssertTrue([v verifyFileAtPath:self.testFile signatures:[[SUSignatures alloc] initWithDsa:dsaSig ed:edSig]], diff --git a/Tests/SUUpdateValidatorTest.swift b/Tests/SUUpdateValidatorTest.swift index 50007d25..be4d8da7 100644 --- a/Tests/SUUpdateValidatorTest.swift +++ b/Tests/SUUpdateValidatorTest.swift @@ -35,7 +35,7 @@ class SUUpdateValidatorTest: XCTestCase { struct SignatureConfig: CaseIterable, Equatable, CustomDebugStringConvertible { enum State: CaseIterable, Equatable { - case none, invalid, valid + case none, invalid, invalidFormat, valid } var dsa: State @@ -62,7 +62,9 @@ class SUUpdateValidatorTest: XCTestCase { let dsaSig: String? switch config.dsa { case .none: dsaSig = nil - case .invalid: dsaSig = "MCwCFCIHCiYYkfZavNzTitTW5tlRp/k5AhQ40poFytqcVhIYdCxQznaXeJPJDQ==" + case .invalid: dsaSig = "ABwCFCIHCIYYkfZavNzTitTW5tlRp/k5AhQ40poFytqcVhIYdCxQznaXeJPJDQ==" + // Use some invalid base64 strings + case .invalidFormat: dsaSig = "%%wCFCIHCIYYkfZavNzTitTW5tlRp/k5AhQ40poFytqcVhIYdCxQznaXeJPJDQ==" case .valid: dsaSig = "MCwCFCIHCIYYkfZavNzTitTW5tlRp/k5AhQ40poFytqcVhIYdCxQznaXeJPJDQ==" } @@ -70,6 +72,8 @@ class SUUpdateValidatorTest: XCTestCase { switch config.ed { case .none: edSig = nil case .invalid: edSig = "wTcpXCgWoa4NrJpsfzS61FXJIbv963//12U2ef9xstzVOLPHYK2N4/ojgpDV5N1/NGG1uWMBgK+kEWp0Z5zMDQ==" + // Use some invalid base64 strings + case .invalidFormat: edSig = "%%cpXCgWoa4NrJpsfzS61FXJIbv963//12U2ef9xstzVOLPHYK2N4/ojgpDV5N1/NGG1uWMBgK+kEWp0Z5zMDQ==" case .valid: edSig = "EIawm2YkDZ2gBfkEMF2+1VuuTeXnCGZOdnMdVgPPvDZioq7bvDayXqKkIIzSjKMmeFdcFJOHdnba5ZV60+gPBw==" } @@ -84,7 +88,6 @@ class SUUpdateValidatorTest: XCTestCase { func testPrevalidation(bundle bundleConfig: BundleConfig, signatures signatureConfig: SignatureConfig, expectedResult: Bool, line: UInt = #line) { let host = SUHost(bundle: self.bundle(bundleConfig)) let signatures = self.signatures(signatureConfig) - let validator = SUUpdateValidator(downloadPath: self.signedTestFilePath, signatures: signatures, host: host) let result = (try? validator.validateDownloadPath()) != nil @@ -95,8 +98,8 @@ class SUUpdateValidatorTest: XCTestCase { for signatureConfig in SignatureConfig.allCases { testPrevalidation(bundle: .none, signatures: signatureConfig, expectedResult: false) testPrevalidation(bundle: .dsaOnly, signatures: signatureConfig, expectedResult: signatureConfig.dsa == .valid) - testPrevalidation(bundle: .edOnly, signatures: signatureConfig, expectedResult: signatureConfig.ed == .valid) - testPrevalidation(bundle: .both, signatures: signatureConfig, expectedResult: signatureConfig.dsa == .valid && signatureConfig.ed != .invalid) + testPrevalidation(bundle: .edOnly, signatures: signatureConfig, expectedResult: signatureConfig.ed == .valid && signatureConfig.dsa != .invalidFormat) + testPrevalidation(bundle: .both, signatures: signatureConfig, expectedResult: signatureConfig.ed == .valid && signatureConfig.dsa != .invalidFormat) } } @@ -124,15 +127,15 @@ class SUUpdateValidatorTest: XCTestCase { for signatureConfig in SignatureConfig.allCases { testPostValidation(bundle: .none, signatures: signatureConfig, expectedResult: false) testPostValidation(bundle: .dsaOnly, signatures: signatureConfig, expectedResult: signatureConfig.dsa == .valid) - testPostValidation(bundle: .edOnly, signatures: signatureConfig, expectedResult: signatureConfig.ed == .valid) - testPostValidation(bundle: .both, signatures: signatureConfig, expectedResult: signatureConfig.dsa == .valid && signatureConfig.ed != .invalid) + testPostValidation(bundle: .edOnly, signatures: signatureConfig, expectedResult: signatureConfig.ed == .valid && signatureConfig.dsa != .invalidFormat) + testPostValidation(bundle: .both, signatures: signatureConfig, expectedResult: signatureConfig.ed == .valid && signatureConfig.dsa != .invalidFormat) } } func testPostValidationWithCodeSigning() { for signatureConfig in SignatureConfig.allCases { testPostValidation(bundle: .codeSignedOnly, signatures: signatureConfig, expectedResult: true) - testPostValidation(bundle: .codeSignedBoth, signatures: signatureConfig, expectedResult: signatureConfig.dsa == .valid && signatureConfig.ed != .invalid) + testPostValidation(bundle: .codeSignedBoth, signatures: signatureConfig, expectedResult: signatureConfig.ed == .valid && signatureConfig.dsa != .invalidFormat) testPostValidation(bundle: .codeSignedInvalidOnly, signatures: signatureConfig, expectedResult: false) testPostValidation(bundle: .codeSignedInvalid, signatures: signatureConfig, expectedResult: false) @@ -150,7 +153,7 @@ class SUUpdateValidatorTest: XCTestCase { func testPostValidationWithKeyRotation() { for signatureConfig in SignatureConfig.allCases { - let signatureIsValid = signatureConfig.dsa == .valid && (signatureConfig.ed == .valid || signatureConfig.ed == .none) + let signatureIsValid = (signatureConfig.ed == .valid && signatureConfig.dsa != .invalidFormat) // It's okay to add DSA keys or add code signing. testPostValidation(oldBundle: .codeSignedOnly, newBundle: .codeSignedBoth, signatures: signatureConfig, expectedResult: signatureIsValid)