Skip to content

Commit 63a0d8c

Browse files
authored
fix(pedm): add additional context to virtual account code paths (#1409)
Some users are experiencing an error with the virtual account elevator that I cannot reproduce, and we do not have the necessary contextual information. This PR adds additional context to the virtual account elevator code paths.
1 parent 4295a41 commit 63a0d8c

4 files changed

Lines changed: 38 additions & 12 deletions

File tree

crates/devolutions-pedm/src/elevator/virtual_account_elevator.rs

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ use win_api_wrappers::Error;
2626

2727
use super::Elevator;
2828

29+
use anyhow::Context as _;
30+
2931
struct VirtualAccountElevation {
3032
_base_token: Token,
3133
elevated_token: Token,
@@ -56,7 +58,7 @@ impl VirtualAccountElevator {
5658
fn create_token(&self, token: &Token) -> anyhow::Result<()> {
5759
let domain = U16CString::from_str(&self.domain)?;
5860

59-
let virtual_account = create_virtual_account(self.rid, &domain, token)?;
61+
let virtual_account = create_virtual_account(self.rid, &domain, token).context("create virtual account")?;
6062

6163
let mut groups = TokenGroups::new(
6264
// Needed for the virtual account to access the user's desktop and window station.
@@ -93,9 +95,12 @@ impl VirtualAccountElevator {
9395
LOGON32_LOGON_INTERACTIVE,
9496
LOGON32_PROVIDER_VIRTUAL,
9597
Some(&groups),
96-
)?;
98+
)
99+
.context("logon virtual account")?;
97100

98-
let profile = base_token.load_profile(virtual_account.name)?;
101+
let profile = base_token
102+
.load_profile(virtual_account.name)
103+
.context("load virtual account profile")?;
99104

100105
let elevated_token = base_token.linked_token()?;
101106

@@ -120,17 +125,22 @@ impl Elevator for VirtualAccountElevator {
120125
let sid = token.sid_and_attributes()?.sid;
121126

122127
if !self.tokens.read().contains_key(&sid) {
123-
self.create_token(token)?;
128+
self.create_token(token).context("create virtual account token")?;
124129
}
125130

126131
self.tokens
127132
.read()
128133
.get(&sid)
129134
.ok_or_else(|| anyhow::anyhow!(Error::from_win32(ERROR_ACCOUNT_EXPIRED)))
130135
.and_then(|vtoken| {
131-
let mut vtoken = vtoken.elevated_token.duplicate_impersonation()?;
132-
133-
vtoken.set_session_id(token.session_id()?)?;
136+
let mut vtoken = vtoken
137+
.elevated_token
138+
.duplicate_impersonation()
139+
.context("duplicate virtual elevated token")?;
140+
141+
vtoken
142+
.set_session_id(token.session_id()?)
143+
.context("set virtual token session id")?;
134144

135145
Ok(vtoken)
136146
})

crates/win-api-wrappers/src/identity/account.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,10 @@ pub fn create_profile(account_sid: &Sid, account_name: &U16CStr) -> anyhow::Resu
242242
account_string_sid.as_u16cstr().as_pcwstr(),
243243
account_name.as_pcwstr(),
244244
profile_path.as_mut_slice(),
245-
)?
245+
)
246+
.with_context(|| {
247+
format!("CreateProfile failed (account_string_sid: {account_string_sid:?}, account_name: {account_name:?}, profile_path: {profile_path:?}")
248+
})?
246249
};
247250

248251
Ok(U16CString::from_vec_truncate(profile_path))
@@ -269,7 +272,10 @@ impl ProfileInfo {
269272
profile_info.raw.lpUserName = profile_info.username.as_pwstr();
270273

271274
// SAFETY: Only prerequisite is for `profile_info`'s `dwSize` member to be set correctly, which it is.
272-
unsafe { LoadUserProfileW(profile_info.token.handle().raw(), &mut profile_info.raw) }?;
275+
unsafe {
276+
LoadUserProfileW(profile_info.token.handle().raw(), &mut profile_info.raw)
277+
.with_context(|| format!("LoadUserProfileW failed (username: {:?})", profile_info.username))?;
278+
};
273279

274280
Ok(profile_info)
275281
}

crates/win-api-wrappers/src/token.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,8 @@ impl Token {
350350
info_class,
351351
value as *const _ as *const _,
352352
u32size_of::<T>(),
353-
)?;
353+
)
354+
.context("SetTokenInformation")?;
354355
}
355356

356357
Ok(())
@@ -441,7 +442,10 @@ impl Token {
441442
None,
442443
None,
443444
)
444-
}?;
445+
.with_context(|| {
446+
format!("LogonUserExExW failed (username: {username:?}, domain: {domain:?}, logon_type: {logon_type:?}, logon_provider: {logon_provider:?}, groups: {groups:?})")
447+
})?
448+
}
445449

446450
// SAFETY: We own the handle.
447451
let handle = unsafe { Handle::new_owned(raw_token)? };

crates/win-api-wrappers/src/token_groups.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::mem;
1+
use std::{fmt, mem};
22

33
use windows::Win32::Security;
44

@@ -86,6 +86,12 @@ impl TokenGroups {
8686
}
8787
}
8888

89+
impl fmt::Debug for TokenGroups {
90+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
91+
f.debug_struct("TokenGroups").field("sids", &self.sids).finish()
92+
}
93+
}
94+
8995
type RawTokenGroups = Win32Dst<TokenGroupsDstDef>;
9096

9197
struct TokenGroupsDstDef;

0 commit comments

Comments
 (0)