Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load dependencies by name #1594

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Conversation

aleksei-fedotov
Copy link
Contributor

@aleksei-fedotov aleksei-fedotov commented Jan 7, 2025

Description

This PR allows loading dependencies by module name only as it is proposed in this RFC. In particular, it:

  • Verifies signatures of all loaded modules.
  • Changes loading of Thread Composability Manager using its module name instead of full path.

The signature verification is enabled by default. To disable it explicitly, user needs specifying -DTBB_VERIFY_DEPENDENCY_SIGNATURE=OFF in the invocation command of CMake. In this case the warning message appears:

CMake Warning at CMakeLists.txt:231 (message):
  Verification of a dependency signature during dynamic run-time linking is
  explicitly disabled.This may lead to security vulnerabilities.  It is
  recommended to leave it enabled by default or set
  TBB_VERIFY_DEPENDENCY_SIGNATURE ON to enable it explicitly.

The patch also adds optional reporting of dynamic link issues. Examples of the output:

oneTBB dynamic link warning: The module "<path-to-dll>" is unsigned or has invalid signature.
oneTBB dynamic link warning: The module "<path-to-dll>" was not found. System error: 126

The issues reporting is disabled by default and can be enabled by setting TBB_DYNAMIC_LINK_WARNING macro during the build.

@aleksei-fedotov aleksei-fedotov marked this pull request as ready for review January 8, 2025 16:11
@aleksei-fedotov aleksei-fedotov requested review from pavelkumbrasev and removed request for pavelkumbrasev January 8, 2025 16:11
CMakeLists.txt Outdated Show resolved Hide resolved
src/tbb/dynamic_link.cpp Outdated Show resolved Hide resolved
src/tbb/dynamic_link.cpp Outdated Show resolved Hide resolved
DYNAMIC_LINK_WARNING( dl_sig_other_error, filepath, retval);
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here: Is the case for an expired certificate covered somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is covered by untrusted root case as the whole chain of certificates is checked.

src/tbb/CMakeLists.txt Outdated Show resolved Hide resolved
pWVTData.dwProvFlags = WTD_CACHE_ONLY_URL_RETRIEVAL | WTD_REVOCATION_CHECK_CHAIN;
pWVTData.dwUIContext = WTD_UICONTEXT_EXECUTE; // UI Context to run the file

const auto rc = WinVerifyTrust((HWND)INVALID_HANDLE_VALUE, &pgActionID, &pWVTData);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use LONG instead of auto to prevent ambiguity and unintended typecasting when doing comparisons later with the return code from WinVerifyTrust

Copy link
Contributor

@egfefey egfefey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the security standpoint, this is good for now.
A few notes:

  • This is still up for discussion but anything beside ERROR_SUCCESS from WinVerifyTrust should result in an error instead of a warning sometimes. The user always has to option to turn off signature verification if they want, so once it's on, it's either success or failure.
  • Might also need review from the TBB team.
  • If there are some testing results that we can share, that will be great.

Copy link
Contributor

@egfefey egfefey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update to use SearchPathA is good from the security point of view.

@aleksei-fedotov aleksei-fedotov force-pushed the load-dependencies-by-name branch from dd5a544 to 5e6c672 Compare January 29, 2025 11:05
@aleksei-fedotov
Copy link
Contributor Author

Update to use SearchPathA is good from the security point of view.

Thanks, @egfefey! Then I proceed with productization of this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants