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-15934] Add agent-forwarding detection and git signing detection parsers #12371

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
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
3 changes: 2 additions & 1 deletion apps/desktop/desktop_native/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 24 additions & 23 deletions apps/desktop/desktop_native/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,20 @@ default = ["sys"]
manual_test = []

sys = [
"dep:widestring",
"dep:windows",
"dep:core-foundation",
"dep:security-framework",
"dep:security-framework-sys",
"dep:zbus",
"dep:zbus_polkit",
"dep:widestring",
"dep:windows",
"dep:core-foundation",
"dep:security-framework",
"dep:security-framework-sys",
"dep:zbus",
"dep:zbus_polkit",
]

[dependencies]
aes = "=0.8.4"
anyhow = "=1.0.94"
arboard = { version = "=3.4.1", default-features = false, features = [
"wayland-data-control",
"wayland-data-control",
] }
argon2 = { version = "=0.5.3", features = ["zeroize"] }
base64 = "=0.22.1"
Expand All @@ -42,12 +42,12 @@ scopeguard = "=1.2.0"
sha2 = "=0.10.8"
ssh-encoding = "=0.2.0"
ssh-key = { version = "=0.6.7", default-features = false, features = [
"encryption",
"ed25519",
"rsa",
"getrandom",
"encryption",
"ed25519",
"rsa",
"getrandom",
] }
bitwarden-russh = { git = "https://github.com/bitwarden/bitwarden-russh.git", rev = "23b50e3bbe6d56ef19ab0e98e8bb1462cb6d77ae" }
bitwarden-russh = { git = "https://github.com/bitwarden/bitwarden-russh.git", rev = "3d48f140fd506412d186203238993163a8c4e536" }
tokio = { version = "=1.41.1", features = ["io-util", "sync", "macros", "net"] }
tokio-stream = { version = "=0.1.15", features = ["net"] }
tokio-util = { version = "=0.7.12", features = ["codec"] }
Expand All @@ -57,20 +57,21 @@ pkcs8 = { version = "=0.10.2", features = ["alloc", "encryption", "pem"] }
rsa = "=0.9.6"
ed25519 = { version = "=2.2.3", features = ["pkcs8"] }
sysinfo = { version = "0.32.0", features = ["windows"] }
bytes = "1.9.0"

[target.'cfg(windows)'.dependencies]
widestring = { version = "=1.1.0", optional = true }
windows = { version = "=0.58.0", features = [
"Foundation",
"Security_Credentials_UI",
"Security_Cryptography",
"Storage_Streams",
"Win32_Foundation",
"Win32_Security_Credentials",
"Win32_System_WinRT",
"Win32_UI_Input_KeyboardAndMouse",
"Win32_UI_WindowsAndMessaging",
"Win32_System_Pipes",
"Foundation",
"Security_Credentials_UI",
"Security_Cryptography",
"Storage_Streams",
"Win32_Foundation",
"Win32_Security_Credentials",
"Win32_System_WinRT",
"Win32_UI_Input_KeyboardAndMouse",
"Win32_UI_WindowsAndMessaging",
"Win32_System_Pipes",
], optional = true }

[target.'cfg(windows)'.dev-dependencies]
Expand Down
41 changes: 38 additions & 3 deletions apps/desktop/desktop_native/core/src/ssh_agent/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ mod peercred_unix_listener_stream;

pub mod importer;
pub mod peerinfo;
mod request_parser;

#[derive(Clone)]
pub struct BitwardenDesktopAgent {
keystore: ssh_agent::KeyStore,
Expand All @@ -35,19 +37,37 @@ pub struct SshAgentUIRequest {
pub cipher_id: Option<String>,
pub process_name: String,
pub is_list: bool,
pub namespace: Option<String>,
pub is_forwarding: bool,
}

impl ssh_agent::Agent<peerinfo::models::PeerInfo> for BitwardenDesktopAgent {
async fn confirm(&self, ssh_key: Key, info: &peerinfo::models::PeerInfo) -> bool {
async fn confirm(&self, ssh_key: Key, data: &[u8], info: &peerinfo::models::PeerInfo) -> bool {
if !self.is_running() {
println!("[BitwardenDesktopAgent] Agent is not running, but tried to call confirm");
return false;
}

let request_id = self.get_request_id().await;
let request_data = match request_parser::parse_request(data) {
Ok(data) => data,
Err(e) => {
println!("[SSH Agent] Error while parsing request: {}", e);
return false;
}
};
let namespace = match request_data {
request_parser::SshAgentSignRequest::SshSigRequest(ref req) => {
Some(req.namespace.clone())
}
_ => None,
};

println!(
"[SSH Agent] Confirming request from application: {}",
info.process_name()
"[SSH Agent] Confirming request from application: {}, is_forwarding: {}, namespace: {}",
info.process_name(),
info.is_forwarding(),
namespace.clone().unwrap_or_default(),
);

let mut rx_channel = self.get_ui_response_rx.lock().await.resubscribe();
Expand All @@ -57,6 +77,8 @@ impl ssh_agent::Agent<peerinfo::models::PeerInfo> for BitwardenDesktopAgent {
cipher_id: Some(ssh_key.cipher_uuid.clone()),
process_name: info.process_name().to_string(),
is_list: false,
namespace,
is_forwarding: info.is_forwarding(),
})
.await
.expect("Should send request to ui");
Expand All @@ -81,6 +103,8 @@ impl ssh_agent::Agent<peerinfo::models::PeerInfo> for BitwardenDesktopAgent {
cipher_id: None,
process_name: info.process_name().to_string(),
is_list: true,
namespace: None,
is_forwarding: info.is_forwarding(),
};
self.show_ui_request_tx
.send(message)
Expand All @@ -93,6 +117,17 @@ impl ssh_agent::Agent<peerinfo::models::PeerInfo> for BitwardenDesktopAgent {
}
false
}

async fn set_is_forwarding(
&self,
is_forwarding: bool,
connection_info: &peerinfo::models::PeerInfo,
) {
// is_forwarding can only be added but never removed from a connection
if is_forwarding {
connection_info.set_forwarding(is_forwarding);
}
}
}

impl BitwardenDesktopAgent {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,7 @@ impl Stream for PeercredUnixListenerStream {
))));
}
},
Err(err) => {
return Poll::Ready(Some(Err(io::Error::new(
io::ErrorKind::Other,
format!("Failed to get peer credentials: {}", err),
))));
}
Err(_) => return Poll::Ready(Some(Ok((stream, PeerInfo::unknown())))),
};
let peer_info = peerinfo::gather::get_peer_info(pid as u32);
match peer_info {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::sync::{atomic::AtomicBool, Arc};

/**
* Peerinfo represents the information of a peer process connecting over a socket.
* This can be later extended to include more information (icon, app name) for the corresponding application.
Expand All @@ -7,6 +9,7 @@ pub struct PeerInfo {
uid: u32,
pid: u32,
process_name: String,
is_forwarding: Arc<AtomicBool>,
}

impl PeerInfo {
Expand All @@ -15,6 +18,16 @@ impl PeerInfo {
uid,
pid,
process_name,
is_forwarding: Arc::new(AtomicBool::new(false)),
}
}

pub fn unknown() -> Self {
Self {
uid: 0,
pid: 0,
process_name: "Unknown".to_string(),
is_forwarding: Arc::new(AtomicBool::new(false)),
}
}

Expand All @@ -29,4 +42,14 @@ impl PeerInfo {
pub fn process_name(&self) -> &str {
&self.process_name
}

pub fn is_forwarding(&self) -> bool {
self.is_forwarding
.load(std::sync::atomic::Ordering::Relaxed)
}

pub fn set_forwarding(&self, value: bool) {
self.is_forwarding
.store(value, std::sync::atomic::Ordering::Relaxed);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
use bytes::{Buf, Bytes};

#[derive(Debug)]
pub(crate) struct SshSigRequest {
pub namespace: String,
}

#[derive(Debug)]
pub(crate) struct SignRequest {}

#[derive(Debug)]
pub(crate) enum SshAgentSignRequest {
SshSigRequest(SshSigRequest),
SignRequest(SignRequest),
}

pub(crate) fn parse_request(data: &[u8]) -> Result<SshAgentSignRequest, anyhow::Error> {
let mut data = Bytes::copy_from_slice(data);
let magic_header = "SSHSIG";
let header = data.split_to(magic_header.len());

// sshsig; based on https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.sshsig
if header == magic_header.as_bytes() {
let _version = data.get_u32();

// read until null byte
let namespace = data
.into_iter()
.take_while(|&x| x != 0)
.collect::<Vec<u8>>();
let namespace =
String::from_utf8(namespace).map_err(|_| anyhow::anyhow!("Invalid namespace"))?;

Ok(SshAgentSignRequest::SshSigRequest(SshSigRequest {
namespace,
}))
} else {
// regular sign request
Ok(SshAgentSignRequest::SignRequest(SignRequest {}))
}
}
9 changes: 8 additions & 1 deletion apps/desktop/desktop_native/napi/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,14 @@ export declare namespace sshagent {
status: SshKeyImportStatus
sshKey?: SshKey
}
export function serve(callback: (err: Error | null, arg0: string | undefined | null, arg1: boolean, arg2: string) => any): Promise<SshAgentState>
export interface SshUiRequest {
cipherId?: string
isList: boolean
processName: string
isForwarding: boolean
namespace?: string
}
export function serve(callback: (err: Error | null, arg: SshUiRequest) => any): Promise<SshAgentState>
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Looks better as a separate struct this way, at least we don't get arg0, arg1, arg2 parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, yeah. Meant to do it sometime either-way, but in this case clippy complained.

export function stop(agentState: SshAgentState): void
export function isRunning(agentState: SshAgentState): boolean
export function setKeys(agentState: SshAgentState, newKeys: Array<PrivateKey>): void
Expand Down
23 changes: 17 additions & 6 deletions apps/desktop/desktop_native/napi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,18 @@ pub mod sshagent {
}
}

#[napi(object)]
pub struct SshUIRequest {
pub cipher_id: Option<String>,
pub is_list: bool,
pub process_name: String,
pub is_forwarding: bool,
pub namespace: Option<String>,
}

#[napi]
pub async fn serve(
callback: ThreadsafeFunction<(Option<String>, bool, String), CalleeHandled>,
callback: ThreadsafeFunction<SshUIRequest, CalleeHandled>,
) -> napi::Result<SshAgentState> {
let (auth_request_tx, mut auth_request_rx) =
tokio::sync::mpsc::channel::<desktop_core::ssh_agent::SshAgentUIRequest>(32);
Expand All @@ -262,11 +271,13 @@ pub mod sshagent {
let auth_response_tx_arc = cloned_response_tx_arc;
let callback = cloned_callback;
let promise_result: Result<Promise<bool>, napi::Error> = callback
.call_async(Ok((
request.cipher_id,
request.is_list,
request.process_name,
)))
.call_async(Ok(SshUIRequest {
cipher_id: request.cipher_id,
is_list: request.is_list,
process_name: request.process_name,
is_forwarding: request.is_forwarding,
namespace: request.namespace,
}))
.await;
match promise_result {
Ok(promise_result) => match promise_result.await {
Expand Down
18 changes: 18 additions & 0 deletions apps/desktop/src/locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3409,9 +3409,27 @@
"sshkeyApprovalTitle": {
"message": "Confirm SSH key usage"
},
"agentForwardingWarningTitle": {
"message": "Warning: Agent Forwarding"
},
"agentForwardingWarningText": {
"message": "This request comes from a remote device that you are logged into"
},
"sshkeyApprovalMessageInfix": {
"message": "is requesting access to"
},
"sshkeyApprovalMessageSuffix": {
"message": "in order to"
},
"sshActionLogin": {
"message": "authenticate to a server"
},
"sshActionSign": {
"message": "sign a message"
},
"sshActionGitSign": {
"message": "sign a git commit"
},
"unknownApplication": {
"message": "An application"
},
Expand Down
11 changes: 10 additions & 1 deletion apps/desktop/src/platform/components/approve-ssh-request.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,17 @@
<bit-dialog>
<div class="tw-font-semibold" bitDialogTitle>{{ "sshkeyApprovalTitle" | i18n }}</div>
<div bitDialogContent>
<app-callout
type="warning"
title="{{ 'agentForwardingWarningTitle' | i18n }}"
*ngIf="params.isAgentForwarding"
>
{{ 'agentForwardingWarningText' | i18n }}
</app-callout>

<b>{{params.applicationName}}</b> {{ "sshkeyApprovalMessageInfix" | i18n }}
<b>{{params.cipherName}}</b>.
<b>{{params.cipherName}}</b>
{{ "sshkeyApprovalMessageSuffix" | i18n }} {{ params.action | i18n }}
</div>
<div bitDialogFooter>
<button type="submit" bitButton bitFormButton buttonType="primary">
Expand Down
Loading
Loading