[PM-20577] Encrypt Risk Insights report and send it to server #14659
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #14659 +/- ##
==========================================
- Coverage 36.38% 36.33% -0.06%
==========================================
Files 3194 3191 -3
Lines 92647 92221 -426
Branches 16674 16535 -139
==========================================
- Hits 33712 33508 -204
+ Misses 56511 56322 -189
+ Partials 2424 2391 -33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (8)Checkmarx found the following issues in this Pull Request
|
There was a problem hiding this comment.
Hey, while I'm not a code owner here, this uses KM APIs and increases KM tech debt, and overlaps with upcoming KM work on org key rotation. My main goal here is to use content encryption keys for the reports, as we have been doing with e.g. ciphers (cipherkeys) and attachments (attachment keys) to not put us in a situation where we have to download and re-upload all reports.
Looking at this though, the unencrypted metadata caught my eye. This may introduce unintended issues, and I'll create a slack thread on this.
| details: ApplicationHealthReportDetail[], | ||
| summary: ApplicationHealthReportSummary, | ||
| ): Promise<RiskInsightsReport> { | ||
| const key = await this.keyService.getOrgKey(organizationId as string); |
There was a problem hiding this comment.
❌ I realize this PR is still in draft, but I imagine there are related PR's (server), so I'm already commenting this. Using the org key here directly both has security implications, but also cryptographic tech debt implications when considering organization key rotation. If we want to rotate the org key, that means downloading and uploading all reports.
Can we please not directly encrypt with the org key here, but use a layer of indirection? This way, the encryption scope is limited, and also key rotation becomes just re-uploading the content-encryption-keys, not the content. This is a pattern we use with attachments and ciphers (featureflagged still).
Specifically something like:
const orgKey = await this.keyService.getOrgKey(...);
const reportContentEncryptionKey = await this.keyGenerationService.createKey(512);
const wrappedReportContentEncryptionKey = await this.encryptService.wrapSymmetricKey(reportContentEncryptionKey, orgKey);
// now save "wrappedReportContentEncryptionKey" along with reportData (encrypted by `reportContentEncryptionkey`)
I realize this is not obvious from the interfaces yet, and the interfaces KM offers here are in-flight / changing. If you have questions / need assistance please feel free to reach out to key-management in slack.
There was a problem hiding this comment.
Even nicer would be to introduce an intermediate key here, a "risk insights key", that would wrap all reportContentEncryptionKeys, and itself be wrapped by the org key. But I don't want to increase scope more than necessary.
| organizationId: OrganizationId; | ||
| date: string; | ||
| reportData: string; | ||
| totalMembers: number; |
There was a problem hiding this comment.
Question ❓ Will this report be automatically generated and submitted on a schedule, or manually created only? If the former, are we OK with this unencrypted metadata?
Unless not possible, I would recommend encrypting all metadata here. Specifically, reportData should be one blob including both the summary data and detail data. Below, I show a theoretical attack that may abuse the summary data to decrypt all org vault data remotely.
Specifically, the following attacks (non exhaustive, and non validated) may be made possible. I have not implemented or validated them, and they are purely on the basis of reading the code.
- Assumption: There is a automated schedule / way to trigger the generation
- Threat model: the attacker controls the API server.
Leak individiual item/application weakness status
Before every report generation, the server forces a sync to the client that will create the report, and present a false view of the ciphers, specifically, just show 1 cipher. The total will then reflect just that cipher, and thus the safety presumed by the use of summarized data is broken. This attack can be optimized to be much faster than a linear amount of queries.
Remote predicate evaluation on arbitrary org encrypted strings
Further, it allows the server to remotely run certain predicates such as findWeakPassword on any arbitrary org-key encrypted data (not just passwords) and obtain the result, by swapping encrypted strings within the cipher.
- Possible full plaintext recovery, breaking organization e2e encryption: Doing a brief evaluation, without further validation,
this.passwordStrengthService.getPasswordStrength(is called, which uses zxcvbn to compare a security score, of the password compared to, including the encrypted "username" field. If the server has access to known plaintext ciphertext pairs here, they may be able to create a decryption oracle capable of remotely decrypting all org vault data with this. NOTE: This is theoretical, I have not validated this thoroughly. Roughly, the attacker would remotely, via the previous attack make the client evaluate password strength. As the password, the server sets the target encstring. As the username, they set one of the known plaintext / ciphertext pairs. zxcvbn will return a different score based on the similarity of the two, and depending on if it reaches a threshold, it will be a cipher at risk. This can be used, given enough plaintext ciphertext pairs to fully recovery the target plaintext, thus making full decryption of all vault data possible. One would find a plaintext-ciphertext pair close to the threshold, then flip each character to figure out which character is the correct character for that position. However, this requires a lot of plaintext ciphertext pairs, or an encryption oracle.
Remote predicate evaluation - equality check
Using the same attack as above, the server can perform an equality check on any two arbitrary org-key encrypted org strings, by presenting 2 items, with the target encstrings set as the password.
Remote clustering of encrypted vault items by app
Further, it allows the server to cluster any ciphers and confirm if the are for the same domain. Example: Server provides cipher A, cipher B. If only one app is reported, then both are the same app and get clustered. This can be improved to be much faster than linear time.
(Out of scope note) Clusters / individual weak items can be tied to real domains
Combining this with a compromised icon server (out of scope) would allow tying encrypted ciphers to domains, and thus leak which accounts are weak.
There was a problem hiding this comment.
I will cc @mandreko-bitwarden, @jlf0dev on this, because these (presumed) attacks are security relevant, possibly threatening all of organization vault encryption confidentiality. I'm keeping these comments public because this is not released code, but just draft.
@jlf0dev I have not implemented these attacks because I looked at this as an early code review for the unrelated issue bellow, but it felt unethical not to look closer here. I don't know if it's worth to spend the time to confirm the validity of the attacks above. If we just encrypt the metadata, then all of them are invalid etiher way.
|
There was a problem hiding this comment.
Hey @voommen-livefront I've taken a look. Thank you for working to address the KM tech debt concern regarding the content encryption key. Sadly the APIs that KM offers are a bit confusing still and I'll bring it up within KM.
I believe this PR currently uses two layers of encryption (accidentally?) which both not necessary, but also makes the content-encryption-key useless, because now rotating the org key would again require a full re-upload of the data since the "outer layer" is encrypted with the org key. I've added a suggestion of how it could look.
| const riskInsightReport = { | ||
| organizationId: organizationId, | ||
| date: new Date().toISOString(), | ||
| reportData: encryptedReportDataWithWrappedKey.encryptedString, |
There was a problem hiding this comment.
| reportData: encryptedReportDataWithWrappedKey.encryptedString, | |
| reportData: reportEncrypted.encryptedString, | |
| reportKey: wrappedReportContentEncryptionKey.encryptedString, |
| const reportDataWithWrappedKey = { | ||
| data: reportEncrypted.encryptedString, | ||
| key: wrappedReportContentEncryptionKey.encryptedString, | ||
| }; | ||
|
|
||
| const encryptedReportDataWithWrappedKey = await this.encryptService.encryptString( | ||
| JSON.stringify(reportDataWithWrappedKey), | ||
| orgKey, | ||
| ); |
There was a problem hiding this comment.
| const reportDataWithWrappedKey = { | |
| data: reportEncrypted.encryptedString, | |
| key: wrappedReportContentEncryptionKey.encryptedString, | |
| }; | |
| const encryptedReportDataWithWrappedKey = await this.encryptService.encryptString( | |
| JSON.stringify(reportDataWithWrappedKey), | |
| orgKey, | |
| ); |
I don't think this is right. This is effectively adds a second layer of encryption around already encrypted data, wrapping all data again with the org key. As far as I can tell, you can send data, key directly to the server, without another layer of encryption because:
- data is encrypted by reportContentEncryptionKey
- key (reportContentEncryptionKey) is wrapped (encrypted) with orgKey
The above is basically creating: orgKey({contentKey(data),orgKey(data)})
where you just need {contentKey(data),orgKey(data)}.
| const decryptedReportDataWithWrappedKey = await this.encryptService.decryptString( | ||
| new EncString(riskInsightsReportResponse.reportData), | ||
| orgKey, | ||
| ); | ||
|
|
||
| const reportDataInJson = JSON.parse(decryptedReportDataWithWrappedKey); | ||
| const reportEncrypted = reportDataInJson.data; | ||
| const wrappedReportContentEncryptionKey = reportDataInJson.key; |
There was a problem hiding this comment.
| const decryptedReportDataWithWrappedKey = await this.encryptService.decryptString( | |
| new EncString(riskInsightsReportResponse.reportData), | |
| orgKey, | |
| ); | |
| const reportDataInJson = JSON.parse(decryptedReportDataWithWrappedKey); | |
| const reportEncrypted = reportDataInJson.data; | |
| const wrappedReportContentEncryptionKey = reportDataInJson.key; | |
| const wrappedReportContentEncryptionKey = riskInsightsReportResponse.reportKey; | |
| const reportEncrypted = riskInsightsReportResponse.reportData; |
With the same reasoning as above, I don't think we need two layers of encryption here?
| throw new Error("Organization key not found"); | ||
| } | ||
|
|
||
| const reportContentEncryptionKey = await this.keyGeneratorService.createKey(512); |
There was a problem hiding this comment.
note/thought (non-actionable): I believe this is a pattern that other teams (in this case DIRT) have to implement over and over, and should really live in KM domain.
So, specifically, the use-case is "I want to encrypt blobs of data (attachment/report/cipherblob/etc.) safely, and be able to easily update the encryption".
I believe in the future KM should/may provide a high level API similar to:
Encrypt:
const { wrappedContentEncryptionKey, sealedData } = this.encryptService.sealDataEnvelope(MyReport, orgKey);
Decrypt:
const { unsealedData } = this.encryptService.unsealDataEnvelope(sealedData, wrappedContentEncryptionKey, orgKey);
Which would eliminate most cryptographic code that this team (dirt) is forced to currently think about, but at the same time enforce correct usage of a content encryption key.
@Thomas-Avery do we want to discuss this in KM dev sync first or do I make a tech debt ticket for us to provide a better interface, and ask for the ticket to be put in this section of code to serve as a reminder?
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-20577
📔 Objective
Encrypt At risk reports data and send it to the server to save
📸 Screenshots
⏰ 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