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

Network: add net_tcp_connect event with DNS support #3738

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

rafaeldtinoco
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco commented Dec 6, 2023

commit c3bb180 (HEAD -> network-connection)
Author: Rafael David Tinoco rafaeldtinoco@gmail.com
Date: Wed Dec 6 23:34:58 2023

feature(event): make net_tcp_connect report only TCP

commit 8d476c9
Author: Rafael David Tinoco rafaeldtinoco@gmail.com
Date: Tue Dec 5 23:43:18 2023

feature(event): add net_tcp_connect event with dns support

- create new set called "flows" to represent network flows.

commit cb3525f
Author: Rafael David Tinoco rafaeldtinoco@gmail.com
Date: Tue Dec 5 23:39:30 2023

chore(ebpf): improve readability for security_socket_connect

Opportunistic code change with no logic changes.

commit 8f40dc5
Author: Rafael David Tinoco rafaeldtinoco@gmail.com
Date: Tue Dec 5 16:09:29 2023

fix(network): important fix to how header pointers are calculated

The network L3 header pointer should always be calculated as:

- header pointer + network header offset

no matter if during ingress or egress.

Considering sk_buff overall structure, it holds pointers to:

                                 ---------------
                                | sk_buff       |
                                 ---------------
    ,---------------------------  + head
   /          ,-----------------  + data
  /          /      ,-----------  + tail
 |          |      |            , + end
 |          |      |           |
 v          v      v           v
  -----------------------------------------------
 | headroom | data |  tailroom | skb_shared_info |
  -----------------------------------------------
                                + [page frag]
                                + [page frag]
                                + [page frag]
                                + [page frag]       ---------
                                + frag_list    --> | sk_buff |
                                                    ---------

Ingress: Headroom is often unused, allowing for header prepending without
reallocating. The data segment holds all packet data and headers. Tailroom,
typically unused, provides space for possible data addition.

Egress: Headroom is utilized for adding necessary headers as packets move up
the network stack. The data starts with payload or higher-layer data, with
headers added during processing. Tailroom, less used, offers space for
potential data appending at the end.

pkg/ebpf/c/vmlinux.h Outdated Show resolved Hide resolved
return deriveSingleEvent(events.NetTCPConnect, deriveNetTCPConnectArgs(cache))
}

func deriveNetTCPConnectArgs(cache *dnscache.DNSCache) deriveArgsFunction {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I'm going to add "flow oriented events" (NetTCPFlowStart NetTCPFlowEnd or something like that) for the "new events", I think this one should be TCP only. For that to happen, I would have to pass if the socket is SOCK_STREAM or SOCK_DGRAM, WDYT ?

Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz Dec 6, 2023

Choose a reason for hiding this comment

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

Because right now this would catch both TCP+UDP. Yeah I guess it would be necessary if we use the old security hooks.

@rafaeldtinoco
Copy link
Contributor Author

Be aware that this event sometimes do not report "port", due to:

#3730

I'll provide events based on "net_packet" events now (won't face that issue).

@itaysk
Copy link
Collaborator

itaysk commented Dec 6, 2023

to clarify, is the next event a succession of this one? and is it going to be delivered in this milestone?

  1. if both events can coexist, how would you call the other event?
  2. if both events are logically the same, can they be the same event? (with a configuration that controls the underlying implementation)

Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

On it's own LGTM with regards to logic, but note the suggested changes.
And FWIW, I didn't find the 2nd commit rewrite more readable, but your call.

And I will join Itay's question on how this will work out along the packet based event. Or will we drop it in favor of only this one?

pkg/events/derive/net_tcp.go Outdated Show resolved Hide resolved
pkg/events/core.go Show resolved Hide resolved
return deriveSingleEvent(events.NetTCPConnect, deriveNetTCPConnectArgs(cache))
}

func deriveNetTCPConnectArgs(cache *dnscache.DNSCache) deriveArgsFunction {
Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz Dec 6, 2023

Choose a reason for hiding this comment

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

Because right now this would catch both TCP+UDP. Yeah I guess it would be necessary if we use the old security hooks.

@rafaeldtinoco
Copy link
Contributor Author

to clarify, is the next event a succession of this one? and is it going to be delivered in this milestone?

this next events will be "flow start" and "flow ended" (showing direction, ingress meaning incoming flow, egress meaning outgoing flow).

I can make a single event and flags that say source and destination IP, source and destination ports, protocol, direction, dns source and dns destination names.

Or I can have multiple events (flow egress start, tcp flow egress start, etc)

i think having a single event with arguments saying what protocol, what direction and what ips and names involved is better (and we can have higher level events based on it if needed).

if both events can coexist, how would you call the other event?

I thought about using the term flow for this event(s) because connection isn't appropriate when we talk about a UDP or ICMP flow, for example.

if both events are logically the same, can they be the same event? (with a configuration that controls the underlying implementation)

i don't think it's the same (based on what I just said).

@rafaeldtinoco
Copy link
Contributor Author

And yes, this milestone (today/tomorrow).

The network L3 header pointer should always be calculated as:

- header pointer + network header offset

no matter if during ingress or egress.

Considering sk_buff overall structure, it holds pointers to:

                                 ---------------
                                | sk_buff       |
                                 ---------------
    ,---------------------------  + head
   /          ,-----------------  + data
  /          /      ,-----------  + tail
 |          |      |            , + end
 |          |      |           |
 v          v      v           v
  -----------------------------------------------
 | headroom | data |  tailroom | skb_shared_info |
  -----------------------------------------------
                                + [page frag]
                                + [page frag]
                                + [page frag]
                                + [page frag]       ---------
                                + frag_list    --> | sk_buff |
                                                    ---------

Ingress: Headroom is often unused, allowing for header prepending without
reallocating. The data segment holds all packet data and headers. Tailroom,
typically unused, provides space for possible data addition.

Egress: Headroom is utilized for adding necessary headers as packets move up
the network stack. The data starts with payload or higher-layer data, with
headers added during processing. Tailroom, less used, offers space for
potential data appending at the end.
Opportunistic code change with no logic changes.
- create new set called "flows" to represent network flows.
@rafaeldtinoco rafaeldtinoco merged commit 9c538dd into aquasecurity:main Dec 7, 2023
2 of 11 checks passed
@rafaeldtinoco rafaeldtinoco deleted the network-connection branch December 7, 2023 03:41
@itaysk itaysk linked an issue Dec 7, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

network connection event
3 participants