app-layer: add defensive checks #895
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add defensive checks to most AppLayer API calls as we've observed some unusual cases making these checks necessary. Bug #1011.
alessandro-guido
pushed a commit
to alessandro-guido/suricata
that referenced
this pull request
Jun 20, 2014
We have follow TCP RFC (http://tools.ietf.org/html/rfc793#section-3.4). There is two cases depending on wether the original packet contains a ACK. If packet has no ACK, the RST seq number is 0 and the ACK is built the standard way. If packet has a ACK, the seq of the RST packet is equal to the ACK of incoming packet and the ACK is build using packet sequence number and size of the data. Regarding standard Ack number, it is computed using seq number of captured packet added to packet length. Finally 1 is added so we respect the RFC: If the ACK control bit is set this field contains the value of the next sequence number the sender of the segment is expecting to receive. Once a connection is established this is always sent. With this patch we have some correct results. With the following rule: reject ssh any any -> 192.168.56.3 any (msg:"no SSH way"; sid:3; rev:1;) ssh connection to 192.168.56.3 is correctly resetted on client side. But this is not perfect. If we have the following rule: reject tcp any any -> 192.168.56.3 22 (msg:"no way"; sid:2; rev:1;) then the connection is not resetted on a standard ethernet network. But if we introduce 20ms delay on packets, then it is correctly resetted. This is explained when looking at the network trace. The reset is sent as answer to the SYN packet and it is emitted after the SYN ACK from server because the exchange is really fast. So this is discarded by the client OS which has already seen a ACK for the same sequence number. This should fix OISF#895.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add defensive checks to most AppLayer API calls as we've observed some
unusual cases making these checks necessary.
Bug #1101. https://redmine.openinfosecfoundation.org/issues/1101
Prscript: