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

af-packet: terminate on same iface and copyiface #9308

Closed
wants to merge 1 commit into from
Closed

af-packet: terminate on same iface and copyiface #9308

wants to merge 1 commit into from

Conversation

inashivb
Copy link
Member

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

Previous PR: #9307

Changes since v2:

  • add void to function params

@inashivb
af-packet: terminate on same iface and copyiface
1b8e7f6 
If the interface and copy-iface are same for an af-packet IPS device
setting then fataly exit.

Bug 5870
@inashivb inashivb marked this pull request as ready for review July 31, 2023 12:49
victorjulien
victorjulien requested changes Jul 31, 2023
View reviewed changes
Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

wouldn't it be enough to check in ParseAFPConfig if strcmp(iface, out_iface) == 0 right after

    if (ConfGetChildValueWithDefault(if_root, if_default, "copy-iface", &out_iface) == 1) {
        if (strlen(out_iface) > 0) {
            aconf->out_iface = out_iface;
        }
    }

?

src/util-device.c
TAILQ_FOREACH (child, &base->head, next) {
ConfNode *subchild;
const char *iface = NULL, *copyiface = NULL;
TAILQ_FOREACH (subchild, &child->head, next) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand what we're looping here?

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I understand, each of the entries in a list item like

 - interface: wlp0s20f3
    threads: auto 
    cluster-id: 99
    cluster-type: cluster_flow
    defrag: yes
    copy-mode: ips
    copy-iface: eth2

src/util-device.c
@@ -271,6 +271,36 @@ const char *LiveGetShortName(const char *dev)
return live_dev->dev_short;
}

int CheckAFPacketIPSDevs(void)
Copy link
Member

Choose a reason for hiding this comment

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

why is this in util-device?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that since we're looping for ifaces and that kinda logic seemed to already be here in this file so this might be the right place.

src/suricata.c
@@ -1024,6 +1024,10 @@ static TmEcode ParseInterfacesList(const int runmode, char *pcap_dev)
SCLogError("No interface found in config for af-packet");
SCReturnInt(TM_ECODE_FAILED);
}
int retval = CheckAFPacketIPSDevs();
Copy link
Member

Choose a reason for hiding this comment

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

can this be checked in the af-packet files instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. As per your main suggestion since all of the other things done are invalid, it should all probably be there.

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW2_autofp_stats_chk
.uptime 276 291 105.43%

Pipeline 15458

@inashivb
Copy link
Member Author

inashivb commented Aug 1, 2023 •
edited
Loading

wouldn't it be enough to check in ParseAFPConfig if strcmp(iface, out_iface) == 0 right after

    if (ConfGetChildValueWithDefault(if_root, if_default, "copy-iface", &out_iface) == 1) {
        if (strlen(out_iface) > 0) {
            aconf->out_iface = out_iface;
        }
    }

?

Please note that this works but the program exits much later than the approach in this PR.
Following are the comparisons:
The approach suggested above:

real	1m12.404s
user	0m0.007s
sys	0m0.014s

vs this PR:

real	0m1.719s
user	0m0.026s
sys	0m0.036s

Is this OK?

@victorjulien
Copy link
Member

Can you explain why this happens?

@inashivb
Copy link
Member Author

inashivb commented Aug 1, 2023

Can you explain why this happens?

For the following conf

af-packet:
  - interface: enp10s0
    cluster-id: 99
    cluster-type: cluster_flow
    defrag: yes
    copy-mode: ips
    copy-iface: enp9s0

  - interface: enp9s0
    cluster-id: 98
    cluster-type: cluster_flow
    defrag: yes
    copy-mode: ips
    copy-iface: enp9s0

It seems to me that with this PR,

  1. runmode is one of the first things to be determined by Suricata
  2. the corresponding runmode configuration is loaded
    I tried to add the check for the interface and copy-iface being correctly set as a part of (2). This made the program exit in the very initial stages.

Note: However, now, I'm thinking this is probably wrong bc we haven't established if it's the AF_PACKET IPS mode yet.

With your approach, I saw that a lot of things happen before we get to checking the configuration.

  1. IPS mode is set
  2. MTU of the ifaces is checked to be the same
  3. unix socket is enabled
  4. output modules are initialized
  5. filestore is set up
  6. detection engine is set up
  • rules are loaded from the files
  • signatures are grouped as per type
  1. af-packet now activates the IPS mode in one direction (enp10s0->enp9s0)
  2. Now, the other direction is checked and it is found that the copy-iface is incorrectly set

It seems to me that this is the correct approach indeed as this happens after we have established that we're dealing with the correct modes (AF_PACKET IPS in this case).
Q: Do you think it makes sense to optimize it and check the conf for correct ifaces right after (1) instead of after going over other stuff?

@victorjulien
Copy link
Member

Thanks for the analysis, I think it explains the issue well. I still think the place I suggested is the correct place to check this, however it does seem inefficient. I wonder if it makes sense to rework the initialization to do more of the config validation work earlier. Perhaps a 2 step approach, where parsing happens early, but actual start of the runtime late.

Anyway, I think we can take the simple approach for now, and create a future work ticket for the optimizations.

@inashivb inashivb mentioned this pull request Aug 1, 2023
Closed
@inashivb inashivb closed this Aug 1, 2023
@inashivb inashivb deleted the bug-5870/v3 branch August 1, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victorjulien victorjulien victorjulien requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@inashivb @suricata-qa @victorjulien