Fix SSL_pending() and SSL_has_pending() with DTLS#18868
Fix SSL_pending() and SSL_has_pending() with DTLS#18868mattcaswell wants to merge 3 commits intoopenssl:masterfrom
Conversation
mattcaswell
added
branch: master
|
I wrote the test for this first while working on #18132. It was only after I had the test that I realised there was a bug in |
|
Does it matter that the data might be discarded later if the Finished message is completely lost or inconsistent? |
In that case the handshake is never completed so you will never be able to read this app data. |
|
Ping for second review? |
| if (SSL3_RECORD_get_type(&s->rlayer.rrec[i]) | ||
| != SSL3_RT_APPLICATION_DATA) | ||
| return 0; | ||
| return num; |
There was a problem hiding this comment.
Wait, er, what? Isn't this supposed to signal an error? How does a value != 0 do that?
There was a problem hiding this comment.
No - this should not be an error. The return value represents the amount of application data that is buffered and available to read.
With DTLS there are 2 places application data might be buffered - either in the buffered_app_data queue - or in the rrec array. The rrec array could contain records that aren't app data. If that's the case then we ignore the data there. In that case we should return the amount of data we found in buffered_app_data.
Previously it was ok to just return 0 if we hit an rrec array entry that wasn't app data. All the rrec entries are guaranteed to be of the same type. So, if that type isn't app data, then we have 0 bytes of app data. Now that we're additionally checking buffered_app_data that is no longer the case.
There was a problem hiding this comment.
The manpage simply says "returns the number of buffered and processed application data bytes that are pending and are available for immediate read". There is no defined error code.
If I understand this code correctly, it's not that this is an error case anyway, but presumably that SSL3_RT_APPLICATION_DATA records can't appear in rrec after records of other types, thus the loop can be terminated in this case. (?)
There was a problem hiding this comment.
SSL3_RT_APPLICATION_DATA records can't appear in rrec after records of other types, thus the loop can be terminated in this case.
Yes - exactly. Its not an error case
hlandau
left a comment
hlandau
left a comment
There was a problem hiding this comment.
Just one typo, otherwise looks good.
| if (SSL3_RECORD_get_type(&s->rlayer.rrec[i]) | ||
| != SSL3_RT_APPLICATION_DATA) | ||
| return 0; | ||
| return num; |
There was a problem hiding this comment.
The manpage simply says "returns the number of buffered and processed application data bytes that are pending and are available for immediate read". There is no defined error code.
If I understand this code correctly, it's not that this is an error case anyway, but presumably that SSL3_RT_APPLICATION_DATA records can't appear in rrec after records of other types, thus the loop can be terminated in this case. (?)
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
force-pushed
the
fix-dtls-pending
branch
from
2c81b5d to
caa50c9
Compare
|
Typo fixed and rebased due to the SSL Object refactor |
hlandau
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
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: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18868)
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: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18868)
|
Merged to master. Thank you. This does not cherry-pick cleanly to 3.0 or 1.1.1, will need another PR. |
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: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl/openssl#18868)
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: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl/openssl#18868)
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: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18868)
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: Tomas Mraz <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18868)
5 participants
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 youhow 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()orSSL_has_pending()were taking account of thisDTLS specific app data buffer.