Closed jianyexi closed 1 year ago
On inspection (I haven't done any testing), I don't think the escape
function fits all cases of escaping. E.g. in Linux bash, one could put $(injected command here)
in the path string, and the injected would be evaluated by oad
's call to the shell. This will also break if a double-quote is somehow in the path string.
Perhaps handling these cases is out of scope, but if oad
is called remotely behind some attacker-controlled input, this could be bad.
Perhaps the shell-invoking command exec
and execSync
do actually make use of the shell features, in which case the escape
function should be improved. But if there's no particular reason a shell needs to be invoked, I strongly recommend using execFile
and execFileSync
instead, since they pass arguments to the invoked program as-is rather than doing any form of shell evaluation.
On inspection (I haven't done any testing), I don't think the
escape
function fits all cases of escaping. E.g. in Linux bash, one could put$(injected command here)
in the path string, and the injected would be evaluated byoad
's call to the shell. This will also break if a double-quote is somehow in the path string.Perhaps handling these cases is out of scope, but if
oad
is called remotely behind some attacker-controlled input, this could be bad.Perhaps the shell-invoking command
exec
andexecSync
do actually make use of the shell features, in which case theescape
function should be improved. But if there's no particular reason a shell needs to be invoked, I strongly recommend usingexecFile
andexecFileSync
instead, since they pass arguments to the invoked program as-is rather than doing any form of shell evaluation.
I didn't use the execFileSync
is because it doesn't work in Windows if I pass the 'node' to it.
And the escape
only applies to an internal argument, so there is no security issue, so I think this would be a designated fix for your issue so that we have the minimal impact, if we can still consider using the execFile
but need to have a further investigation.
Correct me if I am wrong, but isn't the text in this code user-controlled?
const cmd = `${this.dotNetPath()} ${this.openApiDiffDllPath()} -o ${oldSwagger} -n ${newSwagger}`
log.debug(`Executing: "${cmd}"`)
const { stdout } = await exec(cmd, { encoding: "utf8", maxBuffer: 1024 * 1024 * 64 })
And could be attacked by a valid file path like:
$ ls -d $PWD/swagger.json
'/c/Users/jantache/$(echo insert dangerous command here)/swagger.json'
Correct me if I am wrong, but isn't the text in this code user-controlled?
const cmd = `${this.dotNetPath()} ${this.openApiDiffDllPath()} -o ${oldSwagger} -n ${newSwagger}` log.debug(`Executing: "${cmd}"`) const { stdout } = await exec(cmd, { encoding: "utf8", maxBuffer: 1024 * 1024 * 64 })
And could be attacked by a valid file path like:
$ ls -d $PWD/swagger.json '/c/Users/jantache/$(echo insert dangerous command here)/swagger.json'
The tool will check if the file exists, if you provide an invalid file path, the tool will fail before executing this command, and your attack works only when your server do contain such dangerous swagger file path which can't be controlled by the attacker https://github.com/Azure/openapi-diff/blob/51ce7538aa9eaf5a4b7c79e7136f3c433b09703e/src/lib/validators/openApiDiff.ts#L293
Fix issues: https://github.com/Azure/openapi-diff/issues/254 https://github.com/Azure/azure-sdk-tools/issues/3748