#1092 セキュリティ対策

[berryzplus]

セキュリティ対策の観点で修正したもののフィードバックです。

対策内容は
・変数の初期化漏れ
・NUL終端引数を期待する関数に、GetWindowTextなどの呼出直後の変数をノーチェックで渡している箇所の対処
・明らかにエラー処理が必要なのにエラー処理を行ってない箇所の修正
・スタックのドカ食い対策
・ビルド時に判定できるのに実行時に判定しようとしている箇所の対策
（v1では必要な判定でも、v2には要らないものがある）
・バグレポートにあげている修正
・バグレポートには上げていないが直しといたほうがよさげな修正
・NULLを渡したらコケるWin32APIをNULLチェックなしで使ってる箇所の修正

各修正は、修正対象の関数内で完結するものがほとんどです。
内部で解決しないのはAPI誤りのバグだけ。
見ればわかる小規模修正ばかりなので、クレジットは入れていません。

不明点あればここに書いておいていただけば説明します。

以上です。

[berryzplus]

追加。

修正はvs2013で行っていますが、
ビルドチェックはvs2005でもしています。

[Moca]

OleTypes.h
-       wmemcpy(*S, Data, Len);
-       (*S)[Len] = L'\0';
+       ::wcscpy_s( *S, Len, Data );

恐ろしいことですが、サクラエディタではNUL文字を含んだ文字列バッファがそのままWSHマクロと出入りします。
たぶん。
wcscpy_sでは代替にならないかと。

サクラエディタのANSIビルドではWINVERが5だろうと6だろうとそれでWin95以上で動かすことを目標にコードが書かれています。
実際に動くかは知りませんが、たぶんそういうことになっています。

IMEのところがなんでHeapAllocになってるかというと、なんか昔ATOKがおかしくなるとかならないとか試行錯誤されたときに、HeapAllocにしたとか掲示板の過去ログにあった気がしますが定かではありません。

sakura_core\typeprop\CImpExpManager.cpp (869):
CommonSetting_KeyBind &sKeyBind = m_Common.m_sKeyBind;
よくみると、関数の最後で書き戻し処理があり、それに元のm_Commonの値が必要に見えます。

MessageBoxExWについては私も詳細は知りません。Win95であるなら、判定不要というのには同意します。
ただ、Win95のW系APIにはバグがあり、あまりちゃんと動かないという噂がサクラの掲示板に書いてありました。
ので、無理してW使うよりAに変換して読める文字だけでも救済したほうがいいかもしれません。

CEditView_ExecCmdのコメントの所。すみません。書いたの私ですが嘘が混ざってます。

[berryzplus]

oletypes.h
了解しました…。で、あるならサクラの仕様が正しくありませんね。
外部プログラムとのやり取りにstringを使っているのに、
実体はstringじゃなくてバイナリだってことですからマズいと思いいます。
いずれにしても対応は現役メンテナの方と相談して決めないと。

WINVERが5だろうと6だろうと

それはやっちゃいかんでしょう。。。
標準環境でビルドできないOSのサポートは切るべきです。
そうしないと、マイクロソフトの責任までかぶることになっちゃいます。
逆の試みで、v2に移植できてないv1の機能（あるんでしたっけ？）をなくして、
v2がxpでビルドできなくなっても誰も困らないようにすべきだと思いました。

HeapAllocを使っている理由はATOKのバグが原因

これ、ジャストシステムの中の人はご存じなんでしょうか？
売り物ソフトとフリーソフトではバグの重みが全然ちゃいます。
正式修正が済むまでの「暫定対応」なら分かりますが、
ATOKがバグってるみたいなのでこちらで対応しました。
なんて、訴えられますよ(^^;

sKeyBind

そこは変数宣言に&を付けてるのがポイントで、
「巨大オブジェクトをスタック持ちしない」の修正でした。
修正するなら書き戻しが同所コピーになってマズいですね…。

Win95のW系APIにはバグがあり…

これは分かったうえです。
新発売のvisual studioは、w系のstlがまったく動かない、なんてのもよくある話。
何か具体的に困らない限り「標準」を使うってポリシーのが楽だと思いまして。
まぁ、何のためのv1系だ？って話になりますよね。

ExecCmdのコメントのところ

よくわかりませんでした。
修正提案で出しているサイズ限界値、これW版の数字ですね。
CreateProcessAは1024でいいはず。

[berryzplus]

掲示板での要望に応えて「やばいもの」のみに絞りました。
それでも24ファイルです。
内容は↓。

_main/CControlProcess.cpp
nullptrかもしれないメモリをdeleteしてます。
nullチェックしてダメだったかもしれないハンドルをCloseHandleしてます。

_os/CClipboard.cpp
GlobalLockの戻り値をチェックしていません。

cmd/CViewCommander.cpp
bSelLockが設定されないパスがあります。

cmd/CViewCommander_Window.cpp
配列phwndArrが初期化されていません。
HWNDをとるWinAPIに無効なHWNDを渡してはいけません。

dlg/CDialog.cpp
クリア済みのWndProcを呼び出そうとしています。

env/CAppNodeManager.cpp
HWNDをとるWinAPIに無効なHWNDを渡してはいけません。

env/CShareData.cpp
共有メモリが作成できなかった場合にエラー終了していません。
致命的なエラー状態で処理を続行するのはご法度です。

extmodule/CUxTheme.cpp
extmodule/CUxTheme.h
バグリポートにあげました。IF定義誤りです。

io/CFileLoad.cpp
純粋な初期化漏れです。
switch文のdefault記述漏れでもあります。

macro/CWSH.cpp
サブスレッドからの、イベント送出とサブサブスレッド起動がてれこになっとります。
あと、ActiveScriptのパーサの引数が「0ばっかりでいいんだっけ？」と思ったのでコメントしました。

macro/CWSHIfObj.cpp
エラー処理がありません。

outline/CDlgFileTree.cpp
switchにdefaultがありません。

outline/CDlgFuncList.cpp
演算子の結合順序的に括弧が必要です。

outline/CFuncInfoArr.cpp
nullチェックしていません。
「realloc失敗」が考慮されていません。
reallocに失敗するとrealloc前の確保領域がまるまる漏れます。

print/CPrint.cpp
メモリ確保後に、確保できたかどうか確認していません。
致命的なエラー状態で処理を続行するのはご法度です。

print/CPrintPreview.cpp
HWNDをチェックしていません。
HWNDをとるWinAPIに無効なHWNDを渡してはいけません。

prop/CPropComCustmenu.cpp
ただのバグです。

prop/CPropComPlugin.cpp
CreateProcess関数が作成したハンドルを閉じていません。

view/CTextMetrics.cpp
変数の初期化漏れです。
十分に大きな値で初期化してやる必要があります。

window/CEditWnd.cpp
GlobalLockが失敗する場合が考慮されていません。

window/CSplitterWnd.cpp
配列の初期化漏れです。
HWNDをとるWinAPIに無効なHWNDを渡してはいけません。

window/CTabWnd.cpp
HWNDをとるWinAPIに無効なHWNDを渡してはいけません。

[novice123]

extmodule/CUxTheme.cpp
extmodule/CUxTheme.h

[patchunicode:#1091]
で対応済みなので更新しました。

[novice123]

prop/CPropComPlugin.cpp
CreateProcess関数が作成したハンドルを閉じていません。

[patchunicode:#1109]
で対応済みなので更新しました。