search

pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.5k stars 379 forks source link

Proccess is panic in FUZZ test #500

Closed lyfhallelujah closed 2 years ago

lyfhallelujah commented 2 years ago

https://github.com/pkg/sftp/blob/dcfc1d5e41627f88fb7ebedbb3a25d807eac6097/request-server.go#L294 when receive an unexpected pktType package as server，the packetWorker will process in default case，and pkt may be nil. and call id() will panic.

drakkan commented 2 years ago

Hi,

can you please provide a test case or the procedure used for fuzzy testing? An invalid packet should be dropped here and so it should not reach the packet worker. Thanks

puellanivis commented 2 years ago

Indeed, I’m not seeing any way for pkt to logically be nil . Even in unknown extended packets we only override the existing ExtendedPacket when it’s known to be non-nil

The only thing I can think of, is if the SpecificPacket were a typed-nil, as the test above is only the case for an untyped-nil. But I still don’t see anyway way for that to happen.

If you got this result from some sort of fuzzing software, we could really use the output of that tool about what caused it, and the stack trace of the panic as well. (Or at least, as much of the stack trace that is our code.)

lyfhallelujah commented 2 years ago

I'm sorry, we have this problem due to the use of the old version, this problem has been solved by the following submit. https://github.com/pkg/sftp/commit/e56b4ff6adb11495472bd0fcc6a5d24105ae1457

puellanivis commented 2 years ago

Ah yes, that would definitely explain how it would end up as nil 😆 Closing this now.

chemsky commented 2 years ago

This's very helpful. Thanks