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

Changes to enable high integrity testing #856

Merged
merged 1 commit into from
Feb 17, 2022
Merged

Changes to enable high integrity testing #856

merged 1 commit into from
Feb 17, 2022

Conversation

MarkY-LunarG
Copy link
Collaborator

No description provided.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 14991.

@MarkY-LunarG MarkY-LunarG changed the title Changes to enable high integrity testing WIP: Changes to enable high integrity testing Feb 17, 2022
@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1138 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1138 passed.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 15001.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1139 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1139 passed.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 15042.

@MarkY-LunarG MarkY-LunarG changed the title WIP: Changes to enable high integrity testing Changes to enable high integrity testing Feb 17, 2022
@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1140 running.

Enable testing to check if the loader is running with elevated
privileges.  This is to make sure we're ignoring the appropriate
environment variables in those scenarios to potentially avoid
escalation exploits.
@MarkY-LunarG
Copy link
Collaborator Author

Going to update once more. Noticed I missed copyrights in the shim files.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1140 passed.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 15078.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1141 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 1141 passed.

@MarkY-LunarG MarkY-LunarG merged commit 18852aa into KhronosGroup:master Feb 17, 2022
@MarkY-LunarG MarkY-LunarG deleted the test_high_integrity branch February 17, 2022 18:37
@null77
Copy link
Contributor

null77 commented Feb 25, 2022

Hey Mark

Your CL as expected broke ANGLE's integration with the loader on most platforms we test on. Can you confirm if this was intentional, and if so, how we can escalate to ask for some kind of bypass or solution?

@charles-lunarg
Copy link
Collaborator

We don't intent to break ANGLE unless it was relying on potentially unsecured behavior. I can re-review the changes here and try to figure out which may be causing the issue.

@null77
Copy link
Contributor

null77 commented Feb 25, 2022

To be clear, we almost certainly rely on what you refer to as potentially unsecured behaviour. Because we don't ship as a platform loader this is not a security risk for our use case. If you could re-review the changes and quickly help us come up with a solution for us that would be most welcome.

For more information see the conversation here: #281

I was being as direct as I could that these changes are expected to break ANGLE's integration.

@charles-lunarg
Copy link
Collaborator

Ah, then its my bad for not picking up on what was being said.

There is the USE_UNSAFE_FILE_SEARCH build option, which should disable the secure checking/admin mode disabling env-vars. I am not sure that helps here since it only applies to env-var logic, not to calls to is_high_integrity().

This PR only adds a is_high_integrity() check in an APPLE-only codepath, the other relevant change simply replaces the exact same logic that is_high_integrity() has with a call to the function. I'm not sure how these changes could affect builds on all platforms.

@null77
Copy link
Contributor

null77 commented Feb 25, 2022

Charles, maybe what we need to do is add the ability for ANGLE to override this unsafe build option in our GN integration. The additional call to is_high_integrity is indeed what is breaking our build, because likely we don't have that argument set in the GN. I'll propose a patch shortly, thanks for pointing out the workaround.

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

Successfully merging this pull request may close these issues.

4 participants