Repository navigation
Improved security for dynamic loading of highs_extras dependencies - #3547
mathgeekcoder wants to merge 14 commits into
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.69% +0.02%
==========================================
Files 461 461
Lines 113823 113999 +176
Branches 17913 17934 +21
==========================================
+ Hits 86141 86297 +156
- Misses 27394 27414 +20
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
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