-
-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Add support to zeroize plaintext in S3 record layer #12251
Changes from 2 commits
bc2ffab
2440942
d24358e
1a00435
74d2ef9
baf3dd1
97bb6e2
f7abb8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -265,6 +265,20 @@ functionality is not required. Those applications can turn this feature off by | |
setting this option. This is a server-side opton only. It is ignored by | ||
clients. | ||
|
||
=item SSL_OP_CLEANSE_PLAINTEXT | ||
|
||
By default TLS connections keep a copy of received plaintext data in a | ||
static buffer until it is overwritten by the next portion of | ||
data. When enabling SSL_OP_CLEANSE_PLAINTEXT this plaintext is | ||
cleansed by calling OPENSSL_cleanse(3) after passing data to the | ||
application. Data is also cleansed when releasing the connection | ||
(eg. L<SSL_free(3)>). | ||
|
||
Since OpenSSL only cleanses internal buffers, the application is | ||
still responsible for cleansing all other buffers. Most notable this | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: notably (not sure if it should have a , or not) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure either, but https://thecriticalreader.com/because-since-and-commas/ and state the ',' should stay |
||
applies to buffers passed to functions like L<SSL_read(3)>, | ||
SSL_peek(3) but also like L<SSL_write(3)>. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. L<SSL_peek(3)> |
||
|
||
=back | ||
|
||
The following options no longer have any effect but their identifiers are | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,14 +120,18 @@ void DTLS_RECORD_LAYER_set_write_sequence(RECORD_LAYER *rl, unsigned char *seq) | |
static int dtls1_copy_record(SSL *s, pitem *item) | ||
{ | ||
DTLS1_RECORD_DATA *rdata; | ||
SSL3_BUFFER *rbuf; | ||
|
||
rdata = (DTLS1_RECORD_DATA *)item->data; | ||
rbuf = &s->rlayer.rbuf; | ||
|
||
SSL3_BUFFER_release(&s->rlayer.rbuf); | ||
if (s->options & SSL_OP_CLEANSE_PLAINTEXT) | ||
OPENSSL_cleanse(rbuf->buf, rbuf->len); | ||
SSL3_BUFFER_release(rbuf); | ||
|
||
s->rlayer.packet = rdata->packet; | ||
s->rlayer.packet_length = rdata->packet_length; | ||
memcpy(&s->rlayer.rbuf, &(rdata->rbuf), sizeof(SSL3_BUFFER)); | ||
memcpy(rbuf, &(rdata->rbuf), sizeof(SSL3_BUFFER)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we're copying plaintext out of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will recheck and update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am trying to get around where DTLS maintains copies of records.
Is it correct that each of these three queues contain DTLS1_RECORD_DATA items where each of those convey it own 16K buffer for a single record? To my understanding dtls1_copy_record takes an item from one of those queues and replaces s->rlayer.rbuf, with the queued item. The old record is freed (and cleansed) in this process? Note however, as i understand it dtls1_copy_record, does not copy the record it self, in only copies metadata (pointers and stuff). Keeping this in mind I think we can revert the cleanse modification in dtls1_copy_record, since it only cleanses application data after it is delivered (and already cleansed) to the application, but we need to add cleansing of leftofer items in buffered_app_data and processed_rcds DTLS_RECORD_LAYER_clear. I am really not totaly sure about my analysis, please let me know if it is correct. I already have prepared a fixup containing the following changes:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is not cleansed. However, I don't think it needs to be. If the old record contains plaintext application data then we'll only copy in a new record if we've already read that data - and if we've read it then (as a result of this PR) we will have already cleansed that application data.
I agree with your analysis |
||
memcpy(&s->rlayer.rrec, &(rdata->rrec), sizeof(SSL3_RECORD)); | ||
|
||
/* Set proper sequence number for mac calculation */ | ||
|
@@ -514,6 +518,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, | |
if (SSL3_RECORD_get_length(rr) == 0) | ||
SSL3_RECORD_set_read(rr); | ||
} else { | ||
if (s->options & SSL_OP_CLEANSE_PLAINTEXT) | ||
OPENSSL_cleanse(&(SSL3_RECORD_get_data(rr)[SSL3_RECORD_get_off(rr)]), n); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Depending on the read sematics for datagram connections i think the n should be replaced by SSL3_RECORD_get_length(rr) Since when for datagram sockets when reading up to n bytes from a datagram with a length larger than n. The remaining bytes are dropped and never delivered to the application. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry n need to be replaced by: However reading the code twice i think OpenSSL does not drop the data, so n seems fine anyways |
||
SSL3_RECORD_sub_length(rr, n); | ||
SSL3_RECORD_add_off(rr, n); | ||
if (SSL3_RECORD_get_length(rr) == 0) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1483,6 +1483,8 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, | |
if (SSL3_RECORD_get_length(rr) == 0) | ||
SSL3_RECORD_set_read(rr); | ||
} else { | ||
if (s->options & SSL_OP_CLEANSE_PLAINTEXT) | ||
OPENSSL_cleanse(&(rr->data[rr->off]), n); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This cleanses record data that we are expecting to receive. Sometimes we received records of other types, e.g. alerts, or new handshake records (see the sections later in this function for alert handling, and handshake fragment/unexpected handshake messages). We're not cleansing the plaintext for those types of records. Although this in itself begs another question - how far do you go? Information that we learn from plaintext handshake records is stored in the SSL object all over the place. Should we cleanse all of those too (if so then the scope suddenly became a lot more complicated)? Alternatively we clarify that the option is only about application data plaintext and restrict ourselves to worrying about that only. (All of this comment applies to the same section of the DTLS code) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think clearly we should stop at application data. The whole point is to extend residual information protection to application data, which is often a requirement when using secure channels. Applications transport all kind of items within application data of the secure channel (e.g.: Passwords, PSKs, PINs, ...). This feature aims to support the kind of requirements stating that resuduals thereof must be removed as well. These requirements are not that uncommon. Further, the current implementation only clenses the application record payload, but not the header. Regarding handshake data: I think in TLS1.2 most of it is sent plaintext anyways. TLS1.3 is different, but to my understanding this is done mainly to protect the from monitoring metainformation (e.g.: certificates used). Regarding handshake data within the secure channel (e.g.: KEY-UPDATE): prove me wrong, but i do not think that sensitive information is contained in those (other than metainformation). We could add an "if" to exclude records other than application data, but I do not think that clansing a little bit too much notably more hurts perfomance when comparing it to application data cleansing. So regarding: ad (1) totally correct -> I will be a never ending story. |
||
SSL3_RECORD_sub_length(rr, n); | ||
SSL3_RECORD_add_off(rr, n); | ||
if (SSL3_RECORD_get_length(rr) == 0) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1582,6 +1582,119 @@ static int test_large_message_tls_read_ahead(void) | |
TLS1_VERSION, 0, 1); | ||
} | ||
|
||
static int execute_cleanse_plaintext(const SSL_METHOD *smeth, | ||
const SSL_METHOD *cmeth, | ||
int min_version, int max_version) | ||
{ | ||
size_t i; | ||
SSL_CTX *cctx = NULL, *sctx = NULL; | ||
SSL *clientssl = NULL, *serverssl = NULL; | ||
int testresult = 0; | ||
SSL3_RECORD *rr; | ||
void *zbuf; | ||
|
||
static unsigned char cbuf[16000]; | ||
static unsigned char sbuf[16000]; | ||
|
||
if (!TEST_true(create_ssl_ctx_pair(libctx, | ||
smeth, cmeth, | ||
min_version, max_version, | ||
&sctx, &cctx, cert, | ||
privkey))) | ||
goto end; | ||
|
||
if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl, | ||
NULL, NULL))) | ||
goto end; | ||
|
||
if (!TEST_true(SSL_set_options(serverssl, SSL_OP_CLEANSE_PLAINTEXT))) | ||
goto end; | ||
|
||
if (!TEST_true(create_ssl_connection(serverssl, clientssl, | ||
SSL_ERROR_NONE))) | ||
goto end; | ||
|
||
for (i = 0; i < sizeof(cbuf); i++) { | ||
cbuf[i] = i & 0xff; | ||
} | ||
|
||
if (!TEST_int_eq(SSL_write(clientssl, cbuf, sizeof(cbuf)), sizeof(cbuf))) | ||
goto end; | ||
|
||
if (!TEST_int_eq(SSL_peek(serverssl, &sbuf, sizeof(sbuf)), sizeof(sbuf))) | ||
goto end; | ||
|
||
if (memcmp(cbuf, sbuf, sizeof(cbuf))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will do |
||
goto end; | ||
|
||
/* | ||
* Since we called SSL_peek(), we know the data is stored as | ||
* plaintext record. We can gather the pointer to check for | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the data is a |
||
* zerozation after SSL_read(). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. zeroization |
||
*/ | ||
rr = serverssl->rlayer.rrec; | ||
zbuf = &rr->data[rr->off]; | ||
if (!TEST_int_eq(rr->length, sizeof(cbuf))) | ||
goto end; | ||
|
||
/* | ||
* After SSL_peek() the plaintext must still be stored in the | ||
* record. | ||
*/ | ||
if (memcmp(cbuf, zbuf, sizeof(cbuf))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will do |
||
goto end; | ||
|
||
memset(sbuf, 0, sizeof(sbuf)); | ||
if (!TEST_int_eq(SSL_read(serverssl, &sbuf, sizeof(sbuf)), sizeof(sbuf))) | ||
goto end; | ||
|
||
if (memcmp(cbuf, sbuf, sizeof(cbuf))) | ||
goto end; | ||
|
||
/* Check if rbuf is cleansed */ | ||
memset(cbuf, 0, sizeof(cbuf)); | ||
if (memcmp(cbuf, zbuf, sizeof(cbuf))) | ||
goto end; | ||
|
||
testresult = 1; | ||
end: | ||
SSL_free(serverssl); | ||
SSL_free(clientssl); | ||
SSL_CTX_free(sctx); | ||
SSL_CTX_free(cctx); | ||
|
||
return testresult; | ||
} | ||
|
||
static int test_cleanse_plaintext(void) | ||
{ | ||
#if !defined(OPENSSL_NO_TLS1_2) | ||
if (!TEST_true(execute_cleanse_plaintext(TLS_server_method(), | ||
TLS_client_method(), | ||
TLS1_2_VERSION, | ||
TLS1_2_VERSION))) | ||
return 0; | ||
|
||
#endif | ||
|
||
#if !defined(OPENSSL_NO_TLS1_3) | ||
if (!TEST_true(execute_cleanse_plaintext(TLS_server_method(), | ||
TLS_client_method(), | ||
TLS1_3_VERSION, | ||
TLS1_3_VERSION))) | ||
return 0; | ||
#endif | ||
|
||
#if !defined(OPENSSL_NO_DTLS) | ||
if (!TEST_true(execute_cleanse_plaintext(DTLS_server_method(), | ||
DTLS_client_method(), | ||
DTLS1_VERSION, | ||
0))) | ||
return 0; | ||
#endif | ||
return 1; | ||
} | ||
|
||
#ifndef OPENSSL_NO_DTLS | ||
static int test_large_message_dtls(void) | ||
{ | ||
|
@@ -8317,6 +8430,7 @@ int setup_tests(void) | |
#endif | ||
ADD_TEST(test_large_message_tls); | ||
ADD_TEST(test_large_message_tls_read_ahead); | ||
ADD_TEST(test_cleanse_plaintext); | ||
#ifndef OPENSSL_NO_DTLS | ||
ADD_TEST(test_large_message_dtls); | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better phrasing might "...is still responsible for cleansing other copies"