Validate templateid/pluginid in pushConfiguration#1392
Open
TristanInSec wants to merge 1 commit intocdapio:developfrom
Open
Validate templateid/pluginid in pushConfiguration#1392TristanInSec wants to merge 1 commit intocdapio:developfrom
TristanInSec wants to merge 1 commit intocdapio:developfrom
Conversation
Aggregator.pushConfiguration receives templateid and pluginid from the WebSocket message payload and splices both values into an on-disk path that is then read with fs.readFileSync. Because neither value is constrained, an attacker with a valid session can supply values containing ../ or absolute-path components and cause pushConfiguration to read any readable .json file on the server. Restrict the two identifiers to plain identifier characters ([A-Za-z0-9_-]+) and, after resolving the resulting path, verify it still lives inside the templates directory. Invalid combinations now reply with a 400/INVALID_TEMPLATE_OR_PLUGIN_ID response instead of walking the filesystem.
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.
Summary
Aggregator.pushConfiguration()inserver/aggregator.jsbuilds the path of the JSON config it is about to read by concatenatingtemplateidandpluginiddirectly into the string:Both values come from the WebSocket message payload (
template-configaction), so neither is trusted input. Without validation the path can escape thetemplatesdirectory.Changes
TEMPLATE_ID_RE = /^[A-Za-z0-9_-]+$/allowlist for bothtemplateidandpluginid.buildTemplateConfigPaths()helper resolves the two candidate paths withpath.resolve, then verifies each one stays inside its corresponding template directory (templates/<templateid>andtemplates/common).400andresponse: 'INVALID_TEMPLATE_OR_PLUGIN_ID'instead of falling through to the filesystem lookup.The happy path for well-formed template/plugin identifiers resolves to the same absolute paths the previous implementation produced.
Testing
Exercised
buildTemplateConfigPathsin isolation against a representative set of inputs:templateidpluginidetlbatchhdfstemplates/a_b-cA9templates/....[]../../../..../../etc/cdap/conf/cdap-site[]etlbatchhdfs/../../etc/passwd[]''''[]nullhdfs[]etlbatch/absolute/path[]etl.batchhdfs[](dot not allowed)