8000 Merge pull request #235 from crazed/unknown-extended-packet-handling · etherscan-io/sftp@09448a4 · GitHub
[go: up one dir, main page]
Skip to content

Commit 09448a4

Browse files
eikenbeikenb
authored
Merge pull request pkg#235 from crazed/unknown-extended-packet-handling
Send unsupported error on unsupported extended packets.
2 parents 4948837 + 7d7ee5c commit 09448a4

File tree

5 files changed

+53
-20
lines changed

5 files changed

+53
-20
lines changed

‎packet-typing.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,9 @@ func makePacket(p rxPacket) (requestPacket, error) {
127127
return nil, errors.Errorf("unhandled packet type: %s", p.pktType)
128128
}
129129
if err := pkt.UnmarshalBinary(p.pktBytes); err != nil {
130-
return nil, err
130+
// Return partially unpacked packet to allow callers to return
131+
// error messages appropriately with necessary id() method.
132+
return pkt, err
131133
}
132134
return pkt, nil
133135
}

‎packet.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -874,10 +874,18 @@ type sshFxpExtendedPacket struct {
874874
}
875875
}
876876

877-
func (p sshFxpExtendedPacket) id() uint32 { return p.ID }
878-
func (p sshFxpExtendedPacket) readonly() bool { return p.SpecificPacket.readonly() }
877+
func (p sshFxpExtendedPacket) id() uint32 { return p.ID }
878+
func (p sshFxpExtendedPacket) readonly() bool {
879+
if p.SpecificPacket == nil {
880+
return true
881+
}
882+
return p.SpecificPacket.readonly()
883+
}
879884

880885
func (p sshFxpExtendedPacket) respond(svr *Server) error {
886+
if p.SpecificPacket == nil {
887+
return nil
888+
}
881889
return p.SpecificPacket.respond(svr)
882890
}
883891

@@ -897,7 +905,7 @@ func (p *sshFxpExtendedPacket) UnmarshalBinary(b []byte) error {
897905
case "posix-rename@openssh.com":
898906
p.SpecificPacket = &sshFxpExtendedPacketPosixRename{}
899907
default:
900-
return errUnknownExtendedPacket
908+
return errors.Wrapf(errUnknownExtendedPacket, "packet type %v", p.SpecificPacket)
901909
}
902910

903911
return p.SpecificPacket.UnmarshalBinary(bOrig)

‎request-server.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,9 +128,18 @@ func (rs *RequestServer) Serve() error {
128128

129129
pkt, err = makePacket(rxPacket{fxp(pktType), pktBytes})
130130
if err != nil {
131-
debug("makePacket err: %v", err)
132-
rs.conn.Close() // shuts down recvPacket
133-
break
131+
switch errors.Cause(err) {
132+
case errUnknownExtendedPacket:
133+
if err := rs.serverConn.sendError(pkt, ErrSshFxOpUnsupported); err != nil {
134+
debug("failed to send err packet: %v", err)
135+
rs.conn.Close() // shuts down recvPacket
136+
break
137+
}
138+
default:
139+
debug("makePacket err: %v", err)
140+
rs.conn.Close() // shuts down recvPacket
141+
break
142+
}
134143
}
135144

136145
pktChan <- pkt

‎server.go

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ func (svr *Server) sftpServerWorker(pktChan chan requestPacket) error {
134134
case *sshFxpOpenPacket:
135135
readonly = pkt.readonly()
136136
case *sshFxpExtendedPacket:
137-
readonly = pkt.SpecificPacket.readonly()
137+
readonly = pkt.readonly()
138138
}
139139

140140
// If server is operating read-only and a write operation is requested,
@@ -304,9 +304,18 @@ func (svr *Server) Serve() error {
304304

305305
pkt, err = makePacket(rxPacket{fxp(pktType), pktBytes})
306306
if err != nil {
307-
debug("makePacket err: %v", err)
308-
svr.conn.Close() // shuts down recvPacket
309-
break
307+
switch errors.Cause(err) {
308+
case errUnknownExtendedPacket:
309+
if err := svr.serverConn.sendError(pkt, ErrSshFxOpUnsupported); err != nil {
310+
debug("failed to send err packet: %v", err)
311+
svr.conn.Close() // shuts down recvPacket
312+
break
313+
}
314+
default:
315+
debug("makePacket err: %v", err)
316+
svr.conn.Close() // shuts down recvPacket
317+
break
318+
}
310319
}
311320

312321
pktChan <- pkt

‎server_test.go

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -198,24 +198,29 @@ func (p sshFxpTestBadExtendedPacket) MarshalBinary() ([]byte, error) {
198198
}
199199

200200
// test that errors are sent back when we request an invalid extended packet operation
201+
// this validates the following rfc draft is followed https://tools.ietf.org/html/draft-ietf-secsh-filexfer-extensions-00
201202
func TestInvalidExtendedPacket(t *testing.T) {
202203
client, server := clientServerPair(t)
203204
defer client.Close()
204205
defer server.Close()
205206

206207
badPacket := sshFxpTestBadExtendedPacket{client.nextID(), "thisDoesn'tExist", "foobar"}
207-
_, _, err := client.clientConn.sendPacket(badPacket)
208-
if err == nil {
209-
t.Fatal("expected error from bad packet")
208+
typ, data, err := client.clientConn.sendPacket(badPacket)
209+
if err != nil {
210+
t.Fatalf("unexpected error from sendPacket: %s", err)
210211
}
211-
212-
// try to stat a file; the client should have shut down.
213-
filePath := "/etc/passwd"
214-
_, err = client.Stat(filePath)
215-
if err == nil {
216-
t.Fatal("expected error from closed connection")
212+
if typ != ssh_FXP_STATUS {
213+
t.Fatalf("received non-FPX_STATUS packet: %v", typ)
217214
}
218215

216+
err = unmarshalStatus(badPacket.id(), data)
217+
statusErr, ok := err.(*StatusError)
218+
if !ok {
219+
t.Fatal("failed to convert error from unmarshalStatus to *StatusError")
220+
}
221+
if statusErr.Code != ssh_FX_OP_UNSUPPORTED {
222+
t.Errorf("statusErr.Code => %d, wanted %d", statusErr.Code, ssh_FX_OP_UNSUPPORTED)
223+
}
219224
}
220225

221226
// test that server handles concurrent requests correctly

0 commit comments

Comments
 (0)
0