-
-
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 6 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 | ||
application data in a static buffer until it is overwritten by the | ||
next portion of data. When enabling SSL_OP_CLEANSE_PLAINTEXT | ||
deciphered application data 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 | ||
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 |
---|---|---|
|
@@ -74,13 +74,17 @@ void DTLS_RECORD_LAYER_clear(RECORD_LAYER *rl) | |
|
||
while ((item = pqueue_pop(d->processed_rcds.q)) != NULL) { | ||
rdata = (DTLS1_RECORD_DATA *)item->data; | ||
if (rl->s->options & SSL_OP_CLEANSE_PLAINTEXT) | ||
OPENSSL_cleanse(rdata->rbuf.buf, rdata->rbuf.len); | ||
OPENSSL_free(rdata->rbuf.buf); | ||
OPENSSL_free(item->data); | ||
pitem_free(item); | ||
} | ||
|
||
while ((item = pqueue_pop(d->buffered_app_data.q)) != NULL) { | ||
rdata = (DTLS1_RECORD_DATA *)item->data; | ||
if (rl->s->options & SSL_OP_CLEANSE_PLAINTEXT) | ||
OPENSSL_cleanse(rdata->rbuf.buf, rdata->rbuf.len); | ||
OPENSSL_free(rdata->rbuf.buf); | ||
OPENSSL_free(item->data); | ||
pitem_free(item); | ||
|
@@ -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 |
---|---|---|
|
@@ -1595,6 +1595,119 @@ static int test_large_message_dtls(void) | |
} | ||
#endif | ||
|
||
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 (!TEST_mem_eq(cbuf, sizeof(cbuf), sbuf, sizeof(sbuf))) | ||
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 (!TEST_mem_eq(cbuf, sizeof(cbuf), zbuf, sizeof(cbuf))) | ||
goto end; | ||
|
||
memset(sbuf, 0, sizeof(sbuf)); | ||
if (!TEST_int_eq(SSL_read(serverssl, &sbuf, sizeof(sbuf)), sizeof(sbuf))) | ||
goto end; | ||
|
||
if (!TEST_mem_eq(cbuf, sizeof(cbuf), sbuf, sizeof(cbuf))) | ||
goto end; | ||
|
||
/* Check if rbuf is cleansed */ | ||
memset(cbuf, 0, sizeof(cbuf)); | ||
if (!TEST_mem_eq(cbuf, sizeof(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_OCSP | ||
static int ocsp_server_cb(SSL *s, void *arg) | ||
{ | ||
|
@@ -8320,6 +8433,7 @@ int setup_tests(void) | |
#ifndef OPENSSL_NO_DTLS | ||
ADD_TEST(test_large_message_dtls); | ||
#endif | ||
ADD_TEST(test_cleanse_plaintext); | ||
#ifndef OPENSSL_NO_OCSP | ||
ADD_TEST(test_tlsext_status_type); | ||
#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.
NIT: notably (not sure if it should have a , or not)
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.
Not sure either, but
https://thecriticalreader.com/because-since-and-commas/ and
https://ell.stackexchange.com/questions/38051/should-i-add-a-comma-before-since#:~:text=When%20since%20is%20used%20as,starts%20after%20the%20main%20clause.
state the ',' should stay