diff options
author | Leander Schwarz <lschwarz@mozilla.com> | 2022-06-10 10:22:35 +0000 |
---|---|---|
committer | Leander Schwarz <lschwarz@mozilla.com> | 2022-06-10 10:22:35 +0000 |
commit | 1e910430010139a54808439998080804126a51b9 (patch) | |
tree | 6dcf5ac966ef86eaf50f1dad5d21ff98362d2665 | |
parent | d91c4ab506ab671e0d73d69d0a66b5b41147155a (diff) | |
download | nss-hg-1e910430010139a54808439998080804126a51b9.tar.gz |
Bug 1765383 - GatherBuffer: Reduced plaintext buffer allocations by allocating it on initialization. Replaced redundant code with assert. Debug builds: Added buffer freeing/allocation for each record. r=djackson
Differential Revision: https://phabricator.services.mozilla.com/D144034
-rw-r--r-- | gtests/ssl_gtest/ssl_gather_unittest.cc | 27 | ||||
-rw-r--r-- | lib/ssl/ssl3con.c | 37 | ||||
-rw-r--r-- | lib/ssl/ssl3gthr.c | 17 | ||||
-rw-r--r-- | lib/ssl/sslsecur.c | 11 |
4 files changed, 74 insertions, 18 deletions
diff --git a/gtests/ssl_gtest/ssl_gather_unittest.cc b/gtests/ssl_gtest/ssl_gather_unittest.cc index cdcdf5626..2b0b722ae 100644 --- a/gtests/ssl_gtest/ssl_gather_unittest.cc +++ b/gtests/ssl_gtest/ssl_gather_unittest.cc @@ -126,4 +126,31 @@ TEST_F(GatherV2ClientHelloTest, GatherEmptyV2RecordShortHeader) { ConnectExpectMalformedClientHello(buffer); } +/* Test correct gather buffer clearing/freeing and (re-)allocation. + * + * Freeing and (re-)allocation of the gather buffers after reception of single + * records is only done in DEBUG builds. Normally they are created and + * destroyed with the SSL socket. + * + * TLS 1.0 record splitting leads to implicit complete read of the data. + * + * The NSS DTLS impelmentation does not allow partial reads + * (see sslsecur.c, line 535-543). */ +TEST_P(TlsConnectStream, GatherBufferPartialReadTest) { + EnsureTlsSetup(); + Connect(); + + client_->SendData(1000); + + if (version_ > SSL_LIBRARY_VERSION_TLS_1_0) { + for (unsigned i = 1; i <= 20; i++) { + server_->ReadBytes(50); + ASSERT_EQ(server_->received_bytes(), 50U * i); + } + } else { + server_->ReadBytes(50); + ASSERT_EQ(server_->received_bytes(), 1000U); + } +} + } // namespace nss_test diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c index 09d7f098b..de5eb3e97 100644 --- a/lib/ssl/ssl3con.c +++ b/lib/ssl/ssl3con.c @@ -13372,17 +13372,17 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText) return SECFailure; } - if (plaintext->space < cTextSizeLimit) { - rv = sslBuffer_Grow(plaintext, cTextSizeLimit); - if (rv != SECSuccess) { - ssl_ReleaseSpecReadLock(ss); /*************************/ - SSL_DBG(("%d: SSL3[%d]: HandleRecord, tried to get %d bytes", - SSL_GETPID(), ss->fd, cTextSizeLimit)); - /* sslBuffer_Grow has set a memory error code. */ - /* Perhaps we should send an alert. (but we have no memory!) */ - return SECFailure; - } - } +#ifdef DEBUG + /* In debug builds the gather buffers are freed after the handling of each + * record for advanced ASAN coverage. Allocate the buffer again to the + * maximum possibly needed size as on gather initialization in + * ssl3gthr.c/ssl3_InitGather(). */ + PR_ASSERT(sslBuffer_Grow(plaintext, TLS_1_2_MAX_CTEXT_LENGTH) == SECSuccess); +#endif + /* This replaces a dynamic plaintext buffer size check, since the buffer is + * allocated to the maximum size in ssl3gthr.c/ssl3_InitGather(). The buffer + * was always grown to the maximum size at first record gathering before. */ + PR_ASSERT(plaintext->space >= cTextSizeLimit); /* Most record types aside from protected TLS 1.3 records carry the content * type in the first octet. TLS 1.3 will override this value later. */ @@ -13390,7 +13390,6 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText) /* Encrypted application data records could arrive before the handshake * completes in DTLS 1.3. These can look like valid TLS 1.2 application_data * records in epoch 0, which is never valid. Pretend they didn't decrypt. */ - if (spec->epoch == 0 && ((IS_DTLS(ss) && dtls_IsDtls13Ciphertext(0, rType)) || rType == ssl_ct_application_data)) { @@ -13526,8 +13525,18 @@ ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText) return SECFailure; } - return ssl3_HandleNonApplicationData(ss, rType, epoch, cText->seqNum, - plaintext); + rv = ssl3_HandleNonApplicationData(ss, rType, epoch, cText->seqNum, + plaintext); + +#ifdef DEBUG + /* In Debug builds free and zero gather plaintext buffer after its content + * has been used/copied for advanced ASAN coverage/utilization. + * This frees buffer for non application data records, for application data + * records it is freed in sslsecur.c/DoRecv(). */ + sslBuffer_Clear(&ss->gs.buf); +#endif + + return rv; } /* diff --git a/lib/ssl/ssl3gthr.c b/lib/ssl/ssl3gthr.c index 08cbe7fd8..b87454d34 100644 --- a/lib/ssl/ssl3gthr.c +++ b/lib/ssl/ssl3gthr.c @@ -26,16 +26,16 @@ typedef struct ssl2GatherStr ssl2Gather; SECStatus ssl3_InitGather(sslGather *gs) { - SECStatus status; - gs->state = GS_INIT; gs->writeOffset = 0; gs->readOffset = 0; gs->dtlsPacketOffset = 0; gs->dtlsPacket.len = 0; gs->rejectV2Records = PR_FALSE; - status = sslBuffer_Grow(&gs->buf, 4096); - return status; + /* Allocate plaintext buffer to maximum possibly needed size. It needs to + * be larger than recordSizeLimit for TLS 1.0 and 1.1 compatability. + * The TLS 1.2 ciphertext is larger than the TLS 1.3 ciphertext. */ + return sslBuffer_Grow(&gs->buf, TLS_1_2_MAX_CTEXT_LENGTH); } /* Caller must hold RecvBufLock. */ @@ -560,6 +560,15 @@ ssl3_GatherCompleteHandshake(sslSocket *ss, int flags) cText.buf = &ss->gs.inbuf; rv = ssl3_HandleRecord(ss, &cText); } + +#ifdef DEBUG + /* In Debug builds free gather ciphertext buffer after each decryption + * for advanced ASAN coverage/utilization. The buffer content has been + * used at this point, ssl3_HandleRecord() and thereby the decryption + * functions are only called from this point of the implementation. */ + sslBuffer_Clear(&ss->gs.inbuf); +#endif + if (rv < 0) { return ss->recvdCloseNotify ? 0 : rv; } diff --git a/lib/ssl/sslsecur.c b/lib/ssl/sslsecur.c index 2c9a4dbf9..281ea927a 100644 --- a/lib/ssl/sslsecur.c +++ b/lib/ssl/sslsecur.c @@ -551,6 +551,17 @@ DoRecv(sslSocket *ss, unsigned char *out, int len, int flags) PORT_Assert(ss->gs.readOffset <= ss->gs.writeOffset); rv = amount; +#ifdef DEBUG + /* In Debug builds free and zero gather plaintext buffer after its content + * has been used/copied for advanced ASAN coverage/utilization. + * This frees the buffer after reception of application data, + * non-application data is freed at the end of + * ssl3con.c/ssl3_HandleRecord(). */ + if (ss->gs.writeOffset == ss->gs.readOffset) { + sslBuffer_Clear(&ss->gs.buf); + } +#endif + SSL_TRC(30, ("%d: SSL[%d]: amount=%d available=%d", SSL_GETPID(), ss->fd, amount, available)); PRINT_BUF(4, (ss, "DoRecv receiving plaintext:", out, amount)); |