Fix information leakage of confidential features in Search API#6276
Open
DanielRyanSmith wants to merge 3 commits intomainfrom
Open
Fix information leakage of confidential features in Search API#6276DanielRyanSmith wants to merge 3 commits intomainfrom
DanielRyanSmith wants to merge 3 commits intomainfrom
Conversation
DanielRyanSmith
force-pushed
the
fix-search-info-leak
branch
2 times, most recently
from
ed435f7 to
b8dc38f
Compare
Collaborator
Author
|
CodeQL seems to be upset with existing code |
Collaborator
Author
|
I'll fix the CodeQL issues in this PR as well if possible |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
DanielRyanSmith
force-pushed
the
fix-search-info-leak
branch
from
a33d293 to
3a5d152
Compare
DanielRyanSmith
force-pushed
the
fix-search-info-leak
branch
from
3a5d152 to
810e001
Compare
This commit addresses two major issues introduced in the previous fix for the search API information leakage: 1. Information Leakage via Cache: Caching is enabled for `name_only=True` queries, but the cache key didn't partition based on the current user. Thus, if an authorized user cached a confidential result, an unauthorized user could retrieve it from the cache. The cache key has been updated to include the user's email to securely partition the results. 2. Unscalable Model Fetching: The previous implementation used `ndb.get_multi` to fetch all matching confidential `FeatureEntry` models into memory to perform permission filtering. This has been optimized to evaluate permissions via efficient integer ID set subtraction using existing participant cache keys and admin checks, completely avoiding the expensive `ndb.get_multi` call.
DanielRyanSmith
force-pushed
the
fix-search-info-leak
branch
from
0e01135 to
d7525c1
Compare
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
2 participants
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.
Overview
Fixes an information leakage vulnerability in the Search API where
total_countwas calculated before filtering out confidential features.Root Cause / Motivation
During a security review, it was discovered that the
total_countproperty of the Search API response was calculating the total number of features matching a query across the entire datastore before applying confidentiality filtering on the current page. This could be abused to leak the existence or total number of confidential features matching a specific query by an unprivileged user.To fix this,
internals/search.pynow leverages a parallel datastore query to identify all confidential features matching the query, and filters out the ones the user doesn't have explicit permission to view from theresult_id_setbefore thetotal_countis evaluated and the results are paginated.Detailed Changelog
internals/search.py: Addedconfidential_future_opsto parallelize finding confidential features that matched the user's query. Those results are dynamically cross-checked with the user's view permissions (permissions.can_view_feature), and explicitly stripped out ofresult_id_setprior to pagination and total count length check.