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

Create a new chapter for WebRTC. #1995

Merged
merged 21 commits into from
Aug 27, 2024
Merged

Create a new chapter for WebRTC. #1995

merged 21 commits into from
Aug 27, 2024

Conversation

tghosth
Copy link
Collaborator

@tghosth tghosth commented Jul 24, 2024

This Pull Request relates to issue #1612

tghosth and others added 11 commits July 24, 2024 11:01
* WebRTC: first commit based on content in issue

#1612 (comment)

* add control objective title

* update references

add relevant RFCs
fix title

* add link to clienthello DoS blog post

* update levels according to criteria in the details below

- signalling 53.3.2 set to l2 based on 5.4.3
- media 53.2.7 set to l2 based on 5.4.3
- targeted application DoS issues (i.e., 53.1.2, 53.2.3, 53.2.5, 53.2.6 and 53.3.1) set to l2 since they are usually targeted attacks
- 53.2.2 level based on 9.4.1

* fix markdown according to markdownlint-cli
We're referring to the ASVS WebRTC chapter now instead of being vague and referring to "adhering to best practices".
@tghosth tghosth marked this pull request as ready for review August 15, 2024 06:37
@tghosth tghosth requested a review from elarlang August 15, 2024 06:37
@tghosth
Copy link
Collaborator Author

tghosth commented Aug 15, 2024

I would request a final view from @elarlang's discerning eye before we merge this into main :)

Copy link
Collaborator

@elarlang elarlang left a comment

Choose a reason for hiding this comment

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

Acronyms in requirements - avoid or use in style: Web Real-Time Communication (WebRTC)

53.1.1, 53.1.2 - TURN acronym

53.1.2 - is it WebRTC specific? Seems a general requirement and potentially covered by 12.7.1

# Description L1 L2 L3 CWE
12.7.1 [ADDED] Verify that the application proactively releases system resources, such as database connections, open files, threads, etc, when it finishes using them to prevent resource exhaustion. 404

53.2.1 - seems out of scope, and if not, then it is a general requirement for V9

53.2.2 - "Verify that"? It seems general V9 requirement (not WebRTC specific)

53.2.3 - publically known vulnerable sounds like a duplicate of 14.2.1

# Description L1 L2 L3 CWE
14.2.1 Verify that all components are up to date, preferably using a dependency checker during build or compile time. 1026

race condition test is covered by 11.1.6. Is there something WebRTC-specific?

# Description L1 L2 L3 CWE
11.1.6 [MODIFIED] Verify that the application uses synchronization and locking mechanisms for sensitive operations in order to keep internal data consistent, maintain user state, and prevent race conditions, such as 'time of check to time of use (TOCTOU)' vulnerabilities. 367

53.2.4, 53.2.5 - SRTP, RTP acronyms

53.2.5, 53.2.6 - slash (/) instead of "and" or "or"

53.2.6, 53.2.7, 53.3.1, 53.3.2 - "can handle" is a bit vague. What it means?

I only checked requirement texts (not checked chapter texts).

@sandrogauci
Copy link
Contributor

sandrogauci commented Aug 16, 2024

Thanks @elarlang for the checks and feedback!

Acronyms in requirements - avoid or use in style: Web Real-Time Communication (WebRTC)

WebRTC is full of acronyms so we cannot really avoid them much.

I have a general question about this: would it be preferred that the acronyms be defined when mentioned first time, or in a section just for acronyms or do we instead define them each time mentioned in a requirement in the indicated format?

Also - is this information about when and how to define acronyms publicly documented?

53.1.1, 53.1.2 - TURN acronym

53.1.2 - is it WebRTC specific? Seems a general requirement and potentially covered by 12.7.1

This is TURN specific, which is normally a critical component of WebRTC infrastructure. While 12.7.1 is about releasing resources and is very generic, our requirement is about being able to handle the situation where the TURN server is asked for relay/proxy a larger number of IP:ports than it can handle. Releasing resources is not what's recommended in this case. TURN servers, in some ways, are similar to web proxies in that they are meant to relay connections or UDP packets.

Description L1 L2 L3 CWE

12.7.1 [ADDED] Verify that the application proactively releases system resources, such as database connections, open files, threads, etc, when it finishes using them to prevent resource exhaustion. ✓ ✓ 404
53.2.1 - seems out of scope, and if not, then it is a general requirement for V9

I do not think that this is a general requirement because we are specifically writing about DTLS. Please take a look at my replies to @tghosth previously here: #1612 (comment). In addition, please see RFC 5764.

53.2.2 - "Verify that"? It seems general V9 requirement (not WebRTC specific)

We can update the text to state "Verify that". Also see my previous reply to @tghosth.

53.2.3 - publically known vulnerable sounds like a duplicate of 14.2.1

Description L1 L2 L3 CWE

14.2.1 Verify that all components are up to date, preferably using a dependency checker during build or compile time. ✓ ✓ ✓ 1026
race condition test is covered by 11.1.6. Is there something WebRTC-specific?

Description L1 L2 L3 CWE

11.1.6 [MODIFIED] Verify that the application uses synchronization and locking mechanisms for sensitive operations in order to keep internal data consistent, maintain user state, and prevent race conditions, such as 'time of check to time of use (TOCTOU)' vulnerabilities. ✓ ✓ 367

53.2.3 is not about a vulnerability that is necessarily product specific, which is what 14.2.1 is about. We describe the vulnerability itself, which is likely to be found in custom media servers or media server products that have not issued an advisory about it. Thus dependency checking or similar approaches are not as effective for this particular issue.

With regards to 53.2.3, yes this is very specific to WebRTC. If one were to look at 11.1.6, my understanding is that they would not be specifically looking for this vulnerability (which we document here).

53.2.4, 53.2.5 - SRTP, RTP acronyms

I'll update all acronyms.

53.2.5, 53.2.6 - slash (/) instead of "and" or "or"

Makes sense.

53.2.6, 53.2.7, 53.3.1, 53.3.2 - "can handle" is a bit vague. What it means?

I'll define "can handle" in each case.

I only checked requirement texts (not checked chapter texts).

Thanks for the work!

@elarlang
Copy link
Collaborator

elarlang commented Aug 17, 2024

By principle, each requirement must be understandable as an independent line "out of context" - without including any other requirement, chapter texts, or glossary.

If it is not a well-known acronym, we need to write it. The "Well known" we can decide - if it is written in https://ahdictionary.com/word/search.html, it is well known. We don't have it formally documented (yet), but it is discussed here #1875

With "and/or" I expressed myself incorrectly - my goal is to avoid "/" instead of "and" or "or". So in this case I think instead of "video/audio" we can use "video or audio".

All "should" lines - are those recommendations (nice to have) or requirements (must have)?

Thank you for this contribution, @sandrogauci !

@sandrogauci
Copy link
Contributor

Thanks for the explanation. Opened a new PR: #2023 to define the acronyms and change the slash into an "or".

All "should" lines - are those recommendations (nice to have) or requirements (must have)?

I suppose it depends on your threat model. From where we stand, they are requirements since those are usually the effective solutions to the security issues that we're trying to prevent.

@tghosth
Copy link
Collaborator Author

tghosth commented Aug 25, 2024

Hi @elarlang, can you confirm if the updates resolve all the points you raised?

@sandrogauci, thanks for your patience with this process, it seems like we are nearly there :)

@sandrogauci
Copy link
Contributor

Hi @elarlang, can you confirm if the updates resolve all the points you raised?

@sandrogauci, thanks for your patience with this process, it seems like we are nearly there :)

No worries, it has been a learning process for me too! Thanks

sandrogauci and others added 2 commits August 27, 2024 13:11
* define acronyms where possible

* slash replaced with "or"

* Refactor certain requirements (#1)

* Refactor certain requirements

* Split guidance for malformed messages and packets

---------

Co-authored-by: Josh Grossman <tghosth@users.noreply.github.com>
@tghosth
Copy link
Collaborator Author

tghosth commented Aug 27, 2024

Ok @elarlang I think you need to approve the changes now and then we can get this merged in :)

@elarlang elarlang self-requested a review August 27, 2024 10:23
@tghosth tghosth merged commit c8b80e3 into master Aug 27, 2024
6 checks passed
@tghosth tghosth deleted the tghosth-patch-1 branch August 27, 2024 11:20
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