Skip to content

harden client reply parsers against malformed server replies #655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
drakkan wants to merge 1 commit into
base: master
Choose a base branch
from
+633 −65
Open

harden client reply parsers against malformed server replies #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 86 additions & 59 deletions client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,15 +397,25 @@ func (c *Client) ReadDirContext(ctx context.Context, p string) ([]os.FileInfo, e
}
switch typ {
case sshFxpName:
sid, data := unmarshalUint32(data)
if sid != id {
return nil, &unexpectedIDErr{id, sid}
data, err = unmarshalSID(id, data)
if err != nil {
return nil, err
}
var count int
count, data, err = unmarshalCount(data)
if err != nil {
return nil, err
}
count, data := unmarshalUint32(data)
for i := uint32(0); i < count; i++ {
for i := 0; i < count; i++ {

@puellanivis puellanivis Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can switch this to range i since we’re here.

var filename string
filename, data = unmarshalString(data)
_, data = unmarshalString(data) // discard longname
filename, data, err = unmarshalStringSafe(data)
if err != nil {
return nil, err
}
_, data, err = unmarshalStringSafe(data) // discard longname
if err != nil {
return nil, err
}
var attr *FileStat
attr, data, err = unmarshalAttrs(data)
if err != nil {
Expand Down Expand Up @@ -441,11 +451,14 @@ func (c *Client) opendir(ctx context.Context, path string) (string, error) {
}
switch typ {
case sshFxpHandle:
sid, data := unmarshalUint32(data)
if sid != id {
return "", &unexpectedIDErr{id, sid}
data, err := unmarshalSID(id, data)
if err != nil {
return "", err
}
handle, _, err := unmarshalStringSafe(data)
if err != nil {
return "", err
}
handle, _ := unmarshalString(data)
return handle, nil
case sshFxpStatus:
return "", normaliseError(unmarshalStatus(id, data))
Expand Down Expand Up @@ -477,9 +490,9 @@ func (c *Client) Lstat(p string) (os.FileInfo, error) {
}
switch typ {
case sshFxpAttrs:
sid, data := unmarshalUint32(data)
if sid != id {
return nil, &unexpectedIDErr{id, sid}
data, err := unmarshalSID(id, data)
if err != nil {
return nil, err
}
attr, _, err := unmarshalAttrs(data)
if err != nil {
Expand All @@ -506,15 +519,21 @@ func (c *Client) ReadLink(p string) (string, error) {
}
switch typ {
case sshFxpName:
sid, data := unmarshalUint32(data)
if sid != id {
return "", &unexpectedIDErr{id, sid}
data, err := unmarshalSID(id, data)
if err != nil {
return "", err
}
count, data, err := unmarshalCount(data)
if err != nil {
return "", err
}
count, data := unmarshalUint32(data)
if count != 1 {
return "", unexpectedCount(1, count)
return "", unexpectedCount(1, uint32(count))
}
filename, _, err := unmarshalStringSafe(data) // ignore dummy attributes
if err != nil {
return "", err
}
filename, _ := unmarshalString(data) // ignore dummy attributes
return filename, nil
case sshFxpStatus:
return "", normaliseError(unmarshalStatus(id, data))
Expand Down Expand Up @@ -677,11 +696,14 @@ func (c *Client) open(path string, pflags uint32) (*File, error) {
}
switch typ {
case sshFxpHandle:
sid, data := unmarshalUint32(data)
if sid != id {
return nil, &unexpectedIDErr{id, sid}
data, err := unmarshalSID(id, data)
if err != nil {
return nil, err
}
handle, _, err := unmarshalStringSafe(data)
if err != nil {
return nil, err
}
handle, _ := unmarshalString(data)
return &File{c: c, path: path, handle: handle}, nil
case sshFxpStatus:
return nil, normaliseError(unmarshalStatus(id, data))
Expand Down Expand Up @@ -721,9 +743,9 @@ func (c *Client) stat(path string) (*FileStat, error) {
}
switch typ {
case sshFxpAttrs:
sid, data := unmarshalUint32(data)
if sid != id {
return nil, &unexpectedIDErr{id, sid}
data, err := unmarshalSID(id, data)
if err != nil {
return nil, err
}
attr, _, err := unmarshalAttrs(data)
return attr, err
Expand All @@ -745,9 +767,9 @@ func (c *Client) fstat(handle string) (*FileStat, error) {
}
switch typ {
case sshFxpAttrs:
sid, data := unmarshalUint32(data)
if sid != id {
return nil, &unexpectedIDErr{id, sid}
data, err := unmarshalSID(id, data)
if err != nil {
return nil, err
}
attr, _, err := unmarshalAttrs(data)
return attr, err
Expand Down Expand Up @@ -942,15 +964,21 @@ func (c *Client) RealPath(path string) (string, error) {
}
switch typ {
case sshFxpName:
sid, data := unmarshalUint32(data)
if sid != id {
return "", &unexpectedIDErr{id, sid}
data, err := unmarshalSID(id, data)
if err != nil {
return "", err
}
count, data, err := unmarshalCount(data)
if err != nil {
return "", err
}
count, data := unmarshalUint32(data)
if count != 1 {
return "", unexpectedCount(1, count)
return "", unexpectedCount(1, uint32(count))
}
filename, _, err := unmarshalStringSafe(data) // ignore attributes
if err != nil {
return "", err
}
filename, _ := unmarshalString(data) // ignore attributes
return filename, nil
case sshFxpStatus:
return "", normaliseError(unmarshalStatus(id, data))
Expand Down Expand Up @@ -1147,13 +1175,11 @@ func (f *File) readChunkAt(ch chan result, b []byte, off int64) (n int, err erro
return n, normaliseError(unmarshalStatus(id, data))

case sshFxpData:
sid, data := unmarshalUint32(data)
if id != sid {
return n, &unexpectedIDErr{id, sid}
data, err = unmarshalDataReply(id, data)
if err != nil {
return n, err
}

l, data := unmarshalUint32(data)
n += copy(b[n:], data[:l])
n += copy(b[n:], data)

default:
return n, unimplementedPacketErr(typ)
Expand Down Expand Up @@ -1293,13 +1319,10 @@ func (f *File) readAt(b []byte, off int64) (int, error) {
err = normaliseError(unmarshalStatus(packet.id, s.data))

case sshFxpData:
sid, data := unmarshalUint32(s.data)
if packet.id != sid {
err = &unexpectedIDErr{packet.id, sid}

} else {
l, data := unmarshalUint32(data)
n = copy(packet.b, data[:l])
var data []byte
data, err = unmarshalDataReply(packet.id, s.data)
if err == nil {
n = copy(packet.b, data)

// For normal disk files, it is guaranteed that this will read
// the specified number of bytes, or up to end of file.
Expand Down Expand Up @@ -1522,14 +1545,15 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) {
err = normaliseError(unmarshalStatus(readWork.id, s.data))

case sshFxpData:
sid, data := unmarshalUint32(s.data)
if readWork.id != sid {
err = &unexpectedIDErr{readWork.id, sid}

} else {
l, data := unmarshalUint32(data)
b = pool.Get()[:l]
n = copy(b, data[:l])
var data []byte
data, err = unmarshalDataReply(readWork.id, s.data)
if err == nil {
// A malicious server may return more data than we
// requested (up to the packet limit, larger than the
// pool buffer); copy bounds it to the buffer size
// rather than slicing past the page and panicking.
b = pool.Get()
n = copy(b, data)
b = b[:n]
}

Expand Down Expand Up @@ -1645,8 +1669,11 @@ func (f *File) writeChunkAt(ch chan result, b []byte, off int64) (int, error) {

switch typ {
case sshFxpStatus:
id, _ := unmarshalUint32(data)
err := normaliseError(unmarshalStatus(id, data))
id, _, err := unmarshalUint32Safe(data)
if err != nil {
return 0, err
}
err = normaliseError(unmarshalStatus(id, data))
if err != nil {
return 0, err
}
Expand Down
Loading
Loading