Skip to content

DTLS: add api to enforce records do not span datagrams #8642

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

rizlik
Copy link
Contributor

@rizlik rizlik commented Apr 7, 2025

Description

DTLS records should not span UDP datagrams, this PR adds an API to enforce it.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • doc/dox_comments/header_files/ssl.h: Language not supported

WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
struct test_memio_ctx test_ctx;
char * readBuf[50];
Copy link
Preview

Copilot AI Apr 7, 2025

Choose a reason for hiding this comment

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

The declaration of 'readBuf' as an array of char pointers is likely incorrect for use with wolfSSL_read, which expects a contiguous byte buffer. Consider changing it to 'unsigned char readBuf[50];' or 'char readBuf[50];' if signedness is not an issue.

Suggested change
char * readBuf[50];
unsigned char readBuf[50];

Copilot uses AI. Check for mistakes.

WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
struct test_memio_ctx test_ctx;
char * readBuf[50];
Copy link
Preview

Copilot AI Apr 7, 2025

Choose a reason for hiding this comment

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

The declaration of 'readBuf' as an array of char pointers is likely unintended here, as wolfSSL_read is expected to write raw bytes into a contiguous buffer. Consider using 'unsigned char readBuf[50];' or 'char readBuf[50];' to avoid potential memory issues.

Suggested change
char * readBuf[50];
unsigned char readBuf[50];

Copilot uses AI. Check for mistakes.

@rizlik rizlik force-pushed the dtls_no_span_records branch 4 times, most recently from c818750 to b8c12fb Compare April 7, 2025 13:57
@rizlik
Copy link
Contributor Author

rizlik commented Apr 8, 2025

retest this

- 0: Records cannot span datagrams.
- 1: Records can span datagrams (default behavior).
*/
int wolfSSL_dtls_set_records_can_span_datagrams(WOLFSSL* ssl, int value);
Copy link
Member

Choose a reason for hiding this comment

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

Since the RFC explicitly prohibits this, I would make this a compile time const. Its not something that users are going to be changing dynamically.

Each DTLS record MUST fit within a single datagram.

https://www.rfc-editor.org/rfc/rfc9147.html#section-4.3

@julek-wolfssl julek-wolfssl removed their assignment Apr 9, 2025
@gojimmypi
Copy link
Contributor

Jenkins retest this please

for long-running job

@rizlik rizlik force-pushed the dtls_no_span_records branch 3 times, most recently from 7147388 to 72a3693 Compare April 22, 2025 15:55
@rizlik
Copy link
Contributor Author

rizlik commented Apr 22, 2025

retest this please

@rizlik rizlik force-pushed the dtls_no_span_records branch 4 times, most recently from c9b5fb0 to a81e244 Compare April 29, 2025 09:58
@rizlik
Copy link
Contributor Author

rizlik commented Apr 29, 2025

retest this please

rizlik added 7 commits May 7, 2025 12:16
changes:
- alert is sent if SanityCheckCipherText fails, with or without
  `WOLFSSL_EXTRA_ALERTS` defined
- HandleDTLSDecryptFailed is invoked if `SanityCheckCipherText` fails
test_memio read and write callbacks now respect message boundaries, ensuring
data is read and written in discrete chunks as sent. Updated tests to
reflect this behavior.

Changelog:
- Added utility functions for managing message buffers in test_memio:
  `test_memio_clear_buffer`, `test_memio_inject_message`,
  `test_memio_drop_message`, and `test_memio_modify_message_len`.
- Updated tests to use these utilities for improved clarity and
  maintainability.
@rizlik rizlik force-pushed the dtls_no_span_records branch from a81e244 to 2095c71 Compare May 7, 2025 10:27
@rizlik rizlik force-pushed the dtls_no_span_records branch from 2095c71 to 1bb45f5 Compare May 7, 2025 10:34
@kaleb-himes
Copy link
Contributor

retest this please

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.

4 participants