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

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

Conversation

mattcaswell
Copy link
Member

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.

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.
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 Merge to OpenSSL_1_1_1-stable branch approval: otc review pending This pull request needs review by an OTC member labels Aug 10, 2022
@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 This pull request needs review by an OTC member 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

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
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 <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18976)
openssl-machine pushed a commit that referenced this pull request Aug 17, 2022
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 <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18976)
@bernd-edlinger
Copy link
Member

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

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

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

@mattcaswell
Copy link
Member Author

Yes

bernd-edlinger pushed a commit to bernd-edlinger/openssl that referenced this pull request Sep 12, 2022
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 <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(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
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 <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#18976)

(cherry picked from commit d87e99d)
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: 1.1.1 Merge to OpenSSL_1_1_1-stable branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants