Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions apps/cli/src/vault/models/cipher.response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ export class CipherResponse extends CipherWithIdExport implements BaseResponse {
super();
this.object = "item";
this.build(o);
// Redact the decrypted item key to prevent logging it to the CLI output.
this.key = undefined;
Comment on lines +25 to +26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: Redacting key here makes bw edit item silently strip the cipher's item key.

Details and fix

The documented CLI edit flow round-trips bw get item output back through bw edit item. EditCommand passes the freshly decrypted view as the target of CipherExport.toView (apps/cli/src/commands/edit.command.ts:143), and toView overwrites key unconditionally:

view.key = req.key != null ? SymmetricCryptoKey.fromString(req.key) : undefined;

Since req.key is now always absent, the item key already present on cipherView is discarded, and toSdkUpdateCipherRequest / toSdkCipherView then send key: undefined. Every CLI edit of a cipher that has an item key drops it. Previously CipherResponse.key carried the wrapped EncString, so the round trip preserved it.

The redaction itself is correct β€” the unconditional overwrite in toView is the problem. Consider only replacing the key when the request actually supplies one:

if (req.key != null) {
  try {
    view.key = SymmetricCryptoKey.fromString(req.key);
  } catch {
    // Old exports stored the wrapped EncString key, which cannot be used on import
  }
}

BitwardenJsonImporter already sets cipher.key = null immediately after toView, so import behavior is unaffected by the change.

if (o.attachments != null) {
this.attachments = o.attachments.map((a) => new AttachmentResponse(a));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// eslint-disable-next-line no-restricted-imports
import { KeyService } from "@bitwarden/key-management";
import { LogService } from "@bitwarden/logging";
import { UserKeyRotationServiceAbstraction } from "@bitwarden/user-crypto-management";

Check failure on line 7 in libs/common/src/key-management/encrypted-migrator/migrations/v2-key-rotation-migration.spec.ts

View workflow job for this annotation

GitHub Actions / Lint

'EncString' is defined but never used

import { FeatureFlag } from "../../../enums/feature-flag.enum";
import { ConfigService } from "../../../platform/abstractions/config/config.service";
Expand All @@ -16,7 +16,6 @@
import { CipherService } from "../../../vault/abstractions/cipher.service";
import { AttachmentView } from "../../../vault/models/view/attachment.view";
import { CipherView } from "../../../vault/models/view/cipher.view";
import { EncString } from "../../crypto/models/enc-string";
import { MasterPasswordServiceAbstraction } from "../../master-password/abstractions/master-password.service.abstraction";

import { V2KeyRotationMigration } from "./v2-key-rotation-migration";
Expand Down Expand Up @@ -44,11 +43,9 @@
return cipher;
};

const makeAttachment = (hasEncryptedKey: boolean): AttachmentView => {
const makeAttachment = (hasKey: boolean): AttachmentView => {
const a = new AttachmentView();
a.encryptedKey = hasEncryptedKey
? new EncString("2.abc|def|ghi")
: (undefined as unknown as EncString);
a.key = hasKey ? mock<SymmetricCryptoKey>() : undefined;
return a;
};

Expand Down
13 changes: 11 additions & 2 deletions libs/common/src/models/export/cipher.export.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { EncString } from "../../key-management/crypto/models/enc-string";
import { SymmetricCryptoKey } from "../../platform/models/domain/symmetric-crypto-key";
import { CipherRepromptType } from "../../vault/enums/cipher-reprompt-type";
import { CipherType } from "../../vault/enums/cipher-type";
import { Cipher as CipherDomain } from "../../vault/models/domain/cipher";
Expand Down Expand Up @@ -43,7 +44,12 @@ export class CipherExport {
view.notes = req.notes;
view.favorite = req.favorite;
view.reprompt = req.reprompt ?? CipherRepromptType.None;
view.key = req.key != null ? new EncString(req.key) : undefined;
try {
view.key = req.key != null ? SymmetricCryptoKey.fromString(req.key) : undefined;
} catch {
// Old exports stored the wrapped EncString key which cannot be used on import
view.key = undefined;
}

if (req.fields != null) {
view.fields = req.fields.map((f) => FieldExport.toView(f));
Expand Down Expand Up @@ -208,7 +214,10 @@ export class CipherExport {
this.name = safeGetString(o.name) ?? "";
this.notes = safeGetString(o.notes);
if ("key" in o) {
this.key = o.key?.encryptedString;
this.key =
o.key instanceof SymmetricCryptoKey
? o.key.toBase64()
: (o.key as EncString | undefined)?.encryptedString;
Comment on lines +217 to +220

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: The CLI will now print the decrypted item key in bw get item / bw list items output.

Details and fix

CipherResponse (apps/cli/src/vault/models/cipher.response.ts) extends CipherWithIdExport and calls build(cipherView), then serializes the whole object to stdout. Previously this.key was the wrapped key (EncString.encryptedString); with CipherView.key now a SymmetricCryptoKey, this emits the raw base64 item key β€” key material that can decrypt the item's attachments, written to terminals, shell history, and script logs.

The export services already guard against this (delete cipher.key in individual-vault-export.service.ts:354 and org-vault-export.service.ts:384); the CLI response has no equivalent.

Consider redacting key in CipherResponse's constructor, mirroring the existing viewPassword redaction.

}

this.favorite = o.favorite;
Expand Down
1 change: 0 additions & 1 deletion libs/common/src/vault/models/domain/attachment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export class Attachment extends Domain {

if (this.key != null) {
view.key = await this.decryptAttachmentKey(decryptionKey);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: attachment.spec.ts still asserts the removed encryptedKey field, so this test fails.

Details and fix

libs/common/src/vault/models/domain/attachment.spec.ts:93-101 expects:

expect(view).toEqual({
  ...
  key: expect.any(SymmetricCryptoKey),
  encryptedKey: attachment.key,
});

AttachmentView no longer declares encryptedKey and decrypt() no longer assigns it, so the actual view has no such property while the expectation is a defined EncString mock. toEqual only ignores properties that are undefined on both sides, so this assertion fails. Removing the encryptedKey line from the expectation resolves it.

view.encryptedKey = this.key; // Keep the encrypted key for the view

// When the attachment key couldn't be decrypted, mark a decryption error
// The file won't be able to be downloaded in these cases
Expand Down
1 change: 1 addition & 0 deletions libs/common/src/vault/models/domain/cipher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ export class Cipher extends Domain implements Decryptable<CipherView> {
try {
const cipherKey = await encryptService.unwrapSymmetricKey(this.key, userKeyOrOrgKey);
cipherDecryptionKey = cipherKey;
model.key = cipherKey;
bypassValidation = false;
} catch {
model.name = "[error: cannot decrypt]";
Expand Down
11 changes: 3 additions & 8 deletions libs/common/src/vault/models/view/attachment.view.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { AttachmentView as SdkAttachmentView } from "@bitwarden/sdk-internal";

import { mockFromJson } from "../../../../spec";
import { EncString } from "../../../key-management/crypto/models/enc-string";
import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key";

import { AttachmentView } from "./attachment.view";
Expand Down Expand Up @@ -34,8 +33,7 @@ describe("AttachmentView", () => {
size: "size",
sizeName: "sizeName",
fileName: "fileName",
key: "encKeyB64_fromString",
decryptedKey: "decryptedKey_B64",
key: "decryptedKey_B64",
} as SdkAttachmentView;

const result = AttachmentView.fromSdkAttachmentView(sdkAttachmentView);
Expand All @@ -47,7 +45,6 @@ describe("AttachmentView", () => {
sizeName: "sizeName",
fileName: "fileName",
key: "mockKey",
encryptedKey: new EncString(sdkAttachmentView.key as string),
});

expect(SymmetricCryptoKey.fromString).toHaveBeenCalledWith("decryptedKey_B64");
Expand All @@ -57,7 +54,7 @@ describe("AttachmentView", () => {
describe("toSdkAttachmentView", () => {
it("should convert AttachmentView to SdkAttachmentView", () => {
const mockKey = {
toBase64: jest.fn().mockReturnValue("keyB64"),
toSdk: jest.fn().mockReturnValue("keyB64"),
} as any;

const attachmentView = new AttachmentView();
Expand All @@ -66,7 +63,6 @@ describe("AttachmentView", () => {
attachmentView.size = "size";
attachmentView.sizeName = "sizeName";
attachmentView.fileName = "fileName";
attachmentView.encryptedKey = new EncString("encKeyB64");
attachmentView.key = mockKey;

const result = attachmentView.toSdkAttachmentView();
Expand All @@ -77,8 +73,7 @@ describe("AttachmentView", () => {
size: "size",
sizeName: "sizeName",
fileName: "fileName",
key: "encKeyB64",
decryptedKey: "keyB64",
key: "keyB64",
});
});
});
Expand Down
29 changes: 5 additions & 24 deletions libs/common/src/vault/models/view/attachment.view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Jsonify } from "type-fest";

import { AttachmentView as SdkAttachmentView } from "@bitwarden/sdk-internal";

import { DECRYPT_ERROR, EncString } from "../../../key-management/crypto/models/enc-string";
import { DECRYPT_ERROR } from "../../../key-management/crypto/models/enc-string";
import { View } from "../../../models/view/view";
import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key";
import { Attachment } from "../domain/attachment";
Expand All @@ -14,10 +14,6 @@ export class AttachmentView implements View {
sizeName?: string;
fileName?: string;
key?: SymmetricCryptoKey;
/**
* The SDK returns an encrypted key for the attachment.
*/
encryptedKey: EncString | undefined;
private _hasDecryptionError?: boolean;

constructor(a?: Attachment) {
Expand Down Expand Up @@ -52,18 +48,7 @@ export class AttachmentView implements View {

static fromJSON(obj: Partial<Jsonify<AttachmentView>>): AttachmentView {
const key = obj.key == null ? null : SymmetricCryptoKey.fromJSON(obj.key);

let encryptedKey: EncString | undefined;
if (obj.encryptedKey != null) {
if (typeof obj.encryptedKey === "string") {
// If the key is a string, we need to parse it as EncString
encryptedKey = EncString.fromJSON(obj.encryptedKey);
} else if ((obj.encryptedKey as any) instanceof EncString) {
// If the key is already an EncString instance, we can use it directly
encryptedKey = obj.encryptedKey;
}
}
return Object.assign(new AttachmentView(), obj, { key: key, encryptedKey: encryptedKey });
return Object.assign(new AttachmentView(), obj, { key: key });
}

/**
Expand All @@ -76,9 +61,7 @@ export class AttachmentView implements View {
size: this.size,
sizeName: this.sizeName,
fileName: this.fileName,
key: this.encryptedKey?.toSdk(),
// TODO: PM-23005 - Temporary field, should be removed when encrypted migration is complete
decryptedKey: this.key ? this.key.toBase64() : undefined,
key: this.key?.toSdk(),
};
}

Expand All @@ -99,9 +82,7 @@ export class AttachmentView implements View {
view.size = obj.size;
view.sizeName = obj.sizeName;
view.fileName = obj.fileName;
// TODO: PM-23005 - Temporary field, should be removed when encrypted migration is complete
view.key = obj.decryptedKey ? SymmetricCryptoKey.fromString(obj.decryptedKey) : undefined;
view.encryptedKey = obj.key ? new EncString(obj.key) : undefined;
view.key = obj.key ? SymmetricCryptoKey.fromString(obj.key) : undefined;
view._hasDecryptionError = failure;

return view;
Expand All @@ -112,6 +93,6 @@ export class AttachmentView implements View {
* In this case, the attachment is encrypted with the user's user-key
*/
isLegacyAttachment(): boolean {
return this.key == null && this.encryptedKey == null;
return this.key == null && !this.hasDecryptionError;
}
}
34 changes: 11 additions & 23 deletions libs/common/src/vault/models/view/cipher.view.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { Jsonify } from "type-fest";

import {
CiphersClient,
CipherView as SdkCipherView,
Expand All @@ -13,8 +11,8 @@ import {
} from "@bitwarden/sdk-internal";

import { mockFromJson, mockFromSdk } from "../../../../spec";
import { EncString } from "../../../key-management/crypto/models/enc-string";
import { asUuid } from "../../../platform/abstractions/sdk/sdk.service";
import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key";
import { CipherRepromptType } from "../../enums";
import { CipherType } from "../../enums/cipher-type";
import { CipherPermissionsApi } from "../api/cipher-permissions.api";
Expand Down Expand Up @@ -94,23 +92,13 @@ describe("CipherView", () => {
expect(actual).toMatchObject(expected);
});

it("handle both string and object inputs for the cipher key", () => {
const cipherKeyString = "cipherKeyString";
const cipherKeyObject = new EncString("cipherKeyObject");
it("deserializes the cipher key to a SymmetricCryptoKey", () => {
const mockKey = { keyB64: "c29tZS1iYXNlNjQta2V5" };
jest.spyOn(SymmetricCryptoKey, "fromJSON").mockReturnValue(mockKey as any);

// Test with string input
let actual = CipherView.fromJSON({
key: cipherKeyString,
});
expect(actual.key).toBeInstanceOf(EncString);
expect(actual.key?.toJSON()).toBe(cipherKeyString);

// Test with object input (which can happen when cipher view is stored in an InMemory state provider)
actual = CipherView.fromJSON({
key: cipherKeyObject,
} as Jsonify<CipherView>);
expect(actual.key).toBeInstanceOf(EncString);
expect(actual.key?.toJSON()).toBe(cipherKeyObject.toJSON());
const actual = CipherView.fromJSON({ key: mockKey as any });
expect(actual.key).toBe(mockKey);
expect(SymmetricCryptoKey.fromJSON).toHaveBeenCalledWith(mockKey);
});

it("fromJSON should always restore top-level CipherView properties", () => {
Expand Down Expand Up @@ -143,7 +131,7 @@ describe("CipherView", () => {
original.deletedDate = new Date("2022-01-03");
original.archivedDate = new Date("2022-01-04");
original.reprompt = CipherRepromptType.Password;
original.key = new EncString("test-key");
original.key = new SymmetricCryptoKey(new Uint8Array(64));
original.decryptionFailure = true;

// Serialize and deserialize
Expand Down Expand Up @@ -304,7 +292,7 @@ describe("CipherView", () => {
cipherView.organizationId = "000f2a6e-da5e-4726-87ed-1c5c77322c3c";
cipherView.folderId = "41b22db4-8e2a-4ed2-b568-f1186c72922f";
cipherView.collectionIds = ["b0473506-3c3c-4260-a734-dfaaf833ab6f"];
cipherView.key = new EncString("some-key");
cipherView.key = { toSdk: () => "some-key-b64" } as any;
cipherView.name = "name";
cipherView.notes = "notes";
cipherView.type = CipherType.Login;
Expand Down Expand Up @@ -334,7 +322,7 @@ describe("CipherView", () => {
organizationId: asUuid("000f2a6e-da5e-4726-87ed-1c5c77322c3c"),
folderId: asUuid("41b22db4-8e2a-4ed2-b568-f1186c72922f"),
collectionIds: [asUuid("b0473506-3c3c-4260-a734-dfaaf833ab6f")],
key: "some-key" as any,
key: "some-key-b64" as any,
name: "name",
notes: "notes",
type: SdkCipherType.Login,
Expand Down Expand Up @@ -516,7 +504,7 @@ describe("CipherView", () => {
cipherView.reprompt = CipherRepromptType.Password;
cipherView.revisionDate = new Date("2022-01-02T12:00:00.000Z");
cipherView.archivedDate = new Date("2022-01-03T12:00:00.000Z");
cipherView.key = new EncString("cipher-key");
cipherView.key = { toSdk: () => "cipher-key-b64" } as any;

const mockField = new RealFieldView();
mockField.name = "testField";
Expand Down
37 changes: 5 additions & 32 deletions libs/common/src/vault/models/view/cipher.view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ import {
CipherView as SdkCipherView,
} from "@bitwarden/sdk-internal";

import { EncString } from "../../../key-management/crypto/models/enc-string";
import { View } from "../../../models/view/view";
import { asUuid, uuidAsString } from "../../../platform/abstractions/sdk/sdk.service";
import { InitializerMetadata } from "../../../platform/interfaces/initializer-metadata.interface";
import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key";
import { InitializerKey } from "../../../platform/services/cryptography/initializer-key";
import { DeepJsonify } from "../../../types/deep-jsonify";
import { CipherType, LinkedIdType } from "../../enums";
Expand All @@ -23,7 +23,6 @@ import { AttachmentView } from "./attachment.view";
import { BankAccountView } from "./bank-account.view";
import { CardView } from "./card.view";
import { DriversLicenseView } from "./drivers-license.view";
import { Fido2CredentialView } from "./fido2-credential.view";
import { FieldView } from "./field.view";
import { IdentityView } from "./identity.view";
import { ItemView } from "./item.view";
Expand Down Expand Up @@ -65,9 +64,7 @@ export class CipherView implements View, InitializerMetadata {
deletedDate?: Date;
archivedDate?: Date;
reprompt: CipherRepromptType = CipherRepromptType.None;
// We need a copy of the encrypted key so we can pass it to
// the SdkCipherView during encryption
key?: EncString;
key?: SymmetricCryptoKey;

/**
* Flag to indicate if the cipher decryption failed.
Expand Down Expand Up @@ -97,7 +94,6 @@ export class CipherView implements View, InitializerMetadata {
this.archivedDate = c.archivedDate;
// Old locally stored ciphers might have reprompt == null. If so set it to None.
this.reprompt = c.reprompt ?? CipherRepromptType.None;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: Dropping the key copy leaves views from the legacy Cipher.decrypt() path without an item key, which is then lost on re-encryption.

Details and fix

Removing this.key = c.key is correct for the new SymmetricCryptoKey semantics (the wrapped key must not be assigned here), but nothing replaces it on the deprecated decrypt path:

  1. Cipher.decrypt() (models/domain/cipher.ts:152) builds new CipherView(this) and unwraps the cipher key into a local cipherKey, but never assigns it to the view β€” so view.key is now always undefined.
  2. That path is still live: CipherService.decryptOrganizationCiphersResponse (cipher.service.ts:657, admin console org ciphers) and the admin attachment refresh in vault-item-dialog.component.ts:553.
  3. Saving such a view goes through DefaultCipherEncryptionService.encrypt β†’ toSdkCipherView() β†’ key: undefined, so the SDK no longer receives the existing item key.

Suggested fix in Cipher.decrypt(), where the unwrapped key is already available:

const cipherKey = await encryptService.unwrapSymmetricKey(this.key, userKeyOrOrgKey);
cipherDecryptionKey = cipherKey;
model.key = cipherKey;

this.key = c.key;
}

private get item(): ItemView | undefined {
Expand Down Expand Up @@ -252,17 +248,7 @@ export class CipherView implements View, InitializerMetadata {
view.passwordHistory =
obj.passwordHistory?.map((ph: any) => PasswordHistoryView.fromJSON(ph)) ?? [];

if (obj.key != null) {
let key: EncString | undefined;
if (typeof obj.key === "string") {
// If the key is a string, we need to parse it as EncString
key = EncString.fromJSON(obj.key);
} else if ((obj.key as any) instanceof EncString) {
// If the key is already an EncString instance, we can use it directly
key = obj.key;
}
view.key = key;
}
view.key = obj.key != null ? SymmetricCryptoKey.fromJSON(obj.key) : undefined;

switch (obj.type) {
case CipherType.Card:
Expand Down Expand Up @@ -299,7 +285,7 @@ export class CipherView implements View, InitializerMetadata {
/**
* Creates a CipherView from the SDK CipherView.
*/
static fromSdkCipherView(obj: SdkCipherView, sdk?: CiphersClient): CipherView | undefined {
static fromSdkCipherView(obj: SdkCipherView): CipherView | undefined {
if (obj == null) {
return undefined;
}
Expand Down Expand Up @@ -340,7 +326,7 @@ export class CipherView implements View, InitializerMetadata {
cipherView.deletedDate = obj.deletedDate == null ? undefined : new Date(obj.deletedDate);
cipherView.archivedDate = obj.archivedDate == null ? undefined : new Date(obj.archivedDate);
cipherView.reprompt = obj.reprompt ?? CipherRepromptType.None;
cipherView.key = obj.key ? EncString.fromJSON(obj.key) : undefined;
cipherView.key = obj.key ? SymmetricCryptoKey.fromString(obj.key) : undefined;

switch (obj.type) {
case CipherType.Card:
Expand All @@ -353,19 +339,6 @@ export class CipherView implements View, InitializerMetadata {
break;
case CipherType.Login:
cipherView.login = obj.login ? LoginView.fromSdkLoginView(obj.login) : new LoginView();
if (sdk && obj.login?.fido2Credentials?.length) {
const fido2CredentialViews = sdk.decrypt_fido2_credentials(obj);
const decryptedKeyValue = sdk.decrypt_fido2_private_key(obj);
cipherView.login.fido2Credentials = fido2CredentialViews
.map((cred) => {
const view = Fido2CredentialView.fromSdkFido2CredentialView(cred);
if (view) {
view.keyValue = decryptedKeyValue;
}
return view;
})
.filter((cred): cred is Fido2CredentialView => !!cred);
}
break;
case CipherType.SecureNote:
cipherView.secureNote = obj.secureNote
Expand Down
Loading
Loading