squid basic_ldap_auth: add single quotes around -b and -D #729
Conversation
There was a problem hiding this comment.
What if the values contain single quotes?
Since these are user-entered values being passed to a command, these should all be using escapeshellarg() which will wrap in single quotes and handle escaping inside, solving two problems at once (your space issue, plus preventing potential paths to command execution)
|
done Should escapeshellarg() be used for all user supplied arguments ? |
Yes. Any time user data is passed to a command, it should be escaped. The only typical exception is when a field is validated strictly when saved, but even then the best practice is to escape it when used. |
|
done |
| @@ -1917,7 +1917,7 @@ function squid_resync_auth() { | |||
| case 'ldap': | |||
| $port = ((isset($settings['auth_server_port']) && !empty($settings['auth_server_port'])) ? ":{$settings['auth_server_port']}" : ''); | |||
| $password = (isset($settings['ldap_pass']) ? "-w {$settings['ldap_pass']}" : ''); | |||
| $conf .= "auth_param basic program " . SQUID_LOCALBASE . "/libexec/squid/basic_ldap_auth -v {$settings['ldap_version']} -b {$settings['ldap_basedomain']} -D {$settings['ldap_user']} $password -f \"{$settings['ldap_filter']}\" -u {$settings['ldap_userattribute']} -P {$settings['auth_server']}$port\n"; | |||
| $conf .= "auth_param basic program " . SQUID_LOCALBASE . "/libexec/squid/basic_ldap_auth -v {$settings['ldap_version']} -b " . escapeshellarg($settings['ldap_basedomain']) . " -D " . escapeshellarg($settings['ldap_user']) . " " . escapeshellarg($password) . " -f " . escapeshellarg($settings['ldap_filter']) . " -u " . escapeshellarg($settings['ldap_userattribute']) . " -P " . escapeshellarg($settings['auth_server']) . escapeshellarg($port) . "\n"; | |||
There was a problem hiding this comment.
The way $port is formed the address and port should be one string, so this bit:
escapeshellarg($settings['auth_server']) . escapeshellarg($port)
Should be:
escapeshellarg($settings['auth_server'] . $port)
There was a problem hiding this comment.
Also it would be easier to read if this was split into multiple lines. A newline and some tab intents for each command line parameter would make it look a lot better.
vktg
force-pushed
the
basicldapauthquotes
branch
from
587181c
to
53ecdec
Compare
|
done |
Changelog: * Fixed: Version `Qt_5.15' not found (required by /usr/bin/ksnip). (#712) * Fixed: CI packages show continuous suffix for tagged build. (#710) * Fixed: kImageAnnotator not translated with deb package. (#359) * Fixed: Windows packages increased in size. (#713) * Fixed: The string 'Actions' is not available for translation. (#729) * Fixed: HiDPI issue with multiple screen on Windows. (#668) * Fixed: Snipping Area not closing when pressing ESC. (#735) * Fixed: Sometimes "Snipping Area Rulers" not shown after starting rectangular selection. (#684) * Fixed: Cursor not positioned correctly when snipping area opens. (#736) * Fixed: Mouse cursor not captured when triggered via global shortcut. (#737) * Fixed: Dual 4K screens get scrambled on X11. (#734) * Fixed: VCRUNTIME140_1.dll was not found. (#743) * Fixed: Screenshot area issue when monitor count changes on Windows. (#722) * Fixed: Wayland does not support QWindow::requestActivate(). (#656) * Fixed: Wrong area is captured on a Wayland screen scaling. (#691) * Changed: Enforce xdg-desktop-portal screenshots for Gnome >= 41. (#727) * Fixed kImageAnnotator: Crash while typing text on wayland. (#256) * Changed kImageAnnotator: Show scrollbar when not all tools visible. (#258)
Redmine Issue: https://redmine.pfsense.org/issues/9217
Ready for review
If LDAP Base Domain or LDAP Server User DN contains spaces, for example OU=all users, squid ldap auth works only if to put the whole expression in proper webgui fields (squid > Auth) between quotes.
This PR wrap the values in fields LDAP Server User DN and LDAP Base DN in quotes automatically.