[PM-22479] Checking signingCertificateHistory for a valid asset link certificate #6177
Conversation
aj-rosado
requested review from
a team,
brian-livefront and
david-livefront
as code owners
|
Claude finished @aj-rosado's task —— View job Code Review CompleteOverall Assessment: APPROVE This PR correctly implements certificate history validation for asset links, addressing scenarios where apps have rotated their signing certificates. The implementation is secure, well-tested, and follows established patterns. Key Strengths:
Findings: Finding 1: Consider adding a clarifying comment about certificate ordering assumptions in OriginManagerImpl.kt:50 Finding 2: The PR Description Quality:
Security Considerations:
Technical Compliance:
See inline comments for specific details. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| b.toHexString(HexFormat.UpperCase) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎨 Code Quality Suggestion
Consider caching MessageDigest instance
Creating a new MessageDigest instance inside the loop means it's recreated for each signature in the history. Since MessageDigest instances are reusable after calling digest(), consider creating it once outside the map function:
@OptIn(ExperimentalStdlibApi::class)
fun CallingAppInfo.getAllSignatureFingerprintsAsHexStrings(): List<String> {
if (signingInfo.hasMultipleSigners()) return emptyList()
val md = MessageDigest.getInstance(SHA_ALGORITHM)
return signingInfo.signingCertificateHistory.map { signature ->
val hash = md.digest(signature.toByteArray())
hash.joinToString(":") { b ->
b.toHexString(HexFormat.UpperCase)
}
}
}This is already implemented correctly - the md instance is created once and reused. This is more efficient than the previous single-signature approach that created the digest inline.
| return ValidateOriginResult.Error.PasskeyNotSupportedForApp | ||
| } | ||
|
|
||
| var assetLinkFound = false |
There was a problem hiding this comment.
💭 Question about error differentiation logic
Why track assetLinkFound separately?
The assetLinkFound flag differentiates between two scenarios:
- Asset link API succeeded but none matched (
linked = false) →PasskeyNotSupportedForApp - Asset link API failed (network/server error) for all attempts →
AssetLinkNotFound
Question: Is this distinction important for the user experience? Both scenarios result in validation failure.
The logic appears correct, but consider:
- If the asset link service returns
linked = false, it means the .well-known file was found but doesn't contain a matching certificate - If the service throws an exception, it means the .well-known file wasn't found or is inaccessible
This distinction helps provide more accurate error messages to the user about why passkey authentication failed.
| var assetLinkFound = false | ||
|
|
||
| // Check each fingerprint in the signing certificate history | ||
| return fingerprints |
There was a problem hiding this comment.
Verify certificate history ordering assumptions
The code iterates through signingCertificateHistory using firstNotNullOfOrNull, which returns as soon as a match is found. This assumes the certificate history is ordered in a specific way.
Questions:
- What is the ordering of
signingInfo.signingCertificateHistory? (oldest-to-newest or newest-to-oldest?) - Should we prioritize checking the current/newest certificate first for performance?
According to Android documentation, signingCertificateHistory contains the signing certificates with the first entry being the current signer. The current implementation will check certificates in order from current to historical, which is correct.
Recommendation: Add a comment clarifying this ordering assumption:
// Check each fingerprint in the signing certificate history
// Note: signingCertificateHistory is ordered from current to historical certificates
return fingerprints
There was a problem hiding this comment.
Something about this is making my Spidey senses tingle. 🤔
I think checking every cert in the chain is going to make us susceptible to APK signing replay attacks.
Since the first certificate is the latest, it may be more secure to only check it and ignore others in the chain. That would also make this solution simpler. We can remove the if (signingInfo.hasMultipleSigners()) return null statement from getAppSigningSignatureFingerprint and be done. Thoughts?
There was a problem hiding this comment.
That is what we are currently doing and some apps do not have the most recent certificate fingerprint on the well-known file, but have an older one, that is the cause of this issue.
getSigningCertificateHistory should only be used if hasMultipleSigners is false. We were already doing this hasMultipleSigners validation.
Checking all the signatures looks like the intended behaviour from android
When determining if a package is signed by a desired certificate, the returned array should be checked to determine if it is one of the entries.
🤔 might be worth to also check getApkContentsSigners when multipleSigners returns true
There was a problem hiding this comment.
Discussed offline. There are concerns about the mismatch between WhatsApp's assetlinks.json and the app signature being detected.
Accepting a valid certificate signature from anywhere within the chain is a potential vulnerability. The suggested solution at this time is to improve our user-facing error message in this scenario, and reject the request. Additionally, we are reaching out to WhatsApp so we can better understand why key rotation was performed and why their assetlinks.json has not been updated to include the latest key signature.
| ValidateOriginResult.Success(null), | ||
| result, | ||
| ) | ||
| } |
There was a problem hiding this comment.
👍 Excellent test coverage
The test suite comprehensively covers the new functionality:
- ✅ Multiple certificates in history with fallback validation (line 249)
- ✅ Early return on first match (line 320)
- ✅ All certificates fail validation (line 381)
- ✅ Multiple signers edge case (line 451)
This demonstrates thorough thinking about edge cases and validation scenarios.
|
Great job! No new security vulnerabilities introduced in this pull request |
|
Warning @aj-rosado Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22479
📔 Objective
Added method to return signature from current certificate and past signing certificates from
signingCertificateHistorywhenhasMultipleSignersreturns false.Using it to validate AssetLinks as some .well-known might not have the most recent certificate.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes