Skip to content

Commit

Permalink
Bug 1528175 - Include version in SSL_MakeAead arguments, r=ekr
Browse files Browse the repository at this point in the history 
Summary:
We should really include version with the ciphersuite in case we decide to reuse the ciphersuite definitions for TLS 1.4, but also change the way they operate.

I also included a fixup for the clang4 build error from the last set.

Reviewers: ekr

Tags: #secure-revision

Bug #: 1528175

Differential Revision: https://phabricator.services.mozilla.com/D20761

--HG--
extra : amend_source : 045ee0c1b47b0051dc904fd6708ce7f2ae84dc6e
  • Loading branch information
@martinthomson
martinthomson committed Feb 22, 2019
1 parent 1e899e7 commit ae30248
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 40 deletions.
55 changes: 43 additions & 12 deletions gtests/ssl_gtest/ssl_primitive_unittest.cc
View file
Expand Up @@ -128,33 +128,62 @@ static const uint8_t kCiphertextChaCha20Poly1305[] = {
0x4e, 0x89, 0x2c, 0xfa, 0xfc, 0x8c, 0x40, 0x55, 0x6d, 0x7e,
0x99, 0xac, 0x8e, 0x54, 0x58, 0xb1, 0x18, 0xd2, 0x66, 0x22};

TEST_F(AeadTest, AeadBadVersion) {
SSLAeadContext *ctx = nullptr;
ASSERT_EQ(SECFailure,
SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_2, TLS_AES_128_GCM_SHA256,
secret_.get(), kLabel, strlen(kLabel), &ctx));
EXPECT_EQ(nullptr, ctx);
}

TEST_F(AeadTest, AeadUnsupportedCipher) {
SSLAeadContext *ctx = nullptr;
ASSERT_EQ(SECFailure,
SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_RSA_WITH_NULL_MD5,
secret_.get(), kLabel, strlen(kLabel), &ctx));
EXPECT_EQ(nullptr, ctx);
}

TEST_F(AeadTest, AeadOlderCipher) {
SSLAeadContext *ctx = nullptr;
ASSERT_EQ(
SECFailure,
SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_RSA_WITH_AES_128_CBC_SHA,
secret_.get(), kLabel, strlen(kLabel), &ctx));
EXPECT_EQ(nullptr, ctx);
}

TEST_F(AeadTest, AeadNoLabel) {
SSLAeadContext *ctx = nullptr;
ASSERT_EQ(SECFailure, SSL_MakeAead(secret_.get(), TLS_AES_128_GCM_SHA256,
nullptr, 12, &ctx));
ASSERT_EQ(SECFailure,
SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_128_GCM_SHA256,
secret_.get(), nullptr, 12, &ctx));
EXPECT_EQ(nullptr, ctx);
}

TEST_F(AeadTest, AeadLongLabel) {
SSLAeadContext *ctx = nullptr;
ASSERT_EQ(SECFailure,
SSL_MakeAead(secret_.get(), TLS_AES_128_GCM_SHA256, "", 254, &ctx));
SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_128_GCM_SHA256,
secret_.get(), "", 254, &ctx));
EXPECT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError());
EXPECT_EQ(nullptr, ctx);
}

TEST_F(AeadTest, AeadNoPointer) {
SSLAeadContext *ctx = nullptr;
ASSERT_EQ(SECFailure, SSL_MakeAead(secret_.get(), TLS_AES_128_GCM_SHA256,
kLabel, strlen(kLabel), nullptr));
ASSERT_EQ(SECFailure,
SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_128_GCM_SHA256,
secret_.get(), kLabel, strlen(kLabel), nullptr));
EXPECT_EQ(SEC_ERROR_INVALID_ARGS, PORT_GetError());
EXPECT_EQ(nullptr, ctx);
}

TEST_F(AeadTest, AeadAes128Gcm) {
SSLAeadContext *ctxInit;
ASSERT_EQ(SECSuccess, SSL_MakeAead(secret_.get(), TLS_AES_128_GCM_SHA256,
kLabel, strlen(kLabel), &ctxInit));
ASSERT_EQ(SECSuccess,
SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_128_GCM_SHA256,
secret_.get(), kLabel, strlen(kLabel), &ctxInit));
ScopedSSLAeadContext ctx(ctxInit);
EXPECT_NE(nullptr, ctx);

Expand All @@ -163,8 +192,9 @@ TEST_F(AeadTest, AeadAes128Gcm) {

TEST_F(AeadTest, AeadAes256Gcm) {
SSLAeadContext *ctxInit;
ASSERT_EQ(SECSuccess, SSL_MakeAead(secret_.get(), TLS_AES_256_GCM_SHA384,
kLabel, strlen(kLabel), &ctxInit));
ASSERT_EQ(SECSuccess,
SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_AES_256_GCM_SHA384,
secret_.get(), kLabel, strlen(kLabel), &ctxInit));
ScopedSSLAeadContext ctx(ctxInit);
EXPECT_NE(nullptr, ctx);

Expand All @@ -173,9 +203,10 @@ TEST_F(AeadTest, AeadAes256Gcm) {

TEST_F(AeadTest, AeadChaCha20Poly1305) {
SSLAeadContext *ctxInit;
ASSERT_EQ(SECSuccess,
SSL_MakeAead(secret_.get(), TLS_CHACHA20_POLY1305_SHA256, kLabel,
strlen(kLabel), &ctxInit));
ASSERT_EQ(
SECSuccess,
SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3, TLS_CHACHA20_POLY1305_SHA256,
secret_.get(), kLabel, strlen(kLabel), &ctxInit));
ScopedSSLAeadContext ctx(ctxInit);
EXPECT_NE(nullptr, ctx);

Expand Down
2 changes: 1 addition & 1 deletion gtests/ssl_gtest/tls_hkdf_unittest.cc
View file
Expand Up @@ -58,7 +58,7 @@ const size_t kHashLength[] = {

size_t GetHashLength(SSLHashType hash) {
size_t i = static_cast<size_t>(hash);
if (i >= 0 && i < PR_ARRAY_SIZE(kHashLength)) {
if (i < PR_ARRAY_SIZE(kHashLength)) {
return kHashLength[i];
}
ADD_FAILURE() << "Unknown hash: " << hash;
Expand Down
4 changes: 3 additions & 1 deletion gtests/ssl_gtest/tls_protect.cc
View file
Expand Up @@ -5,6 +5,7 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "tls_protect.h"
#include "sslproto.h"
#include "tls_filter.h"

namespace nss_test {
Expand All @@ -25,7 +26,8 @@ TlsCipherSpec::TlsCipherSpec(bool dtls, uint16_t epoc)
bool TlsCipherSpec::SetKeys(SSLCipherSuiteInfo* cipherinfo,
PK11SymKey* secret) {
SSLAeadContext* ctx;
SECStatus rv = SSL_MakeAead(secret, cipherinfo->cipherSuite, "",
SECStatus rv = SSL_MakeAead(SSL_LIBRARY_VERSION_TLS_1_3,
cipherinfo->cipherSuite, secret, "",
0, // Use the default labels.
&ctx);
if (rv != SECSuccess) {
Expand Down
17 changes: 10 additions & 7 deletions lib/ssl/sslexp.h
View file
Expand Up @@ -642,13 +642,16 @@ typedef SECStatus(PR_CALLBACK *SSLRecordWriteCallback)(
*/
typedef struct SSLAeadContextStr SSLAeadContext;

#define SSL_MakeAead(secret, cipherSuite, labelPrefix, labelPrefixLen, ctx) \
SSL_EXPERIMENTAL_API("SSL_MakeAead", \
(PK11SymKey * _secret, PRUint16 _cipherSuite, \
const char *_labelPrefix, \
unsigned int _labelPrefixLen, \
SSLAeadContext **_ctx), \
(secret, cipherSuite, labelPrefix, labelPrefixLen, ctx))
#define SSL_MakeAead(version, cipherSuite, secret, \
labelPrefix, labelPrefixLen, ctx) \
SSL_EXPERIMENTAL_API("SSL_MakeAead", \
(PRUint16 _version, PRUint16 _cipherSuite, \
PK11SymKey * _secret, \
const char *_labelPrefix, \
unsigned int _labelPrefixLen, \
SSLAeadContext **_ctx), \
(version, cipherSuite, secret, \
labelPrefix, labelPrefixLen, ctx))

#define SSL_AeadEncrypt(ctx, counter, aad, aadLen, in, inLen, \
output, outputLen, maxOutputLen) \
Expand Down
2 changes: 1 addition & 1 deletion lib/ssl/sslimpl.h
View file
Expand Up @@ -1760,7 +1760,7 @@ SECStatus SSLExp_GetCurrentEpoch(PRFileDesc *fd, PRUint16 *readEpoch,

#define SSLResumptionTokenVersion 2

SECStatus SSLExp_MakeAead(PK11SymKey *secret, PRUint16 cipherSuite,
SECStatus SSLExp_MakeAead(PRUint16 version, PRUint16 cipherSuite, PK11SymKey *secret,
const char *labelPrefix, unsigned int labelPrefixLen,
SSLAeadContext **ctx);
SECStatus SSLExp_DestroyAead(SSLAeadContext *ctx);
Expand Down
55 changes: 37 additions & 18 deletions lib/ssl/sslprimitive.c
View file
Expand Up @@ -23,8 +23,34 @@ struct SSLAeadContextStr {
ssl3KeyMaterial keys;
};

static SECStatus
tls13_GetHashAndCipher(PRUint16 version, PRUint16 cipherSuite,
SSLHashType *hash, const ssl3BulkCipherDef **cipher)
{
if (version < SSL_LIBRARY_VERSION_TLS_1_3) {
PORT_SetError(SEC_ERROR_INVALID_ARGS);
return SECFailure;
}

// Lookup and check the suite.
SSLVersionRange vrange = { version, version };
if (!ssl3_CipherSuiteAllowedForVersionRange(cipherSuite, &vrange)) {
PORT_SetError(SEC_ERROR_INVALID_ARGS);
return SECFailure;
}
const ssl3CipherSuiteDef *suiteDef = ssl_LookupCipherSuiteDef(cipherSuite);
const ssl3BulkCipherDef *cipherDef = ssl_GetBulkCipherDef(suiteDef);
if (cipherDef->type != type_aead) {
PORT_SetError(SEC_ERROR_INVALID_ARGS);
return SECFailure;
}
*hash = suiteDef->prf_hash;
*cipher = cipherDef;
return SECSuccess;
}

SECStatus
SSLExp_MakeAead(PK11SymKey *secret, PRUint16 cipherSuite,
SSLExp_MakeAead(PRUint16 version, PRUint16 cipherSuite, PK11SymKey *secret,
const char *labelPrefix, unsigned int labelPrefixLen,
SSLAeadContext **ctx)
{
Expand All @@ -41,20 +67,13 @@ SSLExp_MakeAead(PK11SymKey *secret, PRUint16 cipherSuite,
goto loser;
}

// Lookup and check the suite.
SSLVersionRange tls13 = { SSL_LIBRARY_VERSION_TLS_1_3,
SSL_LIBRARY_VERSION_TLS_1_3 };
if (!ssl3_CipherSuiteAllowedForVersionRange(cipherSuite, &tls13)) {
PORT_SetError(SEC_ERROR_INVALID_ARGS);
goto loser;
}
const ssl3CipherSuiteDef *suiteDef = ssl_LookupCipherSuiteDef(cipherSuite);
const ssl3BulkCipherDef *cipher = ssl_GetBulkCipherDef(suiteDef);
if (cipher->type != type_aead) {
PORT_SetError(SEC_ERROR_INVALID_ARGS);
goto loser;
SSLHashType hash;
const ssl3BulkCipherDef *cipher;
SECStatus rv = tls13_GetHashAndCipher(version, cipherSuite,
&hash, &cipher);
if (rv != SECSuccess) {
goto loser; /* Code already set. */
}
SSLHashType hash = suiteDef->prf_hash;

out = PORT_ZNew(SSLAeadContext);
if (out == NULL) {
Expand All @@ -66,10 +85,10 @@ SSLExp_MakeAead(PK11SymKey *secret, PRUint16 cipherSuite,
memcpy(label + labelPrefixLen, ivSuffix, strlen(ivSuffix));
unsigned int labelLen = labelPrefixLen + strlen(ivSuffix);
unsigned int ivLen = cipher->iv_size + cipher->explicit_nonce_size;
SECStatus rv = tls13_HkdfExpandLabelRaw(secret, hash,
NULL, 0, // Handshake hash.
label, labelLen,
out->keys.iv, ivLen);
rv = tls13_HkdfExpandLabelRaw(secret, hash,
NULL, 0, // Handshake hash.
label, labelLen,
out->keys.iv, ivLen);
if (rv != SECSuccess) {
goto loser;
}
Expand Down

0 comments on commit ae30248

Please sign in to comment.