fix: サーバ応答由来のバッファオーバーフロー/NULL参照を修正#132
Merged
Merged
Conversation
NSSモジュールおよびキーラッパーは、改竄され得る(サーバ侵害/MITM/ssl_verify=false/平文HTTP/ローカルcache-stnsd経由)STNSサーバ応答を処理する。 複数のフィールドが長さ/NULL検証なしに固定長スタックバッファへコピーされており、特権プロセス(sshd/login/sudo等)でスタックバッファオーバーフローやNULL参照を引き起こし得た。 - stns.h: STNS_SET_DEFAULT_VALUE が char[MAXBUF] への strcpy ではなく snprintf を使用 (shell/directory/password) - stns.h: SET_ATTRBUTE が NULL を "" として扱い strnlen(NULL)/strcpy クラッシュを防止 (gecos/name) - stns_key_wrapper.c: 内側/外側ループ変数の衝突解消, key の NULL チェック, realloc 戻り値チェック, size ベースのバッファ計算, keys が NULL の場合の出力ガード - Makefile: -fstack-protector-strong -D_FORTIFY_SOURCE=2 を追加 長い shell/password と gecos 欠落のユニットテストを追加。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
セキュリティレビューで発見した、STNSサーバ応答を信頼することに起因するメモリ安全性の問題を修正します。
NSSモジュール(
libnss_stns.so)とSSH鍵ラッパー(stns-key-wrapper)はsshd/login/sudo/cron等の特権プロセス(多くはroot)にロードされ、改竄され得る入力(サーバ侵害・MITM・ssl_verify=false・平文HTTP・ローカルcache-stnsd経由)を処理します。応答値の長さ・NULLを検証していなかったため、特権プロセスのメモリ破壊につながり得ました。修正内容
🔴 Critical: スタックバッファオーバーフロー (
stns.hSTNS_SET_DEFAULT_VALUE)サーバ応答の
shell/directory/passwordを固定長char[MAXBUF](1024) へstrcpyしていたため、1024バイト超でスタックを破壊。→
snprintf(buf, sizeof(buf), ...)で境界制限。🟠 High1: NULL参照によるDoS (
stns.hSET_ATTRBUTE)gecos/nameがNULL(応答にフィールド欠落時)でも未チェックでstrnlen/strcpyしていてクラッシュ。→ NULLを
""として扱う。🟠 High2: 鍵連結ロジックのメモリ不具合 (
stns_key_wrapper.c)iを使用していた →jに分離keyのNULLチェック追加realloc戻り値チェック追加strnlen(keys)ではなく追跡中のsizeで計算(NUL未終端時のヒープオーバーリード回避)keysがNULLのままの出力をガードビルド緩和策 (
Makefile)CFLAGSに-fstack-protector-strong -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2を追加。テスト
stns_passwd_test.c: 長いshellでスタックを破壊しないこと /gecos欠落でクラッシュしないことstns_shadow_test.c: 長いpasswordでスタックを破壊しないことCIのASANビルド(
make test)で境界違反を検出する設計です。※
stns_key_wrapper.cはユニットテスト対象外のため integrationテスト/コードレビューで担保。レビューで検出した残課題(本PR対象外・別途検討)
stns.confが644で配置されauth_token/passwordがworld-readableUser-Highest-Id:値なしでtrim(NULL)、strtokの非スレッドセーフ)/var/tmp/.stns.lockによる名前解決DoS