Skip to content

Commit 36f59b4

Browse files
sayanshaw24Sayan Shaw
andauthored
Enable Security Protocols in MSVC for BinSkim (microsoft#1672)
### Updates This PR explicitly enables stack protection, control flow guard, CET shadow stack, and Spectre mitigations to pass BinSkim checks. We also reorder MSVC compiler options to ensure all binaries, including external ones such those from ORT Extensions, meet our security standards. Among other results, this also means warnings will be treated as errors now, hence we also added external fixes, like this one in ORT Extensions: microsoft/onnxruntime-extensions#986. **Stack Protection** - Our application code should not disable stack protection for individual functions. The stack protector (/GS) is a security feature of the Windows native compiler which makes it more difficult to exploit stack buffer overflow memory corruption vulnerabilities. Disabling the stack protector, even on a function-by-function basis, can compromise the security of code. - This means removing all occurrences of `__declspec(safebuffers)` from not just our code but also 3rd party source code. Since the latter is not viable, we explicitly enable stack protection (/GS) to pass BinSkim checks. **Control Flow Guard** - Also, we must enable CFG at build time to prevent attackers from redirecting execution to unsafe locations. CFG injects a check before every indirect call to ensure the target is safe. If the check fails, the OS closes the program. - To do this, we must pass /guard:cf on both the compiler and linker command lines. Binaries also require the /DYNAMICBASE linker option in order to enable CFG. Although we have these set in our Windows CMake config presets, they may not have worked due to (1) incomplete propagation (sometimes the flags defined in presets don’t propagate into all targets or certain configurations like third-party libs, subprojects, or custom targets), (2) explicit linker flags for our targets, or (3) static libraries and DLLs. Hence, we explicitly enable them in our top CMakeLists.txt. This guarantees all targets (DLLs, EXEs, etc.) get these flags regardless of presets, avoids missing flags due to custom or overridden flags in some targets, and is easy to audit and maintain in one place. **Control-flow Enforcement Technology** - BinSkim currently throws a warning as we miss the Control-flow Enforcement Technology (CET) Shadow Stack mitigation. - To resolve this issue, we must pass /CETCOMPAT on the linker command lines. Please note however that /CETCOMPAT is not supported on ARM64 machines, so we skip it there; BinSkim also does not require it there. **Spectre mitigations** - We also have a BinSkim warning for not enabling code generation mitigations for speculative execution side-channel attack (Spectre) vulnerabilities. Spectre attacks can compromise hardware-based isolation, allowing non-privileged users to retrieve potentially sensitive data from the CPU cache. - To resolve the issue, we must provide the /Qspectre switch on the compiler command-line (or /d2guardspecload in cases where the compiler supports this switch and it is not possible to update to a toolset that supports /Qspectre). This warning should be addressed for code that operates on data that crosses a trust boundary and that can affect execution, such as parsing untrusted file inputs or processing query strings of a web request. ### Validation - [x] Successful local and CI builds - [x] Following the merging of this PR, we will close our BinSkim TSA issues, after which TSA automatically scans our code and artifacts for security issues to ensure the problems are fixed. If they do not reappear, we are set. --------- Co-authored-by: Sayan Shaw <[email protected]>
1 parent 00e5654 commit 36f59b4

File tree

2 files changed

+54
-13
lines changed

2 files changed

+54
-13
lines changed

CMakeLists.txt

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,59 @@ if(MSVC)
2929
# See https://developercommunity.visualstudio.com/t/Access-violation-with-std::mutex::lock-a/10664660#T-N10668856
3030
# Remove this definition once the conda msvcp140.dll dll is updated.
3131
add_compile_definitions(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR)
32+
33+
# Add compiler options for security standards
34+
# Note: Must use separate generator expressions — do NOT use "C,CXX" in COMPILE_LANGUAGE.
35+
36+
# Explicitly enable (/GS) stack protection to pass BinSkim checks
37+
add_compile_options("$<$<COMPILE_LANGUAGE:C>:/GS>")
38+
add_compile_options("$<$<COMPILE_LANGUAGE:CXX>:/GS>")
39+
40+
# Enable Control Flow Guard on compiler and linker to pass BinSkim checks
41+
add_compile_options("$<$<COMPILE_LANGUAGE:C>:/guard:cf>")
42+
add_compile_options("$<$<COMPILE_LANGUAGE:CXX>:/guard:cf>")
43+
add_link_options(/guard:cf /DYNAMICBASE)
44+
45+
# Enable CET shadow stack unless building for ARM64 (not supported there)
46+
message(STATUS "Processor: ${CMAKE_SYSTEM_PROCESSOR}")
47+
if(NOT CMAKE_SYSTEM_PROCESSOR STREQUAL "ARM64")
48+
message(STATUS "CET shadow stack enabled (/CETCOMPAT)")
49+
add_link_options(/CETCOMPAT)
50+
else()
51+
message(STATUS "CET shadow stack skipped for ARM64 build")
52+
endif()
53+
54+
# Enable Spectre mitigations for C and C++ compilations only (avoid CUDA nvcc - not supported there)
55+
add_compile_options(
56+
"$<$<COMPILE_LANGUAGE:C>:/Qspectre>"
57+
"$<$<COMPILE_LANGUAGE:CXX>:/Qspectre>"
58+
)
59+
60+
# Use updated value for __cplusplus macro (e.g. 201703L for C++17) instead of default 199711L
61+
# Required by some libraries/tools that check __cplusplus for feature support
62+
add_compile_options($<$<COMPILE_LANGUAGE:CXX>:/Zc:__cplusplus>)
63+
64+
add_compile_options(
65+
# Suppress warning C5038: data member will be initialized after another
66+
# Common when member initializer list is written out-of-order vs. declaration
67+
"$<$<COMPILE_LANGUAGE:C>:/w15038>"
68+
"$<$<COMPILE_LANGUAGE:CXX>:/w15038>"
69+
70+
# Suppress warning C4100: unreferenced formal parameter
71+
# Often appears in template or virtual function overrides where a param is unused
72+
"$<$<COMPILE_LANGUAGE:C>:/wd4100>"
73+
"$<$<COMPILE_LANGUAGE:CXX>:/wd4100>"
74+
75+
# Enable warning level 4 (more aggressive than default /W3)
76+
# Captures more potential bugs or code smells
77+
"$<$<COMPILE_LANGUAGE:C>:/W4>"
78+
"$<$<COMPILE_LANGUAGE:CXX>:/W4>"
79+
80+
# Treat all warnings as errors (fail build on any warning)
81+
# Combine carefully with /W4 — suppress any known false positives
82+
"$<$<COMPILE_LANGUAGE:C>:/WX>"
83+
"$<$<COMPILE_LANGUAGE:CXX>:/WX>"
84+
)
3285
endif()
3386

3487
include(cmake/ortlib.cmake)
@@ -48,18 +101,6 @@ add_compile_definitions(BUILDING_ORT_GENAI_C)
48101

49102
add_compile_definitions(USE_GUIDANCE=$<BOOL:${USE_GUIDANCE}>)
50103

51-
if(MSVC)
52-
# set updated value for __cplusplus macro instead of 199711L
53-
add_compile_options($<$<COMPILE_LANGUAGE:CXX>:/Zc:__cplusplus>)
54-
55-
add_compile_options(
56-
"$<$<COMPILE_LANGUAGE:C,CXX>:/w15038>"
57-
"$<$<COMPILE_LANGUAGE:C,CXX>:/wd4100>"
58-
"$<$<COMPILE_LANGUAGE:C,CXX>:/W4>"
59-
"$<$<COMPILE_LANGUAGE:C,CXX>:/WX>"
60-
)
61-
endif()
62-
63104
# Suggested by https://gitlab.kitware.com/cmake/cmake/-/issues/20132
64105
# MacCatalyst is not well supported in CMake
65106
# The error that can emerge without this flag can look like:

cmake/deps.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ pybind11;https://github.com/pybind/pybind11/archive/refs/tags/v2.13.6.zip;f78029
1414
googletest;https://github.com/google/googletest/archive/530d5c8c84abd2a46f38583ee817743c9b3a42b4.zip;5e3a61db2aa975cfd0f97ba92c818744e7fa7034
1515
microsoft_wil;https://github.com/microsoft/wil/archive/refs/tags/v1.0.230629.1.zip;e4a542a323c070376f7c2d1973d0f7ddbc1d2fa5
1616
directx_headers;https://github.com/microsoft/DirectX-Headers/archive/refs/tags/v1.613.1.zip;47653509a3371eabb156360f42faf582f314bf2e
17-
onnxruntime_extensions;https://github.com/microsoft/onnxruntime-extensions.git;af289f57acae13a0ee1926605e0a7cf53efd8a0c
17+
onnxruntime_extensions;https://github.com/microsoft/onnxruntime-extensions.git;5f8f774ff31df644eed9fbb2f0384661aca531ef
1818

1919
# These two dependencies are for the optional constrained decoding feature (USE_GUIDANCE)
2020
llguidance;https://github.com/microsoft/llguidance.git;2d2f1de3c87e3289528affc346f734f7471216d9

0 commit comments

Comments
 (0)