Skip to content

Comments

dtls: fix DTLSv1_listen msg_callback to report HelloVerifyRequest#28916

Closed
MegaManSec wants to merge 1 commit intoopenssl:masterfrom
MegaManSec:no_pr_6
Closed

dtls: fix DTLSv1_listen msg_callback to report HelloVerifyRequest#28916
MegaManSec wants to merge 1 commit intoopenssl:masterfrom
MegaManSec:no_pr_6

Conversation

@MegaManSec
Copy link
Contributor

DTLSv1_listen built the HelloVerifyRequest in wbuf but invoked msg_callback with buf and DTLS1_RT_HEADER_LENGTH, and version 0. That caused incorrect logging and could disclose the ClientHello to write callbacks. Use wbuf and the actual record version for the record header, and add a second callback that reports the handshake message bytes. No change to on-wire behavior.

@Sashan
Copy link
Contributor

Sashan commented Oct 16, 2025

ping @mattcaswell DTLSv1 is not my department, will appreciate if you take a look. thanks.

fwh-dc
fwh-dc previously approved these changes Oct 16, 2025
Copy link
Contributor

@fwh-dc fwh-dc left a comment

Choose a reason for hiding this comment

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

LGTM

@fwh-dc fwh-dc added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Oct 16, 2025
mattcaswell
mattcaswell previously approved these changes Oct 16, 2025
@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 16, 2025
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 17, 2025
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m t8m added triaged: feature The issue/pr requests/adds a feature tests: exempted The PR is exempt from requirements for testing approval: review pending This pull request needs review by a committer and removed approval: ready to merge The 24 hour grace period has passed, ready to merge labels Oct 17, 2025
@t8m
Copy link
Member

t8m commented Oct 17, 2025

As there is now a conflict, can you please rebase this?

DTLSv1_listen built the HelloVerifyRequest in wbuf but invoked
msg_callback with buf and DTLS1_RT_HEADER_LENGTH, and version 0.
That caused incorrect logging and could disclose the ClientHello
to write callbacks. Use wbuf and the actual record version for the
record header, and add a second callback that reports the handshake
message bytes. No change to on-wire behavior.

Signed-off-by: Joshua Rogers <[email protected]>
Copy link
Contributor

@fwh-dc fwh-dc left a comment

Choose a reason for hiding this comment

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

Failed CI job is irrelevant.

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@fwh-dc
Copy link
Contributor

fwh-dc commented Dec 6, 2025

ping @mattcaswell for re-approval

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 8, 2025
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 9, 2025
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Dec 11, 2025

Merged to the master branch after applying clang-format. Thank you for your contribution.

@t8m t8m closed this Dec 11, 2025
openssl-machine pushed a commit that referenced this pull request Dec 11, 2025
DTLSv1_listen built the HelloVerifyRequest in wbuf but invoked
msg_callback with buf and DTLS1_RT_HEADER_LENGTH, and version 0.
That caused incorrect logging and could disclose the ClientHello
to write callbacks. Use wbuf and the actual record version for the
record header, and add a second callback that reports the handshake
message bytes. No change to on-wire behavior.

Signed-off-by: Joshua Rogers <[email protected]>

Reviewed-by: Frederik Wedel-Heinen <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #28916)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
DTLSv1_listen built the HelloVerifyRequest in wbuf but invoked
msg_callback with buf and DTLS1_RT_HEADER_LENGTH, and version 0.
That caused incorrect logging and could disclose the ClientHello
to write callbacks. Use wbuf and the actual record version for the
record header, and add a second callback that reports the handshake
message bytes. No change to on-wire behavior.

Signed-off-by: Joshua Rogers <[email protected]>

Reviewed-by: Frederik Wedel-Heinen <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
(Merged from openssl#28916)
@bernd-edlinger
Copy link
Member

This should also be back-ported like #28816

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch tests: exempted The PR is exempt from requirements for testing triaged: feature The issue/pr requests/adds a feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants