Malformed Packet appears in network trace of SASL binding

RESOLVED FIXED

Status

Directory
LDAP C SDK
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Xu Qiang, Assigned: Xu Qiang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.2; WOW64; SV1)
Build Identifier: 6.0.6

Using Mozilla LDAP library, I can do SASL binding successfully. But the network trace shows it is not perfect.

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]
  Lightweight-Directory-Access-Protocol
    LDAPMessage bindRequest(2) "<ROOT>"
      messageID: 2
      protocolOp: bindRequest (0)
        bindRequest
          version: 3
          name: 
          authentication: sasl (3)
            sasl
              mechanism: GSSAPI
              credentials: <MISSING>
    [Malformed Packet: LDAP]
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. Luckily, it will not affect the result of SASL binding.

In contrast, a trace captured with OpenLDAP ldapsearch utility does not have this malformat packet: 
========================================
20	24.622555	13.198.98.190	13.198.98.35	LDAP	bindRequest(1) "<ROOT>" sasl 
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 
  Lightweight-Directory-Access-Protocol
    LDAPMessage bindRequest(2) "<ROOT>"
      messageID: 2
      protocolOp: bindRequest (0)
        bindRequest
          version: 3
          name: 
          authentication: sasl (3)
            sasl
              mechanism: GSSAPI
        [Response In: 29]
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 28 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 28 in OpenLDAP trace is not.


Reproducible: Always

Steps to Reproduce:
1. Set up DNS correctly.
2. Set up Kerberos server accordingly.
3. Use WireShare to capture network trace.
4. Do an SASL binding, either with the client tool ldapsearch, or write a simple program calling the library funcs.

Actual Results:  
In the captured network trace, you will find some LDAP SASL packets are marked with "Malformed Packet".

Expected Results:  
There should be no Malformed Packet.

Looking into the code of MozLDAP, I found the following is suspicous: 
========================================
int
LDAP_CALL
ldap_sasl_bind(
    LDAP		*ld,
    const char		*dn,
    const char		*mechanism,
    const struct berval	*cred,
    LDAPControl		**serverctrls,
    LDAPControl		**clientctrls,
    int			*msgidp
)
{
  ...
		if ( cred == NULL ) {
			rc = ber_printf( ber, "{it{ist{s}}", msgid,
			    LDAP_REQ_BIND, ldapversion, dn, LDAP_AUTH_SASL,
			    mechanism );
		} else {
			rc = ber_printf( ber, "{it{ist{so}}", msgid,
			    LDAP_REQ_BIND, ldapversion, dn, LDAP_AUTH_SASL,
			    mechanism, cred->bv_val,
			    cred->bv_len );
		}
  ...
}
========================================
At the first look, it seem innocent. But...

Let's have a look at OpenLDAP's code as well: 
========================================
int
ldap_sasl_bind(
	LDAP			*ld,
	LDAP_CONST char	*dn,
	LDAP_CONST char	*mechanism,
	struct berval	*cred,
	LDAPControl		**sctrls,
	LDAPControl		**cctrls,
	int				*msgidp )
{
  ...
	if( mechanism == LDAP_SASL_SIMPLE ) {
		/* simple bind */
		rc = ber_printf( ber, "{it{istON}" /*}*/,
			id, LDAP_REQ_BIND,
			ld->ld_version, dn, LDAP_AUTH_SIMPLE,
			cred );
		
	} else if ( cred == NULL || cred->bv_val == NULL ) {
		/* SASL bind w/o credentials */
		rc = ber_printf( ber, "{it{ist{sN}N}" /*}*/,
			id, LDAP_REQ_BIND,
			ld->ld_version, dn, LDAP_AUTH_SASL,
			mechanism );

	} else {
		/* SASL bind w/ credentials */
		rc = ber_printf( ber, "{it{ist{sON}N}" /*}*/,
			id, LDAP_REQ_BIND,
			ld->ld_version, dn, LDAP_AUTH_SASL,
			mechanism, cred );
	}
  ...
}
========================================
Comparing both snippets of MozLDAP and OpenLDAP, I am wondering whether MozLDAP should add the condition "|| cred->bv_val == NULL" to the already existing one "if ( cred == NULL )" to decide whether to do SASL binding without explicit credential.

So, the proposed fix is in the following: 
========================================
int
LDAP_CALL
ldap_sasl_bind(
    LDAP		*ld,
    const char		*dn,
    const char		*mechanism,
    const struct berval	*cred,
    LDAPControl		**serverctrls,
    LDAPControl		**clientctrls,
    int			*msgidp
)
{
  ...
#if 0
		if ( cred == NULL ) {
#else
		if ( cred == NULL || cred->bv_val == NULL || cred->bv_len == 0) {
#endif
			rc = ber_printf( ber, "{it{ist{s}}", msgid,
			    LDAP_REQ_BIND, ldapversion, dn, LDAP_AUTH_SASL,
			    mechanism );
		} else {
			rc = ber_printf( ber, "{it{ist{so}}", msgid,
			    LDAP_REQ_BIND, ldapversion, dn, LDAP_AUTH_SASL,
			    mechanism, cred->bv_val,
			    cred->bv_len );
		}
  ...
}
========================================
Thank Rich, for his suggestion of adding the last condition of credential buffer's value len.

Yet, I cannot directly change the code, which is managed by OS team in Xerox. I will pass the change to them, and try to let them build a new library for me to try out.

At the same time, if any of you guys have a chance to verify the fix, I will be grateful to you.
(Assignee)

Comment 1

8 years ago
Created attachment 398058 [details] [diff] [review]
Patch to fix the Malformed Packet in SASL binding trace

In libldap folder, type "patch -p0 < saslbind.c.diff" to apply the patch. Do SASL binding before and after the patch is applied, and compare the network traces captured, then you'll notice the difference.

Comment 2

8 years ago
The patch looks good.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

8 years ago
Rich: 

Can you help me check in the change? I have difficulty with CVS, you know.

Thanks a lot,
Xu Qiang

Comment 4

8 years ago
I think patch should be marked as review+ and commit is doing by someone from Mozilla if I not mistaken.
Ludo any ideas who should we ping for review and commit?

Updated

8 years ago
Assignee: nobody → Qiang.Xu
Comment on attachment 398058 [details] [diff] [review]
Patch to fix the Malformed Packet in SASL binding trace

According to http://www.mozilla.org/hacking/reviewers.html we can ask dmose. If not he'll know how to ask.
Attachment #398058 - Flags: review?(dmose)

Comment 6

8 years ago
I have reviewed it, but I do not have the ability to set the review+ flag.
(In reply to comment #6)
> I have reviewed it, but I do not have the ability to set the review+ flag.

You need to be a peer for that. Let's wait for dan to have a look or for him to tell us who to ping.
Attachment #398058 - Flags: review?(dmose) → review+
Comment on attachment 398058 [details] [diff] [review]
Patch to fix the Malformed Packet in SASL binding trace

Setting r+ on behalf of Rich based on comment 2 and comment 6.
Checking in saslbind.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c,v  <--  saslbind.c
new revision: 5.6; previous revision: 5.5
done

Assuming this bug is now fixed.

I'm looking at getting comm-central to pick it up in bug 520476.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.