Skip to content

Comments

Fix SSL_pending() and SSL_has_pending() with DTLS (1.1.1)#18976

Closed
mattcaswell wants to merge 2 commits intoopenssl:OpenSSL_1_1_1-stablefrom
mattcaswell:fix-dtls-pending-111
Closed

Fix SSL_pending() and SSL_has_pending() with DTLS (1.1.1)#18976
mattcaswell wants to merge 2 commits intoopenssl:OpenSSL_1_1_1-stablefrom
mattcaswell:fix-dtls-pending-111

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Aug 10, 2022

This is a backport of #18868 to the 1.1.1 branch.

If app data is received before a Finished message in DTLS then we buffer
it to return later. The function SSL_pending() is supposed to tell you
how much processed app data we have already buffered, and SSL_has_pending()
is supposed to tell you if we have any data buffered (whether processed or
not, and whether app data or not).

Neither SSL_pending() or SSL_has_pending() were taking account of this
DTLS specific app data buffer.

@mattcaswell
Fix SSL_pending() and SSL_has_pending() with DTLS
0c0f998 
If app data is received before a Finished message in DTLS then we buffer
it to return later. The function SSL_pending() is supposed to tell you
how much processed app data we have already buffered, and SSL_has_pending()
is supposed to tell you if we have any data buffered (whether processed or
not, and whether app data or not).

Neither SSL_pending() or SSL_has_pending() were taking account of this
DTLS specific app data buffer.
@mattcaswell
Test that swapping the first app data record with Finished msg works
94ed370 
If the first app data record arrives before the Finished message we should
be able to buffer it and move on to the Finished message.
@mattcaswell mattcaswell added approval: review pending This pull request needs review by a committer branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) approval: otc review pending labels Aug 10, 2022
@mattcaswell mattcaswell mentioned this pull request Aug 10, 2022
Closed
@hlandau hlandau removed the approval: review pending This pull request needs review by a committer label Aug 11, 2022
@t8m t8m added approval: done This pull request has the required number of approvals triaged: bug The issue/pr is/fixes a bug and removed approval: otc review pending labels Aug 16, 2022
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Aug 17, 2022
@openssl-machine
Copy link
Collaborator

openssl-machine commented Aug 17, 2022

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Aug 17, 2022
@hlandau
Copy link
Member

hlandau commented Aug 17, 2022

Merged to 1.1.1. Thank you.

@hlandau hlandau closed this Aug 17, 2022
openssl-machine pushed a commit that referenced this pull request Aug 17, 2022
@mattcaswell
Fix SSL_pending() and SSL_has_pending() with DTLS
01fc812 
If app data is received before a Finished message in DTLS then we buffer
it to return later. The function SSL_pending() is supposed to tell you
how much processed app data we have already buffered, and SSL_has_pending()
is supposed to tell you if we have any data buffered (whether processed or
not, and whether app data or not).

Neither SSL_pending() or SSL_has_pending() were taking account of this
DTLS specific app data buffer.

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #18976)
openssl-machine pushed a commit that referenced this pull request Aug 17, 2022
@mattcaswell
Test that swapping the first app data record with Finished msg works
d87e99d 
If the first app data record arrives before the Finished message we should
be able to buffer it and move on to the Finished message.

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #18976)
@bernd-edlinger
Copy link
Member

bernd-edlinger commented Sep 9, 2022

From the commit message it seems that this change is supposed to only have an effect on DTLS,
but this loop is might have a different return value for TLS with RECORD_LAYER_get_numrpipes > 1

     for (i = 0; i < RECORD_LAYER_get_numrpipes(&s->rlayer); i++) {
         if (SSL3_RECORD_get_type(&s->rlayer.rrec[i])
             != SSL3_RT_APPLICATION_DATA)
-            return 0;
+            return num;
         num += SSL3_RECORD_get_length(&s->rlayer.rrec[i]);
     }

I mean if rrec[0] is APPLICATION_DATA and rrec[1] is not,
then num will be returned while previously 0 was returned.
Is that somehow impossible, or was this intentional?

@mattcaswell
Copy link
Member Author

mattcaswell commented Sep 12, 2022

Pipelined data must always be of the same type, so if the first record is APPLICATION_DATA then the second must be too.

@bernd-edlinger
Copy link
Member

bernd-edlinger commented Sep 12, 2022

Ah, okay, then something like an ossl_assert(i == 0); would make the code more clear?

@mattcaswell
Copy link
Member Author

mattcaswell commented Sep 12, 2022

Yes

bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Sep 12, 2022
@mattcaswell @bernd-edlinger
Fix SSL_pending() and SSL_has_pending() with DTLS
18c31bf 
If app data is received before a Finished message in DTLS then we buffer
it to return later. The function SSL_pending() is supposed to tell you
how much processed app data we have already buffered, and SSL_has_pending()
is supposed to tell you if we have any data buffered (whether processed or
not, and whether app data or not).

Neither SSL_pending() or SSL_has_pending() were taking account of this
DTLS specific app data buffer.

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18976)

(cherry picked from commit 01fc812)
bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Sep 12, 2022
@mattcaswell @bernd-edlinger
Test that swapping the first app data record with Finished msg works
38bef54 
If the first app data record arrives before the Finished message we should
be able to buffer it and move on to the Finished message.

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18976)

(cherry picked from commit d87e99d)
a-kromm-rogii pushed a commit to a-kromm-rogii/openssl that referenced this pull request Mar 14, 2025
@mattcaswell @a-kromm-rogii
Fix SSL_pending() and SSL_has_pending() with DTLS
cda7c52 
If app data is received before a Finished message in DTLS then we buffer
it to return later. The function SSL_pending() is supposed to tell you
how much processed app data we have already buffered, and SSL_has_pending()
is supposed to tell you if we have any data buffered (whether processed or
not, and whether app data or not).

Neither SSL_pending() or SSL_has_pending() were taking account of this
DTLS specific app data buffer.

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18976)
a-kromm-rogii pushed a commit to a-kromm-rogii/openssl that referenced this pull request Mar 14, 2025
@mattcaswell @a-kromm-rogii
Test that swapping the first app data record with Finished msg works
3954244 
If the first app data record arrives before the Finished message we should
be able to buffer it and move on to the Finished message.

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18976)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t8m t8m t8m approved these changes

+1 more reviewer

@hlandau hlandau hlandau approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mattcaswell @openssl-machine @hlandau @bernd-edlinger @t8m