Fix CWE-426 (Untrusted Search Path) in example NSIS template

[michalwyszynski93]

Description

Mitigating CWE-426 vulnerability in the example NSIS template by using a fully qualified path instead of naked cmd.exe.

Many dev teams build up on this example for their custom installers. This propagates the vulnerability downstream to other projects.

Checklist - did you ...

• Add a file to the news directory (using the template) for the next release's release notes?
• Add / update necessary tests?
• Add / update outdated documentation?

[conda-bot]

We require contributors to sign our Contributor License Agreement and we don't have one on file for @michalwyszynski93.

In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (conda/infrastructure#1439), and ping the bot to refresh the PR.

[travishathaway]

@conda-bot check

[marcoesters]

Good catch, thanks!

We do already try to figure out where cmd.exe might be located, but this instance has fallen through the cracks:

constructor/constructor/nsis/main.nsi.tmpl

Lines 182 to 197 in 29afc80

	!macro FindWindowsBinaries
	# Find cmd.exe
	ReadEnvStr $R0 SystemRoot
	ReadEnvStr $R1 windir
	${If} ${FileExists} "$R0"
	StrCpy $CMD_EXE "$R0\System32\cmd.exe"
	StrCpy $ICACLS_EXE "$R0\System32\icacls.exe"
	${ElseIf} ${FileExists} "$R1"
	StrCpy $CMD_EXE "$R1\System32\cmd.exe"
	StrCpy $ICACLS_EXE "$R1\System32\icacls.exe"
	${Else}
	# Cross our fingers binaries are in PATH
	StrCpy $CMD_EXE "cmd.exe"
	StrCpy $ICACLS_EXE "icacls.exe"
	${EndIf}
	!macroend

Do you suggest we should rather error out than use PATH as the fallback?

[marcoesters]

Suggested change

	nsExec::Exec '"$SYSDIR\cmd.exe" /D /C RMDIR /Q /S "$INSTDIR"'
	nsExec::Exec '"$CMD_EXE" /D /C RMDIR /Q /S "$INSTDIR"'

[marcoesters]

Suggested change

	* Mitigated a CWE-426 untrusted search path vulnerability in the example `custom.nsi.tmpl` by using a fully qualified path to `$SYSDIR\cmd.exe`.
	* Mitigated a CWE-426 untrusted search path vulnerability in the example `custom.nsi.tmpl` by using a fully qualified path to `$CMD_EXE`. (#1341)

[michalwyszynski93]

Good catch, thanks!

We do already try to figure out where cmd.exe might be located, but this instance has fallen through the cracks:

constructor/constructor/nsis/main.nsi.tmpl

Lines 182 to 197 in 29afc80

	!macro FindWindowsBinaries
	# Find cmd.exe
	ReadEnvStr $R0 SystemRoot
	ReadEnvStr $R1 windir
	${If} ${FileExists} "$R0"
	StrCpy $CMD_EXE "$R0\System32\cmd.exe"
	StrCpy $ICACLS_EXE "$R0\System32\icacls.exe"
	${ElseIf} ${FileExists} "$R1"
	StrCpy $CMD_EXE "$R1\System32\cmd.exe"
	StrCpy $ICACLS_EXE "$R1\System32\icacls.exe"
	${Else}
	# Cross our fingers binaries are in PATH
	StrCpy $CMD_EXE "cmd.exe"
	StrCpy $ICACLS_EXE "icacls.exe"
	${EndIf}
	!macroend

Do you suggest we should rather error out than use PATH as the fallback?

Hi @marcoesters, thanks for your feedback.

I'm more inclined to error it out than to fall back on unqualified path, otherwise the vulnerability is still there. Realistically speaking, the fallback is only needed when the host system has been tampered with badly, so it's OK to wash our hands?

Regarding your suggestion: as far as I understand, the CMD_EXE variable is declared/defined in the default template (constructor/nsis/main.nsi.tmpl) but not in the custom one (examples/custom_nsis_template/custom.nsi.tmpl), so it won't work out of the box and we'll need to add some equivalent of FindWindowsBinaries macro there?

[marcoesters]

I'm more inclined to error it out than to fall back on unqualified path, otherwise the vulnerability is still there. Realistically speaking, the fallback is only needed when the host system has been tampered with badly, so it's OK to wash our hands?

I think that sounds fair enough as long as we exhaust all possible locations for a legitimate cmd.exe binary (and icacls.exe while we're at it).

Regarding your suggestion: as far as I understand, the CMD_EXE variable is declared/defined in the default template (constructor/nsis/main.nsi.tmpl) but not in the custom one (examples/custom_nsis_template/custom.nsi.tmpl), so it won't work out of the box and we'll need to add some equivalent of FindWindowsBinaries macro there?

My apologies, I completely missed the file name because the main template has the same line:

constructor/constructor/nsis/main.nsi.tmpl

Lines 1966 to 1967 in 29afc80

	${Print} "Removing files and folders..."
	nsExec::Exec 'cmd.exe /D /C RMDIR /Q /S "$INSTDIR"'

To answer your question: yes, you would have to add the macro. I want to emphasize though that this example was primarily designed for integration testing and have not been kept up-to-date unless there was a bug in the template that breaks the NSIS compilation. The main template is the better blueprint for custom installers. My suggestion is to:

• Fix CWE-426 in the main template.
• Add a comment to the top of the example template to use the main template as a blueprint for custom templates, not the example template.
• Optional: use the macro to the example template.

[marcoesters]

The failing Windows test is likely not due to this change - they tend to be a little flaky.