[インデックス 10148] ファイルの概要

このコミットは、Go言語の実験的なSSHパッケージ（exp/ssh）における重要なバグ修正とリファクタリングに関するものです。具体的には、SSHチャネルのデータストリームにおいて、ペイロードの先頭に誤って長さヘッダーが混入してしまう問題（"length header leaking into channel data streams"）を修正しています。また、この修正に伴い、channelData および channelExtendedData 構造体が不要になったため削除され、以前の最適化されたコードパスが復元されています。さらに、無効なチャネルにパケットが到着した場合にロックが解放されないバグも修正されています。

コミット

commit 0f6b80c69498d2047d584d365e4056ced9f38adc
Author: Dave Cheney <dave@cheney.net>
Date:   Sat Oct 29 14:22:30 2011 -0400

    exp/ssh: fix length header leaking into channel data streams.
    
    The payload of a data message is defined as an SSH string type,
    which uses the first four bytes to encode its length. When channelData
    and channelExtendedData were added I defined Payload as []byte to
    be able to use it directly without a string to []byte conversion. This
    resulted in the length data leaking into the payload data.
    
    This CL fixes the bug, and restores agl's original fast path code.
    
    Additionally, a bug whereby s.lock was not released if a packet arrived
    for an invalid channel has been fixed.
    
    Finally, as they were no longer used, I have removed
    the channelData and channelExtedendData structs.
    
    R=agl, rsc
    CC=golang-dev
    https://golang.org/cl/5330053

GitHub上でのコミットページへのリンク

https://github.com/golang/go/commit/0f6b80c69498d2047d584d365e4056ced9f38adc

元コミット内容

exp/ssh: チャネルデータストリームへの長さヘッダーの漏洩を修正。

データメッセージのペイロードはSSH文字列型として定義されており、その長さは最初の4バイトでエンコードされます。channelData および channelExtendedData が追加された際、文字列から []byte への変換なしに直接使用できるように Payload を []byte として定義しました。これにより、長さデータがペイロードデータに漏洩していました。

この変更リスト（CL）は、このバグを修正し、aglの元の高速パスコードを復元します。

さらに、無効なチャネルにパケットが到着した場合に s.lock が解放されないバグも修正されました。

最後に、使用されなくなったため、channelData および channelExtendedData 構造体を削除しました。

変更の背景

このコミットの主な背景は、SSHプロトコルにおけるデータメッセージのペイロード処理の誤りです。SSHプロトコルでは、データメッセージのペイロードは「SSH文字列型」として定義されています。このSSH文字列型は、データ自体の前に4バイトの長さ情報（ビッグエンディアンの符号なし32ビット整数）が付加される形式を取ります。

しかし、以前の実装では、channelData および channelExtendedData 構造体において、ペイロードを直接Goのバイトスライス []byte として扱っていました。これは、文字列からバイトスライスへの変換を避けるための意図的な設計でしたが、結果として、SSHプロトコルが期待する4バイトの長さ情報がペイロードデータの一部として誤って解釈されてしまうというバグを引き起こしました。つまり、受信したデータストリームの先頭4バイトが、本来のデータではなく、そのデータの長さとして扱われるべきものが、データそのものとしてアプリケーションに渡されてしまっていたのです。

このバグは、SSHチャネルを通じて送受信されるデータの整合性を損なう深刻な問題でした。この修正は、データの正確な解釈を保証し、SSH通信の信頼性を向上させるために不可欠でした。

また、この修正は、以前に存在した「aglの元の高速パスコード」を復元するものでもあります。これは、おそらくパフォーマンス上の理由から導入された最適化されたデータ処理ロジックが、上記のバグの導入によって一時的に失われていたことを示唆しています。

さらに、無効なチャネルにパケットが到着した場合に、サーバー側のロック（s.lock）が適切に解放されないというデッドロックにつながる可能性のあるバグも同時に修正されました。これは、リソース管理と並行処理の正確性に関する重要な改善です。

最後に、ペイロード処理の変更により、channelData および channelExtendedData 構造体が不要になったため、コードの簡素化とクリーンアップのために削除されました。

前提知識の解説

SSH (Secure Shell) プロトコル

SSHは、ネットワークを介して安全にコンピュータを操作するためのプロトコルです。主にリモートログインやファイル転送（SCP, SFTP）に使用されます。SSHは、クライアントとサーバー間で暗号化されたセキュアなチャネルを確立し、そのチャネル上で様々なサービスを提供します。

SSHチャネル

SSHプロトコルでは、複数の論理的な「チャネル」を単一のSSH接続上で多重化して使用できます。これにより、例えば、一つのSSH接続でシェルセッション、ポートフォワーディング、ファイル転送などを同時に行うことが可能になります。各チャネルは独立したデータストリームを持ち、それぞれに固有の識別子（チャネルID）が割り当てられます。

SSH文字列型 (RFC 4251/4254)

SSHプロトコルにおける「文字列型」は、RFC 4251 (The Secure Shell (SSH) Protocol Architecture) で定義されている基本的なデータ型の一つです。これは、単なる文字のシーケンスではなく、バイナリデータを含むことができる「バイトシーケンス」として扱われます。そのフォーマットは以下の通りです。

長さ (Length): 4バイトの符号なし32ビット整数（ビッグエンディアン）で、続くバイトシーケンスの長さをバイト単位で示します。
データ (Data): 長さで指定されたバイト数の生データ。

例えば、文字列 "hello" をSSH文字列型としてエンコードする場合、まず "hello" の長さである5バイトを4バイトの整数で表現し、その後に "hello" のバイトデータが続きます。このコミットのバグは、この4バイトの長さ情報がペイロードデータの一部として誤って解釈されていたことに起因します。

Go言語の `[]byte` 型

Go言語における []byte は、バイトスライス（byte slice）を表します。これは、可変長のバイトのシーケンスであり、バイナリデータを扱う際によく使用されます。[]byte は、C言語の char* やPythonの bytes オブジェクトに似ていますが、Goのスライスは動的なサイズ変更が可能で、基になる配列への参照を持つという特徴があります。

このコミットの文脈では、SSHプロトコルが期待する「長さ情報を含むバイトシーケンス」を、Goの []byte 型でどのように正確に表現し、解析するかが問題となっていました。以前の実装では、[]byte を直接ペイロードとして扱うことで、先頭の長さ情報がスキップされずにペイロードの一部として含まれてしまっていたのです。

`decode(packet)` と `packet[0]` によるメッセージタイプの識別

SSHプロトコルでは、各メッセージは特定のメッセージタイプコード（1バイト）で始まります。

decode(packet): 以前の実装では、受信したパケット全体を decode 関数に渡し、その戻り値の型アサーション（.(type)）を使ってメッセージの種類を判別していました。この方法は、メッセージ全体の構造を抽象化して扱うには便利ですが、特定のメッセージタイプ（msgChannelData, msgChannelExtendedData）のペイロード構造が特殊である場合、その抽象化が問題を引き起こす可能性がありました。
packet[0]: このコミットでは、msgChannelData と msgChannelExtendedData の処理において、パケットの最初のバイト（packet[0]）を直接参照してメッセージタイプを識別するように変更されています。これにより、これらの特定のメッセージタイプに対しては、より低レベルで直接的なバイト操作によるペイロードの解析が可能になり、SSH文字列型の長さ情報を正しくスキップしてペイロードを抽出できるようになりました。

技術的詳細

このコミットの技術的詳細の核心は、SSHプロトコルにおける SSH_MSG_CHANNEL_DATA および SSH_MSG_CHANNEL_EXTENDED_DATA メッセージのペイロードの解釈方法の修正にあります。

バグの原因と修正

バグの原因: SSHプロトコル（RFC 4254, Section 5.2）では、SSH_MSG_CHANNEL_DATA および SSH_MSG_CHANNEL_EXTENDED_DATA メッセージのペイロードは「SSH文字列型」として定義されています。これは、4バイトの長さフィールドの後に実際のデータが続く形式です。

以前の実装では、channelData および channelExtendedData 構造体において、ペイロードを直接 []byte 型として定義していました。

// 以前の定義 (src/pkg/exp/ssh/messages.go)
type channelData struct {
	PeersId uint32
	Payload []byte `ssh:"rest"` // ここが問題
}

type channelExtendedData struct {
	PeersId  uint32
	Datatype uint32
	Payload  []byte `ssh:"rest"` // ここが問題
}

ssh:"rest" タグは、残りのバイトをすべて Payload フィールドに割り当てることを意図していました。しかし、SSHプロトコルのデコードロジックが、この Payload []byte を「SSH文字列型」として解釈する際に、その先頭の4バイトを長さ情報として消費せず、そのまま Payload に含めてしまっていたと考えられます。結果として、アプリケーションが Payload を読み取ると、本来のデータに加えて、そのデータの長さを示す4バイトのヘッダーが先頭に付加された状態になっていました。これが「長さヘッダーの漏洩」です。

修正: このコミットでは、channelData および channelExtendedData 構造体を完全に削除し、これらのメッセージの処理を client.go と server.go の mainLoop および Accept 関数内で直接行うように変更しました。

新しいアプローチでは、受信したパケットの最初のバイト（packet[0]）を直接チェックして、それが msgChannelData または msgChannelExtendedData であるかを判別します。

// 新しい処理 (src/pkg/exp/ssh/client.go, src/pkg/exp/ssh/server.go)
switch packet[0] {
case msgChannelData:
    // ... バイト操作でpeersIdとlengthを抽出し、ペイロードをスライスする
    peersId := uint32(packet[1])<<24 | uint32(packet[2])<<16 | uint32(packet[3])<<8 | uint32(packet[4])
    length := int(packet[5])<<24 | int(packet[6])<<16 | int(packet[7])<<8 | int(packet[8])
    packet = packet[9:] // 長さ情報とメッセージタイプをスキップ
    c.getChan(peersId).data <- packet[:length] // 正しいペイロードを送信
case msgChannelExtendedData:
    // ... 同様にバイト操作でpeersId, datatype, lengthを抽出し、ペイロードをスライスする
    peersId := uint32(packet[1])<<24 | uint32(packet[2])<<16 | uint32(packet[3])<<8 | uint32(packet[4])
    datatype := uint32(packet[5])<<24 | uint32(packet[6])<<16 | uint32(packet[7])<<8 | uint32(packet[8])
    length := int(packet[9])<<24 | int(packet[10])<<16 | int(packet[11])<<8 | int(packet[12])
    packet = packet[13:] // 長さ情報とメッセージタイプ、データタイプをスキップ
    // ...
    c.getChan(peersId).dataExt <- packet[:length] // 正しいペイロードを送信
default:
    // その他のメッセージタイプはdecode関数で処理
    switch msg := decode(packet).(type) {
        // ...
    }
}

この変更により、SSH文字列型の長さ情報（4バイト）がペイロードから正しく分離され、実際のデータのみがチャネルのデータストリームに渡されるようになりました。

aglの元の高速パスコードの復元

コミットメッセージにある「aglの元の高速パスコードを復元する」とは、おそらく以前に存在した、channelData や channelExtendedData 構造体を介さずに、直接バイトスライスを操作してデータメッセージを処理する効率的なロジックが、この修正によって再導入されたことを指します。構造体による抽象化を避け、直接バイト操作を行うことで、デコードのオーバーヘッドを減らし、パフォーマンスを向上させることができます。

ロック解放のバグ修正

server.go の Accept 関数内で、無効なチャネルIDのパケットが到着した場合に s.lock が解放されないバグが修正されました。以前のコードでは、if !ok { continue } のパスで s.lock.Unlock() が呼び出されていませんでした。

// 以前のコード (server.go)
case *channelRequestMsg:
    s.lock.Lock()
    c, ok := s.channels[msg.PeersId]
    if !ok {
        continue // ここでロックが解放されない
    }
    c.handlePacket(msg)
    s.lock.Unlock()

// 修正後のコード (server.go)
case *channelRequestMsg:
    s.lock.Lock()
    c, ok := s.channels[msg.PeersId]
    if !ok {
        s.lock.Unlock() // ロックが解放されるようになった
        continue
    }
    c.handlePacket(msg)
    s.lock.Unlock()

この修正により、デッドロックの可能性が排除され、サーバーの堅牢性が向上しました。

構造体の削除

channelData および channelExtendedData 構造体は、上記の直接的なバイト操作によるペイロード処理の導入により、その役割を終えました。そのため、src/pkg/exp/ssh/messages.go からこれらの構造体定義と、それらを参照していた decode 関数のケースが削除されました。これはコードベースの簡素化と、不要な抽象化の排除に貢献しています。

コアとなるコードの変更箇所

このコミットは主に以下の3つのファイルに影響を与えています。

src/pkg/exp/ssh/client.go
src/pkg/exp/ssh/messages.go
src/pkg/exp/ssh/server.go

`src/pkg/exp/ssh/client.go`

mainLoop 関数内で、channelData および channelExtendedData メッセージの処理方法が大幅に変更されました。
- 以前は decode(packet).(type) を使用してこれらのメッセージを処理していましたが、新しいコードでは packet[0] を直接チェックし、メッセージタイプが msgChannelData または msgChannelExtendedData である場合に、バイトスライスから直接 peersId、length、および Payload を抽出するように変更されました。
- これにより、SSH文字列型の長さ情報がペイロードから正しく分離されるようになりました。
エラーハンドリングが改善され、c.Close() の呼び出しが defer ステートメントに移動されました。

`src/pkg/exp/ssh/messages.go`

channelData 構造体と channelExtendedData 構造体が完全に削除されました。
decode 関数から、これらの削除された構造体に対応する case msgChannelData: と case msgChannelExtendedData: のデコードロジックが削除されました。

`src/pkg/exp/ssh/server.go`

Accept 関数内で、channelData メッセージの処理方法が client.go と同様に、packet[0] を直接チェックし、バイトスライスから peersId、length、および Payload を抽出するように変更されました。
無効なチャネルにパケットが到着した場合に s.lock が解放されないバグが修正されました。具体的には、channelRequestMsg、channelData、channelEOFMsg、channelCloseMsg の各ケースで、if !ok { continue } の前に s.lock.Unlock() が追加されました。

コアとなるコードの解説

`src/pkg/exp/ssh/client.go` の変更点

client.go の mainLoop 関数は、受信したSSHパケットを処理し、適切なチャネルにルーティングする役割を担っています。

変更前:

// 以前のコードの一部
switch msg := decode(packet).(type) {
case *channelData:
    c.getChan(msg.PeersId).data <- msg.Payload
case *channelExtendedData:
    // ...
    if msg.Datatype == 1 {
        c.getChan(msg.PeersId).dataExt <- msg.Payload
    }
// ...
}

このコードでは、decode 関数がパケット全体を解析し、channelData や channelExtendedData 型の構造体を返していました。問題は、これらの構造体の Payload フィールドが、SSH文字列型の長さ情報を含んだままになっていたことです。

変更後:

// 変更後のコードの一部
switch packet[0] { // パケットの最初のバイトでメッセージタイプを直接判別
case msgChannelData:
    if len(packet) < 9 { // 最小限のパケット長チェック
        // malformed data packet
        break
    }
    // バイト操作でpeersIdを抽出 (4バイト)
    peersId := uint32(packet[1])<<24 | uint32(packet[2])<<16 | uint32(packet[3])<<8 | uint32(packet[4])
    // バイト操作でlengthを抽出 (4バイト)
    if length := int(packet[5])<<24 | int(packet[6])<<16 | int(packet[7])<<8 | int(packet[8]); length > 0 {
        packet = packet[9:] // メッセージタイプ(1) + peersId(4) + length(4) = 9バイトをスキップ
        c.getChan(peersId).data <- packet[:length] // 正しいペイロードをチャネルに送信
    }
case msgChannelExtendedData:
    if len(packet) < 13 { // 最小限のパケット長チェック
        // malformed data packet
        break
    }
    // バイト操作でpeersId (4バイト) と datatype (4バイト) を抽出
    peersId := uint32(packet[1])<<24 | uint32(packet[2])<<16 | uint32(packet[3])<<8 | uint32(packet[4])
    datatype := uint32(packet[5])<<24 | uint32(packet[6])<<16 | uint32(packet[7])<<8 | uint32(packet[8])
    // バイト操作でlengthを抽出 (4バイト)
    if length := int(packet[9])<<24 | int(packet[10])<<16 | int(packet[11])<<8 | int(packet[12]); length > 0 {
        packet = packet[13:] // メッセージタイプ(1) + peersId(4) + datatype(4) + length(4) = 13バイトをスキップ
        // RFC 4254 5.2 defines data_type_code 1 to be data destined for stderr
        if datatype == 1 {
            c.getChan(peersId).dataExt <- packet[:length] // 正しいペイロードをチャネルに送信
        }
    }
default:
    // その他のメッセージタイプは引き続きdecode関数で処理
    switch msg := decode(packet).(type) {
        // ... 既存の他のメッセージ処理
    }
}

この変更により、msgChannelData と msgChannelExtendedData のパケットは、decode 関数を介さずに直接バイト操作で解析されるようになりました。これにより、SSH文字列型の長さ情報が正しくスキップされ、純粋なペイロードデータのみがチャネルに渡されるようになります。

`src/pkg/exp/ssh/messages.go` の変更点

このファイルでは、channelData と channelExtendedData の構造体定義が削除されました。

変更前:

// 以前の定義
type channelData struct {
	PeersId uint32
	Payload []byte `ssh:"rest"`
}

type channelExtendedData struct {
	PeersId  uint32
	Datatype uint32
	Payload  []byte `ssh:"rest"`
}

これらの構造体は、ペイロードの長さ情報が漏洩するバグの原因となっていたため、不要になりました。

また、decode 関数内の対応する case 文も削除されました。

変更前:

// 以前のdecode関数の一部
case msgChannelData:
    msg = new(channelData)
case msgChannelExtendedData:
    msg = new(channelExtendedData)

これらの削除により、コードベースが簡素化され、新しい直接的なバイト操作による処理パスが唯一のデータ処理方法となりました。

`src/pkg/exp/ssh/server.go` の変更点

server.go の Accept 関数は、サーバー側で受信したSSHパケットを処理し、新しいチャネルの確立や既存チャネルへのデータルーティングを行います。

変更前:

// 以前のコードの一部
switch msg := decode(packet).(type) {
case *channelData:
    s.lock.Lock()
    c, ok := s.channels[msg.PeersId]
    if !ok {
        continue // ここでs.lockが解放されないバグがあった
    }
    c.handleData(msg.Payload)
    s.lock.Unlock()
// ...
}

client.go と同様に、channelData メッセージの処理が decode 関数を介していました。また、無効なチャネルIDの場合にロックが解放されないバグがありました。

変更後:

// 変更後のコードの一部
switch packet[0] { // パケットの最初のバイトでメッセージタイプを直接判別
case msgChannelData:
    if len(packet) < 9 { // 最小限のパケット長チェック
        // malformed data packet
        return nil, ParseError{msgChannelData}
    }
    // バイト操作でpeersIdを抽出
    peersId := uint32(packet[1])<<24 | uint32(packet[2])<<16 | uint32(packet[3])<<8 | uint32(packet[4])
    s.lock.Lock()
    c, ok := s.channels[peersId]
    if !ok {
        s.lock.Unlock() // 無効なチャネルの場合でもロックを解放
        continue
    }
    // バイト操作でlengthを抽出し、ペイロードをスライス
    if length := int(packet[5])<<24 | int(packet[6])<<16 | int(packet[7])<<8 | int(packet[8]); length > 0 {
        packet = packet[9:]
        c.handleData(packet[:length]) // 正しいペイロードを処理
    }
    s.lock.Unlock()
default:
    // その他のメッセージタイプは引き続きdecode関数で処理
    switch msg := decode(packet).(type) {
        // ... 既存の他のメッセージ処理
        case *channelRequestMsg:
            s.lock.Lock()
            c, ok := s.channels[msg.PeersId]
            if !ok {
                s.lock.Unlock() // ロック解放の修正
                continue
            }
            c.handlePacket(msg)
            s.lock.Unlock()
        // ... 他のケースでも同様にロック解放の修正
    }
}

この変更により、サーバー側でも msgChannelData のペイロードが正しく解析されるようになり、無効なチャネルIDのパケットが到着した場合のロック解放のバグも修正されました。これにより、サーバーの安定性と信頼性が向上します。

参考にした情報源リンク

RFC 4251 - The Secure Shell (SSH) Protocol Architecture: https://www.rfc-editor.org/rfc/rfc4251
RFC 4254 - The Secure Shell (SSH) Connection Protocol: https://www.rfc-editor.org/rfc/rfc4254
SSH string type explanation: https://vertexaisearch.cloud.google.com/grounding-api-redirect/AUZIYQH8w07D_onpTkmZIW_B2P9h8reDOI0l3ged3BCwtbMH54_9Q4m5nB0vmBJ3V8m3wSbxnArj_dD7qX9wIAyxIvvd67zfb-evqSr-MaQoZScf2vDxKt4bBIrmENpubD_T-Iys-QPT-P13NSlqNkmrdgJS
Go言語のバイトスライスに関するドキュメント (Go公式ドキュメントなど)

comemo

[インデックス 10148] ファイルの概要

コミット

GitHub上でのコミットページへのリンク

元コミット内容

変更の背景

前提知識の解説

SSH (Secure Shell) プロトコル

SSHチャネル

SSH文字列型 (RFC 4251/4254)

Go言語の `[]byte` 型

`decode(packet)` と `packet[0]` によるメッセージタイプの識別

技術的詳細

バグの原因と修正

aglの元の高速パスコードの復元

ロック解放のバグ修正

構造体の削除

コアとなるコードの変更箇所

`src/pkg/exp/ssh/client.go`

`src/pkg/exp/ssh/messages.go`

`src/pkg/exp/ssh/server.go`

コアとなるコードの解説

`src/pkg/exp/ssh/client.go` の変更点

`src/pkg/exp/ssh/messages.go` の変更点

`src/pkg/exp/ssh/server.go` の変更点

関連リンク

参考にした情報源リンク

Keyboard shortcuts

comemo