Skip to content

Harden Windows memory locking: fix ERROR_WORKING_SET_QUOTA and edge cases - #661

Merged
Eugeny merged 2 commits into
Eugeny:mainfrom
coreyleavitt:fix/windows-mlock
Mar 18, 2026
Merged

Harden Windows memory locking: fix ERROR_WORKING_SET_QUOTA and edge cases#661
Eugeny merged 2 commits into
Eugeny:mainfrom
coreyleavitt:fix/windows-mlock

Conversation

@coreyleavitt

@coreyleavitt coreyleavitt commented Mar 17, 2026

Copy link
Copy Markdown
Contributor

Problem

On Windows, VirtualLock fails with ERROR_WORKING_SET_QUOTA (0x5ad) on default system configurations because the process minimum working set (~200KB) leaves no headroom for locked pages. This causes the security warning:

Security warning: OS has failed to lock/unlock memory for a cryptographic buffer: VirtualLock: 0x5ad

While investigating this, I also noticed a few edge cases in the existing implementation that could cause issues in certain scenarios.

Reported in #504.

Changes

All changes are in cryptovec/src/platform/windows.rs and cryptovec/Cargo.toml. The public API (mlock/munlock signatures) is unchanged.

Working set growth on demand (fixes #504)

When VirtualLock fails with ERROR_WORKING_SET_QUOTA, lock_page now grows the process working set by one page via SetProcessWorkingSetSizeEx and retries once. This is the pattern recommended by Microsoft's VirtualLock documentation:

Applications that need to lock larger numbers of pages must first call the SetProcessWorkingSetSize function to increase their minimum and maximum working set sizes.

Working set flags from GetProcessWorkingSetSizeEx are masked to documented QUOTA_LIMITS_HARDWS_* bits before passing back to avoid forwarding undocumented bits from future Windows versions.

Partial mlock rollback

If mlock is locking pages A, B, C and page C fails, the previous implementation left pages A and B locked with orphaned refcounts. Now both newly locked pages and refcount increments on already-locked pages are tracked and rolled back on failure.

Locked page cap

Total locked pages are capped at 256 (~1 MiB on 4 KiB page systems) to prevent unbounded working set growth.

Edge case fixes

  • Page range overflow: get_page_range now uses last-byte address calculation instead of len + page_size - 1, which could overflow when len is near usize::MAX.
  • Address overflow: Page address computed via checked_mul instead of bare * to prevent silent wraparound on 32-bit targets.
  • VirtualLock size: Changed from 1 byte (relies on OS rounding up) to page_size for explicit page-granularity semantics.
  • SYSTEM_INFO init: Changed from Default to mem::zeroed() for guaranteed zero-initialization.

Dependency updates

Tests

Added unit tests for page range arithmetic (cross-platform) and mlock/munlock integration (runs on the existing Windows CI): roundtrip locking, refcount overlap handling, zero-length no-op, multiple distinct buffers exercising working set growth, and munlock-without-mlock error path.

Other improvements

  • log::debug! on working set growth events.
  • Module-level and per-function documentation.
  • Error messages include symbolic names alongside hex codes.
  • Unsafe blocks scoped to individual FFI calls.

Relationship to #653

Builds on #653, which reduced mlock to only the buffers that hold secret material. This PR hardens those remaining mlock calls on Windows so they succeed on default system configurations and handle edge cases correctly.

Fixes #504

@coreyleavitt coreyleavitt changed the title Rewrite Windows memory locking for correctness and robustness Harden Windows memory locking: fix ERROR_WORKING_SET_QUOTA and edge cases Mar 17, 2026
@coreyleavitt
coreyleavitt marked this pull request as ready for review March 18, 2026 01:26
Replace the minimal VirtualLock implementation with a hardened version
that addresses multiple issues:

- Grow working set on demand when VirtualLock fails with
  ERROR_WORKING_SET_QUOTA (0x5ad), then retry. This is the pattern
  recommended by Microsoft's VirtualLock documentation. Fixes Eugeny#504.

- Roll back partially locked pages on mlock failure, preventing
  leaked page locks and working set quota when a multi-page lock
  partially succeeds.

- Cap total locked pages at 256 (~1 MiB) to prevent unbounded
  working set growth from exhausting physical RAM.

- Use checked/saturating arithmetic for page address and working set
  calculations to prevent overflow on 32-bit targets.

- Fix get_page_indices overflow: use last-byte address instead of
  len + page_size - 1 which wraps when len is near usize::MAX.

- Pass page_size (not 1 byte) to VirtualLock/VirtualUnlock for
  correct and self-documenting page-granularity locking.

- Use mem::zeroed() for SYSTEM_INFO instead of Default which may
  not zero all fields.

- Mask SetProcessWorkingSetSizeEx flags to documented
  QUOTA_LIMITS_HARDWS_* bits to avoid forwarding undocumented bits.

- Switch from unmaintained winapi to Microsoft's windows-sys crate
  (already in the dependency tree via other crates).

- Drop libc dependency (memset was already replaced by zeroize).

- Add log::debug diagnostics for working set growth events.
@Eugeny

Eugeny commented Mar 18, 2026

Copy link
Copy Markdown
Owner

LGTM and thank you for the clean implementation 👍

@Eugeny

Eugeny commented Mar 18, 2026

Copy link
Copy Markdown
Owner

@all-contributors add @coreyleavitt for code

@Eugeny
Eugeny merged commit aa43795 into Eugeny:main Mar 18, 2026
@allcontributors

Copy link
Copy Markdown
Contributor

@Eugeny

I've put up a pull request to add @coreyleavitt! 🎉

zeroleo12345 added a commit to zeroleo12345/russh that referenced this pull request Mar 20, 2026
* Add Yazi terminal file manager to adopters (Eugeny#643)

Thank you for maintaining this awesome crate!

* Fix zlib vs zlib@openssh.com compression timing (Eugeny#564) (Eugeny#646)

## Summary

Fixes Eugeny#564.

The `Compression` enum previously mapped both `zlib` (RFC 4253) and
`zlib@openssh.com` to the same `Zlib` variant, losing the critical
timing distinction between the two algorithms:

- **`zlib`** (RFC 4253) - compression activates immediately after
`SSH_MSG_NEWKEYS` (initial key exchange)
- **`zlib@openssh.com`** - compression is deferred until after user
authentication succeeds

Because both were treated identically, russh only activated compression
after authentication, which broke interoperability with clients/servers
that negotiate plain `zlib` and expect compression to start right after
key exchange. This was originally reported by Simon Tatham (PuTTY
maintainer).

## What this fix does

- Adds a new `Compression::ZlibOpenSSH` variant to distinguish
`zlib@openssh.com` from plain `zlib`
- Adds an `is_deferred()` method that returns `true` only for
`ZlibOpenSSH`
- Guards all post-authentication compression activation points with
`is_deferred()` checks, so they only fire for the deferred variant
- Activates non-deferred (`zlib`) compression immediately in
`newkeys_received()`, right after key exchange completes

### Files changed

- `russh/src/compression.rs` - new `ZlibOpenSSH` variant, updated
`new()` constructor, `is_deferred()` method
- `russh/src/session.rs` - activate non-deferred compression at key
exchange time
- `russh/src/client/encrypted.rs` - guard deferred decompression init
with `is_deferred()`
- `russh/src/client/mod.rs` - guard deferred compression init with
`is_deferred()`
- `russh/src/server/encrypted.rs` - guard deferred compress/decompress
init with `is_deferred()`

## Test plan

- All existing tests pass, including the compression integration test
(`cargo test`)
- The fix is a minimal, targeted change that preserves existing
`zlib@openssh.com` behavior while correctly adding the RFC 4253 `zlib`
immediate-activation path

* add gui-wf as a contributor for code (Eugeny#647)

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

* add zeroleo12345 as a contributor for code (Eugeny#648)

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

* fix: do not send keepalive before authentication (Eugeny#642)

Co-authored-by: Eugene <inbox@null.page>

* v0.57.1

* CryptoVec: replace memset with zeroize in resize() method (Eugeny#634)

* chore: bump thiserror to latest version (Eugeny#651)

* perf: eliminate mlock/munlock overhead for non-secret buffers (~21% throughput improvement) (Eugeny#653)

Co-authored-by: Eugene <inbox@null.page>

* fix: use remote channel ID in CHANNEL_REQUEST replies (Eugeny#662)

* add mjc as a contributor for code (Eugeny#663)

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

* add Mota-Link as a contributor for code (Eugeny#664)

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Co-authored-by: Eugene <x@null.page>

* Expose HostConfig fields to external consumers (Eugeny#652)

* Add russh/serde feature to enable serde on russh::keys::PublicKey (Eugeny#655)

* Remove heap allocations from SshId (Eugeny#656)

Co-authored-by: Eugene <inbox@null.page>

* add kpcyrd as a contributor for code (Eugeny#668)

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

* add fbernier as a contributor for code (Eugeny#665)

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Co-authored-by: Eugene <x@null.page>

* fix: accept full 256k channel packets (Eugeny#666)

* Remove Home Crate Dependency (Eugeny#667)

* fixed Eugeny#658 - make `Handle::tcpip_forward` and `Handle::streamlocal_forward` take `&self`

* Harden Windows memory locking: fix ERROR_WORKING_SET_QUOTA and edge cases (Eugeny#661)

Co-authored-by: Eugene <inbox@null.page>

* add coreyleavitt as a contributor for code (Eugeny#669)

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

* v0.58.0

---------

Co-authored-by: 三咲雅 misaki masa <sxyazi@gmail.com>
Co-authored-by: Guilherme Fontes <48162143+gui-wf@users.noreply.github.com>
Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Co-authored-by: Eugene <inbox@null.page>
Co-authored-by: Eric Rodrigues Pires <eric@eric.dev.br>
Co-authored-by: Roger Knecht <155455820+rogkne@users.noreply.github.com>
Co-authored-by: Mika Cohen <mjc@kernel.org>
Co-authored-by: Mota-Link <83714159+Mota-Link@users.noreply.github.com>
Co-authored-by: Eugene <x@null.page>
Co-authored-by: François Bernier <frankbernier@gmail.com>
Co-authored-by: kpcyrd <git@rxv.cc>
Co-authored-by: Corey Leavitt <corey@knurl.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Log warning on windows

2 participants