-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[PM-20577] Encrypt Risk Insights report and send it to server #14659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
d4271b4
9319dbc
585c7b0
09ac1c6
45212c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import { mock } from "jest-mock-extended"; | ||
|
||
import { ApiService } from "@bitwarden/common/abstractions/api.service"; | ||
import { OrganizationId } from "@bitwarden/common/types/guid"; | ||
|
||
import { SaveRiskInsightsReportRequest } from "../models/password-health"; | ||
|
||
import { RiskInsightsApiService } from "./risk-insights-api.service"; | ||
|
||
describe("RiskInsightsApiService", () => { | ||
let service: RiskInsightsApiService; | ||
const apiService = mock<ApiService>(); | ||
|
||
beforeEach(() => { | ||
service = new RiskInsightsApiService(apiService); | ||
}); | ||
|
||
it("should be created", () => { | ||
expect(service).toBeTruthy(); | ||
}); | ||
|
||
it("should call apiService.send with correct parameters for saveRiskInsightsReport", (done) => { | ||
const orgId = "org1" as OrganizationId; | ||
const request: SaveRiskInsightsReportRequest = { | ||
data: { | ||
organizationId: orgId, | ||
date: new Date().toISOString(), | ||
reportData: "test", | ||
totalMembers: 10, | ||
totalAtRiskMembers: 5, | ||
totalApplications: 100, | ||
totalAtRiskApplications: 50, | ||
totalCriticalApplications: 22, | ||
}, | ||
}; | ||
const response = { | ||
...request.data, | ||
}; | ||
|
||
apiService.send.mockReturnValue(Promise.resolve(response)); | ||
|
||
service.saveRiskInsightsReport(orgId, request).subscribe((result) => { | ||
expect(result).toEqual(response); | ||
expect(apiService.send).toHaveBeenCalledWith( | ||
"PUT", | ||
`/reports/risk-insights-report/${orgId.toString()}`, | ||
request.data, | ||
true, | ||
true, | ||
); | ||
done(); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import { from, Observable, of } from "rxjs"; | ||
|
||
import { ApiService } from "@bitwarden/common/abstractions/api.service"; | ||
import { OrganizationId } from "@bitwarden/common/types/guid"; | ||
|
||
import { | ||
GetRiskInsightsReportResponse, | ||
SaveRiskInsightsReportRequest, | ||
SaveRiskInsightsReportResponse, | ||
} from "../models/password-health"; | ||
|
||
export class RiskInsightsApiService { | ||
constructor(private apiService: ApiService) {} | ||
|
||
saveRiskInsightsReport( | ||
orgId: OrganizationId, | ||
request: SaveRiskInsightsReportRequest, | ||
): Observable<SaveRiskInsightsReportResponse> { | ||
const dbResponse = this.apiService.send( | ||
"PUT", | ||
`/reports/risk-insights-report/${orgId.toString()}`, | ||
request.data, | ||
true, | ||
true, | ||
); | ||
|
||
return from(dbResponse as Promise<SaveRiskInsightsReportResponse>); | ||
} | ||
|
||
getRiskInsightsReport(orgId: OrganizationId): Observable<GetRiskInsightsReportResponse | null> { | ||
const dbResponse = this.apiService | ||
Check warning on line 31 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-api.service.ts
|
||
.send("GET", `/reports/risk-insights-report/${orgId.toString()}`, null, true, true) | ||
.catch((error: any): any => { | ||
if (error.statusCode === 404) { | ||
return null; // Handle 404 by returning null or an appropriate default value | ||
Check warning on line 35 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-api.service.ts
|
||
} | ||
throw error; // Re-throw other errors | ||
Check warning on line 37 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-api.service.ts
|
||
}); | ||
|
||
if (dbResponse instanceof Error) { | ||
return of(null); | ||
Check warning on line 41 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-api.service.ts
|
||
} | ||
return from(dbResponse as Promise<GetRiskInsightsReportResponse>); | ||
Check warning on line 43 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-api.service.ts
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,13 +1,18 @@ | ||||||||||||||||||||||
// FIXME: Update this file to be type safe | ||||||||||||||||||||||
// @ts-strict-ignore | ||||||||||||||||||||||
import { concatMap, first, from, map, Observable, zip } from "rxjs"; | ||||||||||||||||||||||
import { concatMap, first, firstValueFrom, from, map, Observable, takeWhile, zip } from "rxjs"; | ||||||||||||||||||||||
|
||||||||||||||||||||||
import { AuditService } from "@bitwarden/common/abstractions/audit.service"; | ||||||||||||||||||||||
import { EncryptService } from "@bitwarden/common/key-management/crypto/abstractions/encrypt.service"; | ||||||||||||||||||||||
import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/key-generation.service"; | ||||||||||||||||||||||
import { Utils } from "@bitwarden/common/platform/misc/utils"; | ||||||||||||||||||||||
import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; | ||||||||||||||||||||||
import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength"; | ||||||||||||||||||||||
import { OrganizationId } from "@bitwarden/common/types/guid"; | ||||||||||||||||||||||
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; | ||||||||||||||||||||||
import { CipherType } from "@bitwarden/common/vault/enums"; | ||||||||||||||||||||||
import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; | ||||||||||||||||||||||
import { KeyService } from "@bitwarden/key-management"; | ||||||||||||||||||||||
|
||||||||||||||||||||||
import { | ||||||||||||||||||||||
ApplicationHealthReportDetail, | ||||||||||||||||||||||
|
@@ -20,8 +25,11 @@ | |||||||||||||||||||||
MemberDetailsFlat, | ||||||||||||||||||||||
WeakPasswordDetail, | ||||||||||||||||||||||
WeakPasswordScore, | ||||||||||||||||||||||
RiskInsightsReport, | ||||||||||||||||||||||
GetRiskInsightsReportResponse, | ||||||||||||||||||||||
} from "../models/password-health"; | ||||||||||||||||||||||
|
||||||||||||||||||||||
import { CriticalAppsService } from "./critical-apps.service"; | ||||||||||||||||||||||
import { MemberCipherDetailsApiService } from "./member-cipher-details-api.service"; | ||||||||||||||||||||||
|
||||||||||||||||||||||
export class RiskInsightsReportService { | ||||||||||||||||||||||
|
@@ -30,6 +38,10 @@ | |||||||||||||||||||||
private auditService: AuditService, | ||||||||||||||||||||||
private cipherService: CipherService, | ||||||||||||||||||||||
private memberCipherDetailsApiService: MemberCipherDetailsApiService, | ||||||||||||||||||||||
private keyService: KeyService, | ||||||||||||||||||||||
private encryptService: EncryptService, | ||||||||||||||||||||||
private criticalAppsService: CriticalAppsService, | ||||||||||||||||||||||
private keyGeneratorService: KeyGenerationService, | ||||||||||||||||||||||
) {} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
|
@@ -162,6 +174,106 @@ | |||||||||||||||||||||
}; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
async generateEncryptedRiskInsightsReport( | ||||||||||||||||||||||
organizationId: OrganizationId, | ||||||||||||||||||||||
details: ApplicationHealthReportDetail[], | ||||||||||||||||||||||
summary: ApplicationHealthReportSummary, | ||||||||||||||||||||||
): Promise<RiskInsightsReport> { | ||||||||||||||||||||||
const orgKey = await this.keyService.getOrgKey(organizationId as string); | ||||||||||||||||||||||
Check warning on line 182 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
if (orgKey === null) { | ||||||||||||||||||||||
throw new Error("Organization key not found"); | ||||||||||||||||||||||
Check warning on line 184 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
const reportContentEncryptionKey = await this.keyGeneratorService.createKey(512); | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Decrypt:
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? |
||||||||||||||||||||||
const reportEncrypted = await this.encryptService.encryptString( | ||||||||||||||||||||||
Check warning on line 188 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
JSON.stringify(details), | ||||||||||||||||||||||
reportContentEncryptionKey, | ||||||||||||||||||||||
); | ||||||||||||||||||||||
|
||||||||||||||||||||||
const wrappedReportContentEncryptionKey = await this.encryptService.wrapSymmetricKey( | ||||||||||||||||||||||
Check warning on line 193 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
reportContentEncryptionKey, | ||||||||||||||||||||||
orgKey, | ||||||||||||||||||||||
); | ||||||||||||||||||||||
|
||||||||||||||||||||||
const reportDataWithWrappedKey = { | ||||||||||||||||||||||
Check warning on line 198 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
data: reportEncrypted.encryptedString, | ||||||||||||||||||||||
key: wrappedReportContentEncryptionKey.encryptedString, | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
const encryptedReportDataWithWrappedKey = await this.encryptService.encryptString( | ||||||||||||||||||||||
Check warning on line 203 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
JSON.stringify(reportDataWithWrappedKey), | ||||||||||||||||||||||
orgKey, | ||||||||||||||||||||||
); | ||||||||||||||||||||||
Comment on lines
+198
to
+206
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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
The above is basically creating: |
||||||||||||||||||||||
|
||||||||||||||||||||||
const criticalApps = await firstValueFrom( | ||||||||||||||||||||||
Check warning on line 208 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
this.criticalAppsService | ||||||||||||||||||||||
.getAppsListForOrg(organizationId) | ||||||||||||||||||||||
.pipe(takeWhile((apps) => apps !== null && apps.length > 0)), | ||||||||||||||||||||||
); | ||||||||||||||||||||||
|
||||||||||||||||||||||
const riskInsightReport = { | ||||||||||||||||||||||
Check warning on line 214 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
organizationId: organizationId, | ||||||||||||||||||||||
date: new Date().toISOString(), | ||||||||||||||||||||||
reportData: encryptedReportDataWithWrappedKey.encryptedString, | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
totalMembers: summary.totalMemberCount, | ||||||||||||||||||||||
totalAtRiskMembers: summary.totalAtRiskMemberCount, | ||||||||||||||||||||||
totalApplications: summary.totalApplicationCount, | ||||||||||||||||||||||
totalAtRiskApplications: summary.totalAtRiskApplicationCount, | ||||||||||||||||||||||
totalCriticalApplications: criticalApps.length, | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
return riskInsightReport; | ||||||||||||||||||||||
Check warning on line 225 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
async decryptRiskInsightsReport( | ||||||||||||||||||||||
organizationId: OrganizationId, | ||||||||||||||||||||||
riskInsightsReportResponse: GetRiskInsightsReportResponse, | ||||||||||||||||||||||
): Promise<[ApplicationHealthReportDetail[], ApplicationHealthReportSummary]> { | ||||||||||||||||||||||
try { | ||||||||||||||||||||||
const orgKey = await this.keyService.getOrgKey(organizationId as string); | ||||||||||||||||||||||
Check warning on line 233 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
if (orgKey === null) { | ||||||||||||||||||||||
throw new Error("Organization key not found"); | ||||||||||||||||||||||
Check warning on line 235 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
const decryptedReportDataWithWrappedKey = await this.encryptService.decryptString( | ||||||||||||||||||||||
Check warning on line 238 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
new EncString(riskInsightsReportResponse.reportData), | ||||||||||||||||||||||
orgKey, | ||||||||||||||||||||||
); | ||||||||||||||||||||||
|
||||||||||||||||||||||
const reportDataInJson = JSON.parse(decryptedReportDataWithWrappedKey); | ||||||||||||||||||||||
const reportEncrypted = reportDataInJson.data; | ||||||||||||||||||||||
const wrappedReportContentEncryptionKey = reportDataInJson.key; | ||||||||||||||||||||||
Check warning on line 245 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
Comment on lines
+238
to
+245
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
With the same reasoning as above, I don't think we need two layers of encryption here? |
||||||||||||||||||||||
|
||||||||||||||||||||||
const freshWrappedReportContentEncryptionKey = new EncString( | ||||||||||||||||||||||
Check warning on line 247 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
wrappedReportContentEncryptionKey, | ||||||||||||||||||||||
); | ||||||||||||||||||||||
|
||||||||||||||||||||||
const unwrappedReportContentEncryptionKey = await this.encryptService.unwrapSymmetricKey( | ||||||||||||||||||||||
Check warning on line 251 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
freshWrappedReportContentEncryptionKey, | ||||||||||||||||||||||
orgKey, | ||||||||||||||||||||||
); | ||||||||||||||||||||||
|
||||||||||||||||||||||
const reportUnencrypted = await this.encryptService.decryptString( | ||||||||||||||||||||||
Check warning on line 256 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
new EncString(reportEncrypted), | ||||||||||||||||||||||
unwrappedReportContentEncryptionKey, | ||||||||||||||||||||||
); | ||||||||||||||||||||||
|
||||||||||||||||||||||
const reportJson: ApplicationHealthReportDetail[] = JSON.parse(reportUnencrypted); | ||||||||||||||||||||||
Check warning on line 261 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
|
||||||||||||||||||||||
const summary: ApplicationHealthReportSummary = { | ||||||||||||||||||||||
Check warning on line 263 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
totalMemberCount: riskInsightsReportResponse.totalMembers, | ||||||||||||||||||||||
totalAtRiskMemberCount: riskInsightsReportResponse.totalAtRiskMembers, | ||||||||||||||||||||||
totalApplicationCount: riskInsightsReportResponse.totalApplications, | ||||||||||||||||||||||
totalAtRiskApplicationCount: riskInsightsReportResponse.totalAtRiskApplications, | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
return [reportJson, summary]; | ||||||||||||||||||||||
Check warning on line 270 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
} catch { | ||||||||||||||||||||||
// console.error("Error decrypting risk insights report :", error); | ||||||||||||||||||||||
return [null, null]; | ||||||||||||||||||||||
Check warning on line 273 in bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/risk-insights-report.service.ts
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
* Associates the members with the ciphers they have access to. Calculates the password health. | ||||||||||||||||||||||
* Finds the trimmed uris. | ||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.