The bug occurs when reading objects that are bigger than the SSL
record size from a SSL bio:
ASN1_d2i_bio calls BIO_read without checking that the amount received
is equal to the amount requested (This causes a problem becuase
BIO_read will never return more than a record worth of data when
reading from a ssl bio). The following patch seems to fix the problem.
To make things clearer here is the current bit of problematic code in
ASN1_d2i_bio with a few comments:
for (;;)
{
if (want >= (len-off))
{
want-=(len-off);
if (!BUF_MEM_grow(b,len+want))
{
ASN1err(ASN1_F_ASN1_D2I_BIO,ERR_R_MALLOC_FAILURE);
goto err;
}
i=BIO_read(in,&(b->data[len]),want);
if ((i < 0) && ((len-off) == 0))
{
ASN1err(ASN1_F_ASN1_D2I_BIO,ASN1_R_NOT_ENOUGH_DATA);
goto err;
}
if (i > 0)
len+=i;
=============================================================================
When the header stradles a ssl record boundary, we will have i>0, but
i<want. This can cause ASN1_get_object to return ASN1_R_TOO_LONG
(which is seemingly ignored here) and will (I think) lead to accessing
uninitialized data. Note that my patch doesn't address this one as I'm
less sure of it.
=============================================================================
}
/* else data already loaded */
p=(unsigned char *)&(b->data[off]);
c.p=p;
c.inf=ASN1_get_object(&(c.p),&(c.slen),&(c.tag),&(c.xclass),
len-off);
if (c.inf & 0x80)
{
unsigned long e;
e=ERR_GET_REASON(ERR_peek_error());
if (e != ASN1_R_TOO_LONG)
goto err;
else
ERR_get_error(); /* clear error */
}
i=c.p-p;/* header length */
off+=i; /* end of data */
if (c.inf & 1)
{
/* no data body so go round again */
eos++;
want=HEADER_SIZE;
}
else if (eos && (c.slen == 0) && (c.tag == V_ASN1_EOC))
{
/* eos value, so go back and read another header */
eos--;
if (eos <= 0)
break;
else
want=HEADER_SIZE;
}
else
{
/* suck in c.slen bytes of data */
want=(int)c.slen;
if (want > (len-off))
{
want-=(len-off);
if (!BUF_MEM_grow(b,len+want))
{
ASN1err(ASN1_F_ASN1_D2I_BIO,ERR_R_MALLOC_FAILURE);
goto err;
}
i=BIO_read(in,&(b->data[len]),want);
if (i <= 0)
{
ASN1err(ASN1_F_ASN1_D2I_BIO,ASN1_R_NOT_ENOUGH_DATA);
goto err;
}
len+=i;
=============================================================================
Here we have a definite problem. Again if we hit the situation where
i>0, but i<want, we just ignore this and return successfully.
=============================================================================
}
off+=(int)c.slen;
if (eos <= 0)
{
break;
}
else
want=HEADER_SIZE;
}
}
I've attached a patch for the second problem, though I believe a
similar thing should be done for the first.
/Sam
diff -u -r openssl-0.9.6c/crypto/asn1/a_d2i_fp.c
openssl-0.9.6c-patched/crypto/asn1/a_d2i_fp.c
--- openssl-0.9.6c/crypto/asn1/a_d2i_fp.c Fri Apr 23 17:08:07 1999
+++ openssl-0.9.6c-patched/crypto/asn1/a_d2i_fp.c Sun Feb 3 14:26:45 2002
@@ -169,13 +169,17 @@
ASN1err(ASN1_F_ASN1_D2I_BIO,ERR_R_MALLOC_FAILURE);
goto err;
}
- i=BIO_read(in,&(b->data[len]),want);
- if (i <= 0)
- {
-
ASN1err(ASN1_F_ASN1_D2I_BIO,ASN1_R_NOT_ENOUGH_DATA);
- goto err;
- }
- len+=i;
+ while(want)
+ {
+ i=BIO_read(in,&(b->data[len]),want);
+ if (i <= 0)
+ {
+
+ASN1err(ASN1_F_ASN1_D2I_BIO,ASN1_R_NOT_ENOUGH_DATA);
+ goto err;
+ }
+ len+=i;
+ want-=i;
+ }
}
off+=(int)c.slen;
if (eos <= 0)
