Backport #1091

possible bad handling of return value of OCSP_basic_verify in ext/openssl/ossl_ocsp.c

Added by lucas (Lucas Nussbaum) over 3 years ago. Updated about 1 year ago.

[ruby-core:21762]
Status:Closed Start date:02/03/2009
Priority:Normal Due date:
Assignee:shyouhei (Shyouhei Urabe) % Done:

100%
Category:-
Target version:-

Description

This bug was reported on the Debian bug tracker. See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=513528

Looking at the code, it affects both ruby 1.8 and 1.9.

Quoting:
> I was looking at return codes for applications making use of
> openssl functions and found this in ext/openssl/ossl_ocsp.c:
> 
>     result = OCSP_basic_verify(bs, x509s, x509st, flg);
>     sk_X509_pop_free(x509s, X509_free);
>     if(!result) rb_warn("%s", ERR_error_string(ERR_peek_error(), NULL));
> 
>     return result ? Qtrue : Qfalse;
> 
> OCSP_basic_verify() can return both 0 and -1 in error cases,
> so this function can incorrectly return information to the
> caller.
> 
> I have no idea if what this code is used for and what the consequences
> of this might be.

ext_openssl_ossl_ocsp.c.diff (905 Bytes) lrz (Laurent Sansonetti), 02/18/2009 04:13 am

Associated revisions

Revision 22440
Added by nobu (Nobuyoshi Nakada) over 3 years ago

* ext/openssl/ossl_ocsp.c (ossl_ocspbres_verify): OCSP_basic_verify returns positive value on success, not non-zero. [ruby-core:21762]

Revision 28004
Added by shyouhei almost 2 years ago

merge revision(s) 26835: * ext/openssl: backport fixes in 1.9. * r25019 by marcandre * ossl_ocsp.c (ossl_ocspres_to_der): Bug fix in Response#to_def. Patch by Chris Chandler [ruby-core:18411] * r25017 by marcandre * ossl_config.c (ossl_config_add_value_m, ossl_config_set_section): Check if frozen (or untrusted for $SECURE >= 4) [ruby-core:18377] * r22925 by nobu * ext/openssl/openssl_missing.h (i2d_of_void): cast for callbacks. [ruby-core:22860] * ext/openssl/ossl_engine.c (ossl_engine_s_by_id): suppress a warning. * ext/openssl/ossl_ssl.c (ossl_sslctx_flush_sessions): time_t may be larger than long. * ext/openssl/ossl_ssl_session.c (ossl_ssl_session_get_time), (ossl_ssl_session_get_timeout): use TIMET2NUM() to convert time_t. * r22924 by nobu * ext/openssl/ossl_x509ext.c (ossl_x509ext_set_value): should use OPENSSL_free instead of free. a patch from Charlie Savage at [ruby-core:22858]. * r22918 by akr * ext/openssl: suppress warnings. * ext/openssl/ossl.h (OSSL_Debug): don't use gcc extention for variadic macro. * r22666 by akr * ext/openssl/lib/openssl/buffering.rb: define Buffering module under OpenSSL. [ruby-dev:37906] * r22440 by nobu * ext/openssl/ossl_ocsp.c (ossl_ocspbres_verify): OCSP_basic_verify returns positive value on success, not non-zero. [ruby-core:21762] * r22378 by akr * ext/openssl: avoid cyclic require. * ext/openssl/lib/openssl/ssl-internal.rb: renamed from ssl.rb * ext/openssl/lib/openssl/x509-internal.rb: renamed from x509.rb. [ruby-dev:38018] * r22101 by nobu * ext/openssl/ossl_cipher.c (add_cipher_name_to_ary): used conditionally. * r21510 by akr * ext/openssl/ossl.c (ossl_raise): abolish a warning. * r21208 by akr * ext/openssl/ossl_digest.c (GetDigestPtr): use StringValueCStr instead of STR2CSTR. * ext/openssl/ossl_pkey_ec.c (ossl_ec_key_initialize): ditto. (ossl_ec_group_initialize): ditto. * r19420 by mame * ext/openssl/ossl_pkey_ec.c (ossl_ec_key_to_string): comment out fragments of unused code. * r18975 by nobu * ext/openssl/ossl_ocsp.c (ossl_ocspres_initialize): fix for initialization of r18168. * r18971 by nobu * ext/openssl/ossl_config.c (Init_ossl_config): removed C99ism. * r18944 by matz * ext/openssl/ossl_config.c (Init_ossl_config): memory leak fixed. a patch <shinichiro.hamaji at gmail.com> in [ruby-dev:35880]. * ext/openssl/ossl_x509ext.c (ossl_x509ext_set_value): ditto. * r18917 by nobu * ext/openssl/ossl_x509attr.c (ossl_x509attr_initialize): fix for initialization of r18168. * ext/openssl/ossl_ocsp.c (ossl_ocspreq_initialize): ditto. * ext/openssl/ossl_x509name.c (ossl_x509name_initialize): ditto. * r18283 by nobu * ext/openssl/ossl_asn1.c (ossl_asn1_get_asn1type): suppress warnings on platforms which int size differs from pointer size. * r18181 by nobu * ext/openssl/openssl_missing.h (d2i_of_void): define for older versions. [ruby-dev:35637] * r18168 by nobu * ext/openssl: suppress warnings.

History

#1
Updated by shyouhei (Shyouhei Urabe) over 3 years ago

  • Assignee set to gotoyuzo (GOTOU Yuuzou)

#2
Updated by lrz (Laurent Sansonetti) over 3 years ago

Here is a patch that I quickly made for 1.8 (but could also be applied in 1.9).

#4
Updated by nobu (Nobuyoshi Nakada) over 3 years ago

Hi,

At Wed, 18 Feb 2009 04:12:29 +0900,
Laurent Sansonetti wrote in [ruby-core:22199]:
> Here is a patch that I quickly made for 1.8 (but could also be applied in 1.9). 

Do you mean that OCSP_basic_verify() returns positive value on
success?

-- 
Nobu Nakada

#5
Updated by lrz (Laurent Sansonetti) over 3 years ago

Yes, a return value >0 means success (though it will apparently always return 1, for now). In the case of a failure, it can return -1 or 0.

#6
Updated by nobu (Nobuyoshi Nakada) over 3 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100
Applied in changeset r22440.

#7
Updated by shyouhei (Shyouhei Urabe) about 3 years ago

  • Status changed from Closed to Open
  • Assignee changed from gotoyuzo (GOTOU Yuuzou) to shyouhei (Shyouhei Urabe)

#8
Updated by shyouhei (Shyouhei Urabe) almost 3 years ago

  • Status changed from Open to Closed

Also available in: Atom PDF