Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -156,4 +156,31 @@ export enum DrawerType {
OrgAtRiskApps = 3,
}

export interface RiskInsightsReport {
organizationId: OrganizationId;
date: string;
reportData: string;
totalMembers: number;
Copy link
Contributor

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?

โš ๏ธโš ๏ธโš ๏ธ Assuming it can be run automated: I don't know the underlying security guarantees that are expected to be achieved here. From the fact that we encrypt reports, but do not encrypt summaries, I assume the underlying assumption here is that summaries do not leak individual data so it's "safe to store". That (assumed) assumption is not valid.

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.

Copy link
Contributor

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.

totalAtRiskMembers: number;
totalApplications: number;
totalAtRiskApplications: number;
totalCriticalApplications: number;
}

export interface SaveRiskInsightsReportRequest {
data: RiskInsightsReport;
}

export interface SaveRiskInsightsReportResponse {
id: string;
organizationId: OrganizationId;
date: string;
reportData: string;
totalMembers: number;
totalAtRiskMembers: number;
totalApplications: number;
totalAtRiskApplications: number;
totalCriticalApplications: number;
}

export type PasswordHealthReportApplicationId = Opaque<string, "PasswordHealthReportApplicationId">;
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ export class CriticalAppsService {

// Get a list of critical apps for a given organization
getAppsListForOrg(orgId: string): Observable<PasswordHealthReportApplicationsResponse[]> {
if (this.criticalAppsList.value.length === 0) {
return of([]);
}

return this.criticalAppsList
.asObservable()
.pipe(map((apps) => apps.filter((app) => app.organizationId === orgId)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ export * from "./critical-apps.service";
export * from "./critical-apps-api.service";
export * from "./risk-insights-report.service";
export * from "./risk-insights-data.service";
export * from "./risk-insights-api.service";
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { mock } from "jest-mock-extended";

import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { OrganizationId } from "@bitwarden/common/types/guid";

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 = {
data: {
organizationId: orgId,
date: new Date().toISOString(),
reportData: "test data",
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,28 @@
import { from, Observable } from "rxjs";

import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { OrganizationId } from "@bitwarden/common/types/guid";

import {
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>);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ›๏ธ I think we should take the encryption/decryption logic and move it to a new service risk-insights-encryption.service The logic is only accessed from the risk-insights-data.service right now and not directly used in the risk-insights-report.service

The report service logic should just contain the logic for calculating the risk insights report. I think adding the encryption decryption adds too much to this service.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As advised - I moved the encryption/decryption to its own service

Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
// 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 { Utils } from "@bitwarden/common/platform/misc/utils";
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,
Expand All @@ -20,8 +23,10 @@ import {
MemberDetailsFlat,
WeakPasswordDetail,
WeakPasswordScore,
RiskInsightsReport,
} from "../models/password-health";

import { CriticalAppsService } from "./critical-apps.service";
import { MemberCipherDetailsApiService } from "./member-cipher-details-api.service";

export class RiskInsightsReportService {
Expand All @@ -30,6 +35,9 @@ export class RiskInsightsReportService {
private auditService: AuditService,
private cipherService: CipherService,
private memberCipherDetailsApiService: MemberCipherDetailsApiService,
private keyService: KeyService,
private encryptService: EncryptService,
private criticalAppsService: CriticalAppsService,
) {}

/**
Expand Down Expand Up @@ -162,6 +170,38 @@ export class RiskInsightsReportService {
};
}

async generateRiskInsightsReport(
organizationId: OrganizationId,
details: ApplicationHealthReportDetail[],
summary: ApplicationHealthReportSummary,
): Promise<RiskInsightsReport> {
const key = await this.keyService.getOrgKey(organizationId as string);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โŒ 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (key === null) {
throw new Error("Organization key not found");
}

const reportData = await this.encryptService.encryptString(JSON.stringify(details), key);

// const atRiskMembers = this.generateAtRiskMemberList(details);
// const atRiskApplications = this.generateAtRiskApplicationList(details);
const criticalApps = await firstValueFrom(
this.criticalAppsService
.getAppsListForOrg(organizationId)
.pipe(takeWhile((apps) => apps !== null && apps.length > 0)),
);

return {
organizationId: organizationId,
date: new Date().toISOString(),
reportData: reportData?.encryptedString?.toString() ?? "",
totalMembers: summary.totalMemberCount,
totalAtRiskMembers: summary.totalAtRiskMemberCount,
totalApplications: summary.totalApplicationCount,
totalAtRiskApplications: summary.totalAtRiskApplicationCount,
totalCriticalApplications: criticalApps.length,
};
}

/**
* Associates the members with the ciphers they have access to. Calculates the password health.
* Finds the trimmed uris.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { CriticalAppsService } from "@bitwarden/bit-common/dirt/reports/risk-ins
import {
CriticalAppsApiService,
MemberCipherDetailsApiService,
RiskInsightsApiService,
RiskInsightsDataService,
RiskInsightsReportService,
} from "@bitwarden/bit-common/dirt/reports/risk-insights/services";
Expand Down Expand Up @@ -32,6 +33,9 @@ import { RiskInsightsComponent } from "./risk-insights.component";
AuditService,
CipherService,
MemberCipherDetailsApiService,
KeyService,
EncryptService,
CriticalAppsService,
],
},
{
Expand All @@ -48,6 +52,10 @@ import { RiskInsightsComponent } from "./risk-insights.component";
useClass: CriticalAppsApiService,
deps: [ApiService],
}),
safeProvider({
provide: RiskInsightsApiService,
deps: [ApiService],
}),
],
})
export class AccessIntelligenceModule {}
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,21 @@ import { Component, DestroyRef, inject, OnInit } from "@angular/core";
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
import { FormControl } from "@angular/forms";
import { ActivatedRoute } from "@angular/router";
import { combineLatest, debounceTime, firstValueFrom, map, Observable, of, skipWhile } from "rxjs";
import {
BehaviorSubject,
combineLatest,
debounceTime,
firstValueFrom,
map,
Observable,
of,
skipWhile,
switchMap,
} from "rxjs";

import {
CriticalAppsService,
RiskInsightsApiService,
RiskInsightsDataService,
RiskInsightsReportService,
} from "@bitwarden/bit-common/dirt/reports/risk-insights";
Expand All @@ -24,6 +35,7 @@ import { getUserId } from "@bitwarden/common/auth/services/account.service";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { OrganizationId } from "@bitwarden/common/types/guid";
import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
import {
Icons,
Expand Down Expand Up @@ -74,6 +86,12 @@ export class AllApplicationsComponent implements OnInit {
isLoading$: Observable<boolean> = of(false);
isCriticalAppsFeatureEnabled = false;

private atRiskInsightsReport = new BehaviorSubject<{
data: ApplicationHealthReportDetailWithCriticalFlag[];
organization: Organization;
summary: ApplicationHealthReportSummary;
}>(null);

async ngOnInit() {
this.isCriticalAppsFeatureEnabled = await this.configService.getFeatureFlag(
FeatureFlag.CriticalApps,
Expand Down Expand Up @@ -112,6 +130,16 @@ export class AllApplicationsComponent implements OnInit {
if (organization) {
this.organization = organization;
}

if (data && organization) {
this.atRiskInsightsReport.next({
data,
organization,
summary: this.applicationSummary,
});
}

// this.persistReportData();
});

this.isLoading$ = this.dataService.isLoading$;
Expand All @@ -129,10 +157,49 @@ export class AllApplicationsComponent implements OnInit {
protected reportService: RiskInsightsReportService,
private accountService: AccountService,
protected criticalAppsService: CriticalAppsService,
protected riskInsightsApiService: RiskInsightsApiService,
) {
this.searchControl.valueChanges
.pipe(debounceTime(200), takeUntilDestroyed())
.subscribe((v) => (this.dataSource.filter = v));

this.atRiskInsightsReport
.asObservable()
.pipe(
debounceTime(500),
switchMap(async (report) => {
if (report && report.organization?.id && report.data && report.summary) {
const data = await this.reportService.generateRiskInsightsReport(
report.organization.id as OrganizationId,
report.data,
report.summary,
);
return data;
}
return null;
}),
switchMap(async (reportData) => {
if (reportData) {
const request = { data: reportData };
try {
const response = await firstValueFrom(
this.riskInsightsApiService.saveRiskInsightsReport(
this.organization.id as OrganizationId,
request,
),
);
// console.log("Risk insights report saved successfully.", JSON.stringify(response));
return response;
} catch {
// console.error("Error saving risk insights report:", error);
}

return null;
}
}),
takeUntilDestroyed(),
)
.subscribe();
}

goToCreateNewLoginItem = async () => {
Expand Down
Loading