ipc: fix D-Bus action denylist bypass via URL path/query ambiguity#1343
Open
acts-1631 wants to merge 1 commit into
Open
ipc: fix D-Bus action denylist bypass via URL path/query ambiguity#1343acts-1631 wants to merge 1 commit into
acts-1631 wants to merge 1 commit into
Conversation
ipc_object_set_current_reference() rejects setCurrentReference calls that request the showStudypad or showImage actions, since those touch the local filesystem or spawn external programs and the D-Bus session bus is reachable by any local peer. The check extracted the action value with strstr(reference, "action=") on the raw reference, but main_url_handler() splits the URL at the first '?' and asks Sword's URL class to parse the action from the query string only. A reference such as passagestudy.jsp/action=showBookmark?action=showImage&value=evil.png has "action=showBookmark" appear first in the URL path, so the raw strstr() picks that up (and it doesn't match the denylist), while Sword's parser only looks at the query string after the '?' and resolves the real action to showImage, which then gets dispatched. Fix this by not re-implementing URL/query parsing a second time in ipc.c. Factor the re-encoding + Sword URL parsing that main_url_handler() already uses for dispatch into main_url_get_action(), and have the IPC gate call that instead. The check and the actual dispatch now always agree, since they're the same code path, so this class of parsing-asymmetry bypass isn't possible regardless of how the URL is structured.
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
1 participant
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.
ipc_object_set_current_reference() rejects setCurrentReference calls that
request the showStudypad or showImage actions, since those touch the local
filesystem or spawn external programs and the D-Bus session bus is
reachable by any local peer.
The check extracted the action value with strstr(reference, "action=") on
the raw reference, but main_url_handler() splits the URL at the first '?'
and asks Sword's URL class to parse the action from the query string only.
A reference such as
passagestudy.jsp/action=showBookmark?action=showImage&value=evil.png
has "action=showBookmark" appear first in the URL path, so the raw
strstr() picks that up (and it doesn't match the denylist), while Sword's
parser only looks at the query string after the '?' and resolves the real
action to showImage, which then gets dispatched.
This fixes it by not re-implementing URL/query parsing a second time in
ipc.c. The re-encoding + Sword URL parsing that main_url_handler() already
uses for dispatch is factored out into main_url_get_action(), and the IPC
gate calls that instead. The check and the actual dispatch now always
agree, since they're the same code path.