Repository navigation
Improved security for dynamic loading of highs_extras dependencies - #3547
Conversation
Specifically, we now force the user (highspy, highs app automatic support) to initialize the loading of highs extras library. They can control exact path or use standard search paths based on their own security concerns. e.g., ```cpp HighsExtras::initialize(); ``` This initialize should only be performed once per process. Also added builtin-baseline for better vcpkg usability.
…correctly without having to call `HighsExtras::initialize()`. Otherwise (dynamically linked) it is basically a no-op.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## latest #3547 +/- ##
==========================================
+ Coverage 75.67% 75.72% +0.05%
==========================================
Files 461 461
Lines 113823 114067 +244
Branches 17913 17939 +26
==========================================
+ Hits 86130 86376 +246
+ Misses 27405 27403 -2
Partials 288 288 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@jajhall @galabovaa (anyone else?): I would love your opinion on the approach, in particular the somewhat breaking change. In order to address security concerns, we may need to make the dynamic loading of the extras library explicit. However:
Otherwise, the user needs to explicitly load extras, either specifying the path or accepting the risk of search. Thoughts? |
|
@galabovaa as we discussed offline, I'll be making some changes here! This current approach is a little overkill, especially since a user can compile in/out HIPO etc. The main security concern is with highspy which has the search for highs_extras built-in with no opt-out support. I'll instead make a smaller change that locks down highspy and doesn't require breaking change for everyone else. |
…dded further restrictions to library search.
* highspy requires absolute directory to be provided and will not search outside that path
* users can disable dynamic loading via compile-time options or by initializing with `Highs::initializeExtras("", false);`
* otherwise by default it will search standard paths for the library.
* On windows we further restrict search to absolute paths (relative paths will not work)
…ier testing. Added unit tests
|
Looking good! I looked at the CI failures for mingw in the open PR. |
Oh wow, I didn't realize that mingw produced |
…python drop `lib` prefix for `highs_extras` in mingw
|
mingw had not failed until now.. The julia build uses mingw and hipo too. I guess the linking worked previously EDIT it is only FAST_BUILD off which has the lib prefix. We keep it for legacy code, all new code should use the default FAST_BUILD=ON |
I believe the julia build uses static linking for HiPO, so avoids any |
Description
This PR addresses a potential security concern for the dynamic loading of the highs_extras library. This fix restricts the search parameters and makes the initialization more explicit.
Previously, the optional highs_extras library was automatically searched and loaded (on first request), even when not officially installed. An attacker could place a malicious library in a searched directory, whereby HiGHS would execute attacker-controlled code.
This change avoids implicit searching when no location is provided (by default and is overridable) and uses explicit paths with restricted dependency-search flags when loading from a specified directory.
Specifically, we now force the user (highspy and the highs application have built-in support) to initialize the loading of highs_extras library. They can control exact path or use standard search paths based on their own security concerns. e.g.,
or
The initialize should only be performed once per process. This does not impact any build that statically links the extra dependencies (i.e., initialize is a no-op). Also, initialize is not required for statically linked builds.
Checklist
latestbranch