Re: [PATCH] openssl: avoid NULL pointer dereference in three places

[Xufeng Zhang <xufeng.zhang@windriver.com>]

On 08/20/2013 12:13 PM, Saul Wold wrote:
> On 08/19/2013 06:59 PM, Xufeng Zhang wrote:
>> Hi All,
>>
>> Anybody help me review this?
>>
>>
>> Thanks,
>> Xufeng
>>
>> On 06/04/2013 02:15 PM, Xufeng Zhang wrote:
>>> There are three potential NULL pointer dereference in
>>> EVP_DigestInit_ex(), dh_pub_encode() and dsa_pub_encode()
>>> functions.
>>> Fix them by adding proper null pointer check.
>>>
>>> [YOCTO #4600]
>>> [ CQID: WIND00373257 ]
>>>
>>> Signed-off-by: Xufeng Zhang<xufeng.zhang@windriver.com>
>>> ---
>>>   ...-pointer-dereference-in-EVP_DigestInit_ex.patch |   16 +++++++++
>>>   ...NULL-pointer-dereference-in-dh_pub_encode.patch |   34
>>> ++++++++++++++++++++
>>>   meta/recipes-connectivity/openssl/openssl.inc      |    2 +-
>>>   .../recipes-connectivity/openssl/openssl_1.0.1e.bb |    2 +
>>>   4 files changed, 53 insertions(+), 1 deletions(-)
>>>   create mode 100644
>>> meta/recipes-connectivity/openssl/openssl-1.0.1e/openssl-avoid-NULL-pointer-dereference-in-EVP_DigestInit_ex.patch 
>>>
>>>
>>>   create mode 100644
>>> meta/recipes-connectivity/openssl/openssl-1.0.1e/openssl-avoid-NULL-pointer-dereference-in-dh_pub_encode.patch 
>>>
>>>
>>>
>>> diff --git
>>> a/meta/recipes-connectivity/openssl/openssl-1.0.1e/openssl-avoid-NULL-pointer-dereference-in-EVP_DigestInit_ex.patch 
>>>
>>> b/meta/recipes-connectivity/openssl/openssl-1.0.1e/openssl-avoid-NULL-pointer-dereference-in-EVP_DigestInit_ex.patch 
>>>
>>>
>>> new file mode 100644
>>> index 0000000..69924a4
>>> --- /dev/null
>>> +++
>>> b/meta/recipes-connectivity/openssl/openssl-1.0.1e/openssl-avoid-NULL-pointer-dereference-in-EVP_DigestInit_ex.patch 
>>>
>>>
>>> @@ -0,0 +1,16 @@
>>> +openssl: avoid NULL pointer dereference in EVP_DigestInit_ex()
>>> +
>>> +We should avoid accessing the type pointer if it's NULL,
>>> +this could happen if ctx->digest is not NULL.
>
> These patches both need Upstream-Status: and Signed-off-by: Tags

Ok, I'll add these stuff when I send V2.

>
>>> +---
>>> +--- a/crypto/evp/digest.c
>>> ++++ b/crypto/evp/digest.c
>>> +@@ -199,7 +199,7 @@
>>> +         return 0;
>>> +         }
>>> + #endif
>>> +-    if (ctx->digest != type)
>>> ++    if (type&&  (ctx->digest != type))
>>> +         {
>>> +         if (ctx->digest&&  ctx->digest->ctx_size)
>>> +             OPENSSL_free(ctx->md_data);
>
> I am more concerned about this patch as I do not know the code well, 
> but if ctx->digest is non-NULL and type is, this code would not be 
> executed which would be wrong, correct? 

No, that's correct, if type is NULL, we should not execute the below 
code as type pointer is accessed in the following code, see:
int EVP_DigestInit_ex()
{
...
     if (ctx->digest != type)
    {
         if (ctx->digest && ctx->digest->ctx_size)
                         OPENSSL_free(ctx->md_data);
         ctx->digest=type;
         if (!(ctx->flags & EVP_MD_CTX_FLAG_NO_INIT) && type->ctx_size)
             {
                         ctx->update = type->update;
     ...
    }
...
}


> Your changing the behavior of the code here. 

But if we don't change, NULL pointer dereference would appear.

> Also reading the code, it should be be possible for type to be NULL, 
I think you are saying "it should not be possible"

> unless some memory corruption occurs earlier as there is a check at 
> line 153 for !type, which should cause it to goto skip_to_init.

Yes, it's true, but such code is executed only when OPENSSL_NO_ENGINE is 
not defined.

>
> Please verify this patch is really needed.
>
>
>>> diff --git
>>> a/meta/recipes-connectivity/openssl/openssl-1.0.1e/openssl-avoid-NULL-pointer-dereference-in-dh_pub_encode.patch 
>>>
>>> b/meta/recipes-connectivity/openssl/openssl-1.0.1e/openssl-avoid-NULL-pointer-dereference-in-dh_pub_encode.patch 
>>>
>>>
>>> new file mode 100644
>>> index 0000000..642b0ea
>>> --- /dev/null
>>> +++
>>> b/meta/recipes-connectivity/openssl/openssl-1.0.1e/openssl-avoid-NULL-pointer-dereference-in-dh_pub_encode.patch 
>>>
>>>
>>> @@ -0,0 +1,34 @@
>>> +openssl: avoid NULL pointer dereference in
>>> dh_pub_encode()/dsa_pub_encode()
>>> +
>>> +We should avoid accessing the pointer if ASN1_STRING_new()
>>> +allocates memory failed.
>>> +---
>>> +--- a/crypto/dh/dh_ameth.c
>>> ++++ b/crypto/dh/dh_ameth.c
>>> +@@ -139,6 +139,12 @@
>>> +     dh=pkey->pkey.dh;
>>> +
>>> +     str = ASN1_STRING_new();
>>> ++    if (!str)
>>> ++        {
>>> ++        DHerr(DH_F_DH_PUB_ENCODE, ERR_R_MALLOC_FAILURE);
>>> ++        goto err;
>>> ++        }
>>> ++
>>> +     str->length = i2d_DHparams(dh,&str->data);
>>> +     if (str->length<= 0)
>>> +         {
>>> +--- a/crypto/dsa/dsa_ameth.c
>>> ++++ b/crypto/dsa/dsa_ameth.c
>>> +@@ -148,6 +148,11 @@
>>> +         {
>>> +         ASN1_STRING *str;
>>> +         str = ASN1_STRING_new();
>>> ++        if (!str)
>>> ++            {
>>> ++            DSAerr(DSA_F_DSA_PUB_ENCODE, ERR_R_MALLOC_FAILURE);
>>> ++            goto err;
>>> ++            }
>>> +         str->length = i2d_DSAparams(dsa,&str->data);
>>> +         if (str->length<= 0)
>>> +             {
>>> diff --git a/meta/recipes-connectivity/openssl/openssl.inc
>>> b/meta/recipes-connectivity/openssl/openssl.inc
>>> index f5b2432..c753a27 100644
>>> --- a/meta/recipes-connectivity/openssl/openssl.inc
>>> +++ b/meta/recipes-connectivity/openssl/openssl.inc
>>> @@ -5,7 +5,7 @@ BUGTRACKER =
>>> "http://www.openssl.org/news/vulnerabilities.html"
>>>   SECTION = "libs/network"
>>>
>>>   # Big Jump for OpenSSL 1.0 support with meta-oe
>>> -INC_PR = "r15"
>>> +INC_PR = "r16"
>>>
> No PR or INC_PR Bump needed

Ok, will drop it.


Thanks,
Xufeng

>
>>>   # "openssl | SSLeay" dual license
>>>   LICENSE = "openssl"
>>> diff --git a/meta/recipes-connectivity/openssl/openssl_1.0.1e.bb
>>> b/meta/recipes-connectivity/openssl/openssl_1.0.1e.bb
>>> index 61de3a6..afd5576 100644
>>> --- a/meta/recipes-connectivity/openssl/openssl_1.0.1e.bb
>>> +++ b/meta/recipes-connectivity/openssl/openssl_1.0.1e.bb
>>> @@ -31,6 +31,8 @@ SRC_URI += "file://configure-targets.patch \
>>>               file://openssl_fix_for_x32.patch \
>>>               file://openssl-fix-doc.patch \
>>>               file://find.pl \
>>> +
>>> file://openssl-avoid-NULL-pointer-dereference-in-dh_pub_encode.patch \
>>> +
>>> file://openssl-avoid-NULL-pointer-dereference-in-EVP_DigestInit_ex.patch 
>>>
>>> \
>>>              "
>>>
>>>   SRC_URI[md5sum] = "66bf6f10f060d561929de96f9dfe5b8c"
>>
>> _______________________________________________
>> Openembedded-core mailing list
>> Openembedded-core@lists.openembedded.org
>> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>>
>>
>