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

[Saul Wold <sgw@linux.intel.com>]

On 08/19/2013 10:07 PM, Xufeng Zhang wrote:
> 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.
>
Ok, looking at more details, these are good patches and should be 
upstreamed also.

Sau!

>>
>>>> +---
>>>> +--- 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
>>>
>>>
>>
>
>
>