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.

Sign up for GitHub

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

Jump to bottom

Add support to zeroize plaintext in S3 record layer #12251

Closed
wants to merge 8 commits into from
Closed

Add support to zeroize plaintext in S3 record layer #12251

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bc2ffab
Add support to zeroize plaintext in S3 record layer
martin-cherry Jun 23, 2020
2440942
fixup! Add support to zeroize plaintext in S3 record layer
martin-cherry Jun 24, 2020
d24358e
fixup! Add support to zeroize plaintext in S3 record layer
martin-cherry Jun 24, 2020
1a00435
fixup! Add support to zeroize plaintext in S3 record layer
martin-cherry Jun 26, 2020
74d2ef9
fixup! Add support to zeroize plaintext in S3 record layer
martin-cherry Jun 26, 2020
baf3dd1
fixup! Add support to zeroize plaintext in S3 record layer
martin-cherry Jun 30, 2020
97bb6e2
fixup! Add support to zeroize plaintext in S3 record layer
martin-cherry Jul 3, 2020
f7abb8b
fixup! Add support to zeroize plaintext in S3 record layer
martin-cherry Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1057,6 +1057,14 @@ OpenSSL 3.0

*Boris Pismenny*

* The SSL option SSL_OP_CLEANSE_PLAINTEXT is introduced. If that
option is set, openssl cleanses (zeroize) plaintext bytes from
internal buffers after delivering them to the application. Note,
the application is still responsible to cleanse other copies (e.g.:
Copy link
Member

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"

data received by SSL_read(3)).

*Martin Elshuber*

OpenSSL 1.1.1
-------------

Expand Down
14 changes: 14 additions & 0 deletions doc/man3/SSL_CTX_set_options.pod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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)

Copy link
Contributor Author

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

applies to buffers passed to functions like L<SSL_read(3)>,
SSL_peek(3) but also like L<SSL_write(3)>.
Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
3 changes: 2 additions & 1 deletion include/openssl/ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,8 @@ typedef int (*SSL_async_callback_fn)(SSL *s, void *arg);
/* Disable Extended master secret */
# define SSL_OP_NO_EXTENDED_MASTER_SECRET 0x00000001U

/* Reserved value (until OpenSSL 3.0.0) 0x00000002U */
/* Cleanse plaintext copies of data delivered to the application */
# define SSL_OP_CLEANSE_PLAINTEXT 0x00000002U

/* Allow initial connection to servers that don't support RI */
# define SSL_OP_LEGACY_SERVER_CONNECT 0x00000004U
Expand Down
6 changes: 6 additions & 0 deletions ssl/record/rec_layer_d1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Copy link
Contributor Author

@melshuber melshuber Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry n need to be replaced by:
len - SSL3_RECORD_get_off(rr)

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) {
Expand Down
2 changes: 2 additions & 0 deletions ssl/record/rec_layer_s3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:
(1) "if so then the scope suddenly became a lot more complicated" and
(2) "Alternatively we clarify that the option is only about application data plaintext and restrict ourselves to worrying about that only."

ad (1) totally correct -> I will be a never ending story.
ad (2) I think this is the way to got: I will clarify this in the manpage.

SSL3_RECORD_sub_length(rr, n);
SSL3_RECORD_add_off(rr, n);
if (SSL3_RECORD_get_length(rr) == 0) {
Expand Down
2 changes: 2 additions & 0 deletions ssl/record/ssl3_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ int ssl3_release_read_buffer(SSL *s)
SSL3_BUFFER *b;

b = RECORD_LAYER_get_rbuf(&s->rlayer);
if (s->options & SSL_OP_CLEANSE_PLAINTEXT)
OPENSSL_cleanse(b->buf, b->len);
OPENSSL_free(b->buf);
b->buf = NULL;
return 1;
Expand Down
114 changes: 114 additions & 0 deletions test/sslapitest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@slontis slontis Jul 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the data is a

* zerozation after SSL_read().
Copy link
Member

Choose a reason for hiding this comment

The 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)
{
Expand Down Expand Up @@ -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
Expand Down