Check the second ClientHello's PSK binder on resumption.
We perform all our negotiation based on the first ClientHello (for
consistency with what |select_certificate_cb| observed), which is in the
transcript, so we can ignore most of the second one.
However, we ought to check the second PSK binder. That covers the client
key share, which we do consume. In particular, we'll want to check if it
we ever send half-RTT data on these connections (we do not currently do
this). It is also a tricky computation, so we enforce the peer handled
it correctly.
Tested that both Chrome and Firefox continue to interop with this check,
when configuring uncommon curve preferences that trigger HRR. (Normally
neither browser sees HRRs against BoringSSL servers.)
Update-Note: This does enforce some client behavior that we hadn't been
enforcing previously. However, it only figures into TLS 1.3 (not many
implementations yet), and only clients which hit HelloRetryRequest
(rare), so this should be low risk.
Change-Id: I42126585ec0685d009542094192e674cbd22520d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/37124
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index ddb383c..132c9e0 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -72,6 +72,7 @@
SSL,155,HTTPS_PROXY_REQUEST
SSL,156,HTTP_REQUEST
SSL,157,INAPPROPRIATE_FALLBACK
+SSL,303,INCONSISTENT_CLIENT_HELLO
SSL,259,INVALID_ALPN_PROTOCOL
SSL,158,INVALID_COMMAND
SSL,256,INVALID_COMPRESSION_LIST
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 3d2bc07..679ee44 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -5035,6 +5035,7 @@
#define SSL_R_TOO_MUCH_READ_EARLY_DATA 300
#define SSL_R_INVALID_DELEGATED_CREDENTIAL 301
#define SSL_R_KEY_USAGE_BIT_INCORRECT 302
+#define SSL_R_INCONSISTENT_CLIENT_HELLO 303
#define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
#define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
#define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index a53e430..e1a506a 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -326,7 +326,7 @@
// Now that the length prefixes have been computed, fill in the placeholder
// PSK binder.
if (hs->needs_psk_binder &&
- !tls13_write_psk_binder(hs, msg.data(), msg.size())) {
+ !tls13_write_psk_binder(hs, MakeSpan(msg))) {
return false;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index b355c7f..580cf6e 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -596,9 +596,11 @@
// is released.
bool UpdateForHelloRetryRequest();
- // CopyHashContext copies the hash context into |ctx| and returns true on
- // success.
- bool CopyHashContext(EVP_MD_CTX *ctx);
+ // CopyToHashContext initializes |ctx| with |digest| and the data thus far in
+ // the transcript. It returns true on success and false on failure. If the
+ // handshake buffer is still present, |digest| may be any supported digest.
+ // Otherwise, |digest| must match the transcript hash.
+ bool CopyToHashContext(EVP_MD_CTX *ctx, const EVP_MD *digest);
Span<const uint8_t> buffer() {
return MakeConstSpan(reinterpret_cast<const uint8_t *>(buffer_->data),
@@ -1309,7 +1311,7 @@
// tls13_write_psk_binder calculates the PSK binder value and replaces the last
// bytes of |msg| with the resulting value. It returns true on success, and
// false on failure.
-bool tls13_write_psk_binder(SSL_HANDSHAKE *hs, uint8_t *msg, size_t len);
+bool tls13_write_psk_binder(SSL_HANDSHAKE *hs, Span<uint8_t> msg);
// tls13_verify_psk_binder verifies that the handshake transcript, truncated up
// to the binders has a valid signature using the value of |session|'s
@@ -1738,7 +1740,8 @@
CBS *contents);
bool ssl_ext_pre_shared_key_parse_clienthello(
SSL_HANDSHAKE *hs, CBS *out_ticket, CBS *out_binders,
- uint32_t *out_obfuscated_ticket_age, uint8_t *out_alert, CBS *contents);
+ uint32_t *out_obfuscated_ticket_age, uint8_t *out_alert,
+ const SSL_CLIENT_HELLO *client_hello, CBS *contents);
bool ssl_ext_pre_shared_key_add_serverhello(SSL_HANDSHAKE *hs, CBB *out);
// ssl_is_sct_list_valid does a shallow parse of the SCT list in |contents| and
diff --git a/ssl/ssl_transcript.cc b/ssl/ssl_transcript.cc
index 8bb513d..c1cef2b 100644
--- a/ssl/ssl_transcript.cc
+++ b/ssl/ssl_transcript.cc
@@ -137,6 +137,7 @@
#include <openssl/buf.h>
#include <openssl/digest.h>
+#include <openssl/err.h>
#include "internal.h"
@@ -205,8 +206,20 @@
return true;
}
-bool SSLTranscript::CopyHashContext(EVP_MD_CTX *ctx) {
- return EVP_MD_CTX_copy_ex(ctx, hash_.get());
+bool SSLTranscript::CopyToHashContext(EVP_MD_CTX *ctx, const EVP_MD *digest) {
+ const EVP_MD *transcript_digest = Digest();
+ if (transcript_digest != nullptr &&
+ EVP_MD_type(transcript_digest) == EVP_MD_type(digest)) {
+ return EVP_MD_CTX_copy_ex(ctx, hash_.get());
+ }
+
+ if (buffer_) {
+ return EVP_DigestInit_ex(ctx, digest, nullptr) &&
+ EVP_DigestUpdate(ctx, buffer_->data, buffer_->length);
+ }
+
+ OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+ return false;
}
bool SSLTranscript::Update(Span<const uint8_t> in) {
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index c1c41a8..52cea6c 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -1901,7 +1901,17 @@
bool ssl_ext_pre_shared_key_parse_clienthello(
SSL_HANDSHAKE *hs, CBS *out_ticket, CBS *out_binders,
- uint32_t *out_obfuscated_ticket_age, uint8_t *out_alert, CBS *contents) {
+ uint32_t *out_obfuscated_ticket_age, uint8_t *out_alert,
+ const SSL_CLIENT_HELLO *client_hello, CBS *contents) {
+ // Verify that the pre_shared_key extension is the last extension in
+ // ClientHello.
+ if (CBS_data(contents) + CBS_len(contents) !=
+ client_hello->extensions + client_hello->extensions_len) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_PRE_SHARED_KEY_MUST_BE_LAST);
+ *out_alert = SSL_AD_ILLEGAL_PARAMETER;
+ return false;
+ }
+
// We only process the first PSK identity since we don't support pure PSK.
CBS identities, binders;
if (!CBS_get_u16_length_prefixed(contents, &identities) ||
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index b56b9b3..d1cf757 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -1459,6 +1459,14 @@
// rejected. See draft-davidben-tls-grease-01.
ExpectGREASE bool
+ // OmitPSKsOnSecondClientHello, if true, causes the client to omit the
+ // PSK extension on the second ClientHello.
+ OmitPSKsOnSecondClientHello bool
+
+ // OnlyCorruptSecondPSKBinder, if true, causes the options below to
+ // only apply to the second PSK binder.
+ OnlyCorruptSecondPSKBinder bool
+
// SendShortPSKBinder, if true, causes the client to send a PSK binder
// that is one byte shorter than it should be.
SendShortPSKBinder bool
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 2574ec3..aa18ff3 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -125,7 +125,7 @@
srtpProtectionProfiles: c.config.SRTPProtectionProfiles,
srtpMasterKeyIdentifier: c.config.Bugs.SRTPMasterKeyIdentifer,
customExtension: c.config.Bugs.CustomExtension,
- pskBinderFirst: c.config.Bugs.PSKBinderFirst,
+ pskBinderFirst: c.config.Bugs.PSKBinderFirst && !c.config.Bugs.OnlyCorruptSecondPSKBinder,
omitExtensions: c.config.Bugs.OmitExtensions,
emptyExtensions: c.config.Bugs.EmptyExtensions,
delegatedCredentials: !c.config.Bugs.DisableDelegatedCredentials,
@@ -603,6 +603,12 @@
}
hello.hasEarlyData = c.config.Bugs.SendEarlyDataOnSecondClientHello
+ // The first ClientHello may have skipped this due to OnlyCorruptSecondPSKBinder.
+ hello.pskBinderFirst = c.config.Bugs.PSKBinderFirst
+ if c.config.Bugs.OmitPSKsOnSecondClientHello {
+ hello.pskIdentities = nil
+ hello.pskBinders = nil
+ }
hello.raw = nil
if len(hello.pskIdentities) > 0 {
@@ -1997,18 +2003,24 @@
}
func generatePSKBinders(version uint16, hello *clientHelloMsg, pskCipherSuite *cipherSuite, psk, firstClientHello, helloRetryRequest []byte, config *Config) {
- if config.Bugs.SendNoPSKBinder {
- return
- }
-
+ maybeCorruptBinder := !config.Bugs.OnlyCorruptSecondPSKBinder || len(firstClientHello) > 0
binderLen := pskCipherSuite.hash().Size()
- if config.Bugs.SendShortPSKBinder {
- binderLen--
- }
-
numBinders := 1
- if config.Bugs.SendExtraPSKBinder {
- numBinders++
+ if maybeCorruptBinder {
+ if config.Bugs.SendNoPSKBinder {
+ // The binders may have been set from the previous
+ // ClientHello.
+ hello.pskBinders = nil
+ return
+ }
+
+ if config.Bugs.SendShortPSKBinder {
+ binderLen--
+ }
+
+ if config.Bugs.SendExtraPSKBinder {
+ numBinders++
+ }
}
// Fill hello.pskBinders with appropriate length arrays of zeros so the
@@ -2023,11 +2035,13 @@
binderSize := len(hello.pskBinders)*(binderLen+1) + 2
truncatedHello := helloBytes[:len(helloBytes)-binderSize]
binder := computePSKBinder(psk, version, resumptionPSKBinderLabel, pskCipherSuite, firstClientHello, helloRetryRequest, truncatedHello)
- if config.Bugs.SendShortPSKBinder {
- binder = binder[:binderLen]
- }
- if config.Bugs.SendInvalidPSKBinder {
- binder[0] ^= 1
+ if maybeCorruptBinder {
+ if config.Bugs.SendShortPSKBinder {
+ binder = binder[:binderLen]
+ }
+ if config.Bugs.SendInvalidPSKBinder {
+ binder[0] ^= 1
+ }
}
for i := range hello.pskBinders {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 877a239..5a4b0cc 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -8033,94 +8033,132 @@
expectedError: ":OLD_SESSION_PRF_HASH_MISMATCH:",
})
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Resume-Server-BinderWrongLength",
- resumeSession: true,
- config: Config{
- MaxVersion: VersionTLS13,
- Bugs: ProtocolBugs{
- SendShortPSKBinder: true,
+ for _, secondBinder := range []bool{false, true} {
+ var suffix string
+ var defaultCurves []CurveID
+ if secondBinder {
+ suffix = "-SecondBinder"
+ // Force a HelloRetryRequest by predicting an empty curve list.
+ defaultCurves = []CurveID{}
+ }
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-BinderWrongLength" + suffix,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ DefaultCurves: defaultCurves,
+ Bugs: ProtocolBugs{
+ SendShortPSKBinder: true,
+ OnlyCorruptSecondPSKBinder: secondBinder,
+ },
},
- },
- shouldFail: true,
- expectedLocalError: "remote error: error decrypting message",
- expectedError: ":DIGEST_CHECK_FAILED:",
- })
+ shouldFail: true,
+ expectedLocalError: "remote error: error decrypting message",
+ expectedError: ":DIGEST_CHECK_FAILED:",
+ })
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-NoPSKBinder" + suffix,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ DefaultCurves: defaultCurves,
+ Bugs: ProtocolBugs{
+ SendNoPSKBinder: true,
+ OnlyCorruptSecondPSKBinder: secondBinder,
+ },
+ },
+ shouldFail: true,
+ expectedLocalError: "remote error: error decoding message",
+ expectedError: ":DECODE_ERROR:",
+ })
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-ExtraPSKBinder" + suffix,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ DefaultCurves: defaultCurves,
+ Bugs: ProtocolBugs{
+ SendExtraPSKBinder: true,
+ OnlyCorruptSecondPSKBinder: secondBinder,
+ },
+ },
+ shouldFail: true,
+ expectedLocalError: "remote error: illegal parameter",
+ expectedError: ":PSK_IDENTITY_BINDER_COUNT_MISMATCH:",
+ })
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-ExtraIdentityNoBinder" + suffix,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ DefaultCurves: defaultCurves,
+ Bugs: ProtocolBugs{
+ ExtraPSKIdentity: true,
+ OnlyCorruptSecondPSKBinder: secondBinder,
+ },
+ },
+ shouldFail: true,
+ expectedLocalError: "remote error: illegal parameter",
+ expectedError: ":PSK_IDENTITY_BINDER_COUNT_MISMATCH:",
+ })
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-InvalidPSKBinder" + suffix,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ DefaultCurves: defaultCurves,
+ Bugs: ProtocolBugs{
+ SendInvalidPSKBinder: true,
+ OnlyCorruptSecondPSKBinder: secondBinder,
+ },
+ },
+ shouldFail: true,
+ expectedLocalError: "remote error: error decrypting message",
+ expectedError: ":DIGEST_CHECK_FAILED:",
+ })
+
+ testCases = append(testCases, testCase{
+ testType: serverTest,
+ name: "Resume-Server-PSKBinderFirstExtension" + suffix,
+ resumeSession: true,
+ config: Config{
+ MaxVersion: VersionTLS13,
+ DefaultCurves: defaultCurves,
+ Bugs: ProtocolBugs{
+ PSKBinderFirst: true,
+ OnlyCorruptSecondPSKBinder: secondBinder,
+ },
+ },
+ shouldFail: true,
+ expectedLocalError: "remote error: illegal parameter",
+ expectedError: ":PRE_SHARED_KEY_MUST_BE_LAST:",
+ })
+ }
testCases = append(testCases, testCase{
testType: serverTest,
- name: "Resume-Server-NoPSKBinder",
+ name: "Resume-Server-OmitPSKsOnSecondClientHello",
resumeSession: true,
config: Config{
- MaxVersion: VersionTLS13,
+ MaxVersion: VersionTLS13,
+ DefaultCurves: []CurveID{},
Bugs: ProtocolBugs{
- SendNoPSKBinder: true,
- },
- },
- shouldFail: true,
- expectedLocalError: "remote error: error decoding message",
- expectedError: ":DECODE_ERROR:",
- })
-
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Resume-Server-ExtraPSKBinder",
- resumeSession: true,
- config: Config{
- MaxVersion: VersionTLS13,
- Bugs: ProtocolBugs{
- SendExtraPSKBinder: true,
+ OmitPSKsOnSecondClientHello: true,
},
},
shouldFail: true,
expectedLocalError: "remote error: illegal parameter",
- expectedError: ":PSK_IDENTITY_BINDER_COUNT_MISMATCH:",
- })
-
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Resume-Server-ExtraIdentityNoBinder",
- resumeSession: true,
- config: Config{
- MaxVersion: VersionTLS13,
- Bugs: ProtocolBugs{
- ExtraPSKIdentity: true,
- },
- },
- shouldFail: true,
- expectedLocalError: "remote error: illegal parameter",
- expectedError: ":PSK_IDENTITY_BINDER_COUNT_MISMATCH:",
- })
-
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Resume-Server-InvalidPSKBinder",
- resumeSession: true,
- config: Config{
- MaxVersion: VersionTLS13,
- Bugs: ProtocolBugs{
- SendInvalidPSKBinder: true,
- },
- },
- shouldFail: true,
- expectedLocalError: "remote error: error decrypting message",
- expectedError: ":DIGEST_CHECK_FAILED:",
- })
-
- testCases = append(testCases, testCase{
- testType: serverTest,
- name: "Resume-Server-PSKBinderFirstExtension",
- resumeSession: true,
- config: Config{
- MaxVersion: VersionTLS13,
- Bugs: ProtocolBugs{
- PSKBinderFirst: true,
- },
- },
- shouldFail: true,
- expectedLocalError: "remote error: illegal parameter",
- expectedError: ":PRE_SHARED_KEY_MUST_BE_LAST:",
+ expectedError: ":INCONSISTENT_CLIENT_HELLO:",
})
}
diff --git a/ssl/tls13_enc.cc b/ssl/tls13_enc.cc
index b6a402f..7a98128 100644
--- a/ssl/tls13_enc.cc
+++ b/ssl/tls13_enc.cc
@@ -376,7 +376,7 @@
static bool tls13_verify_data(const EVP_MD *digest, uint16_t version,
uint8_t *out, size_t *out_len,
const uint8_t *secret, size_t hash_len,
- uint8_t *context, size_t context_len) {
+ const uint8_t *context, size_t context_len) {
uint8_t key[EVP_MAX_MD_SIZE];
unsigned len;
if (!hkdf_expand_label(key, digest, secret, hash_len, kTLS13LabelFinished,
@@ -454,9 +454,9 @@
static const char kTLS13LabelPSKBinder[] = "res binder";
static bool tls13_psk_binder(uint8_t *out, uint16_t version,
- const EVP_MD *digest, uint8_t *psk, size_t psk_len,
- uint8_t *context, size_t context_len,
- size_t hash_len) {
+ const EVP_MD *digest, const uint8_t *psk,
+ size_t psk_len, const uint8_t *context,
+ size_t context_len, size_t hash_len) {
uint8_t binder_context[EVP_MAX_MD_SIZE];
unsigned binder_context_len;
if (!EVP_Digest(NULL, 0, binder_context, &binder_context_len, digest, NULL)) {
@@ -465,14 +465,14 @@
uint8_t early_secret[EVP_MAX_MD_SIZE] = {0};
size_t early_secret_len;
- if (!HKDF_extract(early_secret, &early_secret_len, digest, psk, hash_len,
- NULL, 0)) {
+ if (!HKDF_extract(early_secret, &early_secret_len, digest, psk, psk_len, NULL,
+ 0)) {
return false;
}
uint8_t binder_key[EVP_MAX_MD_SIZE] = {0};
size_t len;
- if (!hkdf_expand_label(binder_key, digest, early_secret, hash_len,
+ if (!hkdf_expand_label(binder_key, digest, early_secret, early_secret_len,
kTLS13LabelPSKBinder, strlen(kTLS13LabelPSKBinder),
binder_context, binder_context_len, hash_len) ||
!tls13_verify_data(digest, version, out, &len, binder_key, hash_len,
@@ -480,66 +480,65 @@
return false;
}
+ assert(len == hash_len);
return true;
}
-bool tls13_write_psk_binder(SSL_HANDSHAKE *hs, uint8_t *msg, size_t len) {
+static bool hash_transcript_and_truncated_client_hello(
+ SSL_HANDSHAKE *hs, uint8_t *out, size_t *out_len, const EVP_MD *digest,
+ Span<const uint8_t> client_hello, size_t binders_len) {
+ // Truncate the ClientHello.
+ if (binders_len + 2 < binders_len || client_hello.size() < binders_len + 2) {
+ return false;
+ }
+ client_hello = client_hello.subspan(0, client_hello.size() - binders_len - 2);
+
+ ScopedEVP_MD_CTX ctx;
+ unsigned len;
+ if (!hs->transcript.CopyToHashContext(ctx.get(), digest) ||
+ !EVP_DigestUpdate(ctx.get(), client_hello.data(), client_hello.size()) ||
+ !EVP_DigestFinal_ex(ctx.get(), out, &len)) {
+ return false;
+ }
+
+ *out_len = len;
+ return true;
+}
+
+bool tls13_write_psk_binder(SSL_HANDSHAKE *hs, Span<uint8_t> msg) {
SSL *const ssl = hs->ssl;
const EVP_MD *digest = ssl_session_get_digest(ssl->session.get());
size_t hash_len = EVP_MD_size(digest);
- if (len < hash_len + 3) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return false;
- }
-
ScopedEVP_MD_CTX ctx;
uint8_t context[EVP_MAX_MD_SIZE];
- unsigned context_len;
-
- if (!EVP_DigestInit_ex(ctx.get(), digest, NULL) ||
- !EVP_DigestUpdate(ctx.get(), hs->transcript.buffer().data(),
- hs->transcript.buffer().size()) ||
- !EVP_DigestUpdate(ctx.get(), msg, len - hash_len - 3) ||
- !EVP_DigestFinal_ex(ctx.get(), context, &context_len)) {
- return false;
- }
-
+ size_t context_len;
uint8_t verify_data[EVP_MAX_MD_SIZE] = {0};
- if (!tls13_psk_binder(verify_data, ssl->session->ssl_version, digest,
+ if (!hash_transcript_and_truncated_client_hello(
+ hs, context, &context_len, digest, msg,
+ 1 /* length prefix */ + hash_len) ||
+ !tls13_psk_binder(verify_data, ssl->session->ssl_version, digest,
ssl->session->master_key,
ssl->session->master_key_length, context, context_len,
hash_len)) {
return false;
}
- OPENSSL_memcpy(msg + len - hash_len, verify_data, hash_len);
+ OPENSSL_memcpy(msg.data() + msg.size() - hash_len, verify_data, hash_len);
return true;
}
bool tls13_verify_psk_binder(SSL_HANDSHAKE *hs, SSL_SESSION *session,
const SSLMessage &msg, CBS *binders) {
size_t hash_len = hs->transcript.DigestLen();
-
- // The message must be large enough to exclude the binders.
- if (CBS_len(&msg.raw) < CBS_len(binders) + 2) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return false;
- }
-
- // Hash a ClientHello prefix up to the binders. This includes the header. For
- // now, this assumes we only ever verify PSK binders on initial
- // ClientHellos.
uint8_t context[EVP_MAX_MD_SIZE];
- unsigned context_len;
- if (!EVP_Digest(CBS_data(&msg.raw), CBS_len(&msg.raw) - CBS_len(binders) - 2,
- context, &context_len, hs->transcript.Digest(), NULL)) {
- return false;
- }
-
+ size_t context_len;
uint8_t verify_data[EVP_MAX_MD_SIZE] = {0};
CBS binder;
- if (!tls13_psk_binder(verify_data, hs->ssl->version, hs->transcript.Digest(),
+ if (!hash_transcript_and_truncated_client_hello(hs, context, &context_len,
+ hs->transcript.Digest(),
+ msg.raw, CBS_len(binders)) ||
+ !tls13_psk_binder(verify_data, hs->ssl->version, hs->transcript.Digest(),
session->master_key, session->master_key_length,
context, context_len, hash_len) ||
// We only consider the first PSK, so compare against the first binder.
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc
index 6a00dfa..1e87bb9 100644
--- a/ssl/tls13_server.cc
+++ b/ssl/tls13_server.cc
@@ -241,10 +241,6 @@
return ssl_hs_error;
}
- if (!ssl_hash_message(hs, msg)) {
- return ssl_hs_error;
- }
-
hs->tls13_state = state_select_session;
return ssl_hs_ok;
}
@@ -263,20 +259,11 @@
return ssl_ticket_aead_ignore_ticket;
}
- // Verify that the pre_shared_key extension is the last extension in
- // ClientHello.
- if (CBS_data(&pre_shared_key) + CBS_len(&pre_shared_key) !=
- client_hello->extensions + client_hello->extensions_len) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_PRE_SHARED_KEY_MUST_BE_LAST);
- *out_alert = SSL_AD_ILLEGAL_PARAMETER;
- return ssl_ticket_aead_error;
- }
-
CBS ticket, binders;
uint32_t client_ticket_age;
- if (!ssl_ext_pre_shared_key_parse_clienthello(hs, &ticket, &binders,
- &client_ticket_age, out_alert,
- &pre_shared_key)) {
+ if (!ssl_ext_pre_shared_key_parse_clienthello(
+ hs, &ticket, &binders, &client_ticket_age, out_alert, client_hello,
+ &pre_shared_key)) {
return ssl_ticket_aead_error;
}
@@ -449,6 +436,10 @@
return ssl_hs_error;
}
+ if (!ssl_hash_message(hs, msg)) {
+ return ssl_hs_error;
+ }
+
if (ssl->s3->early_data_accepted) {
if (!tls13_derive_early_secrets(hs)) {
return ssl_hs_error;
@@ -532,6 +523,41 @@
return ssl_hs_error;
}
+ // We perform all our negotiation based on the first ClientHello (for
+ // consistency with what |select_certificate_cb| observed), which is in the
+ // transcript, so we can ignore most of this second one.
+ //
+ // We do, however, check the second PSK binder. This covers the client key
+ // share, in case we ever send half-RTT data (we currently do not). It is also
+ // a tricky computation, so we enforce the peer handled it correctly.
+ if (ssl->s3->session_reused) {
+ CBS pre_shared_key;
+ if (!ssl_client_hello_get_extension(&client_hello, &pre_shared_key,
+ TLSEXT_TYPE_pre_shared_key)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INCONSISTENT_CLIENT_HELLO);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return ssl_hs_error;
+ }
+
+ CBS ticket, binders;
+ uint32_t client_ticket_age;
+ uint8_t alert = SSL_AD_DECODE_ERROR;
+ if (!ssl_ext_pre_shared_key_parse_clienthello(
+ hs, &ticket, &binders, &client_ticket_age, &alert, &client_hello,
+ &pre_shared_key)) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
+ return ssl_hs_error;
+ }
+
+ // Note it is important that we do not obtain a new |SSL_SESSION| from
+ // |ticket|. We have already selected parameters based on the first
+ // ClientHello (in the transcript) and must not switch partway through.
+ if (!tls13_verify_psk_binder(hs, hs->new_session.get(), msg, &binders)) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECRYPT_ERROR);
+ return ssl_hs_error;
+ }
+ }
+
bool need_retry;
if (!resolve_ecdhe_secret(hs, &need_retry, &client_hello)) {
if (need_retry) {
