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

eve: revert ethernet addresses when needed #9651

Closed
wants to merge 1 commit into from

Conversation

regit
Copy link
Contributor

@regit regit commented Oct 18, 2023

EVE logging has a direction parameter that can cause the logging of an application layer to be done in a direction that is not linked to the packet. As a result the source IP addres could be assigned the MAC address of the destination IP and reverse.

This patch addresses this by propagating the direction to the ethernet logging function and using it there to define the correct mapping.

Issue #6405

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/6405

Describe changes:

  • Invert ethernet address when needed

EVE logging has a direction parameter that can cause the logging
of an application layer to be done in a direction that is not linked
to the packet. As a result the source IP addres could be assigned the
MAC address of the destination IP and reverse.

This patch addresses this by propagating the direction to the ethernet
logging function and using it there to define the correct mapping.

Issue OISF#6405
@regit regit mentioned this pull request Oct 18, 2023
3 tasks
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #9651 (86b501f) into master (986a441) will decrease coverage by 0.02%.
The diff coverage is 83.78%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9651      +/-   ##
==========================================
- Coverage   82.38%   82.37%   -0.02%     
==========================================
  Files         968      968              
  Lines      274338   274368      +30     
==========================================
- Hits       226026   226017       -9     
- Misses      48312    48351      +39     
Flag Coverage Δ
fuzzcorpus 64.65% <10.81%> (-0.02%) ⬇️
suricata-verify 60.89% <83.78%> (-0.04%) ⬇️
unittests 62.85% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

uint8_t *src;
uint8_t *dst;
switch (dir) {
case LOG_DIR_FLOW:
Copy link
Contributor

Choose a reason for hiding this comment

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

LOG_DIR_FLOW and LOG_DIR_FLOW_TOSERVER behave the same way ? What is the difference ?

Copy link
Contributor Author

@regit regit Oct 18, 2023

Choose a reason for hiding this comment

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

I don't think there is a real difference there. LOG_DIR_FLOW is used by the flow logger who log client->server. And LOG_DIR_FLOW_TOSERVER is used in netflow for the to server way. Maybe we can merge them at least in this switch.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 16230

Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

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

Looks good to me. :) Does it also need an s-v test with the dns request and answer as mentioned in the redmine ticket?

@victorjulien
Copy link
Member

@regit can you add some SV tests? Esp important for logging behavior changes. Thanks!

Also, formatting check is unhappy.

@regit
Copy link
Contributor Author

regit commented Oct 30, 2023

@regit can you add some SV tests? Esp important for logging behavior changes. Thanks!

Also, formatting check is unhappy.

I'm doing that now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants