Skip to content

[Security] ServiceDtoHelper.ApplyDefaults silently clobbers RunAsLocalSystem/UserAccount/Password — XML/JSON imports always force LocalSystem #930

@Christophe-Rogiers

Description

@Christophe-Rogiers

Severity: Warning

File: src/Servy.Core/Helpers/ServiceDtoHelper.cs (lines 27-29)

Code:

public static void ApplyDefaults(ServiceDto? dto)
{
    if (dto == null) return;

    // Identity & Behavior
    dto.StartupType = dto.StartupType ?? (int)AppConfig.DefaultStartupType;
    dto.Priority = dto.Priority ?? (int)AppConfig.DefaultPriority;
    dto.RunAsLocalSystem = AppConfig.DefaultRunAsLocalSystem;   // <-- UNCONDITIONAL
    dto.UserAccount = null;                                       // <-- UNCONDITIONAL
    dto.Password = null;                                          // <-- UNCONDITIONAL
    dto.EnableDebugLogs = dto.EnableDebugLogs ?? AppConfig.DefaultEnableDebugLogs;
    ...
}

Explanation:
Note the asymmetry: every other field uses ?? to apply a default only when null. The three identity-related fields use plain = and clobber whatever the caller had loaded:

  • RunAsLocalSystem is unconditionally set to AppConfig.DefaultRunAsLocalSystem (which is true).
  • UserAccount is unconditionally set to null.
  • Password is unconditionally set to null.

This is called during XML/JSON deserialization (per the class XML doc — "populated during the deserialization of XML and JSON configurations"). If a user-supplied <service> element has:

<RunAsLocalSystem>false</RunAsLocalSystem>
<UserAccount>DOMAIN\svc_account</UserAccount>

then after ApplyDefaults, the DTO carries RunAsLocalSystem=true and UserAccount=null — silently switching the imported service to LocalSystem with no warning to the operator. The original imported values are gone before any validator can complain.

There are two reasonable design intents that could've been hidden behind this:

  1. Password security: don't carry plaintext passwords across config files. That justifies clobbering Password, but NOT RunAsLocalSystem/UserAccount.
  2. First-use bootstrap: only true for new services, not for imports. The current code can't tell the difference.

Suggested fix:

  • Keep Password = null (defensible: don't accept passwords in plain config).
  • Use ?? for RunAsLocalSystem and UserAccount like every other field:
    dto.RunAsLocalSystem = dto.RunAsLocalSystem ?? AppConfig.DefaultRunAsLocalSystem;
    dto.UserAccount = dto.UserAccount;  // (no-op; remove this line)
    dto.Password = null;                 // intentionally reset
  • After import, surface a clear warning to the user via the existing message-box service: "Imported service is configured to run as 'X' — please re-enter the password before installing."

This change preserves the explicit "no plaintext credentials" posture while not silently degrading service identity to LocalSystem.

Metadata

Metadata

Assignees

Labels

discussRequires discussion before implementation or decision

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions