search

sakura-editor / sakura

SAKURA Editor (Japanese text editor for MS Windows)
https://sakura-editor.github.io/
Other
1.23k stars 162 forks source link

セキュアでない関数をセキュアな関数に置き換える #394

Open beru opened 6 years ago

beru commented 6 years ago

セキュアでない関数をセキュアな関数に置き換える https://github.com/sakura-editor/sakura/pull/392#discussion_r214512425

m-tmatma commented 6 years ago

CNativeA::AppendStringF/CNativeW::AppendStringF (#303) という関数があります。 現状、内部的にはバッファは 2048 個 固定ですが、この関数を有効活用することで セキュア関数への置き換えを隠蔽できるので対応が楽になります。

berryzplus commented 6 years ago

個別対応で、やれるときにやる、でなければ放置でもいいのかな、と思っています。

セキュア版関数への置き換えを考える、ということはC言語で書かれているわけで、 「C++で書きなおしたら？」というツッコミが成り立つようにも思えます。 ただ、読みやすさの面で、sstreamよりもsprintfの方が優れている気がしています。

とても重要な課題ですが、他課題が片付くまで塩漬けにしたい所存です。

beru commented 6 years ago

https://github.com/sakura-editor/sakura/blob/e1bd5e37401e85d3a54636335c5c7e762cef51d5/sakura_core/cmd/CViewCommander_Clipboard.cpp#L1167

こことかどうでしょうか？

出力先の文字列長が280文字ですが現在編集中のファイルのパス名を書き込むので。

とはいっても MAX_PATH を超えるようなファイルはそもそも開けないで異常終了するので特に問題はありませんでした。

https://github.com/sakura-editor/sakura/blob/e1bd5e37401e85d3a54636335c5c7e762cef51d5/sakura_core/basis/CMyString.h#L92

beru commented 6 years ago

結構長いパス（D:\123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890\aaaaaaaaaaa.txt）のファイルを作成して編集して保存したら下記のメッセージが表示されました。

---------------------------
sakura
---------------------------
バックアップ先のパス作成中にエラーになりました。
パスが長すぎるか不正な書式です。
バックアップを作成せずに上書き保存してよろしいですか？
---------------------------
はい(Y)   いいえ(N)   
---------------------------
beru commented 6 years ago

CViewCommander::Command_COPYTAG メソッドでは極端に大きい行数と列数のテキストを用意すれば280文字のバッファを超えますが、後続のローカル変数 ptColLine の領域にはみ出しても特に障害は起きないので結果的に問題無さそうです。LFN対応はまた別の問題ですね。

berryzplus commented 6 years ago

検討内容はいい雰囲気だと思います。

ただ、いくつか話が混ざってる気がします。

https://github.com/sakura-editor/sakura/blob/e1bd5e37401e85d3a54636335c5c7e762cef51d5/sakura_core/cmd/CViewCommander_Clipboard.cpp#L1159

windowsプログラマの常識 MAX_PATH = 260 。 MAX_PATH は パス文字列の最大を定義する定数です。 たとえば cmd.exe(コマンドプロンプト) はMAX_PATHを超えるパスを扱えません。

最近のWindowsは起動設定のフラグを変更することにより、 この制限を外すことができるようになっているらしいです。 これが LFN(Long File Name) というやつです。

LFN対応以前の環境で、パス文字列に対してMAX_PATHを超える文字数を確保することは間違っている気がします。組み立てたパスが使えないことが分かっているなら組み立てる意味はないわけで。

では、なぜわざわざMAX_PATHを超える文字数を確保しているか？ですが、これは ANSI → WCHAR の変換で文字数が増える場合があるのを意識して、サクラエディタのコードではメモリを余分に確保しておく慣習があったためと考えられます。

セキュア対応という以前にいろいろ累積の課題はあるように感じています。

beru commented 6 years ago

たとえば cmd.exe(コマンドプロンプト) はMAX_PATHを超えるパスを扱えません。

OSのバージョンによって違うかもしれませんが、むしろ explorer.exe がリネーム操作が出来なかったりで対応が不十分のようです。

最近のWindowsは起動設定のフラグを変更することにより、 この制限を外すことができるようになっているらしいです。

これについて知らなかったので調べてみました。

詳しい内容が https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file に色々と書いてありました。

WindowsAPIのUnicode版の関数では \\?\ プレフィックスを付加する事で長いファイル名が扱えますが、それ以外にもいろいろ書かれてました。

何やら Windows10, version 1607 からWin32のファイルとディレクトリを扱う関数のMAX_PATH制限を取り除く事が出来るようですが、有効にするには下記の操作を行う必要があるようです。

レジストリを編集する

1) run regedit.exe as administrator 2) locate [HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem] 3) change data value [LongPathsEnabled] (DWORD) to "1" 4) close regedit and restart Windows

グループポリシーを編集する

これも結局レジストリが変更されるみたいです。 gpedit.msc を起動して、

英語の場合

マニフェストファイルを使う

アプリケーション（プロセス）単位でこれも実施しないと有効にならないようです。

<application xmlns="urn:schemas-microsoft-com:asm.v3">
    <windowsSettings xmlns:ws2="http://schemas.microsoft.com/SMI/2016/WindowsSettings">
        <ws2:longPathAware>true</ws2:longPathAware>
    </windowsSettings>
</application>

影響を受ける関数

ディレクトリ操作

ファイル操作

LFN対応以前の環境で、パス文字列に対してMAX_PATHを超える文字数を確保することは間違っている気がします。組み立てたパスが使えないことが分かっているなら組み立てる意味はないわけで。

NTFS前提ですが、結構前から \\?\ プレフィックスを付ける事で MAX_PATH を超える文字数のファイルを扱えたので、そういうわけではないと思います。

m-tmatma commented 6 years ago

MAX_PATH を超えるファイルの扱いに関しては、別途議論するのがいいと思います。

また、MAX_PATH を超えるファイルの扱いに関してはファイルパスに関係する処理を 扱うクラスを作ってそのクラスの中に隠蔽した方がいいと思います。

NTFS前提ですが、結構前から \\?\ プレフィックスを付ける事で MAX_PATH を超える文字数のファイルを扱えたので、そういうわけではないと思います。

はい。そうですね。

参考: http://www2.biglobe.ne.jp/sota/backup.html では \\?\ プレフィックスを使うことで MAX_PATH を超えるファイルも正しく扱えます。

berryzplus commented 6 years ago

NTFS前提ですが、結構前から \\?\ プレフィックスを付ける事で MAX_PATH を超える文字数のファイルを扱えたので、そういうわけではないと思います。

これは抜けてました。

サクラエディタのコードの中にもパスに \\?\ を考慮するユーティリティ関数があります。 ただ、「存在しているだけ」で利用されてはいないと思っています。 「MAX_PATHを越えたら \\?\ を付ける」ならば MAX_PATH を超えるメモリを確保する意味があると思います。そうでないのだから意味はない、で一応理屈は通ってる気がします。

言い方がよろしくないのかも知れませんね。 MAX_PATH を超えるファイルについては別の issue を設けましょう。

m-tmatma commented 6 years ago

MAX_PATH を超えるファイルについては別の issue を設けましょう。

401 を登録しました。