Bug #4445

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

Added by Ippei Obayashi about 3 years ago. Updated almost 3 years ago.

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

Description

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

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

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

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

Associated revisions

Revision 32537
Added by Hiroshi Nakamura almost 3 years ago

  • ext/openssl/ossl.c (osslverifycb): trap the exception from
    verify callback of SSLContext and X509Store and make the
    verification fail normally. Raising exception directly from callback
    causes orphan resouces in OpenSSL stack. Patched by Ippei Obayashi.
    See #4445.

  • test/openssl/testssl.rb
    (test
    exceptioninverifycallbackis_ignored): test it.

History

#1 Updated by Yui NARUSE almost 3 years ago

  • Status changed from Open to Assigned
  • Assignee set to Hiroshi Nakamura

#2 Updated by Hiroshi Nakamura almost 3 years ago

  • Target version set to 1.9.3

#3 Updated by Hiroshi Nakamura almost 3 years ago

  • Assignee changed from Hiroshi Nakamura to Martin Bosslet

Martin, can you handle this? Original reporter said that verifycb does not use rbprotect 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.

#4 Updated by Martin Bosslet almost 3 years ago

Yes, I'll look into it!

#5 Updated by Hiroshi Nakamura almost 3 years ago

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

#6 Updated by Martin Bosslet almost 3 years ago

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

#7 Updated by Hiroshi Nakamura almost 3 years ago

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

#8 Updated by Martin Bosslet almost 3 years ago

  • Assignee changed from Martin Bosslet to 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?

#9 Updated by Hiroshi Nakamura almost 3 years ago

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

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

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

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

#10 Updated by Hiroshi Nakamura almost 3 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.

#11 Updated by Martin Bosslet almost 3 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!

#12 Updated by Hiroshi Nakamura almost 3 years ago

  • Status changed from Assigned to Closed

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

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

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

Also available in: Atom PDF