Skip to content
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.

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

fix fragmentation bug; remove an unnecessary code in SplitData function #112

Merged
merged 1 commit into from
May 3, 2020

Conversation

rmaksimov
Copy link
Contributor

The following error was occurred every time i received [13 10 13 10 0 0 0 0 0 0]

[*] RPC/HTTP forced, trying RPC/HTTP
[+] Found cached Autodiscover record. Using this (use --nocache to force new lookup)
[*] RPC URL set: https://mail.contoso.com/rpc/rpcproxy.dll?x98cf1j-10ez-481v-a25g-1337f3s4132i@contoso.com:6001
[*] Setting up channels
[+] Binding to RPC
panic: runtime error: slice bounds out of range [:10] with capacity 8

goroutine 6 [running]:
github.com/sensepost/ruler/utils.ReadUint16(...)
        /home/hulk/ruler/utils/utils.go:213
github.com/sensepost/ruler/rpc-http.(*RPCResponse).Unmarshal(0xc000312e41, 0xc0000a1120, 0x2, 0x8, 0x2, 0xc000112000, 0x1)
        /home/hulk/ruler/rpc-http/packets.go:466 +0x3a3
github.com/sensepost/ruler/rpc-http.RPCOpenOut(0xc0003022c0, 0x35, 0xc00005a520, 0xc00004e2f0, 0xc00002bd00, 0x0)
        /home/hulk/ruler/rpc-http/rpctransport.go:239 +0x264
created by github.com/sensepost/ruler/rpc-http.RPCOpen
        /home/hulk/ruler/rpc-http/rpctransport.go:190 +0x125
exit status 2

but everything was fine when i received [13 10 13 10 49 99 13 10 5 0]

Looking into the code i found a strange part of it that doesn't seem to work
Moreover the following looks like an incorrect condition (i have never seen 0x31, 0x0d, 0x0a sequence at all)

if bytes.Equal(data[k:k+3], []byte{0x31, 0x0d, 0x0a}) { //this is a part of a fragment
    dbuf = []byte{0x05} //start the new fragment
    offset = 9          //adjust the offset, because the rest of the packet is in another fragment
    k += 4              //jump ahead to the next fragment
    continue

@staaldraad
Copy link
Collaborator

thanks for all the updates @rmaksimov! 🙏

This fragmentation code is/was definitely necessary, I recall having lots of issues getting it to a point where it was generic and stable. From what I remember that particular fragmentation was against an older exchange server.

It might be that this was a problem that came up with address book retrieval / parsing. I'm going to leave this open until I get a chance to test/recall what exactly the problem was here

@rmaksimov
Copy link
Contributor Author

there was a problem related to RPC packets that weren't parsed properly, i have tested it on several servers and everything seems fine
Anyway it's up to you :)

@staaldraad
Copy link
Collaborator

there was a problem related to RPC packets that weren't parsed properly, i have tested it on several servers and everything seems fine

In that case I'm going to pull it into the updated version. I'm not in a position to actively test Ruler against different servers, so I'm happy having your "in the wild" testing of this

@staaldraad staaldraad merged commit 5e4ecb1 into sensepost:master May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants