CHORE: load bundled Windows driver and auth DLLs from package-local directories - #735
Open
Gaurav Sharma (bewithgaurav) wants to merge 7 commits into
Open
CHORE: load bundled Windows driver and auth DLLs from package-local directories#735Gaurav Sharma (bewithgaurav) wants to merge 7 commits into
Gaurav Sharma (bewithgaurav) wants to merge 7 commits into
Conversation
…irectories On Windows the vendored ODBC driver (msodbcsql18.dll) and Entra auth DLL (mssql-auth.dll) were loaded with LoadLibraryW, so their dependencies - including the bundled VC++ runtime - were resolved via the default process search order, which also consults the current working directory and %PATH%. Load them with LoadLibraryExW using LOAD_LIBRARY_SEARCH_DEFAULT_DIRS | LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR, and register the driver directory plus its co-located vcredist subfolder via AddDllDirectory, so dependency resolution is confined to trusted, package-local directories (System32, the application directory, and the package's own folders). The vcredist subfolder is added explicitly because LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR only covers each DLL's own directory. macOS and Linux are unaffected: the change is inside _WIN32 blocks, and the shipped Unix libraries already carry @loader_path / $ORIGIN. Adds a Windows-only regression test that plants a bogus msvcp140.dll on the CWD and %PATH% and confirms the driver still loads from the package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ibraryW Replace the success-path load smoke test with a source-contract regression guard. The previous test only confirmed the driver still loads, which also passes on the unhardened code (any host with msvcp140.dll in System32), so it guarded nothing. The new test fails if the loader reintroduces a bare LoadLibraryW call or drops the constrained-search flags / vcredist registration -- i.e. it fails on the pre-fix source and passes on the current source. Deterministic and platform-independent (no DLL, no live driver). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Gaurav Sharma (bewithgaurav)
marked this pull request as ready for review
September 1, 2026 08:46
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 1, 2026 08:46
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens Windows DLL loading for the bundled ODBC driver and Entra auth DLL by constraining dependency resolution to trusted, package-local directories, reducing exposure to CWD/%PATH% hijacking and nondeterministic loads.
Changes:
- Switch Windows loads from
LoadLibraryWtoLoadLibraryExWwith constrained search flags. - Register the driver directory and its
vcredistsubfolder as trusted DLL search directories (once per process). - Add a regression test (source-contract) and document the behavior in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
mssql_python/pybind/ddbc_bindings.cpp |
Uses LoadLibraryExW with constrained search flags and registers package-local DLL directories via AddDllDirectory. |
tests/test_026_windows_dll_search.py |
Adds a source-contract regression test to guard against reintroducing unconstrained LoadLibraryW usage. |
CHANGELOG.md |
Notes the Windows DLL search hardening behavior change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Drop the AddDllDirectory registration (and its once_flag / <mutex>): the bundled VC++ runtime is already loaded from the trusted copy next to the pybind .pyd (built /MD) before the driver loads, so registering the vcredist directory added a permanent process-global search-dir side effect for no resolution benefit. The per-load LoadLibraryExW flags (LOAD_LIBRARY_SEARCH_DEFAULT_DIRS | LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR) are the whole change: they keep the current directory and %PATH% out of the driver's and auth DLL's dependency search without mutating any global process state. Also revert the CHANGELOG entry and trim the regression test's assertions to match (no more AddDllDirectory / vcredist checks). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
pr-size: small
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 58.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 75.5%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.6%
mssql_python.pybind.connection.connection.cpp: 84.4%
mssql_python.logging.py: 85.5%
mssql_python.connection.py: 85.9%🔗 Quick Links
|
gargsaumya
reviewed
Sep 1, 2026
The previous assertions checked that the search-flag names appeared anywhere in the source, but they also appear in the #define block -- so gutting both LoadLibraryExW calls' flags to 0 still passed. Match each LoadLibraryExW(...) call and require both flags within that call's own argument list instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ening' into bewithgaurav/win-dll-search-hardening
gargsaumya
approved these changes
Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
On Windows the vendored ODBC driver (
msodbcsql18.dll) and Entra auth DLL (mssql-auth.dll) are loaded withLoadLibraryW, so their dependencies are resolved via the default process search order, which also consults the current working directory and%PATH%. This makes dependency resolution dependent on ambient environment.This change loads both DLLs with
LoadLibraryExWusingLOAD_LIBRARY_SEARCH_DEFAULT_DIRS | LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR, which keeps the current working directory and%PATH%out of the search and confines dependency resolution to the driver's own directory, the application directory, and System32.Changes
mssql_python/pybind/ddbc_bindings.cpp:LoadLibraryExWwith constrained search flags on the driver and auth-DLL loads (replacingLoadLibraryW). The flag constants are#defined defensively in case the build SDK gates them behind an older_WIN32_WINNT.tests/test_026_windows_dll_search.py: a source-contract regression test that fails if the loader reintroduces a bareLoadLibraryWcall or drops the constrained-search flags. It is deliberately a source check, not a runtime one — the search restriction only manifests at DLL-resolution time on Windows and cannot be observed without dropping a file on disk, and a "does it still load" check would pass on the unhardened code too (any host with the dependency already in System32), so it would guard nothing.Scope / notes
#ifdef _WIN32blocks, and the shipped Unix libraries already carry@loader_path/$ORIGIN.SetDefaultDllDirectories/AddDllDirectoryare intentionally not used), so a consuming application's own DLL resolution is unchanged. The bundled VC++ runtime already loads from the trusted copy next to the pybind extension (built/MD), so no extra search directory is needed.Validation
This is a Windows-only native change and is validated by the ADO Windows build/test legs (x64/arm64/x86); it cannot be exercised on a non-Windows dev host.