Fix panic in MSSQL when Login7 package is invalid #10452
Conversation
github-actions
bot
added
the
database-access
| f.Fuzz(func(t *testing.T, packet []byte) { | ||
| reader := bytes.NewReader(packet) | ||
| // no assertion, check for panic | ||
| ReadLogin7Packet(reader) |
There was a problem hiding this comment.
How about using https://pkg.go.dev/github.com/stretchr/testify/require#NotPanics?
There was a problem hiding this comment.
Good idea. I forgot about that function. testify has an assertion for every occasion :)
| if len(pkt.Data) <= int(header.IbUserName) { | ||
| return "", errInvalidPackage | ||
| } |
There was a problem hiding this comment.
Isn't this condition already covered by the check above? Unless CchUserName is somehow negative...
There was a problem hiding this comment.
You're right. I remamber that at some point I was getting a negative value, but after some cleanup now this if is not needed anymore and CchUserName is uint16, so it will never be negative. Re-tested and removed.
| if len(pkt.Data) < int(header.IbDatabase) { | ||
| return "", errInvalidPackage | ||
| } |
There was a problem hiding this comment.
Same q as above. Also, you have < condition here and <= condition in a similar check above.
There was a problem hiding this comment.
The same as above. Removed, but here is why < is required in this case:
% go test -count=1 ./...
--- FAIL: TestReadLogin7 (0.00s)
protocol_test.go:50:
Error Trace: protocol_test.go:50
Error: Received unexpected error:
invalid login7 packet
Test: TestReadLogin7
FAIL
FAIL github.com/gravitational/teleport/lib/srv/db/sqlserver/protocol 0.496s
? github.com/gravitational/teleport/lib/srv/db/sqlserver/protocol/fixtures [no test files]
FAIL
Otherwise the TestReadLogin7 fails as it tries to read the last element from the slice (it won't panic. In this case last element is bounded by last element here: pkt.Data[136:136] with len 136).
jakule
force-pushed
the
jakule/mssql-fuzz-panic
branch
from
1a5d620 to
7982cad
Compare
jakule
force-pushed
the
jakule/mssql-fuzz-panic
branch
from
7982cad to
c69479f
Compare
jakule
force-pushed
the
jakule/mssql-fuzz-panic
branch
from
c69479f to
8d33c45
Compare
|
@r0mant Should we backport this to v9 branch? |
|
@jakule Sure but please make sure that MSSQL access still works with this fix cc @smallinsky |
I decided to give a shoot to new Go's fuzzer and this is one of my findings. Function
ReadLogin7Packetpanics for some inputs. I added a fix for the inputs that fuzzed discovered. I also added a fuzz test guarded bygo1.18for now, as it would fail to build with our current setup.