[Freeipa-devel] [PATCH 0019] Add proper DN escaping before LDAP library calls
Adam Tkac
atkac at redhat.com
Wed May 9 12:17:01 UTC 2012
On 05/09/2012 02:11 PM, Petr Spacek wrote:
> On 05/09/2012 01:24 PM, Adam Tkac wrote:
>> On 05/03/2012 03:46 PM, Petr Spacek wrote:
>>> On 05/03/2012 11:25 AM, Petr Spacek wrote:
>>>> Hello,
>>>>
>>>> this patch adds missing DNS->LDAP escaping conversion. It's
>>>> necessary to
>>>> prevent (potential) LDAP injection attacks in future.
>>>>
>>>> Code isn't very nice, because DNS users decimal escaping \123, LDAP
>>>> uses
>>>> hexadecimal escaping \ab and set of escaped characters is smaller
>>>> in DNS than
>>>> in LDAP.
>>>>
>>>> Any improvements are welcome.
>>>>
>>>> Petr^2 Spacek
>>>
>>> Here is second version of the patch.
>>>
>>> Changes:
>>> - comments
>>> - order of [._-] in if()
>>> - function was renamed to dns_to_ldap_dn_escape()
>>>
>>> Escaping logic itself wasn't changed.
>>
>> Hello Peter,
>>
>> please check my comments inside the patch.
> Oh, I feel so ashamed. All errors were corrected, see attachment.
>
> Petr^2 Spacek
Ack, please push it to master.
>
>>
>> Regards, Adam
>>
>>>
>>> Petr^2 Spacek
>>>
>>> bind-dyndb-ldap-pspacek-0019-2-Add-proper-DN-escaping-before-LDAP-library-calls.patch
>>>
>
> bind-dyndb-ldap-pspacek-0019-3-Add-proper-DN-escaping-before-LDAP-library-calls.patch
>
>
> From 1db133bf26a3c8bec7da3b045258bad630378d50 Mon Sep 17 00:00:00 2001
> From: Petr Spacek<pspacek at redhat.com>
> Date: Mon, 23 Apr 2012 11:38:43 +0200
> Subject: [PATCH] Add proper DN escaping before LDAP library calls.
> Signed-off-by: Petr Spacek<pspacek at redhat.com>
>
> ---
> src/ldap_convert.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++--
> src/zone_register.c | 7 +++
> src/zone_register.h | 3 +
> 3 files changed, 114 insertions(+), 5 deletions(-)
>
> diff --git a/src/ldap_convert.c b/src/ldap_convert.c
> index 6405a98fda942cbba10b4c862351c4e3695aba85..6cb761db97cff12b999263e2fcb4f31603ccd733 100644
> --- a/src/ldap_convert.c
> +++ b/src/ldap_convert.c
> @@ -21,6 +21,7 @@
> #include<isc/buffer.h>
> #include<isc/mem.h>
> #include<isc/util.h>
> +#include<isc/string.h>
>
> #include<dns/name.h>
> #include<dns/rdatatype.h>
> @@ -32,6 +33,7 @@
>
> #include<errno.h>
> #include<strings.h>
> +#include<ctype.h>
>
> #include "str.h"
> #include "ldap_convert.h"
> @@ -189,6 +191,96 @@ cleanup:
> return result;
> }
>
> +/**
> + * Convert a string from DNS escaping to LDAP escaping.
> + * The Input string dns_str is expected to be the result of dns_name_tostring().
> + * The DNS label can contain any binary data as described in
> + * http://tools.ietf.org/html/rfc2181#section-11 .
> + *
> + * DNS escaping uses form "\123" = ASCII value 123 (decimal)
> + * LDAP escaping users form "\7b" = ASCII value 7b (hexadecimal)
> + *
> + * Input (DNS escaped) example : _aaa,bbb\255\000ccc.555.ddd-eee
> + * Output (LDAP escaped) example: _aaa\2cbbb\ff\00ccc.555.ddd-eee
> + *
> + * The DNS to text functions from ISC libraries do not convert certain
> + * characters (e.g. ","). This function converts \123 form to \7b form in all
> + * cases. Other characters (not escaped by ISC libraries) will be additionally
> + * converted to the LDAP escape form.
> + * Input characters [a-zA-Z0-9._-] are left in raw ASCII form.
> + *
> + * If dns_str consists only of the characters in the [a-zA-Z0-9._-] set, it
> + * will be checked& copied to the output buffer, without any additional escaping.
> + */
> +isc_result_t
> +dns_to_ldap_dn_escape(isc_mem_t *mctx, const char const * dns_str, char ** ldap_name) {
> + isc_result_t result = ISC_R_FAILURE;
> + char * esc_name = NULL;
> + int idx_symb_first = -1; /* index of first "nice" printable symbol in dns_str */
> + int dns_idx = 0;
> + int esc_idx = 0;
> +
> + REQUIRE(dns_str != NULL);
> + REQUIRE(ldap_name != NULL&& *ldap_name == NULL);
> +
> + int dns_str_len = strlen(dns_str);
> +
> + /**
> + * In worst case each symbol from DNS dns_str will be represented
> + * as "\xy" in ldap_name. (xy are hexadecimal digits)
> + */
> + CHECKED_MEM_ALLOCATE(mctx, *ldap_name, 3 * dns_str_len + 1);
> + esc_name = *ldap_name;
> +
> + for (dns_idx = 0; dns_idx< dns_str_len; dns_idx++) {
> + if (isalnum(dns_str[dns_idx]) || dns_str[dns_idx] == '.'
> + || dns_str[dns_idx] == '-' || dns_str[dns_idx] == '_' ) {
> + if (idx_symb_first == -1)
> + idx_symb_first = dns_idx;
> + continue;
> + } else { /* some not very nice symbols */
> + int ascii_val;
> + if (idx_symb_first != -1) { /* copy previous nice part */
> + int length_ok = dns_idx - idx_symb_first;
> + memcpy(esc_name + esc_idx, dns_str + idx_symb_first, length_ok);
> + esc_idx += length_ok;
> + idx_symb_first = -1;
> + }
> + if (dns_str[dns_idx] != '\\') { /* not nice raw value, e.g. ',' */
> + ascii_val = dns_str[dns_idx];
> + } else { /* not nice value in DNS \123 decimal format */
> + /* check if input length<= expected size */
> + if (dns_str_len<= dns_idx + 3) { /* this should never happen */
> + log_bug("dns string too short");
> + result = ISC_R_NOSPACE;
> + goto cleanup;
> + }
> + ascii_val = 100 * (dns_str[dns_idx + 1] - '0')
> + + 10 * (dns_str[dns_idx + 2] - '0')
> + + (dns_str[dns_idx + 3] - '0');
> + dns_idx += 3;
> + }
> + /* LDAP uses \xy escaping. "xy" represent two hexadecimal digits.*/
> + /* TODO: optimize to bit mask& rotate& dec->hex table? */
> + CHECK(isc_string_printf(esc_name + esc_idx, 4, "\\%02x", ascii_val));
> + esc_idx += 3; /* isc_string_printf wrote 4 bytes including '\0' */
> + }
> + }
> + if (idx_symb_first != -1) { /* copy last nice part */
> + int length_ok = dns_idx - idx_symb_first;
> + memcpy(esc_name + esc_idx, dns_str + idx_symb_first, dns_idx - idx_symb_first);
> + esc_idx += length_ok;
> + idx_symb_first = -1;
> + }
> + esc_name[esc_idx] = '\0';
> + return ISC_R_SUCCESS;
> +
> +cleanup:
> + if (*ldap_name)
> + isc_mem_free(mctx, *ldap_name);
> + return result;
> +}
> +
> static isc_result_t
> explode_dn(const char *dn, char ***explodedp, int notypes)
> {
> @@ -243,11 +335,15 @@ dnsname_to_dn(zone_register_t *zr, dns_name_t *name, ld_string_t *target)
> isc_result_t result;
> int label_count;
> const char *zone_dn = NULL;
> + char *dns_str = NULL;
> + char *escaped_name = NULL;
>
> REQUIRE(zr != NULL);
> REQUIRE(name != NULL);
> REQUIRE(target != NULL);
>
> + isc_mem_t * mctx = zr_get_mctx(zr);
> +
> /* Find the DN of the zone we belong to. */
> {
> DECLARE_BUFFERED_NAME(zone);
> @@ -264,26 +360,28 @@ dnsname_to_dn(zone_register_t *zr, dns_name_t *name, ld_string_t *target)
>
> str_clear(target);
> if (label_count> 0) {
> - DECLARE_BUFFER(buffer, DNS_NAME_MAXTEXT);
> dns_name_t labels;
>
> - INIT_BUFFER(buffer);
> dns_name_init(&labels, NULL);
> -
> dns_name_getlabelsequence(name, 0, label_count,&labels);
> - CHECK(dns_name_totext(&labels, ISC_TRUE,&buffer));
> + CHECK(dns_name_tostring(&labels,&dns_str, mctx));
>
> + CHECK(dns_to_ldap_dn_escape(mctx, dns_str,&escaped_name));
> CHECK(str_cat_char(target, "idnsName="));
> - CHECK(str_cat_isc_buffer(target,&buffer));
> + CHECK(str_cat_char(target, escaped_name));
> /*
> * Modification of following line can affect modify_ldap_common().
> * See line with: char *zone_dn = strstr(str_buf(owner_dn),", ") + 1;
> */
> CHECK(str_cat_char(target, ", "));
> }
> CHECK(str_cat_char(target, zone_dn));
>
> cleanup:
> + if (dns_str)
> + isc_mem_free(mctx, dns_str);
> + if (escaped_name)
> + isc_mem_free(mctx, escaped_name);
> return result;
> }
>
> @@ -328,3 +426,4 @@ rdatatype_to_ldap_attribute(dns_rdatatype_t rdtype, const char **target)
>
> return ISC_R_SUCCESS;
> }
> +
> diff --git a/src/zone_register.c b/src/zone_register.c
> index fc6dc076ac91ae2f21ecf934d0d6c837f1581766..81d208fc6e3c0dba6efb72ae10db54a79c336eca 100644
> --- a/src/zone_register.c
> +++ b/src/zone_register.c
> @@ -61,6 +61,13 @@ zr_get_rbt(zone_register_t *zr)
> return zr->rbt;
> }
>
> +isc_mem_t *
> +zr_get_mctx(zone_register_t *zr) {
> + REQUIRE(zr);
> +
> + return zr->mctx;
> +}
> +
> /*
> * Create a new zone register.
> */
> diff --git a/src/zone_register.h b/src/zone_register.h
> index e2408cbf8630effc0036c3765535a84381f83117..fa8ef255ef9cf212bca04aaafba0e6450d40a5c6 100644
> --- a/src/zone_register.h
> +++ b/src/zone_register.h
> @@ -45,4 +45,7 @@ zr_get_zone_ptr(zone_register_t *zr, dns_name_t *name, dns_zone_t **zonep);
> dns_rbt_t *
> zr_get_rbt(zone_register_t *zr);
>
> +isc_mem_t *
> +zr_get_mctx(zone_register_t *zr);
> +
> #endif /* !_LD_ZONE_REGISTER_H_ */
More information about the Freeipa-devel
mailing list