Closed Bug 394071 Opened 17 years ago Closed 17 years ago

LDAP SASL bind fails when the given hostname is numeric IPv6 address

Categories

(Directory :: LDAP C SDK, defect)

Product:
Directory
Component:
LDAP C SDK
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: nhosoi, Assigned: mcs)

Details

Attachments

(2 files, 2 obsolete files)

2nd revised diff of common.c
6.14 KB, patch
Details | Diff | Splinter Review
cvs commit message
1.53 KB, text/plain
Details 
Sample error case:
$ ./ldapsearch -h [3ffe:1111:2222:1000::11] -p <port> -o "mech=GSSAPI" -s sub -b "ou=TestPeople,o=sasl.com" "(uid=testuser1)"
Bind Error: Local error
Bind Error: additional info: SASL(-1): generic failure: GSSAPI Error: An invalid name was supplied (Hostname cannot be canonicalized)

When binding, LDAP tools call nsldapi_sasl_open via 
ldap_sasl_interactive_bind_ext_s.  In the function, it calls the sasl client 
init function sasl_client_new.  nsldapi_sasl_open passes the "host" as users
type, but sasl_client_new expects serverRQDN (as attached below).  Note that
sasl_client_new handles correctly if the host is numeric IPv4 address.
[mozilla/directory/c-sdk/ldap/libldap/saslio.c]
int nsldapi_sasl_open( LDAP *ld, LDAPConn *lconn, sasl_conn_t **ctx, sasl_ssf_t ssf )
{
    int saslrc;
    char *host = NULL;
		[...]
    if ( 0 != ldap_get_option( ld, LDAP_OPT_HOST_NAME, &host ) ) {
                LDAP_SET_LDERRNO( ld, LDAP_LOCAL_ERROR, NULL, NULL );
                return( LDAP_LOCAL_ERROR );
    }

    saslrc = sasl_client_new( "ldap", host,
	NULL, NULL, /* iplocalport, ipremoteport - use defaults */
	NULL, 0, ctx );
    ldap_memfree(host);
	[...]
	

[cyrus-sasl/lib/client.c]
/* initialize a client exchange based on the specified mechanism
 *  service       -- registered name of the service using SASL (e.g. "imap")
 *  serverFQDN    -- the fully qualified domain name of the server
 *  iplocalport   -- client IPv4/IPv6 domain literal string with port
 *                    (if NULL, then mechanisms requiring IPaddr are disabled)
 *  ipremoteport  -- server IPv4/IPv6 domain literal string with port
 *                    (if NULL, then mechanisms requiring IPaddr are disabled)
 *  prompt_supp   -- list of client interactions supported
 *                   may also include sasl_getopt_t context & call
 *                   NULL prompt_supp = user/pass via SASL_INTERACT only
 *                   NULL proc = interaction supported via SASL_INTERACT
 *  secflags      -- security flags (see above)
 * in/out:
 *  pconn         -- connection negotiation structure
 *                   pointer to NULL => allocate new
 *                   non-NULL => recycle storage and go for next available mech
 *
 * Returns:
 *  SASL_OK       -- success
 *  SASL_NOMECH   -- no mechanism meets requested properties
 *  SASL_NOMEM    -- not enough memory
 */

I also tried the OpenLDAP client.  It does not support the numeric IPv6
address correctly either:
$ ldapsearch -h [3ffe:1111:2222:1000::11] -p 11111 -Y "mech=GSSAPI" -s sub -b "ou=TestPeople,o=sasl.com" "(uid=testuser1)"
SASL/GSSAPI authentication started
ldap_sasl_interactive_bind_s: Local error (-2)
        additional info: SASL(-1): generic failure: GSSAPI Error: An invalid name was supplied (Cannot determine realm for numeric host address)


To get FQDN from a numeric IPv6 address, we may need to use getaddrinfo & getnameinfo.
Interesting.  This may be a silly question, but is it important to support SASL with IP addresses (if a fully qualified domain name is needed by SASL, I assume most LDAP SASL applications will know to pass one in).  Adding reverse DNS lookups inside libldap is a little messy.  Maybe the LDAP tools should do a reverse lookup when needed.

Also, I do not understand why this already works for IPv4 but not for IPv6.  Is lack of IPv6 support a limitation of the Cyrus SASL library?  Can that be fixed?
(In reply to comment #1)
> Interesting.  This may be a silly question, but is it important to support SASL
> with IP addresses (if a fully qualified domain name is needed by SASL, I assume
> most LDAP SASL applications will know to pass one in).  Adding reverse DNS
> lookups inside libldap is a little messy.  Maybe the LDAP tools should do a
> reverse lookup when needed.

Hi Mark!  Actually, I am asking the same question to myself. :)  And I have to agree with you that introducing reverse DNS lookup for IPv6 in the generic form looks pretty tough.  Regarding the importance, besides for our QA effort :), only a case I could think of is for the Directory Server administrators to check the IPv6 configuration.  When some network problem occurs, it might help them to troubleshoot.  Other than that... :)

> Also, I do not understand why this already works for IPv4 but not for IPv6.  Is
> lack of IPv6 support a limitation of the Cyrus SASL library?  Can that be
> fixed?
We could file a bug to Cyrus SASL.  Based upon this spec "serverFQDN -- the fully qualified domain name of the server" for the second argument of sasl_client_new, working for IPv4 would be a bonus, I guess, though...
Attached patch cvs diffs (obsolete) — — Splinter Review 
Files:
 configure.in
 config/autoconf.mk.in
 ldap/clients/tools/Makefile.in
 ldap/clients/tools/common.c
 (configure is also modified, but not attached here)

Fix Description:
As Mark suggested, I'm proposing to add the conversion function just to tools/common.c.
It's called from ldaptool_ldap_init and used only if the given host name is IPv6 numeric address.  If the conversion fails, the ldap client utilities do the operation with the use-passed host name.
The function is made from 3 parts:
1. check if the given host name looks like IPv6 numeric address or not
2. if it is, try 

Here's the sample usage:
$ ./ldapsearch -h '3ffe:1111:2222:3333::ff' -v -D "cn=Directory Manager" -w password -b "cn=config" -s base "(objectclass=*)" dn
[...]
ldap_init( hostA.full.domain.com, 389 )
filter pattern: (objectclass=*)
returning: dn

configure.in: added getaddrinfo and getnameinfo to AC_CHECK_FUNCS
config/autoconf.mk.in: added macros HAVE_GETADDRINFO and HAVE_GETNAMEINFO
Attachment #280836 - Flags: review?(mcs)
Oope, this part was not completed.

The function is made from 3 parts:
1. check if the given host name looks like IPv6 numeric address or not
2. if it is, try getaddrinfo + getnameinfo if available
3. if they are not or they failed, try NSPR functions, which are a bit old handling IPv6 addresses, but for this case, they return the expected result -- FQDN.
If you are looking for two colons, I don't think this will work:
    hostp = strchr( numaddr, ':' );
    if ( hostp ) {
        hostp = strchr( numaddr, ':' );
/* should use (hostp+1) instead of numaddr here? */
        if ( hostp )


Do you need to see if len(numaddr) >= blen here, before doing strcpy into buf?
            } else {
                isIPv6Numeric = 1;
                strcpy( buf, numaddr );
            }
You should use type size_t for string lengths and buffer sizes and structure sizes.  See man strlen, man strncpy, man getaddrinfo, and man getnameinfo.  All of the length/size parameters and return values (including sizeof) are size_t.
For example, 
    size_t         hlen = sizeof(hname);
or
    size_t         slen = 0;
Attached patch revised diff of common.c (obsolete) — — Splinter Review 
Thank you for reviewing my proposal, Rich.
I fixed the 3 places you pointed out.
Attachment #280836 - Attachment is obsolete: true
Attachment #280907 - Flags: review?(richm)
Attachment #280836 - Flags: review?(mcs)
(In reply to comment #6)
> Created an attachment (id=280907) [details]
> revised diff of common.c
> 
> Thank you for reviewing my proposal, Rich.
> I fixed the 3 places you pointed out.

Ok.  Looks good.


Do you need to check against blen before this strcpy() as well:

 if ( workp ) { /* [address] form */
     strncpy( buf, hostp, workp - hostp );
     buf[workp - hostp] = '\0';
 } else {
     strcpy( buf, hostp );
     ^^^^^^^^^^^^^^^^^^^^^
 }
?

The parsing is messy.  Can you avoid that by letting getnameinfo() tell you if it is a numeric IPv6 address or not (I have not thought about this problem a lot, so maybe this is a bad suggestion).
Thank you, Mark!  *shame shame* ... :(

Regarding letting getnameinfo handle the address, I need to remove '[' and ']' from the string, but other than that, it works fine.  On the system with no getnameinfo and getaddrinfo, it converts all input into FQDN.  I thought we did not want to do it.  I don't see any harm, but it may be redundant if we do IPv4 address -> FQDN and use it.
The parse code is a little messy, but to make the impact to the code minimized, I'd like to keep it until we don't have to support platforms without getaddrinfo and getnameinfo...
Attachment #280907 - Attachment is obsolete: true
Attachment #280907 - Flags: review?(richm)
(In reply to comment #10)
> Created an attachment (id=280970) [details]
> 2nd revised diff of common.c
> 
> The parse code is a little messy, but to make the impact to the code minimized,
> I'd like to keep it until we don't have to support platforms without
> getaddrinfo and getnameinfo...
> 

Ok.
Attached file cvs commit message — 
Reviewed by Rich and Mark (Thank you very much!).

Checked in into HEAD.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Hi, Noriko: 

I have hit a similar problem as you. But it seems I can't use your fix, because your fix is in "clients/tools/common.c" which is probably used by the utility "ldapsearch".

My case is related to the dynamic library "libldap60.so". :-(

And, another scenario I hit is that when the Kerberos authentication server is configured with IPv6 address, SASL binding will fail.

Have you ever met the same problem as mine?

By the way, I am not sure if this is a MozLDAP defect or Cyrus SASL library defect.
Hi Xu,

I agree with you that it should be taken care in Cyrus SASL.  Since we were urgent to support the ldap client utilities, I put the fix into the command line code.

Luckily, we haven't run into the problem in the library level.  Could it be possible for you to do something similar as we did in your application?
If the passed-in LDAP server is an IPv6 address, we used a similar strategy as you. 

The difference is that we did a preliminary simple binding to find the server's attribute "dnsHostName", which usually is an FQDN. Then we use this FQDN to do SASL binding. 

That way, we walked around this problem. And this walk-around is done in the application level, beyond MozLDAP libraries.

Anyway, I think probably Cyrus SASL library need a fix to handle hard-coded IPv6 address of LDAP server.

My problem is a little bit more serious. As you know, SASL binding is done in conjunction with Kerberos authentication server. But when the Kerberos authentication server is configured with hard-coded IPv6 address, the binding breaks.

Another scenario is Kerberos authentication server is configured with hostname, and the option "Prefer IPv6 address over IPv4 address" in DNS server settings in our printer is ticked, so that DNS server will resolve the Kerberos hostname to IPv6 address, then SASL binding will break as well.

It should be a Cyrus SASL defect, right? Need your confirmation.
I think you should ask the question of the Cyrus SASL community.
http://cyrusimap.web.cmu.edu/lists.html
I didn't find a place to report a bug in Cyrus mailist, is it here: cyrus-bugzilla@lists.andrew.cmu.edu?
Hi, Noriko: 

Want to ask a silly question.

In your fix, you called getaddrinfo( buf, NULL, &hints, &ainfo ) and then rc = getnameinfo( saddr, slen, buf, (socklen_t)blen, NULL, 0, NI_NAMEREQD ) to convert the IPv6 address to an FQDN. 

But, isn't it enough by calling just the latter, i.e. getnameinfo() with the IPv6 address in buf?

I just don't understand why you call getaddrinfo() when buf already stores an IPv6 address.

Sorry for my dumb,
Xu Qiang
(In reply to comment #17)
> I didn't find a place to report a bug in Cyrus mailist, is it here:
> cyrus-bugzilla@lists.andrew.cmu.edu?

There is a bugzilla instance for Cyrus SASL bugs at http://bugzilla.andrew.cmu.edu/
(In reply to comment #18)
> Hi, Noriko: 
> 
> Want to ask a silly question.
> 
> In your fix, you called getaddrinfo( buf, NULL, &hints, &ainfo ) and then rc =
> getnameinfo( saddr, slen, buf, (socklen_t)blen, NULL, 0, NI_NAMEREQD ) to
> convert the IPv6 address to an FQDN. 
> 
> But, isn't it enough by calling just the latter, i.e. getnameinfo() with the
> IPv6 address in buf?
> 
> I just don't understand why you call getaddrinfo() when buf already stores an
> IPv6 address.
> 
> Sorry for my dumb,
> Xu Qiang

I called getaddrinfo to convert the numeric address in "char *" into sockaddr.  I also thought by calling getaddrinfo, we could guarantee the input string is valid. If not, it returns an error...
(In reply to comment #20)
> I called getaddrinfo to convert the numeric address in "char *" into sockaddr. 
> I also thought by calling getaddrinfo, we could guarantee the input string is
> valid. If not, it returns an error...

Just realized that the func getnameinfo() can't take a "char *" type as input directly. The source must be a "const struct sockaddr *" type. My negligence. 

Thanks for your clarification, Noriko!
Hi, Noriko: 

Another thing to seek your help.

Although my SASL binding is successful, there is some minor defect. Please look at the following trace I captured.

As you can see, the Malformed Packet is in the 2nd round of binding interaction with the server: 
========================================
32	17.839052	13.198.98.107	13.198.98.35	LDAP	bindRequest(1) "<ROOT>" sasl 
33	17.917608	13.198.98.35	13.198.98.107	LDAP	bindResponse(1) saslBindInProgress 
35	17.919333	13.198.98.107	13.198.98.35	LDAP	bindRequest(2) "<ROOT>" [Malformed Packet]
36	17.919637	13.198.98.35	13.198.98.107	LDAP	bindResponse(2) saslBindInProgress 
37	17.920316	13.198.98.107	13.198.98.35	LDAP	bindRequest(3) "<ROOT>" sasl 
38	17.920691	13.198.98.35	13.198.98.107	LDAP	bindResponse(3) success 
========================================
I am not sure if packet 35 is normal or not? After all, it says the packet is malformed.

In contrast, a trace captured with OpenLDAP ldapsearch utility does not have this malformat packet: 
========================================
22	24.805633	13.198.98.35	13.198.98.190	LDAP	bindResponse(1) saslBindInProgress 
28	26.616093	13.198.98.190	13.198.98.35	LDAP	bindRequest(2) "<ROOT>" sasl 
29	26.616459	13.198.98.35	13.198.98.190	LDAP	bindResponse(2) saslBindInProgress 
31	26.616705	13.198.98.190	13.198.98.35	LDAP	bindRequest(3) "<ROOT>" sasl 
32	26.633134	13.198.98.35	13.198.98.190	LDAP	bindResponse(3) success 
========================================
When I compare the content of packet 35 in MozLDAP trace and packet 29 in OpenLDAP trace, it is noted that the MozLDAP packet has extra bytes "04 00" after "mechanism: GSSAPI". These extra bytes are interpreted as "<MISSING> credentials" by WireShark. In contrast, although the OpenLDAP packet doesn't have any credential information as well, it doesn't have these extra bytes. That's why packet 35 in MozLDAP trace is marked as Malformed Packet, while packet 29 in OpenLDAP trace is not.

In your SASL binding with MozLDAP, have you observed the same [Malformed Packet] as in my network trace?

I want to make sure whether this is caused by the defect in MozLDAP or it is due to my incorrect calling of the SASL LDAP interface.

Thanks,
Xu Qiang
Hi Xu,

The case 1 with the Malformed Packet message is from you application?  Could it be possible to run mozldap ldapsearch doing the same bind?  If you could run it, does it also show "Malformed Packet"?

For instance, the LDAP C SDK api ldap_sasl_bind checks if the given credential is NULL or not.  If NULL, it does not put the octet string for the credential.  If an empty string "" is given, it might send such missing credential...

I'm talking about this if clause.  If you could run your application in some debugger and see which path it's taking, it might be able to tell us what's going on...
libldap/saslbind.c
540 int
541 LDAP_CALL
542 ldap_sasl_bind(
    [...]
620     /* fill it in */
621     if ( simple ) {     /* simple bind; works in LDAPv2 or v3 */
    [...]
633     } else {        /* SASL bind; requires LDAPv3 or better */
634         if ( cred == NULL ) {
635             rc = ber_printf( ber, "{it{ist{s}}", msgid,
636                 LDAP_REQ_BIND, ldapversion, dn, LDAP_AUTH_SASL,
637                 mechanism );
638         } else {
639             rc = ber_printf( ber, "{it{ist{so}}", msgid,
640                 LDAP_REQ_BIND, ldapversion, dn, LDAP_AUTH_SASL,
641                 mechanism, cred->bv_val,
642                 cred->bv_len );
643         }
644     }

Thanks,
--noriko
Thank you, Noriko. (In reply to comment #23)
> Hi Xu,
> The case 1 with the Malformed Packet message is from you application?  Could it
> be possible to run mozldap ldapsearch doing the same bind?  If you could run
> it, does it also show "Malformed Packet"?

Yes, it happens when our code is calling MozLDAP library to do SASL binding. I don't know how to use ldapsearch (from MozLDAP C-SDK) to do it. It hasn't been compiled, yet. The OS team only delivered the library part.

> For instance, the LDAP C SDK api ldap_sasl_bind checks if the given credential
> is NULL or not.  If NULL, it does not put the octet string for the credential. 
> If an empty string "" is given, it might send such missing credential...
> I'm talking about this if clause.  If you could run your application in some
> debugger and see which path it's taking, it might be able to tell us what's
> going on...
> libldap/saslbind.c
> 540 int
> 541 LDAP_CALL
> 542 ldap_sasl_bind(
>     [...]
> 620     /* fill it in */
> 621     if ( simple ) {     /* simple bind; works in LDAPv2 or v3 */
>     [...]
> 633     } else {        /* SASL bind; requires LDAPv3 or better */
> 634         if ( cred == NULL ) {
> 635             rc = ber_printf( ber, "{it{ist{s}}", msgid,
> 636                 LDAP_REQ_BIND, ldapversion, dn, LDAP_AUTH_SASL,
> 637                 mechanism );
> 638         } else {
> 639             rc = ber_printf( ber, "{it{ist{so}}", msgid,
> 640                 LDAP_REQ_BIND, ldapversion, dn, LDAP_AUTH_SASL,
> 641                 mechanism, cred->bv_val,
> 642                 cred->bv_len );
> 643         }
> 644     }

I have almost figured it out the other day. Here is the link: 
https://bugzilla.mozilla.org/show_bug.cgi?id=511817

But I must wait for OS team to build a new library with the change to test. If you help me to test the fix, I'll be very grateful.

Thanks,
Xu Qiang
Hi, Noriko: 

Just want you to know that the fix for the problem of "Malfomed Packet" (https://bugzilla.mozilla.org/show_bug.cgi?id=511817) has been verified. 

A patch has been submitted to be reviewed and checked in.

Thanks,
Xu Qiang
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: