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

RTP: treat it as an encrypted protocol. #1955

Closed
wants to merge 1 commit into from

Conversation

IvanNardi
Copy link
Collaborator

The goal is to have "RTP.ZOOM" flows classified as "Encrypted" and not as "Cleartext".

To be fair, RTP might be in cleartext (meaning: you can play the media stream without any other info than the flow itself) but nowadays RTP is almost always encrypted (i.e SRTP), especially on the public internet.

The other solution would be to add a special case for RTP in ndpi_is_encrypted_proto() (plain RTP -> cleartext, RTP as master -> encrypted, or depending on the app), but it seemed overwhelming.

The goal is to have "RTP.ZOOM" flows classified as "Encrypted" and
not as "Cleartext".

To be fair, RTP might be in cleartext (meaning: you can play
the media stream without any other info than the flow itself) but nowadays
RTP is almost always encrypted (i.e SRTP), especially on the public
internet.

The other solution would be to add a special case for RTP in
`ndpi_is_encrypted_proto()` (plain RTP -> cleartext, RTP as master
-> encrypted, or depending on the app), but it seemed overwhelming.
@sonarcloud
Copy link

sonarcloud bot commented Apr 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@lucaderi
Copy link
Member

The idea is not bad per-se, just wondering if it add another problem for non-encrypted RTP. To make things clear IMHO we should check inside RTP the media type and if it's proprietary as in Zoom and Teams, mark it as encrypted. What do you think?

@IvanNardi
Copy link
Collaborator Author

Something like that?

u_int8_t ndpi_is_encrypted_proto(struct ndpi_detection_module_struct *ndpi_str,
				                             ndpi_protocol proto) {
[...]
  if(ndpi_is_valid_protoId(proto.master_protocol) && ndpi_is_valid_protoId(proto.app_protocol)) {
    /* RTP itself might be in cleartext, but if it is a master protocol, the flow is usually encrypted (examples: RTP.Zoom or RTP.Teams */
    if(proto.master_protocol == NDPI_PROTOL_RTP) {
      return !ndpi_str->proto_defaults[proto.app_protocol].isClearTextProto;
    }
  }
[...]
}

[We can't tell if RTP is encrypted or not only looking at the RTP Payload Type field]

Another possibility: introduce NDPI_PROTOCOL_SRTP (encrypted, obviously) and classify those Zoom flows as SRTP.Zoom

@IvanNardi
Copy link
Collaborator Author

Thinking a lit bit more, the SRTP solution seems the cleaner one: RTP -> cleartext; SRTP -> encrypted

@IvanNardi IvanNardi marked this pull request as draft May 5, 2023 11:45
@IvanNardi
Copy link
Collaborator Author

Superseded by #1977

@IvanNardi IvanNardi closed this May 15, 2023
@IvanNardi IvanNardi deleted the rtp-encrypted branch May 15, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants