Not only is the Internet dead, it's starting to smell really bad.:2014年06月29日分

2014/06/29(Sun)

[Citrus][FreeBSD][Security] FreeBSD-SA-14:15およびCVE-2014-3951 について (その3)

前回の続きです。

一転攻勢一点訂正、FreeBSD-SA-14:15 って14と15連番の Security Advisary の意味じゃなくて、2014年の15番目の意味なのね… まぁ俺西暦2114年まで生きてないから困らんしどーでもいいけど。

@citrus_viqr モジュールにおける Out Of Array Index Bounds 問題

これは II. Problem Description における以下の部分です。

an out of bounds array access in the initialization code of the VIQR module

libVIQR( citrus_viqr.c)の問題で、配列にその長を超えた添字でアクセス発生しとるちゅー指摘です。

最初にこの文言と対策 patch を見たとき(おっ off-by-oneか?)とおもいまんた。 どれだけ気をつけて実装してもチョロリとはみ出すのは、皆さん日常生活でも鼻毛、ズボンからシャツの裾、チャックから陰茎とか良くあることです(ボロリ)。

ところが実際に FreeBSD をクラッシュさせてみると Bus Error 起こしてるのでちょっと普通じゃないのですな

$ echo "Any *.exe that crash is a VC, Anyone that stands still is a well disciplined VC6." | iconv -f VIQR -t UTF-8
Bus error (core dumped)

普通 off-by-one 程度ならクラッシュする原因は Stack Smashing Protection によるバッファオーバーラン絶対殺すマンか Segmention Fault 程度の致命傷で済むわけですが。

あとは 新井が悪いアライメント問題?そんなの気にするコード書いた覚えないしのう、サクラメント中島ならスターの球団でショート守ってくれてええんやで?年俸次第だけど。

考えても理由わからんので、今回の Security Advisary によるコードの修正箇所を当バグにおいてのみ抜き出してみましょう。

Index: lib/libiconv_modules/VIQR/citrus_viqr.c
===================================================================
--- lib/libiconv_modules/VIQR/citrus_viqr.c	(revision 267591)
+++ lib/libiconv_modules/VIQR/citrus_viqr.c	(working copy)
@@ -431,7 +431,6 @@ static int
 _citrus_VIQR_encoding_module_init(_VIQREncodingInfo * __restrict ei,
     const void * __restrict var __unused, size_t lenvar __unused)
 {
-	const mnemonic_def_t *p;
 	const char *s;
 	size_t i, n;
 	int errnum;
@@ -455,7 +454,10 @@ _citrus_VIQR_encoding_module_init(_VIQREncodingInf
 			return (errnum);
 		}
 	}
-	for (i = 0;; ++i) {
+	/* a + 1 < b + 1 here to silence gcc warning about unsigned < 0. */
+	for (i = 0; i + 1 < mnemonic_ext_size + 1; ++i) {
+		const mnemonic_def_t *p;
+
 		p = &mnemonic_ext[i];
 		n = strlen(p->name);
 		if (ei->mb_cur_max < n)

変更前は、for(i = 0;; ++i) で無限ループだった部分を、for (i = 0; i + 1 < mnemonic_ext_size + 1; ++i) に変更してます。

ファッ!? 俺は無限ループするコードなんぞ書いた覚えないってば、どういうこと!?(驚愕)

@そもそも VIRQ って何?

まずワイの書いたコードを理解するため寄り道、VIRQ(=VIetnamese Quoted Readable)ってのはかつてベトナムで利用されていた文字コードです、 RFC1456になってます。

ヴィカーと発音するけど、モリッシーの巻き舌とはたぶん無関係、 デュルルヴィカーインチュッチュー

実際には文字コードちゅうか、7bitなUS-ASCIIの文字のみを使ってラテン文字の ダイアクリティカルマークをニーモニックで表現する 翻字ですやね *1。古のUsenet時代の非8bit clean環境における遺物(日本におけるISO-2022-JP的存在)ですが、今でも情報交換用としてではなく TelexVNIと共にベトナム語入力メソッド用として生き残ってます。

かつてベトナム語の記述には漢字と チュノムという文字が使われていましたが、フランスによる植民地支配の結果 クオック・グーと呼ばれる方法でラテン文字でベトナム語を記述するようになった結果ですな。 *2

日本でも某巨大匿名掲示板や某動画サイトで、某やきう選手の名前がアルファベットに翻字される例がみられますが、たった一度の過ちを深く追求してはいけない(戒め)。

ちょww角川書店がKADOKAWAに!?ww

@VIQRのニーモニックには多くの変種があり、実用上 RFC1456 では十分でない

これは RFC1456 が VISCII との変換のみ定義してることによる問題。 ワイが libVIRQ の実装した頃にはまだインターネット上で VIQR の変換サービスを提供してるサイトがあったんですが、どこも RFC1456 以外にもニーモニック定義してるようです。 ですがドキュメント化されてないので、ワイ仕様が無いから実装できません状態に。

なもんで現地ベトナムの人が必要であれば、後から拡張ニーモニックを追加できるよう、 拡張性を持たせたコードにしたわけです。

 94 typedef struct {
 95 	const char *name;
 96 	wchar_t value;
 97 } mnemonic_def_t;
 98 
 99 static const mnemonic_def_t mnemonic_ext[] = {
100 /* add extra mnemonic here (should be sorted by wchar_t order). */
101 };
102 static const size_t mnemonic_ext_size =
103 	sizeof(mnemonic_ext) / sizeof(mnemonic_def_t);
...

524 	for (i = 0; i < mnemonic_ext_size; ++i) {
525 		p = &mnemonic_ext[i];
526 		_DIAGASSERT(p != NULL && p->name != NULL);
527 		n = strlen(p->name);
528 		_DIAGASSERT(n <= MB_LEN_MAX);
529 		if (ei->mb_cur_max < n)
530 			ei->mb_cur_max = n;
531 		errnum = mnemonic_append_child(ei->mroot,
532 		    p->name, p->value, ei->invalid);
533 		if (errnum != 0) {
534 			_citrus_VIQR_encoding_module_uninit(ei);
535 			return errnum;
536 		}
537 	}
538 

RFC1456 で足りないニーモニックが後に明文化されれば、mnemonic_ext配列に随時追加していけばいいという親切設計。

親切といってもl10nの原則「余計な親切心を持ち込まない」の精神で、どのニーモニックを追加するかは現地ベトナムの人に丸投げしてます。 罷り間違っても アイルランドのアレのような自分勝手は NG ちゅうことで。

@話を戻して、問題のコードに注目

んでんで 524行目に注目、ちゃんとワイの書いたコードは無限ループではなく i < mnemonic_ext_size という条件になってます。

つまり今回の Out Of Array Index Bounds を引き起こした無限ループは、FreeBSDへのCitrus iconv移植の際に元のコードの意図を無視して勝手に削られたちゅうことです。

ば~~~っかじゃねえの!?(AA略)

@なぜ FreeBSD は愚かにもループ終了条件を削ってしまったのか?

すでに Citrus iconv のツリーへの インポート時点でこの脆弱性は 存在するので、誰のやらかしかは判りません。

ですがこの脆弱性報告者による 最初の修正、そしてその後の 一連のcommit logをみると

102 static const size_t mnemonic_ext_size =
103 	sizeof(mnemonic_ext) / sizeof(mnemonic_def_t);

が、mnemonic_extにニーモニックが1つも定義されてない現状だと、mnemonic_ext_sizeはコンパイル時にゼロになるから

524 	for (i = 0; i < mnemonic_ext_size; ++i) {

は gcc のバージョン問題なのか最適化オプションなのかバグなのか知らんけど

for (i = 0; i < 0; ++i)

と解釈されてしまった結果

-Wtype-limits
	Warn if a comparison is always true or always false due to the limited range of the data type,
	but do not warn for constant expressions. For example, warn if an unsigned variable is compared against zero with ‘<’ or ‘>=’.
	This warning is also enabled by -Wextra. 

の警告に引っかかったちゅー感じ?ちな手元のgcc 4.5.3と4.8.3では問題なさそうなんだけど。

よく考えたらFはもうclangだっけ、そっちだと-Wtype-limitsではなくて-Wtautological-compareだけども、これだって定数マクロじゃないんだし警告出るのがおかしい。 というか手元でいくら試しても再現しないんだが、これamd64以外の環境でのclangバグじゃない?(適当)

まぁ細けぇことはいいんだよ、要するにFreeBSDでこの作業やってた連中は

  • 警告が出たコードのビルドを通すため
  • 中身も読まずに終了条件を消すという愚かな行為をやらかして
  • 無限ループによる Out Of Array Index Bounds で脆弱性を引き起こした
  • その上ろくすっぽ原因を検証してないので、修正patchで「i + 1 < mnemonic_ext_size + 1」みたいないずれoff-by-oneやらかしそうなコードを平気で出してくる

つまりは極めつけのアホということです。もしかして サルによるコーディングかな?

@次回予告

めんどくさいから放置するかもしれないけど、前回今回みたいな馬鹿げたバグを出さないためにlibc開発者はどうすべきかを書く予定。

*1:同様の翻字の例としては、チベット文字をローマ字転写する ワイリー法とかあります。
*2:国際政治( 落合信彦感)による文字強制としては、モンゴル人民共和国がソ連による支援で独立した後にモンゴル文字を捨ててラテン文字→後にキリル文字での記法に移行してますな。