[EMAIL PROTECTED] - Wed Sep 29 10:50:39 2004]:
> Hello,
>
> Going through Red Hat's 7.3 openssl-0.9.6b-35.7 openssl-0.9.6b-sec.patch
> I noticed that in openssl-engine-0.9.6m (& SNAP 2004-09-26) there is a
> hunk from this patch missing. Everything else has been merged, so I
> believe this to be an unintentional omission.
>
> In RHL 7.3's openssl-0.9.6b-sec.patch there is this one hunk:
>
> --- ./ssl/ssl_asn1.c.chats Thu Apr 5 21:28:48 2001
> +++ ./ssl/ssl_asn1.c Thu Jul 25 16:41:00 2002
> @@ -275,6 +276,7 @@
> os.length=i;
>
> ret->session_id_length=os.length;
> + die(os.length <= sizeof ret->session_id);
> memcpy(ret->session_id,os.data,os.length);
>
> M_ASN1_D2I_get(osp,d2i_ASN1_OCTET_STRING);
>
> In 0.9.6m die() is usually substituted with a
> + if (os.length > sizeof ret->session_id)
> + {
> + SSLerr(,);
> + return -1;
> + }
> block.
>
> In 0.9.6m this check is missing and should be added:
>
> ret->session_id_length=os.length;
> memcpy(ret->session_id,os.data,os.length);
>
> It is there in 0.9.7d:
>
> ret->session_id_length=os.length;
> OPENSSL_assert(os.length <= sizeof ret->session_id);
> memcpy(ret->session_id,os.data,os.length);
>
> The fact that there is an OPENSSL_assert in 0.9.7d makes me believe the
> hunk was dropped unintentionally in the 0.9.6 branch, and should be
> reintroduced.
>
> The patch below reintroduces die(), and the declaration of OpenSSLDie().
> The definition of OpenSSLDie() is still in cryptlib.c. If you decide to
> use another construct instead of die() the definition of OpenSSLDie()
> should probably be removed.
>
The OPENSSL_assert() above isn't in OpenSSL 0.9.7e.
Considering this issue in detail:
If we consider the lines before the check we have:
if ((ssl_version>>8) == SSL3_VERSION)
i=SSL3_MAX_SSL_SESSION_ID_LENGTH;
else /* if (ssl_version == SSL2_VERSION) */
i=SSL2_MAX_SSL_SESSION_ID_LENGTH;
if (os.length > i)
os.length = i;
if (os.length > sizeof ret->session_id) /* can't happen */
os.length = sizeof ret->session_id;
ret->session_id_length=os.length;
memcpy(ret->session_id,os.data,os.length);
In the first case becasue of the values of
SSL{2,3}_MAX_SSL_SESSION_ID_LENGTH (both 32) and the deinifion of
ret->sess_id:
unsigned char session_id[SSL_MAX_SSL_SESSION_ID_LENGTH];
and because SSL_MAX_SSL_SESSION_ID_LENGTH is also 32
sizeof(ret->seession_id) is 32.
So in the event that os.length > 32 it will be reduced to 32 by the
first check:
if (os.length > i)
os.length = i;
the second check (which will never be performed in practice):
if (os.length > sizeof ret->session_id) /* can't happen */
os.length = sizeof ret->session_id;
would also keep os.length <= sizeof ret->session_id. So the following
memcpy() can never overflow the buffer.
Steve.
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List [EMAIL PROTECTED]
Automated List Manager [EMAIL PROTECTED]
