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

DTLS 1.2 Application Data prior to CCS/Finish #20597

Closed
bfussell opened this issue Mar 24, 2023 · 5 comments
Closed

DTLS 1.2 Application Data prior to CCS/Finish #20597

bfussell opened this issue Mar 24, 2023 · 5 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 triaged: bug The issue/pr is/fixes a bug

Comments

@bfussell
Copy link

bfussell commented Mar 24, 2023

In 1.1.1t we have a case where DTLS1.2 application data is received by the client prior to the CCS/Finish and it is dropped rather than queued. OpenSSL has queuing capability so the question is whether it should be queued and made known to the client via the SSL_pending/SSL_has_pending APIs. The reason for the drop is the epoch is 1 and thus fails on the call to dtls1_get_bitmap() in dtls1_get_record() because the code is expecting epoch to still be 0.

A note from some of my investigation. In reviewing the UT created due to a similar case, PR #18868, there is a test which attempts to swap packets to force the app data between CCS and Finish. However, at some point prior to client processing they are reordered and the app data is processed after Finish thus not really testing the flow I would expect. It does queue the app data since the handshake is still considered active until the whole packet has been processed. I've modified that test so that the app data is prior to both CCS and Finish and now it doesn't get reordered and the test fails since the app data gets dropped for the exact reason I described above.

Thanks,

Barry

@bfussell bfussell added the issue: question The issue was opened to ask a question label Mar 24, 2023
@mattcaswell mattcaswell added triaged: bug The issue/pr is/fixes a bug and removed issue: question The issue was opened to ask a question labels Mar 27, 2023
@mattcaswell
Copy link
Member

Sounds like a potential bug. Can you share the changes that you made to the test case?

@bfussell
Copy link
Author

Seems the app data packet gets dropped because dtls1_get_bitmap() does not allow out of order packets except for HM and ALERTS. UT changes attached. Let me know if you need additional information.

dtls_swap.patch

@t8m t8m added branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Mar 27, 2023
@mattcaswell
Copy link
Member

Confirmed as a bug. Working on a fix.

@bfussell
Copy link
Author

Great, thanks ! One note of difference between our customer case and the UT flow. The UT flow has the app data, CCS and Finish all in the same packet whereas the the customer is seeing app data in first packet and CCS/Finish in the next packet. Mentioning that in case separate packets may impact app data moving from unprocessed_rcds queue to buffered_app_data queue.

mattcaswell added a commit to mattcaswell/openssl that referenced this issue Mar 28, 2023
It is possible that DTLS records are received out of order such that
records from the next epoch arrive before we have finished processing the
current epoch. We are supposed to buffer such records but for some reason
we only did that for handshake and alert records. This is incorrect since
it is perfectly possible for app data records to arrive early too.

Fixes openssl#20597
@mattcaswell
Copy link
Member

I have a fix for the master branch in #20628. Once that is reviewed I will backport it to 3.1/3.0. Unfortunately this won't be backported to 1.1.1 since it is in security fix only mode.

mattcaswell added a commit to mattcaswell/openssl that referenced this issue Mar 29, 2023
It is possible that DTLS records are received out of order such that
records from the next epoch arrive before we have finished processing the
current epoch. We are supposed to buffer such records but for some reason
we only did that for handshake and alert records. This is incorrect since
it is perfectly possible for app data records to arrive early too.

Fixes openssl#20597
mattcaswell added a commit to mattcaswell/openssl that referenced this issue Mar 29, 2023
It is possible that DTLS records are received out of order such that
records from the next epoch arrive before we have finished processing the
current epoch. We are supposed to buffer such records but for some reason
we only did that for handshake and alert records. This is incorrect since
it is perfectly possible for app data records to arrive early too.

Fixes openssl#20597
openssl-machine pushed a commit that referenced this issue Mar 31, 2023
It is possible that DTLS records are received out of order such that
records from the next epoch arrive before we have finished processing the
current epoch. We are supposed to buffer such records but for some reason
we only did that for handshake and alert records. This is incorrect since
it is perfectly possible for app data records to arrive early too.

Fixes #20597

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20637)
openssl-machine pushed a commit that referenced this issue Mar 31, 2023
It is possible that DTLS records are received out of order such that
records from the next epoch arrive before we have finished processing the
current epoch. We are supposed to buffer such records but for some reason
we only did that for handshake and alert records. This is incorrect since
it is perfectly possible for app data records to arrive early too.

Fixes #20597

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #20638)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants