Require verified email for all authorization gates, not just domain trust#5366
Open
gn00295120 wants to merge 1 commit into
Open
Require verified email for all authorization gates, not just domain trust#5366gn00295120 wants to merge 1 commit into
gn00295120 wants to merge 1 commit into
Conversation
…rust PR google#5314 (e68fd25) correctly required email_verified for domain- allowlist trust (_is_trusted_domain_user) to prevent GitHub Enterprise Managed Users from asserting an arbitrary unverified email to gain whitelisted-domain access. However, the same unverified email was still trusted by: - _is_privileged_user (get_access line 141) - external_users.is_job_allowed_for_user (get_access line 144) - external_users.is_fuzzer_allowed_for_user (get_access line 147) - testcase uploader_email equality (can_user_access_testcase line 170) - issue assignee/CC/reporter equality (can_user_access_testcase line 201) An attacker controlling a GitHub EMU with an unverified email matching a privileged_users entry could obtain full privileged access to all endpoints, security test cases, and embargoed crash reproducers. This change gates every email-identity authorization check on user.email_verified, consistent with the threat model established in PR google#5314.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #5314 (
e68fd259ca) correctly requiredemail_verifiedfor domain-allowlist trust (_is_trusted_domain_user) to prevent GitHub Enterprise Managed Users (EMU) from asserting an arbitrary unverified email to gain whitelisted-domain access.However, the same unverified email was still trusted by several other authorization paths in
get_access()andcan_user_access_testcase():_is_privileged_user(email)access.py:141is_job_allowed_for_user(email, job_type)access.py:144is_fuzzer_allowed_for_user(email, fuzzer_name)access.py:147testcase.uploader_email == user_emailaccess.py:170emails_equal(user_email, issue.assignee)access.py:201An attacker controlling a GitHub Enterprise with managed users could configure an EMU account whose email matches a
privileged_usersentry (maintainer emails are public in commit history). Firebase would authenticate the GitHub sign-in and pass throughemail_verified=false. The_is_privileged_usergate would then match the asserted email against the privileged list without verifying ownership, granting full admin access — including all security-flagged test cases and embargoed crash reproducers.Changes
src/appengine/libs/access.py:get_access(): Gate_is_privileged_user,is_job_allowed_for_user, andis_fuzzer_allowed_for_useronuser.email_verified. Only_is_trusted_domain_user(which already checksemail_verified) remains reachable by unverified-email users.can_user_access_testcase(): Gate uploader/assignee/CC/reporter email-equality checks onuser.email_verified. Reuse the already-fetcheduserobject for the_is_trusted_domain_usercall (avoids a redundantauth.get_current_user()round-trip).src/clusterfuzz/_internal/tests/appengine/libs/access_test.py:test_get_access_unverified_privileged: Unverified email matchingprivileged_users→ Deniedtest_get_access_unverified_external_job: Unverified email with job ACL → Deniedtest_get_access_unverified_external_fuzzer: Unverified email with fuzzer ACL → Deniedtest_denied_unverified_uploader: Unverified email matchinguploader_email→ no testcase accesstest_denied_unverified_bug_owner: Unverified email matching bug assignee → no testcase accessBackward compatibility
email_verifieddefaults toTruein theUsernamedtuple.privileged_users, external-user ACLs, and testcase uploader/assignee — this is the intended security hardening, consistent with the threat model established in PR Require verified email for whitelisted-domain trust #5314.