RE: I think there's a bug with p->sasl_maxbuf in cyrus.c (ITS#2770)

[hyc@highlandsun.com]

It looks like all of that cruft was only needed for Cyrus SASL 1.5, which we
are no longer supporting. Cyrus SASL 2 checks its buffer sizes itself, so we
can just delete everything that deals with maxbuf from libldap.

  -- Howard Chu
  Chief Architect, Symas Corp.       Director, Highland Sun
  http://www.symas.com               http://highlandsun.com/hyc
  Symas: Premier OpenSource Development and Support

> -----Original Message-----
> From: owner-openldap-bugs@OpenLDAP.org
> [mailto:owner-openldap-bugs@OpenLDAP.org]On Behalf Of
> kingsnoopy7@yahoo.com
> Sent: Wednesday, October 15, 2003 4:53 PM
> To: openldap-its@OpenLDAP.org
> Subject: I think there's a bug with p->sasl_maxbuf in cyrus.c
> (ITS#2770)
>
>
> Full_Name: David Abouav
> Version: 2.1.22
> OS: FreeBSD
> URL: ftp://ftp.openldap.org/incoming/
> Submission from: (NULL) (66.121.131.203)
>
>
> The best description can be found from the emails I posted to
> the newsgroup.
> I've reproduced them here:
>
> --- first email, sent on 10/15/03
>
> Hi All,
>
> I think there's a bug with p->sasl_maxbuf in cyrus.c.
> I'll explain what I think the bug is first, and then
> explain how I came across it afterward.
>
> In sb_sasl_setup of cyrus.c, the variable
> p->sasl_maxbuf is set like this:
>
>   sasl_getprop( p->sasl_context, SASL_MAXOUTBUF,
>                (SASL_CONST void **) &p->sasl_maxbuf );
>
> And it's then later used in sb_sasl_read like this:
>
>   /* The new packet always starts at
>      p->sec_buf_in.buf_base
>    */
>   ret = sb_sasl_pkt_length( p->sec_buf_in.buf_base,
>                            *p->sasl_maxbuf,
>                            sbiod->sbiod_sb->sb_debug
> );
>
>   /* Grow the packet buffer if neccessary */
>   if ( ( p->sec_buf_in.buf_size < (ber_len_t) ret ) &&
>        ber_pvt_sb_grow_buffer( &p->sec_buf_in, ret ) <
> 0 )
>     {
>       errno = ENOMEM;
>       return -1;
>     }
>
> In other words, it's used to limit the length of a
> packet a client can read from a server. I think that
> using this buffer size value for this is incorrect.
>
> The sasl_getprop call returns the value of the SASL
> variable o->sasl_context->oparams.maxoutbuf.
> oparams.maxoutbuf is the maximum data that can be fed
> into the SASL layer for encoding. In other words, the
> maximum amount of data that can be *sent to the
> server* by a client. However, here in sb_sasl_read
> it's instead being used to set a limit on the maximum
> packet size we can *read from the server*.
>
> I base all of this on the following code in Cyrus Sasl
> 2.1.15:
>
> function gssapi_client_mech_step in plugins/gssapi.c:
>
>   oparams->maxoutbuf =
>    (((unsigned char *) output_token->value)[1] << 16)
> |
>    (((unsigned char *) output_token->value)[2] << 8) |
>    (((unsigned char *) output_token->value)[3] << 0);
>
> where output_token has been unwrapped from a server
> response. To further backup my claim, the only place
> in the entire SASL code where oparams->maxoutbuf is
> referenced is in the function sasl_encodev, which is a
> function used for encoding data to be sent (in this
> case, sent to the server). In that function,
> oparams->maxoutbuf is checked to make sure that not
> too much data is being encoded (i.e. more than the
> server advertised it could handle).
>
> Incidentally, why is there a need for incoming data to
> be limited to any negotiated size, since the code just
> below the call to sb_sasl_pkt_length grows the local
> buffer anyhow?
>
> Am I way off base here, or am I correct that the code
> in cyrus.c is using the wrong buffer size to
> unnecessarily limit the packets read from the server?
>
> Here's how I discovered this:
>
> I have an LDAP client that talks to Windows domain
> controllers using GSSAPI authentication and
> signing/sealing. It worked fine against Windows 2000
> domain controllers, but then broke against Windows
> 2003 DCs (in fact, it caused LSASS to die on the DC
> and the DC to then reboot). So I tried upgrading to
> use Cyrus 2.1.15. This fixed all problems with 2003,
> but now Windows 2000 no longer worked (though it
> didn't crash).
>
> I found the problem to be in in plugins/gssapi.c. The
> code which reads the oparams->maxoutbuf used to look
> like this in Cyrus 2.1.4:
>   oparams->maxoutbuf = (*(unsigned long
> *)output_token->value & 0xFFFFFF);
>
> but in Cyrus 2.1.15, looks like this (same as above):
>
>   oparams->maxoutbuf =
>    (((unsigned char *) output_token->value)[1] << 16)|
>    (((unsigned char *) output_token->value)[2] << 8) |
>    (((unsigned char *) output_token->value)[3] << 0);
>
> Also, I found out that the output_token->value sent
> from the server is different for Windows 2000 versus
> 2003:
>   Windows 2000: 00 00 a0 07
>   Windows 2003: 00 40 00 07
>
> Note that byte 0 (0x07) is not part of the maxoutbuf.
> So it seems that at some point the SASL folks realized
> that they were incorrectly parsing this field, and in
> Windows 2003 the Microsoft folks realized that they
> were incorrectly sending this field.
>
> In either case, the incorrect parsing of the field
> resulted in a server buffer size which was lower than
> the packet sizes that I was actually receiving from
> the server. I didn't think that this should really
> matter, since I send very small packets (requests) to
> the server anyway. Researching this problem led me to
> the OpenLDAP find.
>
> ------ end of first email
>
> ------ first response, sent by Howard Chu
>
> Perhaps there's a bug here. The Cyrus code has changed enough
> times that we
> may have missed something. But the SASL RFCs (see RFC 2222
> and 2831) specify
> that both the client and the server send each other a maxbuf
> value, and I
> presume that we have to honor it. A comment was made at one
> time that the
> SASL library insures this itself, so perhaps we can remove
> those checks. I've
> found that relying on the Cyrus SASL library to Do The Right
> Thing has often
> led to frustration... Maybe you should submit a new ITS for
> this if you
> actually want someone to investigate it.
>
>   -- Howard Chu
>
> ------ next response, sent by myself
>
> What you said about the max buffer sizes is true, but
> it should only ever apply to writes, and not reads.
>
> The server advertises the most data it can receive,
> which means that SASL and or OpenLDAP needs to ensure
> that it doesn't write more than this when it's used as
> a client (i.e. ldapsearch performing a query).
>
> The client also advertises how much data it can
> receive, which means that SASL and or OpenLDAP needs
> to ensure that it doesn't write more than this when
> it's used as a server (i.e. the slapd LDAP server
> writing a response).
>
> The code in sb_sasl_read should not care about any
> buffer sizes except those needed to store the data
> read. In OpenLDAP's case, this is malloced anyway.
>
> What is an ITS and how do I submit one?
>
> Dave
>
> ----- next response, sent by Quanah Gibson-Mount
>
> http://www.openldap.org/its/
>
> It is the Issue Tracking System.
>
> --Quanah
>
> ----- next response, sent by myself
>
> Thanks.
>
> Incidentally, the correct way to check for SASL's read
> buffer size is with a call to:
>
> ldap_int_sasl_get_option(ld,
> LDAP_OPT_X_SASL_MAXBUFSIZE, &value);
>
> Unfortunately, doing this the proper way will break
> Windows 2000 compatibility if you're using the latest
> Cyrus-SASL (2.1.15). Why? I just noticed that my
> response packets from a Windows 2000 DC were sometimes
> quite large (~272KB). This is despite the fact that
> the SASL client maxbufsize (advertised to the server)
> is set by the OpenLDAP code by default to 65KB.
> Windows 2003 is better behaved, and never returns
> responses bigger than this.
>
> By a miracle though, everything still worked with
> Windows 2000 and SASL 2.1.4 up until now because the
> OpenLDAP code is using the wrong buffer size to check
> the size of the server response with. It's using the
> buffer size of how much we can write to the server,
> which is generally very large (> 4MB). But using the
> newer SASL against a 2000 DC results in this server
> buffer size being very small (~16KB) due to the flag
> misinterpretation I mentioned before. In other words,
> SNAFU.
>