Project

General

Profile

Bug #4445

ext/openssl の verify_callback が rb_protect で保護されていない

Added by ohai (Ippei Obayashi) over 9 years ago. Updated almost 9 years ago.

Status:
Closed
Priority:
Normal
Target version:
ruby -v:
ruby 1.9.2p180 (2011-02-18 revision 30909) [x86_64-linux]
Backport:
[ruby-dev:43274]

Description

=begin
openssl では 証明書の検証に付加的な機能を付けるための
callback を設定できます。これをrubyから利用できるようになっていますが
rb_protect を使っていないため、openssl ライブラリ内部を飛び越えて
例外が飛ぶようになってしまう状態です。

現在ではSEGVが発生する等の問題は見つかってはいませんがメモリリークなど
起きている可能性が高いです。

とりあえず大域脱出を止めるパッチを添付します。例外を適当な場所で再送するべきかもしれませんが
それはしていません。
=end


Files

verify_cb.diff (662 Bytes) verify_cb.diff ohai (Ippei Obayashi), 02/25/2011 12:09 AM

Updated by naruse (Yui NARUSE) almost 9 years ago

  • Status changed from Open to Assigned
  • Assignee set to nahi (Hiroshi Nakamura)

Updated by nahi (Hiroshi Nakamura) almost 9 years ago

  • Target version set to 1.9.3

Updated by nahi (Hiroshi Nakamura) almost 9 years ago

  • Assignee changed from nahi (Hiroshi Nakamura) to MartinBosslet (Martin Bosslet)

Martin, can you handle this? Original reporter said that verify_cb does not use rb_protect to invoke a callback so an error raised from the callback passed directly to Ruby interpreter. Proposed patch looks good but we should check the rational of the current code (svn blame & svn log) and do some test around this.

Updated by MartinBosslet (Martin Bosslet) almost 9 years ago

Yes, I'll look into it!

Updated by nahi (Hiroshi Nakamura) almost 9 years ago

Note: #4611 and #4875 both crashes at ossl_ssl_verify_callback (1.9.2p274, 1.9.2p180.) I suspected this issue related to those issues.

Updated by MartinBosslet (Martin Bosslet) almost 9 years ago

Thanks for the input, I will keep them in mind when investigating this!

Updated by nahi (Hiroshi Nakamura) almost 9 years ago

Martin, how's the status? Can I take over this issue again? I think #4875 and #4611 relates to this issue.

Updated by MartinBosslet (Martin Bosslet) almost 9 years ago

  • Assignee changed from MartinBosslet (Martin Bosslet) to nahi (Hiroshi Nakamura)

Hiroshi Nakamura wrote:

Martin, how's the status? Can I take over this issue again? I think #4875 and #4611 relates to this issue.

Sure - if you feel it's related to the other two issues then you are clearly in a better position to design this properly. Should I look into #4923 and #4961 instead? Or are there any other urgencies where I could help?

Updated by nahi (Hiroshi Nakamura) almost 9 years ago

騒いでおいてすいません。 #4611 #4875 へのリンクを外しました。ちゃんとスタックを見たら、関係ありませんでした。

で、このチケットですが、Obayashiのおっしゃる通り、確かに今のコードはダメですね。他のcallback同様、事後に例外を上げられるといいんですが、verify callbackはOpenSSL側から何度も呼ばれるので、「事後」がうまく定義できません。

というわけで、1.9.4ではverify_callback内での例外は検証失敗扱いにしようと思います。(従来のようには例外は上がらない→SSLの場合は別途SSLErrorが上がる。X509Storeの場合はただの検証失敗になる)

ただ、1.9.3でどうするか悩んでいます。↑の変更を入れてしまいたいところですが、どこかで例外を投げているカスタマイズされたverify_callbackを見たことがあります。この時期に非互換を入れるのも。。。というわけで、挙動は従来どおりとして、rb_jump_tagの前にdeprecation警告、とかかなあ。。。

Updated by nahi (Hiroshi Nakamura) almost 9 years ago

Martin Bosslet wrote:

Sure - if you feel it's related to the other two issues then you are clearly in a better position to design this properly. Should I look into #4923 and #4961 instead? Or are there any other urgencies where I could help?

OK, I take this.

Do you think you can handle #4961? I don't think it's a release blocker since we just added tests which does not run with OpenSSL 0.9.7. It has not yet worked ever. But there could be a chance to find a easy way to fix the bug.

Updated by MartinBosslet (Martin Bosslet) almost 9 years ago

Hiroshi Nakamura wrote:

Do you think you can handle #4961? I don't think it's a release blocker since we just added tests which does not run with OpenSSL 0.9.7. It has not yet worked ever. But there could be a chance to find a easy way to fix the bug.

I tried OpenSSL.decode on the PEM data and it was valid. I'll try my best, probably debugging it directly in C will show us what fails there.

So I will concentrate on #4961, and if I can solve that, I will continue on #4923. If I can help you with anything, please let me know!

Updated by nahi (Hiroshi Nakamura) almost 9 years ago

  • Status changed from Assigned to Closed

r32537でtrunk、r32538でruby_1_9_3を修正しました。結果的には、Obayashiさん(前回呼び捨てですいません)のpatchをそのまま当て、かつ例外を投げている場合にはwarnで警告をする(しかし無視して、通常の検証失敗とする)ようにしました。テストを書いてみたらSSL接続が残ったままになり、GCで回収されるまでそのままという、いかにも危ない挙動になった(当たり前)ので、この時期ではありますが対処しました。

挙動変更は以下です: verify callbackで例外を投げた場合、従来はその例外が飛んでいましたが、代わりにwarnでやめろと警告され、SSLErrorが上がるようになります。

Obayashiさん、ご報告ありがとうございました。

Also available in: Atom PDF