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.

Sign up for GitHub

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

Jump to bottom

Use non-header-including byte size field for tcp connections #726

Closed
wants to merge 1 commit into from
Closed

Use non-header-including byte size field for tcp connections #726

wants to merge 1 commit into from

Conversation

lisaSW
Copy link
Contributor

@lisaSW lisaSW commented Mar 21, 2022

Closes #725

  • updated incoming/outgoing bytes for tcp connections to use the orig_bytes/resp_bytes fields, which do not include header size. Non-tcp connections will use orig_ip_bytes/resp_ip_bytes, which include the header size.
  • field usage is now consistent

@lisaSW
incoming/outgoing bytes for tcp connections now use the orig_bytes/re…
cd10635 
…sp_bytes fields, while non-tcp connections use orig_ip_bytes/resp_ip_bytes
@lisaSW lisaSW requested review from Zalgo2462 and ethack March 21, 2022 20:24
ethack
ethack approved these changes Mar 22, 2022
View reviewed changes
Copy link
Collaborator

@ethack ethack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to me.

There is one edge case I've seen that may be worth addressing: when the OrigBytes/RespBytes are incorrect. In this case they will be greater than the OrigIPBytes/RespIPBytes values. But I think that should never actually be true, so another check could be to fall back to using OrigIPBytes/RespIPBytes if OrigBytes > OrigIPBytes || RespBytes > RespIPBytes.

Zalgo2462
Zalgo2462 suggested changes Mar 24, 2022
View reviewed changes
Copy link
Contributor

@Zalgo2462 Zalgo2462 left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks compatibility with ACM's Active Flow. Active Flow only reports the IP bytes fields since this is what is available in netflow v9 data. Annoyingly, it falsely reports the OrigBytes and RespBytes fields as 0's instead of unset.

We either need to add a special case in here for RITA, patch Active Flow, or both.

Conflicting Active Flow code: https://github.com/activecm/AC-Hunter/blob/8c7b481e2e757c8dc2e6c59de42c753f458a8897/active-flow/writer/bro/util.go#L42

@lisaSW lisaSW closed this Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zalgo2462 Zalgo2462 Zalgo2462 requested changes

@ethack ethack ethack approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use non-header-including byte size field for tcp connections
3 participants
@lisaSW @Zalgo2462 @ethack