From 9807737734ec16e19dcfa95bc7187f8fc6b195c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Arenaza?= Date: Sat, 13 Sep 2014 02:37:26 +0200 Subject: [PATCH 1/3] MDL-43430 enrol_ldap: Enrolment lost if member of group with parenthesis As part of fixing MDL-43430, a typo has been detected in the quoted alphanumeric characters (the '=' and '>' were transposed). --- lib/ldaplib.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ldaplib.php b/lib/ldaplib.php index 0a941a072ad..370159af79b 100644 --- a/lib/ldaplib.php +++ b/lib/ldaplib.php @@ -337,7 +337,7 @@ function ldap_get_dn_special_chars() { return array ( LDAP_DN_SPECIAL_CHARS => array('\\', ' ', '"', '#', '+', ',', ';', '<', '=', '>', "\0"), LDAP_DN_SPECIAL_CHARS_QUOTED_NUM => array('\\5c','\\20','\\22','\\23','\\2b','\\2c','\\3b','\\3c','\\3d','\\3e','\\00'), - LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA => array('\\\\','\\ ', '\\"', '\\#', '\\+', '\\,', '\\;', '\\<', '\\>', '\\=', '\\00'), + LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA => array('\\\\','\\ ', '\\"', '\\#', '\\+', '\\,', '\\;', '\\<', '\\=', '\\>', '\\00'), ); } From 21302343eeed8ba84ae330b5a7eefe43b0e4e8e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Arenaza?= Date: Mon, 27 Oct 2014 00:15:47 +0100 Subject: [PATCH 2/3] MDL-43430 enrol_ldap: Enrolment lost if member of group with parenthesis The group names we get from ldap_find_user_groups() can contain invalid LDAP filter characters. So we need to escape them for LDAP filter usage. --- enrol/ldap/lib.php | 1 + 1 file changed, 1 insertion(+) diff --git a/enrol/ldap/lib.php b/enrol/ldap/lib.php index a806e99eaf1..42ef21fbbf0 100644 --- a/enrol/ldap/lib.php +++ b/enrol/ldap/lib.php @@ -720,6 +720,7 @@ class enrol_ldap_plugin extends enrol_plugin { $usergroups = $this->ldap_find_user_groups($extmemberuid); if(count($usergroups) > 0) { foreach ($usergroups as $group) { + $group = ldap_filter_addslashes($group); $ldap_search_pattern .= '('.$this->get_config('memberattribute_role'.$role->id).'='.$group.')'; } } From cc2203250277218af30b654cd385b13f0e0f3112 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Arenaza?= Date: Mon, 27 Oct 2014 00:30:46 +0100 Subject: [PATCH 3/3] MDL-43430 enrol_ldap: Enrolment lost if member of group with parenthesis While testing the fix, it was found that ldap_stripslashes() was broken and using a deprecated PCRE patter modifier, so it's also been fixed. Thanks to Damyon Wiese, some units tests have been added to cover ldap_addslashes() and ldap_stripslashes(). --- lib/ldaplib.php | 62 ++++++++++---- lib/tests/ldaplib_test.php | 169 +++++++++++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 18 deletions(-) create mode 100644 lib/tests/ldaplib_test.php diff --git a/lib/ldaplib.php b/lib/ldaplib.php index 370159af79b..c23560d29e4 100644 --- a/lib/ldaplib.php +++ b/lib/ldaplib.php @@ -327,6 +327,9 @@ if(!defined('LDAP_DN_SPECIAL_CHARS_QUOTED_NUM')) { if(!defined('LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA')) { define('LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA', 2); } +if(!defined('LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA_REGEX')) { + define('LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA_REGEX', 3); +} /** * The order of the special characters in these arrays _IS IMPORTANT_. @@ -334,22 +337,36 @@ if(!defined('LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA')) { * Otherwise we'll double replace '\' with '\5C' which is Bad(tm) */ function ldap_get_dn_special_chars() { - return array ( + static $specialchars = null; + + if ($specialchars !== null) { + return $specialchars; + } + + $specialchars = array ( LDAP_DN_SPECIAL_CHARS => array('\\', ' ', '"', '#', '+', ',', ';', '<', '=', '>', "\0"), LDAP_DN_SPECIAL_CHARS_QUOTED_NUM => array('\\5c','\\20','\\22','\\23','\\2b','\\2c','\\3b','\\3c','\\3d','\\3e','\\00'), LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA => array('\\\\','\\ ', '\\"', '\\#', '\\+', '\\,', '\\;', '\\<', '\\=', '\\>', '\\00'), ); + $alpharegex = implode('|', array_map (function ($expr) { return preg_quote($expr); }, + $specialchars[LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA])); + $specialchars[LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA_REGEX] = $alpharegex; + + return $specialchars; } /** - * Quote control characters in distinguished names used in LDAP - See RFC 4514/2253 + * Quote control characters in AttributeValue parts of a RelativeDistinguishedName + * used in LDAP distinguished names - See RFC 4514/2253 * - * @param string The text to quote - * @return string The text quoted + * @param string the AttributeValue to quote + * @return string the AttributeValue quoted */ function ldap_addslashes($text) { $special_dn_chars = ldap_get_dn_special_chars(); + // Use the preferred/universal quotation method: ESC HEX HEX + // (i.e., the 'numerically' quoted characters) $text = str_replace ($special_dn_chars[LDAP_DN_SPECIAL_CHARS], $special_dn_chars[LDAP_DN_SPECIAL_CHARS_QUOTED_NUM], $text); @@ -357,25 +374,34 @@ function ldap_addslashes($text) { } /** - * Unquote control characters in distinguished names used in LDAP - See RFC 4514/2253 + * Unquote control characters in AttributeValue parts of a RelativeDistinguishedName + * used in LDAP distinguished names - See RFC 4514/2253 * - * @param string The text quoted - * @return string The text unquoted + * @param string the AttributeValue quoted + * @return string the AttributeValue unquoted */ function ldap_stripslashes($text) { - $special_dn_chars = ldap_get_dn_special_chars(); + $specialchars = ldap_get_dn_special_chars(); - // First unquote the simply backslashed special characters. If we - // do it the other way, we remove too many slashes. - $text = str_replace($special_dn_chars[LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA], - $special_dn_chars[LDAP_DN_SPECIAL_CHARS], - $text); - - // Next unquote the 'numerically' quoted characters. We don't use - // LDAP_DN_SPECIAL_CHARS_QUOTED_NUM because the standard allows us - // to quote any character with this encoding, not just the special + // We can't unquote in two steps, as we end up unquoting too much in certain cases. So + // we need to build a regexp containing both the 'numerically' and 'alphabetically' + // quoted characters. We don't use LDAP_DN_SPECIAL_CHARS_QUOTED_NUM because the + // standard allows us to quote any character with this encoding, not just the special // ones. - $text = preg_replace('/\\\([0-9A-Fa-f]{2})/e', "chr(hexdec('\\1'))", $text); + // @TODO: This still misses some special (and rarely used) cases, but we need + // a full state machine to handle them. + $quoted = '/(\\\\[0-9A-Fa-f]{2}|' . $specialchars[LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA_REGEX] . ')/'; + $text = preg_replace_callback($quoted, + function ($match) use ($specialchars) { + if (ctype_xdigit(ltrim($match[1], '\\'))) { + return chr(hexdec($match[1])); + } else { + return str_replace($specialchars[LDAP_DN_SPECIAL_CHARS_QUOTED_ALPHA], + $specialchars[LDAP_DN_SPECIAL_CHARS], + $match[1]); + } + }, + $text); return $text; } diff --git a/lib/tests/ldaplib_test.php b/lib/tests/ldaplib_test.php new file mode 100644 index 00000000000..21ab2544771 --- /dev/null +++ b/lib/tests/ldaplib_test.php @@ -0,0 +1,169 @@ +. + +/** + * ldap tests. + * + * @package core + * @category phpunit + * @copyright Damyon Wiese, Iñaki Arenaza 2014 + * @license http://www.gnu.org/copyleft/gpl.html GNU Public License + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->libdir . '/ldaplib.php'); + +class core_ldaplib_testcase extends advanced_testcase { + + public function test_ldap_addslashes() { + // See http://tools.ietf.org/html/rfc4514#section-5.2 if you want + // to add additional tests. + + $tests = array( + array ( + 'test' => 'Simplest', + 'expected' => 'Simplest', + ), + array ( + 'test' => 'Simple case', + 'expected' => 'Simple\\20case', + ), + array ( + 'test' => 'Medium ‒ case', + 'expected' => 'Medium\\20‒\\20case', + ), + array ( + 'test' => '#Harder+case#', + 'expected' => '\\23Harder\\2bcase\\23', + ), + array ( + 'test' => ' Harder (and); harder case ', + 'expected' => '\\20Harder\\20(and)\\3b\\20harder\\20case\\20', + ), + array ( + 'test' => 'Really \\0 (hard) case!\\', + 'expected' => 'Really\\20\\5c0\\20(hard)\\20case!\\5c', + ), + array ( + 'test' => 'James "Jim" = Smith, III', + 'expected' => 'James\\20\\22Jim\22\\20\\3d\\20Smith\\2c\\20III', + ), + array ( + 'test' => ' ', + 'expected' => '\\20\\20\\3cjsmith@test.local\\3e\\20', + ), + ); + + + foreach ($tests as $test) { + $this->assertSame($test['expected'], ldap_addslashes($test['test'])); + } + } + + public function test_ldap_stripslashes() { + // See http://tools.ietf.org/html/rfc4514#section-5.2 if you want + // to add additional tests. + + // IMPORTANT NOTICE: While ldap_addslashes() only produces one + // of the two defined ways of escaping/quoting (the ESC HEX + // HEX way defined in the grammar in Section 3 of RFC-4514) + // ldap_stripslashes() has to deal with both of them. So in + // addition to testing the same strings we test in + // test_ldap_stripslashes(), we need to also test strings + // using the second method. + + $tests = array( + array ( + 'test' => 'Simplest', + 'expected' => 'Simplest', + ), + array ( + 'test' => 'Simple\\20case', + 'expected' => 'Simple case', + ), + array ( + 'test' => 'Simple\\ case', + 'expected' => 'Simple case', + ), + array ( + 'test' => 'Simple\\ \\63\\61\\73\\65', + 'expected' => 'Simple case', + ), + array ( + 'test' => 'Medium\\ ‒\\ case', + 'expected' => 'Medium ‒ case', + ), + array ( + 'test' => 'Medium\\20‒\\20case', + 'expected' => 'Medium ‒ case', + ), + array ( + 'test' => 'Medium\\20\\E2\\80\\92\\20case', + 'expected' => 'Medium ‒ case', + ), + array ( + 'test' => '\\23Harder\\2bcase\\23', + 'expected' => '#Harder+case#', + ), + array ( + 'test' => '\\#Harder\\+case\\#', + 'expected' => '#Harder+case#', + ), + array ( + 'test' => '\\20Harder\\20(and)\\3b\\20harder\\20case\\20', + 'expected' => ' Harder (and); harder case ', + ), + array ( + 'test' => '\\ Harder\\ (and)\\;\\ harder\\ case\\ ', + 'expected' => ' Harder (and); harder case ', + ), + array ( + 'test' => 'Really\\20\\5c0\\20(hard)\\20case!\\5c', + 'expected' => 'Really \\0 (hard) case!\\', + ), + array ( + 'test' => 'Really\\ \\\\0\\ (hard)\\ case!\\\\', + 'expected' => 'Really \\0 (hard) case!\\', + ), + array ( + 'test' => 'James\\20\\22Jim\\22\\20\\3d\\20Smith\\2c\\20III', + 'expected' => 'James "Jim" = Smith, III', + ), + array ( + 'test' => 'James\\ \\"Jim\\" \\= Smith\\, III', + 'expected' => 'James "Jim" = Smith, III', + ), + array ( + 'test' => '\\20\\20\\3cjsmith@test.local\\3e\\20', + 'expected' => ' ', + ), + array ( + 'test' => '\\ \\\\ ', + 'expected' => ' ', + ), + array ( + 'test' => 'Lu\\C4\\8Di\\C4\\87', + 'expected' => 'Lučić', + ), + ); + + foreach ($tests as $test) { + $this->assertSame($test['expected'], ldap_stripslashes($test['test'])); + } + } +}