Skip to content

exception-policy: add 'reject-both' option #14060

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+34 −4
Open

exception-policy: add 'reject-both' option #14060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions etc/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8753,6 +8753,10 @@
"reject": {
"type": "integer",
"minimum": 0
},
"reject_both": {
"type": "integer",
"minimum": 0
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/app-layer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ ExceptionPolicyStatsSetts app_layer_error_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ false,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
.valid_settings_ips = {
/* EXCEPTION_POLICY_NOT_SET */ false,
Expand All @@ -125,6 +126,7 @@ ExceptionPolicyStatsSetts app_layer_error_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ true,
/* EXCEPTION_POLICY_DROP_FLOW */ true,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
};
// clang-format on
Expand Down
4 changes: 4 additions & 0 deletions src/decode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ ExceptionPolicyStatsSetts defrag_memcap_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ false,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
.valid_settings_ips = {
/* EXCEPTION_POLICY_NOT_SET */ false,
Expand All @@ -103,6 +104,7 @@ ExceptionPolicyStatsSetts defrag_memcap_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ true,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
};
// clang-format on
Expand All @@ -119,6 +121,7 @@ ExceptionPolicyStatsSetts flow_memcap_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ false,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
.valid_settings_ips = {
/* EXCEPTION_POLICY_NOT_SET */ false,
Expand All @@ -129,6 +132,7 @@ ExceptionPolicyStatsSetts flow_memcap_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ true,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
};
// clang-format on
Expand Down
8 changes: 8 additions & 0 deletions src/stream-tcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ ExceptionPolicyStatsSetts stream_memcap_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ false,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
.valid_settings_ips = {
/* EXCEPTION_POLICY_NOT_SET */ false,
Expand All @@ -112,6 +113,7 @@ ExceptionPolicyStatsSetts stream_memcap_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ true,
/* EXCEPTION_POLICY_DROP_FLOW */ true,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
};
// clang-format on
Expand All @@ -128,6 +130,7 @@ ExceptionPolicyStatsSetts stream_reassembly_memcap_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ false,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
.valid_settings_ips = {
/* EXCEPTION_POLICY_NOT_SET */ false,
Expand All @@ -138,6 +141,7 @@ ExceptionPolicyStatsSetts stream_reassembly_memcap_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ true,
/* EXCEPTION_POLICY_DROP_FLOW */ true,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
};
// clang-format on
Expand All @@ -154,6 +158,7 @@ ExceptionPolicyStatsSetts stream_midstream_enabled_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ false,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ false,
/* EXCEPTION_POLICY_REJECT_BOTH */ false,
},
.valid_settings_ips = {
/* EXCEPTION_POLICY_NOT_SET */ false,
Expand All @@ -164,6 +169,7 @@ ExceptionPolicyStatsSetts stream_midstream_enabled_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ false,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ false,
/* EXCEPTION_POLICY_REJECT_BOTH */ false,
},
};
// clang-format on
Expand All @@ -180,6 +186,7 @@ ExceptionPolicyStatsSetts stream_midstream_disabled_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ false,
/* EXCEPTION_POLICY_DROP_FLOW */ false,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
.valid_settings_ips = {
/* EXCEPTION_POLICY_NOT_SET */ false,
Expand All @@ -190,6 +197,7 @@ ExceptionPolicyStatsSetts stream_midstream_disabled_eps_stats = {
/* EXCEPTION_POLICY_DROP_PACKET */ false,
/* EXCEPTION_POLICY_DROP_FLOW */ true,
/* EXCEPTION_POLICY_REJECT */ true,
/* EXCEPTION_POLICY_REJECT_BOTH */ true,
},
};
// clang-format on
Expand Down
5 changes: 3 additions & 2 deletions src/util-exception-policy-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ enum ExceptionPolicy {
EXCEPTION_POLICY_BYPASS_FLOW,
EXCEPTION_POLICY_DROP_PACKET,
EXCEPTION_POLICY_DROP_FLOW,
EXCEPTION_POLICY_REJECT,
EXCEPTION_POLICY_REJECT, /**< reject src */
EXCEPTION_POLICY_REJECT_BOTH /**< reject both src and dest */
};

#define EXCEPTION_POLICY_MAX (EXCEPTION_POLICY_REJECT + 1)
#define EXCEPTION_POLICY_MAX (EXCEPTION_POLICY_REJECT_BOTH + 1)
Comment on lines -36 to +37
Copy link
Member

Choose a reason for hiding this comment

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

This looks prone to error if we forget to update it. Should we make MAX a part of the enum above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, but that then has as a consequence that all the runtime code paths need to handle the size value in switch statements.


/* Max length = possible exception policy scenarios + counter names
* + exception policy type. E.g.:
Expand Down
15 changes: 13 additions & 2 deletions src/util-exception-policy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ const char *ExceptionPolicyEnumToString(enum ExceptionPolicy policy, bool is_jso
return "reject";
case EXCEPTION_POLICY_BYPASS_FLOW:
return "bypass";
case EXCEPTION_POLICY_REJECT_BOTH:
return "reject_both";
Copy link

Choose a reason for hiding this comment

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

Should that be reject-both (instead of reject_both) to be consistent with the value that gets set in the yaml, or does it need to be conditional on is_json like the others below?

Copy link
Member

Choose a reason for hiding this comment

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

By convention, in the YAML config we use -, but for JSON field names we use _.

case EXCEPTION_POLICY_DROP_FLOW:
return is_json ? "drop_flow" : "drop-flow";
case EXCEPTION_POLICY_DROP_PACKET:
Expand Down Expand Up @@ -145,8 +147,14 @@ void ExceptionPolicyApply(Packet *p, enum ExceptionPolicy policy, enum PacketDro
case EXCEPTION_POLICY_NOT_SET:
break;
case EXCEPTION_POLICY_REJECT:
SCLogDebug("EXCEPTION_POLICY_REJECT");
PacketDrop(p, ACTION_REJECT, drop_reason);
case EXCEPTION_POLICY_REJECT_BOTH:
if (policy == EXCEPTION_POLICY_REJECT) {
SCLogDebug("EXCEPTION_POLICY_REJECT");
PacketDrop(p, ACTION_REJECT, drop_reason);
} else {
SCLogDebug("EXCEPTION_POLICY_REJECT_BOTH");
PacketDrop(p, ACTION_REJECT_BOTH, drop_reason);
}
if (!EngineModeIsIPS()) {
break;
}
Copy link

Choose a reason for hiding this comment

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

Potentially controversial suggestion below but it is a requirement for our environment so we would still need to patch this in if not accepted.

The fallthrough to drop-flow below results in an edge case that can cause this not to work when packet loss is present in the network. When the midstream reject-both is triggered specifically by a retransmitted packet from the server side, the reset that gets sent to the client will not be accepted by the client host because its sequence number is no longer current. This is the scenario:

  1. Server sends a data packet to client with sequence number A and B bytes of data
  2. Client sends an ACK to server for A+B
  3. That ACK gets lost in the network before reaching Suricata
  4. Suricata flow state gets lost
  5. Server times out and retransmits the last data packet with sequence A and B bytes of data to client
  6. Suricata receives it and does not have matching flow state, so it resets both sides.
  7. The reset sent to the server side is accepted and resets the server end of the connection
  8. The reset sent to the client side is not accepted because it uses sequence number A, which is no longer in its window because the client has already ACK'ed A+B
  9. Client application stuck hanging as it retries, because all of its sent packets now get dropped quietly by drop-flow behavior.

At very large scale this unlikely sequence of events happens enough to cause a major problem for us. Our solution is effectively to disable the drop-flow behavior by making line 164 below look like this:

            if (policy == EXCEPTION_POLICY_DROP_FLOW && p->flow) {

This avoids setting the FLOW_ACTION_DROP on these flows, which causes the first client retry (step 9 above) to also trigger a reset which resets the client so it no longer hangs. I looked at this situation for a while and concluded that it's not possible for Suricata to always use the right sequence number in the presence of packet loss in the network, so the only other option would be for reject-both to send one reset to the src (which is always correct) and two to the dst, one of which would be accepted depending on whether the last ACK happened to be lost.

Note that there is a potential to create something like a DDoS reflection/amplification vector with the solution above, if Suricata responds to every unknown packet by generating two resets. The details of our infrastructure makes that not possible for us, but in general it is when Suricata is directly reachable from untrusted sources. So you would probably want to do something like disable that "reset everything" behavior from the untrusted side of the network (presumably EXTERNAL_NET) or just simply make it configurable via a YAML option.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Suricata flow state gets lost

How does this happen if the flow is presumably in the established state? If the timeout that is configured very low?

Copy link

Choose a reason for hiding this comment

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

For us it's a failover operation where we see this happen - so a host level hardware failure causes existing flows to be redirected to other redundant Suricata hosts, and when the existing connections land on those new hosts the Suricata process running there doesn't know about them. It's actually the failover operation that makes it likely for one of the packets to be lost too - so while the sequence of events described above is possible in general for other reasons too (process crashing at the right time, etc) it goes from "extremely unlikely" to "likely enough to cause an active problem" in our environment.

We do have the ability to migrate connections with their state between hosts in controlled circumstances, but for uncontrolled ones (unplanned host failure) resetting those connections is the next best choice which allows the endpoints to fail fast and establish new connections.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you expect "persistent TCP resets" (https://redmine.openinfosecfoundation.org/issues/960) to help with this? The idea being that we'll keep sending RST packets as long as we keep receiving more packets. There should perhaps be some kind of backoff mechanism to it, but it could help address the race condition?

Expand Down Expand Up @@ -204,6 +212,7 @@ static enum ExceptionPolicy PickPacketAction(const char *option, enum ExceptionP
case EXCEPTION_POLICY_PASS_PACKET:
break;
case EXCEPTION_POLICY_REJECT:
case EXCEPTION_POLICY_REJECT_BOTH:
break;
case EXCEPTION_POLICY_NOT_SET:
break;
Expand All @@ -229,6 +238,8 @@ static enum ExceptionPolicy ExceptionPolicyConfigValueParse(
policy = EXCEPTION_POLICY_PASS_PACKET;
} else if (strcmp(value_str, "reject") == 0) {
policy = EXCEPTION_POLICY_REJECT;
} else if (strcmp(value_str, "reject-both") == 0) {
policy = EXCEPTION_POLICY_REJECT_BOTH;
Copy link

Choose a reason for hiding this comment

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

Github won't let me comment on the unchanged parts of this file, but a few lines below this on line 248, the FatalErrorOnInit message should be updated to mention reject-both as a valid value now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note to also update docs to mention this (including the suricata.yaml bits)

} else if (strcmp(value_str, "ignore") == 0) { // TODO name?
policy = EXCEPTION_POLICY_NOT_SET;
} else if (strcmp(value_str, "auto") == 0) {
Expand Down
Loading