Blog

PHP5 __destruct() and unserialize() function

http://co3k.org/diary/12
このへんみておもったこと。

unserialize() 関数はオブジェクトの unserialize もできるのだが、5 以後では __destruct() が導入されているので、その unserialize したオブジェクトの __destruct() がよばれてしまう。この際に、たとえば cache の処理などで __destruct() でファイルにデータをかきこむ、などの処理をしていると、そのファイルが汚染されてしまったりするということがおきうる。

で、実際に cakephp ではそういう状況になって、任意のコードが実行可能になった、と。

まとめると 5 以後では unserialize() をユーザーからの入力にたいしてはかならず検証してからおこなうようにするべき。
っていうか、ユーザーの入力にたいしては unserialize() をつかわずに、なんか他のものをつかえるならつかったほうがいいんじゃないかな、と。

<?php

class MyCache {
    function __construct() {
        // data load here...
    }
    function __destruct() {
        file_put_contents('/tmp/cache.txt', $this->dat); # ここで、キャッシュが汚染される
        print("__destruct() !!\n");
    }
}

if (count($argv) === 2) {
    print("make serialized\n");
    $x = new MyCache();
    $x->dat = 'hoge'; # ここで任意のデータをつっこむ
    $serialized = serialize($x);
    print($serialized);
    print("\n");
} else { # 引数なかったらこっちだけど
    $serialized = 'O:7:"MyCache":1:{s:3:"dat";s:4:"hoge";}';
    unserialize($serialized); # このタイミングで、MyCache::__destruct() がよばれちゃう。
}

で、これは Perl でいうとこんなかんじですよ、と。 Storable も任意のユーザー入力にたいしてかけちゃだめよ、と。

use common::sense;
use Storable qw/nfreeze thaw/;

{
    package MyCache;
    sub DESTROY {
        my $class = shift;
        say "MyCache::DESTROY!";
        open my $fh, '>', '/tmp/foo';
        print {$fh} $class->{dat};
        close $fh;
    }
}

if (@ARGV==1) {
    say pack('u*', nfreeze(bless {dat => 'hoge'}, 'MyCache'));
} else {
    thaw(unpack('u*', '=!0<1!TUY0V%C:&4#`````0H$:&]G90````-D870`'));
}

オブジェクトをデシリアライズ可能なシリアライザを、任意のユーザー入力にたいしてつかってはいけませんよ、ということですね。

PHP の場合は、5 で後づけで __destruct() をつけたから、こういう脆弱性がうまれた、という話らしいです。

【追記】
5.3 ってのはうそ。ほかのやつとまざってた。ので s/5.3/5/g

あと、この問題については
http://www.suspekt.org/downloads/POC2009-ShockingNewsInPHPExploitation.pdf
これがくわしいですね。

【さらに追記】

__destruct() がはしると、たとえばテンポラリファイルをあつかうクラスがあったとすると、たとえばこういうクラスがロードされていたときに危険ですよ、と。

# 擬似コードです。
class My_File_Temp {
  ...
  function __destruct() {
     unlink($this->filename);
  }
}

で、これをふせぐために、クラスのプロパティではなく static 変数をつかって隠蔽するというのはまったく本質的ではないです。
https://github.com/cakephp/cakephp/commit/bed7767258969698b04dcd31ad98f0f70b9938fb
この変更は本質的ではない(そもそもオブジェクトの1パラメータを global 変数でかくって、どうなの……)。

__destruct() をつかっているクラスすべてでこういう風にチェックするのって現実的じゃないよね。

この問題をふせぐには、unserialize() をつかうのではなく、JSON や messagepack などの、object を deserialize しないデシリアライザをつかうということが本質的な解決策だとおもう。

【さらにさらに追記】
本質的には、オブジェクトをデシリアライズできるデシリアライザをユーザーからの入力にたいしてつかえば、どんな言語でもだめですよ、と。