libstagefrightに見る、C/C++プログラミングの課題

バグの概要

やや旧聞にはなりますが、スマートフォン開発ではリアルタイムに進んでいる対策となる、libstagefrightのバグについて。
今回のこのバグは、実はC/C++でプログラミングをしている我々に対する大きな教訓を残すものと感じ、書かせて頂きました。
志を同じくする方々に読んで頂き、同じ過ちが二度と繰り返さないためのノウハウとし頂きたく思っております。

ANDROID bug 2件に計6件のCVEを含むバグは、JVNがJVNVU\#92141772 「Android Stagefright に複数の脆弱性」として報告しています。
https://jvn.jp/vu/JVNVU92141772/

ANDROID-20139950
- CVE-2015-1538 Integer overflows during MP4 atom processing
- CVE-2015-1539 An integer underflow in ESDS processing

ANDROID-20923261
- CVE-2015-3824 Integer overflow in libstagefright when parsing the MPEG4 tx3g atom
- CVE-2015-3826 Unbounded buffer read in libstagefright while parsing 3GPP metadata allows reading arbitrary memory
- CVE-2015-3827 Integer underflow in libstagefright when processing MPEG4 covr atoms
- CVE-2015-3828 Integer underflow in libstagefright if size is below 6 while processing 3GPP metadata
- CVE-2015-3829 Integer overflow in libstagefright processing MPEG4 covr atoms when chunk_data_size is SIZE_MAX

Androidに限らずですが、どうも範囲チェックが甘いコーディングが多いように思われます。

CVE-2015-1538

今回は「範囲チェックしたつもりがされていなかった」以下について、書きたいと思います

- CVE-2015-1538 Integer overflows during MP4 atom processing

差分は次の通りです。

https://android.googlesource.com/platform/frameworks/av/+/030d8d0%5E!/

Fix several ineffective integer overflow checks

Commit edd4a76 (which addressed bugs 15328708, 15342615, 15342751) added
several integer overflow checks. Unfortunately, those checks fail to take into
account integer promotion rules and are thus themselves subject to an integer
overflow. Cast the sizeof() operator to a uint64_t to force promotion while
multiplying.

Bug: 20139950

(cherry picked from commit e2e812e58e8d2716b00d7d82db99b08d3afb4b32)

Change-Id: I080eb3fa147601f18cedab86e0360406c3963d7b
diff --git a/media/libstagefright/SampleTable.cpp b/media/libstagefright/SampleTable.cpp
index 8dfa365..1358582 100644
--- a/media/libstagefright/SampleTable.cpp
+++ b/media/libstagefright/SampleTable.cpp
@@ -330,7 +330,7 @@
     }
 
     mTimeToSampleCount = U32_AT(&header[4]);
-    uint64_t allocSize = mTimeToSampleCount * 2 * sizeof(uint32_t);
+    uint64_t allocSize = mTimeToSampleCount * 2 * (uint64_t)sizeof(uint32_t);
     if (allocSize > SIZE_MAX) {
         return ERROR_OUT_OF_RANGE;
     }
@@ -376,7 +376,7 @@
     }
 
     mNumCompositionTimeDeltaEntries = numEntries;
-    uint64_t allocSize = numEntries * 2 * sizeof(uint32_t);
+    uint64_t allocSize = numEntries * 2 * (uint64_t)sizeof(uint32_t);
     if (allocSize > SIZE_MAX) {
         return ERROR_OUT_OF_RANGE;
     }
@@ -426,7 +426,7 @@
         ALOGV("Table of sync samples is empty or has only a single entry!");
     }
 
-    uint64_t allocSize = mNumSyncSamples * sizeof(uint32_t);
+    uint64_t allocSize = mNumSyncSamples * (uint64_t)sizeof(uint32_t);
     if (allocSize > SIZE_MAX) {
         return ERROR_OUT_OF_RANGE;
     }

なぜバグっているのか

修正内容は、次のように (uint64_t) でキャストするだけですが、ではなぜこの方法で不具合が治るのでしょうか？

-    uint64_t allocSize = mTimeToSampleCount * 2 * sizeof(uint32_t);
+    uint64_t allocSize = mTimeToSampleCount * 2 * (uint64_t)sizeof(uint32_t);

実はこの差分を一瞬見ただけでは、私もその理由が分かりませんでした。

32ビットARM環境では、mTimeToSampleCountも、sizeofの返却値であるsize_t型も共に32ビットであるため、この式は「32ビット同士の積を64ビット変数に代入」が仮定されています。
32ビット同士の積は32ビットを超えることもあるため、その結果を64ビット変数で得ることが想定された処理となっています。

そこでふと思い、本当にそのような結果が得られるのかどうか、短いプログラムを書いてみてその結果をみて、私は事の重大さを理解するに至りました。

実際には、この式では32ビット同士の積の解は、32ビットで得られオーバーフロー分は切り捨てられるのです。
その後に64ビットに拡張されるため得られる結果は誤ったものとなり、実際より小さな値が格納されることからその次のif文も成立せず、結果としてオーバーフローチェックをすり抜けてしまっていました。

例
mTimeToSampleCount = 0xffffffff;
sizeof(uint32_t) == 4

この条件では、次が期待される
allocSize = 0xffffffff * 2 * 4 → 0x7fffffff8

ところが現実は厳しい。実際には次のような動きにしかならないのです。

allocSize = (uint32_t)((uint32_t)(0xffffffff * 2) * 4) → 0xfffffff8

結果、計算結果が本来よりも小さいものとなるため、せっかくの以下のif文は決して成立せず、オーバーフローは見逃されてしまうのです。

     if (allocSize > SIZE_MAX) {
         return ERROR_OUT_OF_RANGE;
     }

32ビットと64ビットの間にはクレバスがある

C/C++では、32ビット同士の演算結果は32ビットでしか得られません。自動的に64ビットに拡張されることはないのです。つまり、一般的なILP32環境の場合、long同士の積が自動的にlong longに拡張されるようなことはないということです。
したがって、結果を期待通り64ビット長で得るためには式中に一つ以上64ビット長が必要となり、式全体を64ビットの演算とする必要があります。
修正においては、sizeofの返却値をuint64_tでキャストすることで式中に64ビット長を含め、期待通りの結果を得られるようにしていました。


演算式を記述するときは長さを強く意識することが必要です。
半分寝ている状態でコーディングしていて意識レベルが低いときは、せめてキャストをするべきでしょう。