Add support to zeroize plaintext in S3 record layer #12251

melshuber · 2020-06-23T17:26:15Z

Some applications want even all plaintext copies beeing
zeroized. However, currently plaintext residuals are kept in rbuf
within the s3 record layer.

This patch add the option SSL_OP_CLEANSE_PLAINTEXT to its friends to
optionally enable cleansing of decrypted plaintext data.

This patch is a followup of the discussion with Matt on openssl-users
https://mta.openssl.org/pipermail/openssl-users/2020-June/012604.html

Checklist

documentation is added or updated
tests are added or updated

Some applications want even all plaintext copies beeing zeroized. However, currently plaintext residuals are kept in rbuf within the s3 record layer. This patch add the option SSL_OP_CLEANSE_PLAINTEXT to its friends to optionally enable cleansing of decrypted plaintext data.

mattcaswell · 2020-06-23T17:43:22Z

CHANGES.md

@@ -1057,6 +1057,12 @@ OpenSSL 3.0

   *Boris Pismenny*

+ * The SSL option SSL_OP_CLEANSE_PLAINTEXT is introduced. If that
+   option is set, openssl cleanses plaintext bytes after delivering


Probably we should clarify that this means we cleanse internal buffers. The application is still responsible for cleansing its own buffers.

mattcaswell · 2020-06-23T17:45:43Z

ssl/record/rec_layer_s3.c

@@ -1477,6 +1477,9 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
                n = len - totalbytes;

            memcpy(buf, &(rr->data[rr->off]), n);
+            if (s->options & SSL_OP_CLEANSE_PLAINTEXT)


This shouldn't be done if peek is set

mattcaswell · 2020-06-23T17:49:44Z

test/sslapitest.c

+
+    certbio = NULL;
+    if (!TEST_ptr(chaincert))
+        goto end;


The block above seems unnecessary. chaincert is never used.

mattcaswell · 2020-06-23T17:51:33Z

test/sslapitest.c

+        cbuf[i] = i & 0xff;
+    }
+
+    if (!TEST_true(SSL_write(clientssl, cbuf, sizeof(cbuf)) == sizeof(cbuf)))


TEST_int_eq seems more appropriate than TEST_true here.

mattcaswell · 2020-06-23T17:52:43Z

test/sslapitest.c

+    if (!TEST_true(SSL_write(clientssl, cbuf, sizeof(cbuf)) == sizeof(cbuf)))
+        goto end;
+
+    while ((err = SSL_read(serverssl, &sbuf, sizeof(sbuf))) != sizeof(sbuf)) {


The loop here seems unnecessary. We know the data is going to be there because there is no network being used here.

mattcaswell · 2020-06-23T17:54:08Z

test/sslapitest.c

+    if (!TEST_true(SSL_write(clientssl, cbuf, sizeof(cbuf)) == sizeof(cbuf)))
+        goto end;
+
+    while ((err = SSL_read(serverssl, &sbuf, sizeof(sbuf))) != sizeof(sbuf)) {


I think we should do a peek first,

mattcaswell · 2020-06-23T17:58:06Z

Please can you submit a CLA:

https://www.openssl.org/policies/cla.html

beldmit · 2020-06-23T18:02:38Z

doc/man3/SSL_CTX_set_options.pod

+=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 followup data. When enabling


followup => next portion of?

melshuber · 2020-06-23T19:26:58Z

Please can you submit a CLA:

https://www.openssl.org/policies/cla.html

Sure the CCLA is already in my mailbox, the ICLA I am singing tomorrow (my printer is dead) and planning to submit them together

melshuber · 2020-06-23T20:01:04Z

are you preferring to amend the commit or a followup and squash later?

melshuber · 2020-06-23T20:06:18Z

I am wondering, if we should add a similar behaviour in in dtls1_read_bytes() as well. But since I am not sure about the peek/read behaviour for datagrams I am a little bit hesitant.

mattcaswell · 2020-06-24T10:04:59Z

are you preferring to amend the commit or a followup and squash later?

Preferred is to add additional commits fixing up previous ones using git commit --fixup. That way whoever merges can just use git rebase -i --autosquash

mattcaswell · 2020-06-24T10:07:36Z

I am wondering, if we should add a similar behaviour in in dtls1_read_bytes() as well. But since I am not sure about the peek/read behaviour for datagrams I am a little bit hesitant.

Oh, yes. Good point - we should do that. It should be virtually the same.

mattcaswell · 2020-06-24T10:09:28Z

Hmm. The travis failures look odd - but I don't think they are related to this change.

melshuber · 2020-06-24T14:51:29Z

Hmm. The travis failures look odd - but I don't think they are related to this change.

Well that concerns me as well. I run the testsuite on my local machine and there all looks good.
However since travis checks more this is just an indication.

The odd thing is that we see red Xes all over the place even on merged commits. I did not check in detail but the logs look similar.

However since it least the 'if' statements are executed in many configurations, I am not sure what to do about this.

melshuber · 2020-06-24T14:53:03Z

Please can you submit a CLA:
https://www.openssl.org/policies/cla.html

Sure the CCLA is already in my mailbox, the ICLA I am singing tomorrow (my printer is dead) and planning to submit them together

I sent the ICLA and CCLA a couple of hours ago, please tell me if something is missing

melshuber · 2020-06-24T14:55:45Z

To summarize the delta:

Addressed your comments
Added zeroization for DTLS (and test)
Test: Added checks to explicitly check internal buffer contents after SSL_peek, and after SSL_read
Test: Moved test functions down, it was in between the large_message tests

melshuber · 2020-06-24T15:02:53Z

are you preferring to amend the commit or a followup and squash later?

Preferred is to add additional commits fixing up previous ones using git commit --fixup. That way whoever merges can just use git rebase -i --autosquash

So hold back with rebasing myself for now

mattcaswell · 2020-06-24T16:47:32Z

Close/reopen to kick CLA bot

mattcaswell · 2020-06-25T09:07:54Z

Travis is now passing :-)

melshuber · 2020-06-25T09:28:58Z

Travis is now passing :-)

Feels better this way :)

melshuber · 2020-06-25T09:31:11Z

Since there are still changes requested. Is something missing from my side?

mattcaswell · 2020-06-25T09:25:58Z

ssl/record/rec_layer_d1.c


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


Here we're copying plaintext out of rdata, which comes from item->data. There are two callers of dtls1_copy_record, and in both cases item->data is immediately freed, so we probably also need to cleanse that. This function could be called for any type of record - not just application data. Which makes me wonder whether this option is supposed to cleanse all plaintext (e.g. including handshake/alert plaintext), or just application data plaintext. If the former then I think there are some spots that might have been missed (see further comments below).

will recheck and update

I am trying to get around where DTLS maintains copies of records.
Sof far i found three queues

unprocessed_rcds.q:
I think this contains ciphertext records (no cleansing required)

processed_rcds.q: I think this contains plaintext records, which can be either app data or handshake.
Cleansing is required only when we free it and there is still data left (DTLS_RECORD_LAYER_clear)

buffered_app_data.q: Bufferd but not yet deliverd plaintext application data records. Must be cleansed in (DTLS_RECORD_LAYER_clear, in dtls1_read_bytes)

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:

Add cleansing in DTLS_RECORD_LAYER_clear

Revert cleansing in dtls1_copy_record

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?

Yes

The old record is freed (and cleansed) in this process?

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 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:

I agree with your analysis

mattcaswell · 2020-06-25T09:31:13Z

ssl/record/rec_layer_s3.c

@@ -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);


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)

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.

mattcaswell · 2020-06-25T09:33:46Z

test/sslapitest.c

+    if (!TEST_int_eq(SSL_peek(serverssl, &sbuf, sizeof(sbuf)), sizeof(sbuf)))
+        goto end;
+
+    if (memcmp(cbuf, sbuf, sizeof(cbuf)))


Use TEST_mem_eq instead of memcmp. You get better diagnostic info if the test ever fails.

mattcaswell · 2020-06-25T09:34:35Z

test/sslapitest.c

+     * After SSL_peek() the plaintext must still be stored in the
+     * record.
+     */
+    if (memcmp(cbuf, zbuf, sizeof(cbuf)))


TEST_mem_eq...and so on below

melshuber · 2020-06-26T12:02:56Z

ssl/record/rec_layer_d1.c

@@ -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);


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.

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

mattcaswell

Approved subject to the minor wording tweak.

Needs second review.

mattcaswell · 2020-06-30T13:47:09Z

CHANGES.md

+ * 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.:


A better phrasing might "...is still responsible for cleansing other copies"

melshuber · 2020-06-30T21:09:25Z

Fixed proposal in CHANGES.md

mattcaswell

Reconfirmed.

Ping @openssl/committers.

slontis · 2020-07-02T23:42:20Z

doc/man3/SSL_CTX_set_options.pod

+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


NIT: notably (not sure if it should have a , or not)

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

slontis · 2020-07-02T23:44:09Z

doc/man3/SSL_CTX_set_options.pod

+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)>.


L<SSL_peek(3)>

slontis · 2020-07-02T23:51:37Z

test/sslapitest.c

+
+    /*
+     * Since we called SSL_peek(), we know the data is stored as
+     * plaintext record. We can gather the pointer to check for


the data is a

slontis · 2020-07-02T23:53:44Z

test/sslapitest.c

+    /*
+     * Since we called SSL_peek(), we know the data is stored as
+     * plaintext record. We can gather the pointer to check for
+     * zerozation after SSL_read().


zeroization

slontis

Looks good apart from some small NITS.

melshuber · 2020-07-03T10:16:12Z

@slontis: thx for the review,

I added update according to to your proposial (except the one i commeneted on).

mattcaswell · 2020-07-03T10:26:14Z

doc/man3/SSL_CTX_set_options.pod

+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


@slontis made two comments about this. I think you missed the one "notable" -> "notably"

So @slontis also meant the comma after the notably.

I am not sure about this either.
What do you think?

@slontis; sorry i misread your comment

fixed it as well

I'm not sure about the comma - so whatever.

mattcaswell · 2020-07-03T10:27:50Z

doc/man3/SSL_CTX_set_options.pod

+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)>,
+L<SSL_peek(3)|SSL_read(3)> but also like L<SSL_write(3)>.


Just L<SSL_peek(3)> should be sufficient here (and is consistent with what we normally do). There are symlinks in place to resolve where multiple functions are defined in a single file.

I tried with L<SSL_peek(3)> and this leads to a dead link in the html man page.
therefor i did not add it in the beginning

at least when using links2(1)

Hmm. Strange. But this should work, so if it isn't then we've got that problem throughout the man pages. Not an issue for this PR to resolve - so I think we should just have it asL<SSL_peek(3)> for consistency.

Ok,

JFI: Here is what i tried:

./Configure --prefix=$(pwd)/tmp linux-x86_64 make install links2 tmp/share/doc/openssl/html/man3/SSL_CTX_set_options.html

there are no symlinks in tmp/share/doc/openssl/html/man3/

@levitte...any thoughts on that?

beldmit

LGTM

openssl-machine · 2020-07-07T09:00:16Z

This pull request is ready to merge

Some applications want even all plaintext copies beeing zeroized. However, currently plaintext residuals are kept in rbuf within the s3 record layer. This patch add the option SSL_OP_CLEANSE_PLAINTEXT to its friends to optionally enable cleansing of decrypted plaintext data. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #12251)

beldmit · 2020-07-07T09:09:25Z

Squashed and merged. Thanks!

melshuber · 2020-07-07T09:13:03Z

Thanks all

Some applications want even all plaintext copies beeing zeroized. However, currently plaintext residuals are kept in rbuf within the s3 record layer. This patch add the option SSL_OP_CLEANSE_PLAINTEXT to its friends to optionally enable cleansing of decrypted plaintext data. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#12251)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 23, 2020

mattcaswell added the branch: master Merge to master branch label Jun 23, 2020

mattcaswell requested changes Jun 23, 2020

View reviewed changes

beldmit reviewed Jun 23, 2020

View reviewed changes

martin-cherry added 2 commits June 24, 2020 16:37

fixup! Add support to zeroize plaintext in S3 record layer

2440942

fixup! Add support to zeroize plaintext in S3 record layer

d24358e

mattcaswell closed this Jun 24, 2020

mattcaswell reopened this Jun 24, 2020

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jun 24, 2020

mattcaswell requested changes Jun 25, 2020

View reviewed changes

martin-cherry added 2 commits June 26, 2020 13:11

fixup! Add support to zeroize plaintext in S3 record layer

1a00435

fixup! Add support to zeroize plaintext in S3 record layer

74d2ef9

melshuber commented Jun 26, 2020

View reviewed changes

mattcaswell approved these changes Jun 30, 2020

View reviewed changes

mattcaswell added the approval: review pending This pull request needs review by a committer label Jun 30, 2020

fixup! Add support to zeroize plaintext in S3 record layer

baf3dd1

mattcaswell approved these changes Jul 1, 2020

View reviewed changes

slontis reviewed Jul 2, 2020

View reviewed changes

fixup! Add support to zeroize plaintext in S3 record layer

97bb6e2

mattcaswell requested changes Jul 3, 2020

View reviewed changes

fixup! Add support to zeroize plaintext in S3 record layer

f7abb8b

slontis approved these changes Jul 3, 2020

View reviewed changes

mattcaswell approved these changes Jul 6, 2020

View reviewed changes

mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jul 6, 2020

beldmit approved these changes Jul 6, 2020

View reviewed changes

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jul 7, 2020

beldmit closed this Jul 7, 2020

Add support to zeroize plaintext in S3 record layer #12251

Add support to zeroize plaintext in S3 record layer #12251

Conversation

melshuber commented Jun 23, 2020

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell commented Jun 23, 2020

Choose a reason for hiding this comment

melshuber commented Jun 23, 2020

melshuber commented Jun 23, 2020

melshuber commented Jun 23, 2020

mattcaswell commented Jun 24, 2020

mattcaswell commented Jun 24, 2020

mattcaswell commented Jun 24, 2020

melshuber commented Jun 24, 2020

melshuber commented Jun 24, 2020 • edited

melshuber commented Jun 24, 2020

melshuber commented Jun 24, 2020

mattcaswell commented Jun 24, 2020

mattcaswell commented Jun 25, 2020

melshuber commented Jun 25, 2020

melshuber commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melshuber Jun 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melshuber commented Jun 30, 2020

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slontis Jul 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slontis left a comment

Choose a reason for hiding this comment

melshuber commented Jul 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beldmit left a comment

Choose a reason for hiding this comment

openssl-machine commented Jul 7, 2020

beldmit commented Jul 7, 2020

melshuber commented Jul 7, 2020

melshuber commented Jun 24, 2020 •

edited

melshuber Jun 26, 2020 •

edited

slontis Jul 2, 2020 •

edited