Project

General

Profile

Bug #12292

Race between OpenSSL::SSL::SSLSocket#stop and #connect can cause a segmentation fault

Added by rhenium (Kazuki Yamaguchi) over 3 years ago. Updated about 3 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
ruby 2.4.0dev (2016-04-15 trunk 54594) [x86_64-linux]
[ruby-core:74978]

Description

The following code will demonstrate the issue:

require "openssl"
require "socket"

ctx = OpenSSL::SSL::SSLContext.new
ctx.ciphers = "aNULL"

sock1, sock2 = UNIXSocket.pair
ssl1 = OpenSSL::SSL::SSLSocket.new(sock1, ctx)
ssl2 = OpenSSL::SSL::SSLSocket.new(sock2, ctx)

t = Thread.new { ssl1.connect } # => segmentation fault
ssl2.accept

ssl1.close # calls #stop (private method)
sock1.close

t.value

The SSL (OpenSSL land object) can be freed by SSLSocket#stop (#close) while SSLSocket#connect is still using it. This happens because SSLSocket#connect releases GVL while waiting for the peer.

There are two ways to resolve this:

  • Check that the SSL object is still set every time after reacquiring GVL
  • Change SSLSocket#stop not to free the SSL object

The latter introduces an incompatibility (#stop is currently documented as "prepares it for another connection").
I however prefer this because similar bugs can be introduced in future if we choose an ad-hoc way, and I don't think anyone reuses the SSLSocket object (keep in mind that we don't have interface to replace the underlying IO object).

Anyway I attach fixes in both way.

  • 0001-ext-openssl-check-that-the-SSL-object-is-still-set-a.patch: the fix in the first way
  • 0001-ext-openssl-make-OpenSSL-SSL-SSLSocket-non-reusable.patch: the fix in the second way
  • 0002-ext-openssl-some-trivial-cleanups.patch: some minor cleanups, not actually related to this issue (what's the most desirable way to submit such trivial patches?)

Files


Related issues

Related to Ruby master - Bug #10799: Segmentation fault in TestsRejectedActions
Related to Ruby master - Bug #12309: Segmentation fault caused by OpenSSLRejectedActions
Related to Ruby master - Bug #12327: Seg Fault - ruby 2.3.0 mongo & OpenSSL issue??ClosedActions
Has duplicate Ruby master - Bug #12646: When using sidekiq in rails and call multiple timesRejectedActions
Has duplicate Ruby master - Bug #12661: Multithreading http get doesn't workRejectedActions

Associated revisions

Revision 118ee2a7
Added by rhe over 3 years ago

openssl: fix possible SEGV on race between SSLSocket#stop and #connect

  • ext/openssl/ossl_ssl.c (ossl_ssl_stop): Don't free the SSL struct
    here. Since some methods such as SSLSocket#connect releases GVL,
    there is a chance of use after free if we free the SSL from another
    thread. SSLSocket#stop was documented as "prepares it for another
    connection" so this is a slightly incompatible change. However when
    this sentence was added (r30090, Add toplevel documentation for
    OpenSSL, 2010-12-06), it didn't actually. The current behavior is
    from r40304 (Correct shutdown behavior w.r.t GC., 2013-04-15).
    [ruby-core:74978] [Bug #12292]

  • ext/openssl/lib/openssl/ssl.rb (sysclose): Update doc.

  • test/openssl/test_ssl.rb: Test this.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@55100 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 55100
Added by rhenium (Kazuki Yamaguchi) over 3 years ago

openssl: fix possible SEGV on race between SSLSocket#stop and #connect

  • ext/openssl/ossl_ssl.c (ossl_ssl_stop): Don't free the SSL struct
    here. Since some methods such as SSLSocket#connect releases GVL,
    there is a chance of use after free if we free the SSL from another
    thread. SSLSocket#stop was documented as "prepares it for another
    connection" so this is a slightly incompatible change. However when
    this sentence was added (r30090, Add toplevel documentation for
    OpenSSL, 2010-12-06), it didn't actually. The current behavior is
    from r40304 (Correct shutdown behavior w.r.t GC., 2013-04-15).
    [ruby-core:74978] [Bug #12292]

  • ext/openssl/lib/openssl/ssl.rb (sysclose): Update doc.

  • test/openssl/test_ssl.rb: Test this.

Revision 55100
Added by rhe over 3 years ago

openssl: fix possible SEGV on race between SSLSocket#stop and #connect

  • ext/openssl/ossl_ssl.c (ossl_ssl_stop): Don't free the SSL struct
    here. Since some methods such as SSLSocket#connect releases GVL,
    there is a chance of use after free if we free the SSL from another
    thread. SSLSocket#stop was documented as "prepares it for another
    connection" so this is a slightly incompatible change. However when
    this sentence was added (r30090, Add toplevel documentation for
    OpenSSL, 2010-12-06), it didn't actually. The current behavior is
    from r40304 (Correct shutdown behavior w.r.t GC., 2013-04-15).
    [ruby-core:74978] [Bug #12292]

  • ext/openssl/lib/openssl/ssl.rb (sysclose): Update doc.

  • test/openssl/test_ssl.rb: Test this.

Revision 55100
Added by rhe over 3 years ago

openssl: fix possible SEGV on race between SSLSocket#stop and #connect

  • ext/openssl/ossl_ssl.c (ossl_ssl_stop): Don't free the SSL struct
    here. Since some methods such as SSLSocket#connect releases GVL,
    there is a chance of use after free if we free the SSL from another
    thread. SSLSocket#stop was documented as "prepares it for another
    connection" so this is a slightly incompatible change. However when
    this sentence was added (r30090, Add toplevel documentation for
    OpenSSL, 2010-12-06), it didn't actually. The current behavior is
    from r40304 (Correct shutdown behavior w.r.t GC., 2013-04-15).
    [ruby-core:74978] [Bug #12292]

  • ext/openssl/lib/openssl/ssl.rb (sysclose): Update doc.

  • test/openssl/test_ssl.rb: Test this.

Revision 55100
Added by rhe over 3 years ago

openssl: fix possible SEGV on race between SSLSocket#stop and #connect

  • ext/openssl/ossl_ssl.c (ossl_ssl_stop): Don't free the SSL struct
    here. Since some methods such as SSLSocket#connect releases GVL,
    there is a chance of use after free if we free the SSL from another
    thread. SSLSocket#stop was documented as "prepares it for another
    connection" so this is a slightly incompatible change. However when
    this sentence was added (r30090, Add toplevel documentation for
    OpenSSL, 2010-12-06), it didn't actually. The current behavior is
    from r40304 (Correct shutdown behavior w.r.t GC., 2013-04-15).
    [ruby-core:74978] [Bug #12292]

  • ext/openssl/lib/openssl/ssl.rb (sysclose): Update doc.

  • test/openssl/test_ssl.rb: Test this.

Revision 88d76cfd
Added by nagachika (Tomoyuki Chikanaga) about 3 years ago

merge revision(s) 55100: [Backport #12292]

    * ext/openssl/ossl_ssl.c (ossl_ssl_stop): Don't free the SSL struct
      here. Since some methods such as SSLSocket#connect releases GVL,
      there is a chance of use after free if we free the SSL from another
      thread. SSLSocket#stop was documented as "prepares it for another
      connection" so this is a slightly incompatible change. However when
      this sentence was added (r30090, Add toplevel documentation for
      OpenSSL, 2010-12-06), it didn't actually. The current behavior is
      from r40304 (Correct shutdown behavior w.r.t GC., 2013-04-15).
      [ruby-core:74978] [Bug #12292]

    * ext/openssl/lib/openssl/ssl.rb (sysclose): Update doc.

    * test/openssl/test_ssl.rb: Test this.

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_3@55866 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 55866
Added by nagachika (Tomoyuki Chikanaga) about 3 years ago

merge revision(s) 55100: [Backport #12292]

* ext/openssl/ossl_ssl.c (ossl_ssl_stop): Don't free the SSL struct
  here. Since some methods such as SSLSocket#connect releases GVL,
  there is a chance of use after free if we free the SSL from another
  thread. SSLSocket#stop was documented as "prepares it for another
  connection" so this is a slightly incompatible change. However when
  this sentence was added (r30090, Add toplevel documentation for
  OpenSSL, 2010-12-06), it didn't actually. The current behavior is
  from r40304 (Correct shutdown behavior w.r.t GC., 2013-04-15).
  [ruby-core:74978] [Bug #12292]

* ext/openssl/lib/openssl/ssl.rb (sysclose): Update doc.

* test/openssl/test_ssl.rb: Test this.

History

Updated by shyouhei (Shyouhei Urabe) over 3 years ago

  • Status changed from Open to Assigned
  • Assignee set to rhenium (Kazuki Yamaguchi)

Kazuki, I think it's OK for you to fix a segmentation fault now.

#2

Updated by Anonymous over 3 years ago

  • Status changed from Assigned to Closed

Applied in changeset r55100.


openssl: fix possible SEGV on race between SSLSocket#stop and #connect

  • ext/openssl/ossl_ssl.c (ossl_ssl_stop): Don't free the SSL struct
    here. Since some methods such as SSLSocket#connect releases GVL,
    there is a chance of use after free if we free the SSL from another
    thread. SSLSocket#stop was documented as "prepares it for another
    connection" so this is a slightly incompatible change. However when
    this sentence was added (r30090, Add toplevel documentation for
    OpenSSL, 2010-12-06), it didn't actually. The current behavior is
    from r40304 (Correct shutdown behavior w.r.t GC., 2013-04-15).
    [ruby-core:74978] [Bug #12292]

  • ext/openssl/lib/openssl/ssl.rb (sysclose): Update doc.

  • test/openssl/test_ssl.rb: Test this.

Updated by usa (Usaku NAKAMURA) over 3 years ago

  • Backport changed from 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN to 2.1: WONTFIX, 2.2: REQUIRED, 2.3: REQUIRED

Updated by nagachika (Tomoyuki Chikanaga) about 3 years ago

  • Backport changed from 2.1: WONTFIX, 2.2: REQUIRED, 2.3: REQUIRED to 2.1: WONTFIX, 2.2: REQUIRED, 2.3: DONE

ruby_2_3 r55866 merged revision(s) 55100.

#5

Updated by rhenium (Kazuki Yamaguchi) almost 3 years ago

  • Has duplicate Bug #12646: When using sidekiq in rails and call multiple times added
#6

Updated by rhenium (Kazuki Yamaguchi) almost 3 years ago

  • Related to Bug #10799: Segmentation fault in Tests added
#7

Updated by rhenium (Kazuki Yamaguchi) almost 3 years ago

  • Related to Bug #12309: Segmentation fault caused by OpenSSL added
#8

Updated by rhenium (Kazuki Yamaguchi) almost 3 years ago

  • Related to Bug #12327: Seg Fault - ruby 2.3.0 mongo & OpenSSL issue?? added
#9

Updated by rhenium (Kazuki Yamaguchi) almost 3 years ago

  • Has duplicate Bug #12661: Multithreading http get doesn't work added

Also available in: Atom PDF