Skip to content
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

[PM-7486] Detect Libsecret Service #8776

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
600 changes: 584 additions & 16 deletions apps/desktop/desktop_native/Cargo.lock

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions apps/desktop/desktop_native/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,4 @@ security-framework-sys = "=2.9.1"
[target.'cfg(target_os = "linux")'.dependencies]
gio = "=0.19.2"
libsecret = "=0.5.0"
zbus = "=4.1.2"
1 change: 1 addition & 0 deletions apps/desktop/desktop_native/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export namespace passwords {
export function setPassword(service: string, account: string, password: string): Promise<void>
/** Delete the stored password from the keychain. */
export function deletePassword(service: string, account: string): Promise<void>
export function osSupport(): boolean
}
export namespace biometrics {
export function prompt(hwnd: Buffer, message: string): Promise<boolean>
Expand Down
5 changes: 5 additions & 0 deletions apps/desktop/desktop_native/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ pub mod passwords {
super::password::delete_password(&service, &account)
.map_err(|e| napi::Error::from_reason(e.to_string()))
}

#[napi]
pub fn os_support() -> bool {
super::password::has_password_management()
}
}

#[napi]
Expand Down
4 changes: 4 additions & 0 deletions apps/desktop/desktop_native/src/password/macos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ pub fn get_password(service: &str, account: &str) -> Result<String> {
Ok(result)
}

pub fn has_password_management() -> bool {
true
}

pub fn get_password_keytar(service: &str, account: &str) -> Result<String> {
get_password(service, account)
}
Expand Down
29 changes: 28 additions & 1 deletion apps/desktop/desktop_native/src/password/unix.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use anyhow::{anyhow, Result};
use libsecret::{password_clear_sync, password_lookup_sync, password_store_sync, Schema};
use std::collections::HashMap;
use std::{collections::HashMap, time::Duration};

pub fn get_password(service: &str, account: &str) -> Result<String> {
let res = password_lookup_sync(
Expand All @@ -15,6 +15,33 @@ pub fn get_password(service: &str, account: &str) -> Result<String> {
}
}

#[zbus::proxy(
interface = "org.freedesktop.DBus",
default_service = "org.freedesktop.DBus",
default_path = "/org/freedesktop/DBus"
)]
trait ListNames {
fn list_names(&self) -> Result<Vec<String>>;
}

// Query dbus names looking for the org.freedesktop.secrets service, which indicates an active libsecret provider
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see any risk by simply querying dbus names and searching for org.freedesktop.secrets which is similar to performing the following:

dbus-send --session --dest=org.freedesktop.DBus --type=method_call --print-reply /org/freedesktop/DBus org.freedesktop.DBus.ListNames | grep 'org.freedesktop.secrets'

Result
string "org.freedesktop.secrets"

However, what we do after the query matters, so I reviewed the zbus project and tested the logic in your code, which produces the expected results (you know what you're doing and the approach is good).

I would note that the zbus project doesn't have any information regarding the handling of security vulnerabilities in SECURITY.md at https://github.com/dbus2/zbus/security

And since we're using zbus API to essentially broker the validation of libsecret provider on a host where we need to securely store secrets, we want to make sure that the maintainer is security conscious.

Copy link
Contributor

@bwdil bwdil Apr 18, 2024

Choose a reason for hiding this comment

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

I'd note too that zbus api does not provide any assurances on what comes back from the message bus, so I think we are just trusting that org.freedesktop.secrets belongs to org.freedesktop.DBus if this is the only gate before we hand deliver secrets, then I would be concerned that using a string value from the message bus could be spoofed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with what you're saying except for

trusting that org.freedesktop.secrets belongs to org.freedesktop.DBus

We actually have no expectation that the DBus service has anything to do with what is providing the secrets service. What we're trusting is that the machine we're running on does not have a bad actor running a service as a libsecret provider.

At that point, though, we're just taking issue with the way an OS's secure storage works -- which is fine and we've done before -- but the alternative in this case is to store the access tokens and refresh tokens in plaintext to a well-known location on disk. Without handling some form of PAM listening service ourselves I don't currently know a better way to do it. Even then, validating the receiving end of IPC is roughly impossible without a trusted intermediary hosting/validating both services.

pub fn has_password_management() -> bool {
let conn = match zbus::blocking::Connection::session() {
Ok(conn) => conn,
Err(_) => return false,
};
let proxy = match ListNamesProxyBlocking::new(&conn) {
Ok(proxy) => proxy,
Err(_) => return false,
};
let names = match proxy.list_names() {
Ok(names) => names,
Err(_) => return false,
};

names.contains(&"org.freedesktop.secrets".to_string())
}

pub fn get_password_keytar(service: &str, account: &str) -> Result<String> {
get_password(service, account)
}
Expand Down
4 changes: 4 additions & 0 deletions apps/desktop/desktop_native/src/password/windows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ pub fn get_password<'a>(service: &str, account: &str) -> Result<String> {
Ok(String::from(password))
}

pub fn has_password_management() -> bool {
true
}

// Remove this after sufficient releases
pub fn get_password_keytar<'a>(service: &str, account: &str) -> Result<String> {
let target_name = U16CString::from_str(target_name(service, account))?;
Expand Down
15 changes: 9 additions & 6 deletions apps/desktop/src/app/services/services.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@
LogService as LogServiceAbstraction,
} from "@bitwarden/common/platform/abstractions/log.service";
import { MessagingService as MessagingServiceAbstraction } from "@bitwarden/common/platform/abstractions/messaging.service";
import { PlatformUtilsService as PlatformUtilsServiceAbstraction } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import {

Check warning on line 40 in apps/desktop/src/app/services/services.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/app/services/services.module.ts#L40

Added line #L40 was not covered by tests
PlatformUtilsService,
PlatformUtilsService as PlatformUtilsServiceAbstraction,
} from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { StateService as StateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service";
import { AbstractStorageService } from "@bitwarden/common/platform/abstractions/storage.service";
import { SystemService as SystemServiceAbstraction } from "@bitwarden/common/platform/abstractions/system.service";
Expand All @@ -63,10 +66,7 @@
import { DesktopSettingsService } from "../../platform/services/desktop-settings.service";
import { ElectronCryptoService } from "../../platform/services/electron-crypto.service";
import { ElectronLogRendererService } from "../../platform/services/electron-log.renderer.service";
import {
ELECTRON_SUPPORTS_SECURE_STORAGE,
ElectronPlatformUtilsService,
} from "../../platform/services/electron-platform-utils.service";
import { ElectronPlatformUtilsService } from "../../platform/services/electron-platform-utils.service";

Check warning on line 69 in apps/desktop/src/app/services/services.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/app/services/services.module.ts#L69

Added line #L69 was not covered by tests
import { ElectronRendererMessageSender } from "../../platform/services/electron-renderer-message.sender";
import { ElectronRendererSecureStorageService } from "../../platform/services/electron-renderer-secure-storage.service";
import { ElectronRendererStorageService } from "../../platform/services/electron-renderer-storage.service";
Expand Down Expand Up @@ -135,7 +135,10 @@
// the TokenService having to inject the PlatformUtilsService which introduces a
// circular dependency on Desktop only.
provide: SUPPORTS_SECURE_STORAGE,
useValue: ELECTRON_SUPPORTS_SECURE_STORAGE,
useFactory: (platformUtilsService: PlatformUtilsService) => {
return platformUtilsService.supportsSecureStorage();

Check warning on line 139 in apps/desktop/src/app/services/services.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/app/services/services.module.ts#L139

Added line #L139 was not covered by tests
},
deps: [PlatformUtilsServiceAbstraction],
}),
safeProvider({
provide: I18nServiceAbstraction,
Expand Down
4 changes: 2 additions & 2 deletions apps/desktop/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import { StateEventRegistrarService } from "@bitwarden/common/platform/state/state-event-registrar.service";
import { MemoryStorageService as MemoryStorageServiceForStateProviders } from "@bitwarden/common/platform/state/storage/memory-storage.service";
/* eslint-enable import/no-restricted-paths */
import { passwords } from "@bitwarden/desktop-native";

Check warning on line 34 in apps/desktop/src/main.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/main.ts#L34

Added line #L34 was not covered by tests

import { DesktopAutofillSettingsService } from "./autofill/services/desktop-autofill-settings.service";
import { MenuMain } from "./main/menu/menu.main";
Expand All @@ -47,7 +48,6 @@
import { MainCryptoFunctionService } from "./platform/main/main-crypto-function.service";
import { DesktopSettingsService } from "./platform/services/desktop-settings.service";
import { ElectronLogMainService } from "./platform/services/electron-log.main.service";
import { ELECTRON_SUPPORTS_SECURE_STORAGE } from "./platform/services/electron-platform-utils.service";
import { ElectronStateService } from "./platform/services/electron-state.service";
import { ElectronStorageService } from "./platform/services/electron-storage.service";
import { I18nMainService } from "./platform/services/i18n.main.service";
Expand Down Expand Up @@ -179,7 +179,7 @@
this.tokenService = new TokenService(
singleUserStateProvider,
globalStateProvider,
ELECTRON_SUPPORTS_SECURE_STORAGE,
passwords.osSupport(),
illegalSecureStorageService,
this.keyGenerationService,
this.encryptService,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
) {}

init() {
// Handle password management detection sync
ipcMain.on("osSupportsPasswords", (event) => {
event.returnValue = passwords.osSupport();

Check warning on line 23 in apps/desktop/src/platform/main/desktop-credential-storage-listener.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/main/desktop-credential-storage-listener.ts#L22-L23

Added lines #L22 - L23 were not covered by tests
});
ipcMain.handle("keytar", async (event: any, message: any) => {
try {
let serviceName = this.serviceName;
Expand Down
1 change: 1 addition & 0 deletions apps/desktop/src/platform/preload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
ipcRenderer.invoke("keytar", { action: "setPassword", key, keySuffix, value }),
delete: (key: string, keySuffix: string): Promise<void> =>
ipcRenderer.invoke("keytar", { action: "deletePassword", key, keySuffix }),
osSupport: (): boolean => ipcRenderer.sendSync("osSupportsPasswords"),

Check warning on line 37 in apps/desktop/src/platform/preload.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/preload.ts#L37

Added line #L37 was not covered by tests
};

const biometric = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@

import { ClipboardWriteMessage } from "../types/clipboard";

export const ELECTRON_SUPPORTS_SECURE_STORAGE = true;

export class ElectronPlatformUtilsService implements PlatformUtilsService {
constructor(
protected i18nService: I18nService,
Expand Down Expand Up @@ -144,7 +142,7 @@
}

supportsSecureStorage(): boolean {
return ELECTRON_SUPPORTS_SECURE_STORAGE;
return ipc.platform.passwords.osSupport();

Check warning on line 145 in apps/desktop/src/platform/services/electron-platform-utils.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/electron-platform-utils.service.ts#L145

Added line #L145 was not covered by tests
}

getAutofillKeyboardShortcut(): Promise<string> {
Expand Down