search

TeraTermProject / teraterm

Other
465 stars 38 forks source link

add：ttpmacro setpassword、getpassword暗号化機能追加 #213

Closed hkanou closed 3 months ago

hkanou commented 4 months ago

ttpmacroのsetpassword、getpassword暗号化機能追加を提案させて頂きます。 https://github.com/hkanou/ttpmacro/tree/main/ttpmacro2 https://github.com/TeraTermProject/teraterm/pull/211

nmaya commented 4 months ago

こちらへの移動ありがとうございます。

現状と提案部分の比較

以下の認識なのですが、間違いはないでしょうか。

現行の Encrypt()

PR の実装（拡張部分）

PR内容について

hkanou commented 4 months ago

ご検討頂き、大変ありがとうございます。

現状と提案部分の比較

以下の認識なのですが、間違いはないでしょうか。

ご認識のとおりです。

現行の Encrypt() 暗号方式: ぱっと見では、どのような暗号化？難読化？なのかわかりませんでした

私には独自方式のように見受けられました。

PR の実装（拡張部分） 暗号化のためのパスワード: 必要（encryptstr 引数） ソースのコメントでは「暗号化文字列」となっているが、パスワード文字列を暗号化するためのパスワード（共通鍵）という認識でよいか？

コメント「暗号化文字列」は誤り、パスワード文字列を暗号化するためのパスワード（共通鍵）です。 コメント修正いたします。

ファイル中のパスワード識別子（"password name"）をも難読化して解読を難しくするものではない

ご認識のとおりです。 本ご指摘により、パスワード識別子（"password name"）も暗号化した方が良いと気づきました。

PR内容について

現状でも暗号化？難読化？されています。この修正の目的はなんでしょうか？ パスワードをファイルに保存するときの暗号を強化し、解読を難しくすることでしょうか？ 共通鍵がないと復号できない形にしたいということでしょうか？

後者が目的になります。 パスワードファイルが第3者に渡った場合でも、パスワード文字列を知ることが出来ないようにしたいという意図になります。 パスワード文字列を暗号化するためのパスワードはマスターキーのような位置づけで、マクロ開始時にマスターキーを入力させ、それを用いてパスワード文字列を取り出すという使い方になります。 現行のEncrypt()はgetpasswordを用いることでパスワード文字列を知ることができるため、現行のEncrypt()を難読化、拡張部分を暗号化と区別して記載しました。

現行の Encrypt() と比較して、AES-256-CTR はパスワードの保護にどの程度のアドバンテージがあるでしょうか？

パスワード文字列を暗号化するためのパスワードを知らないと、パスワード文字列を取り出せない部分がアドバンテージになります。

実装上 AES-256-CTR とは別の暗号方式を使いたくなったとき、またはユーザによって複数の中から暗号方式を選ばせることになった場合、どのように暗号方式を区別しますか？ setpassword/getpassword の拡張よりも、setpassword2/getpassword2 のような新コマンドにしたほうが将来の拡張がしやすくなる？

おっしゃるとおり、時間の経過等により暗号方式は危殆化します。 setpassword2,3,4.../getpassword2,3,4...にしたほうが分かりやすく、拡張し易そうです。

setpassword/getpassword の拡張よりも「文字列を AES-256-CTR で暗号化/復号する関数」を作ったほう（その結果をパスワードファイルに入出力すれば同じ結果が得られますか？）が、柔軟性や汎用性が高くなる？

実は、ファイル操作コマンドの暗号化も出来ればと思い試作中です。 https://github.com/hkanou/ttpmacro/tree/main/ttpmacro3 暗号化されたファイルの読み書き等を可能とするものですがsetpassword、getpasswordの拡張と重複する部分があり、つい先日、全体で考えないといけないと思い始めたところです。 ご指摘いただいたお陰で、いろいろ気づくことが出来ました。 ありがとうございます。

hkanou commented 4 months ago

ファイル中のパスワード識別子（"password name"）をも難読化して解読を難しくするものではない

パスワード識別子（"password name"）がわからないよう、ハッシュ値をファイルに格納するよう修正しました。

setpassword/getpassword の拡張よりも、setpassword2/getpassword2 のような新コマンドにしたほうが将来の拡張がしやすくなる？

新コマンドを4つ追加する形で見直しいたしました。

  1. setpassword2
  2. getpassword2
  3. ispassword2
  4. delpassword2

また、ソースに下記2ファイルを追加しております。

  1. ttmenc2.h
  2. ttmenc2.c

setpassword/getpassword の拡張よりも「文字列を AES-256-CTR で暗号化/復号する関数」を作ったほう（その結果をパスワー>ドファイルに入出力すれば同じ結果が得られますか？）が、柔軟性や汎用性が高くなる？

処理が複雑でしたので一旦見送りとしておりますが、ご要望等ございましたら追加を検討いたします。

zmatsuo commented 3 months ago

setpassword/getpassword の拡張よりも「文字列を AES-256-CTR で暗号化/復号する関数」を作ったほう（その結果をパスワー>ドファイルに入出力すれば同じ結果が得られますか？）が、柔軟性や汎用性が高くなる？

処理が複雑でしたので一旦見送りとしておりますが、ご要望等ございましたら追加を検討いたします。

文字変数の上限 512 をそのうち撤廃したいなと思っています。 (#90 を参照ください) ひとまず、ttmenc2 で TStrVal を引数に使わないようにして 上限を持たないようにできれば後の修正を楽に進めることができそうに思います。 「文字列を AES-256-CTR で暗号化/復号する関数」はとても筋良しな感じがします。

base64のencode/decodeはソースツリーのどこかにあったと思うのですが… 後で探しておきます。

暗号化を何とかしたいなと思ったところが他にあったのですが思い出せず…

hkanou commented 3 months ago

文字変数の上限 512 をそのうち撤廃したいなと思っています。 (#90 を参照ください) ひとまず、ttmenc2 で TStrVal を引数に使わないようにして 上限を持たないようにできれば後の修正を楽に進めることができそうに思います。

ご指摘頂きありがとうございます。 ttmenc2でTStrValを使用しないよう修正しました。 また、CloseHandle漏れの修正と強度が増すよう下記の修正等を行いました。

「文字列を AES-256-CTR で暗号化/復号する関数」はとても筋良しな感じがします。

案1 strencrypt mode=encrypt/decrypt password=XXXX [cipher=aes-256-ctr] [salt=XXXX] [kdf=pbkdf2[,digest=sha2-256][,iter=XXXX]] kdfは別コマンドにしてkeyとivをstrencryptに渡すのが良さそうですが、keyもivもcipherによって長さが変わるのがちょっと難です。 [kdf=argon2id,m=XXX,t=X,p=X] [salt=XXXX] [kdf=scrypt,N=XXX,r=X,p=X] [salt=XXXX] ブロック暗号のパディングの指定(PKCS#7,PKCS#5他)やaes-gcmのpapperもありますが、まずはaes-256-ctrをベースに検討いたします。

base64のencode/decodeはソースツリーのどこかにあったと思うのですが… 後で探しておきます。

私も探してみましたが、関数としては無さそうでした。 hex, hmacもあれば便利かもしれません。

暗号化を何とかしたいなと思ったところが他にあったのですが思い出せず…

他になにかあるかもしれない旨、承知しました。

zmatsuo commented 3 months ago

案1 strencrypt mode=encrypt/decrypt password=XXXX [cipher=aes-256-ctr] [salt=XXXX] [kdf=pbkdf2[,digest=sha2-256][,iter=XXXX]]

「文字列を AES-256-CTR で暗号化/復号する関数」というのを 内部(ttmenc2.c内)の関数(Encrypt2EncDec() ですね)の仕様を そのようにすればよいではないかと間違って解釈していました。

文字列を暗号/復号するマクロコマンドがあれば、 暗号化したファイル読み書きとかもできるようになりますね。

base64エンコード/デコード

コードありました。

ttpmenu内のパスワードエンコード/デコード

設定(レジストリ/iniファイル)にパスワードをplan textで保存しないことを目的にしていると思われます。 いまのままでよさそう。

設定をコピーしてTera Term Menu起動できれば登録ホストにアクセスできるのは 当初の設計通りなのかなと思います。

起動時にパスワード入れないと使えないオプションがあっても(あるほうが)よさそう。

hkanou commented 3 months ago

文字列を暗号/復号するマクロコマンドがあれば、 暗号化したファイル読み書きとかもできるようになりますね。

使いこなしが煩雑ですがあるといろいろ使えそうです。

base64エンコード/デコード

コードありました。

ありがとうございます、common/ttlib.c b64encode()とb64decode()を使用するよう修正しました。

ttpmenu内のパスワードエンコード/デコード

ttpmenu/winmisc.cpp EncodePassword() 設定(レジストリ/iniファイル)にパスワードをplan textで保存しないことを目的にしていると思われます。

気づいておりませんでしたが、言われてみれば確かにttpmenuも暗号がありますね。

起動時にパスワード入れないと使えないオプションがあっても(あるほうが)よさそう。

zmatsuo commented 3 months ago

211 をマージしました

添付を作ってテストしていました。 パスワードマネージャみたいな動作をします。 #211.ttl.txt

history.html でコンフリクトがあったので修正しながらマージしました。

zmatsuo commented 3 months ago

clang でビルドしてみるとこんな警告が出ました。

ttpmacro/ttmenc2.c:146:9: warning: unknown pragma ignored [-Wunknown-pragmas]
  146 | #pragma optimize("", off)
      |         ^
ttpmacro/ttmenc2.c:206:9: warning: unknown pragma ignored [-Wunknown-pragmas]
  206 | #pragma optimize("", on)
      |         ^
ttpmacro/ttmenc2.c:153:11: warning: variable 'Dummy' set but not used [-Wunused-but-set-variable]
  153 |         int Ret, Dummy;
      |                  ^

Dummy は消し忘れかな? pragma optimize() は必要でしょうか?

zmatsuo commented 3 months ago

マクロのコマンドに何か関係がありそうなスクリプトが ソースツリー内に存在することに気づきました。 teraterm/installer/macrotemplate.pl どう使うのかな?というのが今の私の状態です。

このスクリプト使われましたか? @hkanou

hkanou commented 3 months ago

https://github.com/TeraTermProject/teraterm/pull/211 をマージしました

マージ頂きまして大変ありがとうございます。

添付を作ってテストしていました。 パスワードマネージャみたいな動作をします。 #211.ttl.txt

添付頂いたttlの方がマニュアルの例に良さそうですね。

history.html でコンフリクトがあったので修正しながらマージしました。

ご多用な中お手数をおかけし申し訳ございません。

パスワードファイルからパスワード識別子にマッチする行を見つける処理をEncrypt2ProfileSearch()で行っています。 サイドチャネル(タイミング)攻撃対策のため、Encrypt2ProfileSearch()ではマッチする行が見つかった場合でも見つからなかった場合でも処理時間が同じになるよう、Dummyな処理を入れています。 Dummyは意味のない処理のためコンパイラの最適化で削除されないよう、下記の設定で最適化を無効にしています。

pragma optimize("", off) ～ #pragma optimize("", on)

https://learn.microsoft.com/ja-jp/cpp/preprocessor/optimize Dummy部分の処理時間はごくわずかですので、Dummyと#pragma optimizeを削除しても実際はほとんど影響はなさそうです。(脆弱性を気にするあまり、clangへの影響に気づいておりませんでした)

teraterm/installer/macrotemplate.pl このスクリプト使われましたか? @hkanou

いえ、使用しておりませんでした。(教えていただいて初めて存在を認識しました)

ttpmenu内のパスワードエンコード/デコード 起動時にパスワード入れないと使えないオプションがあっても(あるほうが)よさそう。

ttpmenuのソースを読みました。 UI部分をどうするか迷っていますが、ttpmenuのパスワード暗号化対応を試みてみます。

zmatsuo commented 3 months ago

サイドチャネル(タイミング)攻撃対策のため、Encrypt2ProfileSearch()ではマッチする行が見つかった場合でも見つからなかった場合でも処理時間が同じになるよう、Dummyな処理を入れています。

なるほど了解です。 gcc,clangでMSVCと同等になるよう pragma をいれました。

teraterm/installer/macrotemplate.pl このスクリプト使われましたか? @hkanou

いえ、使用しておりませんでした。(教えていただいて初めて存在を認識しました)

了解です。 どうやってつかうのかな…。

hkanou commented 3 months ago

サイドチャネル(タイミング)攻撃対策のため、Encrypt2ProfileSearch()ではマッチする行が見つかった場合でも見つからなかった場合でも処理時間が同じになるよう、Dummyな処理を入れています。

gcc,clangでMSVCと同等になるよう pragma をいれました。

お手数をおかけいたしました、ありがとうございます。 五月雨で大変申し訳ありませんが、タイミング攻撃対策に不備がありました。 下記のように修正させて頂ければと存じます。

対象ファイル：teraterm/teraterm/ttpmacro/ttmenc2.c 不具合：比較にかかる時間が比較する内容に依存するmemcmp()を使用している 修正案：memcmp()を定数時間で比較するCRYPTO_memcmp()に変更する 修正内容：#include <openssl/crypto.h> を追加し、memcmpをCRYPTO_memcmpにリプレースする

teraterm/installer/macrotemplate.pl どうやってつかうのかな…。

このスクリプトは、マクロコマンドのヘルプ/htmlファイルの整合性をチェックするもののようですが、実行するとkeyfile.iniがないためエラーになります。

https://github.com/TeraTermProject/teraterm/commit/9a1e6b01848a2db27ad89b558b309765cfae635d KeyFile.iniは LogMeTT および TTLEditor に含まれるため、アーカイブからは削除してよいとのことなので、消すことにする。

macrotemplate.plから下記のkeyfile.ini関係の行を削除するとエラーなく実行できました。

26      $keyfile = 'release\keyfile.ini';
140 $ret = read_keyword($keyfile, $pat);
141 if ($ret eq '') {
142     print "$keyfile ファイルに $pat コマンドがありません\n";
143 }
zmatsuo commented 3 months ago

CRYPTO_memcmp()へ変更したブランチを作りました。

213_side_channel です。

この修正でokでしょうか?

macrotemplate.plから下記のkeyfile.ini関係の行を削除するとエラーなく実行できました。

ブランチ #213_macrotemplate に修正を入れました。 macrotemplate.pl が動くようになりました。 ありがとうございます。

次のようなメッセージが出ます。

==== else マクロを検証中...
..\teraterm\common\helpid.h のIDが一致していません (92013 != 92050)
#define RsvElse         13
#define HlpMacroCommandIfthenelseif     92050

対策が必要なのか考えてみます。

hkanou commented 3 months ago

CRYPTO_memcmp()へ変更したブランチを作りました。

213_side_channel です。

いつもありがとうございます。

この修正でokでしょうか?

ttmenc2.c内のmemcmpを全てCRYPTO_memcmpにリプレース頂けると助かります。

ブランチ #213_macrotemplate に修正を入れました。 macrotemplate.pl が動くようになりました。

ごめんなさい、下記の修正も必要でした。 16行目 改版履歴を追加 138行目 $pat = "\b$macro\b"; も削除

zmatsuo commented 3 months ago

直したものをpushしました。

macrotemplate.pl から キーワードのIDとヘルプのIDが一意に変換できるようにしたい気持ちが伝わりますが、 どうして一意(+92000)に変換したかったのかがわからないです。

nmaya commented 3 months ago

どうして一意(+92000)に変換したかったのかがわからないです。

私はこのスクリプトを使ったことはありませんが、そう決めてあるからです。

https://teratermproject.github.io/manual/5/ja/reference/develop-memo.html#add-macro-command

ヘルプページのコンテキスト ID を採番する。 値は コマンドの内部 ID + 92000。

hkanou commented 3 months ago

直したものをpushしました。

ありがとうございます。 本来はPRするべきでしたでしょうか？ (お手間をおかけしないようにしたいと思っております)

どうして一意(+92000)に変換したかったのかがわからないです。

https://github.com/TeraTermProject/teraterm/commit/9a1e6b01848a2db27ad89b558b309765cfae635d# のコメントに下記の記載があります。 ; Numeric values represent content sensitive help page indexes in teraterm.chm, ; LogMeTT.chm and TTLEdit.chm files.

想像になりますが、ヘルプファイル間の整合性のため一意(+92000)の値にするよう決められたのかもしれません

zmatsuo commented 3 months ago

本来はPRするべきでしたでしょうか？

ちょっとした修正ならいまのところ大丈夫です。 難しいものはPRがいいですね。

作ったブランチは整理してmainにマージしますね。

どうして一意(+92000)に変換したかったのかがわからないです。

もしかするとこうしたかったのでは? というのを思いつきました。

MACRO:Error ダイアログのヘルプボタン等で、 エラーが出たマクロコマンドのヘルプを出せるように したかったのでは、というものです。

できれば便利そうです。

hkanou commented 3 months ago

本来はPRするべきでしたでしょうか？

ちょっとした修正ならいまのところ大丈夫です。 難しいものはPRがいいですね。

お手数をおかけしております、複雑なものはPRいたします。

作ったブランチは整理してmainにマージしますね。

ありがとうございます。

どうして一意(+92000)に変換したかったのかがわからないです。 MACRO:Error ダイアログのヘルプボタン等で、 エラーが出たマクロコマンドのヘルプを出せるように したかったのでは、というものです。

なんと深謀遠慮な...

zmatsuo commented 3 months ago

マージしました。

https://teratermproject.github.io/manual/5/ja/reference/develop-memo.html#add-macro-command

ヘルプページのコンテキスト ID を採番する。 値は コマンドの内部 ID + 92000。

この修正は急ぎでやらなくても大丈夫そうなので そのうちやりますか。

なんと深謀遠慮な... 😀

このissueはいちだんらくかなと思いますがいかがでしょう?

hkanou commented 3 months ago

マージしました。

ありがとうございます。

https://teratermproject.github.io/manual/5/ja/reference/develop-memo.html#add-macro-command ヘルプページのコンテキスト ID を採番する。 値は コマンドの内部 ID + 92000。 この修正は急ぎでやらなくても大丈夫そうなので そのうちやりますか。

そうですね、現状実害はないように思われます。

このissueはいちだんらくかなと思いますがいかがでしょう?

はい、クローズ頂ければと思います。 お力添えいただきまして大変ありがとうございました。

hkanou commented 3 months ago

ソースをマージ頂きましたので、本issueをクローズさせて頂きます。

zmatsuo commented 3 months ago

1つ直せれば、というところがでてきました。64bit考慮です。

ソース全体的には、致命的な箇所は修正済み、警告はあちこちで出ている状態です。

今回追加したソースでもちょっと出てます。

teraterm\ttpmacro\ttmenc2.c(179,33): warning C4267: '関数': 'size_t' から 'int' に変換しました。データが失われているかもしれません。
  :

たとえば strlen() の戻り値は size_t でこれを int で受けるとこまるやん、という類ものです。 64bit化よりバグなどの修正を先に、と考えていますが、 新しく書くところは考慮していければなと思っています。

armでのビルドも試さないと別の警告(バグの可能性)が残っているかもですね。#228 無理なキャストじゃなく用途にあった自然な型を使っていれば大丈夫なはず、な気持ちです。

後回し、そのうち対応で。

hkanou commented 3 months ago

1つ直せれば、というところがでてきました。64bit考慮です。 ソース全体的には、致命的な箇所は修正済み、警告はあちこちで出ている状態です。

ご指摘ありがとうございます。

今回追加したソースでもちょっと出てます。 64bit化よりバグなどの修正を先に、と考えていますが、 新しく書くところは考慮していければなと思っています。

まさに、TTPmenuのパスワード暗号化対応で当該箇所を流用していました。 TTPmenuでは警告メッセージが出ないよう対応いたします。

armでのビルドも試さないと別の警告(バグの可能性)が残っているかもですね。https://github.com/TeraTermProject/teraterm/issues/228 無理なキャストじゃなく用途にあった自然な型を使っていれば大丈夫なはず、な気持ちです。

はい、承知いたしました。

nmaya commented 2 months ago

PR #248 でドキュメントの修正をマージしました。 ご確認ください。

hkanou commented 2 months ago

PR https://github.com/TeraTermProject/teraterm/pull/248 でドキュメントの修正をマージしました。

不備を多数修正頂き大変ありがとうございます。

doc/ja/html/macro/command/ispassword.html
40行目 
#248   <dd>パスワードファイルに &lt;password name&gt; に対応するパスワードが設定されているならば、、1 が格納される。<br>
修正案 <dd>パスワードファイルに &lt;password name&gt; に対応するパスワードが設定されているならば、1 が格納される。<br>

doc/en/html/about/history.html
72行名
#248 <li>added the <a href="../macro/command/setpassword2.html">setpassword2</a>, <a href="../macro/command/getpassword2.html">getpassword2</a>, <a href="../macro/command/ispassword2.html">ispassword2</a> and <a href="../macro/command/delpassword2.html">delpassword2</a> commands. The contents that is saved with setpassword2 command is protected by a password. not compatible. This password file is not compatible with prior password file.</li>
修正案 行中の"not compatible."は"Not compatible with the legacy commands."の方が意図が明確なように思われました。
nmaya commented 2 months ago

確認ありがとうございます。 あとで修正しますが、「従来のコマンドを非推奨にする」とか「そのうち削除する」とは思っていませんので legacy という言い方はしないようにすると思います。

hkanou commented 2 months ago

legacyに他意はなかったのですが、ネガティブな印象を避けるべきでした。 失礼いたしました。

nmaya commented 2 months ago

単体の "not compatible." は推敲中の消し忘れだったので削除しました。 コマンドの非互換性について書くつもりはありませんでした（日本語版にもその記述はありません）。

hkanou commented 2 months ago

修正頂きありがとうございます。 ispassword2.htmlの修正もありがとうございます。