Skip to content

Commit

Permalink
Bug 1879954 - exempt errors after Server Hello from Xyber retry logic…
Browse files Browse the repository at this point in the history
…. r=keeler

Differential Revision: https://phabricator.services.mozilla.com/D201638
  • Loading branch information
jschanck committed Feb 15, 2024
1 parent c74c015 commit b54645e
Show file tree
Hide file tree
Showing 7 changed files with 89 additions and 38 deletions.
1 change: 1 addition & 0 deletions security/manager/ssl/NSSSocketControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ NSSSocketControl::NSSSocketControl(const nsCString& aHostName, int32_t aPort,
mNotedTimeUntilReady(false),
mEchExtensionStatus(EchExtensionStatus::kNotPresent),
mSentXyberShare(false),
mHasTls13HandshakeSecrets(false),
mIsShortWritePending(false),
mShortWritePendingByte(0),
mShortWriteOriginalAmount(-1),
Expand Down
11 changes: 11 additions & 0 deletions security/manager/ssl/NSSSocketControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,16 @@ class NSSSocketControl final : public CommonSocketControl {
return mSentXyberShare;
}

void SetHasTls13HandshakeSecrets() {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
mHasTls13HandshakeSecrets = true;
}

bool HasTls13HandshakeSecrets() {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
return mHasTls13HandshakeSecrets;
}

bool GetJoined() {
COMMON_SOCKET_CONTROL_ASSERT_ON_OWNING_THREAD();
return mJoined;
Expand Down Expand Up @@ -296,6 +306,7 @@ class NSSSocketControl final : public CommonSocketControl {
bool mNotedTimeUntilReady;
EchExtensionStatus mEchExtensionStatus; // Currently only used for telemetry.
bool mSentXyberShare;
bool mHasTls13HandshakeSecrets;

// True when SSL layer has indicated an "SSL short write", i.e. need
// to call on send one or more times to push all pending data to write.
Expand Down
13 changes: 13 additions & 0 deletions security/manager/ssl/nsNSSCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1137,3 +1137,16 @@ void HandshakeCallback(PRFileDesc* fd, void* client_data) {
infoObject->NoteTimeUntilReady();
infoObject->SetHandshakeCompleted();
}

void SecretCallback(PRFileDesc* fd, PRUint16 epoch, SSLSecretDirection dir,
PK11SymKey* secret, void* arg) {
// arg must be set to an NSSSocketControl* in SSL_SecretCallback
MOZ_ASSERT(arg);
NSSSocketControl* infoObject = (NSSSocketControl*)arg;
if (epoch == 2 && dir == ssl_secret_read) {
// |secret| is the server_handshake_traffic_secret. Set a flag to indicate
// that the Server Hello has been processed successfully. We use this when
// deciding whether to retry a connection in which a Xyber share was sent.
infoObject->SetHasTls13HandshakeSecrets();
}
}
2 changes: 2 additions & 0 deletions security/manager/ssl/nsNSSCallbacks.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ class nsILoadGroup;
char* PK11PasswordPrompt(PK11SlotInfo* slot, PRBool retry, void* arg);

void HandshakeCallback(PRFileDesc* fd, void* client_data);
void SecretCallback(PRFileDesc* fd, PRUint16 epoch, SSLSecretDirection dir,
PK11SymKey* secret, void* arg);
SECStatus CanFalseStartCallback(PRFileDesc* fd, void* client_data,
PRBool* canFalseStart);

Expand Down
5 changes: 4 additions & 1 deletion security/manager/ssl/nsNSSIOLayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ bool retryDueToTLSIntolerance(PRErrorCode err, NSSSocketControl* socketInfo) {
}

if (!socketInfo->IsPreliminaryHandshakeDone() &&
socketInfo->SentXyberShare()) {
!socketInfo->HasTls13HandshakeSecrets() && socketInfo->SentXyberShare()) {
nsAutoCString errorName;
const char* prErrorName = PR_ErrorToName(err);
if (prErrorName) {
Expand Down Expand Up @@ -1279,6 +1279,9 @@ static PRFileDesc* nsSSLIOLayerImportFD(PRFileDesc* fd,
SECSuccess) {
return nullptr;
}
if (SSL_SecretCallback(sslSock, SecretCallback, infoObject) != SECSuccess) {
return nullptr;
}
if (SSL_SetCanFalseStartCallback(sslSock, CanFalseStartCallback,
infoObject) != SECSuccess) {
return nullptr;
Expand Down
86 changes: 53 additions & 33 deletions security/manager/ssl/tests/unit/test_faulty_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,50 +73,70 @@ add_task(
skip_if: () => AppConstants.MOZ_SYSTEM_NSS,
},
async function testRetryXyber() {
const retryDomains = [
"xyber-net-interrupt.example.com",
"xyber-alert-unexpected.example.com",
];
const retryDomain = "xyber-net-interrupt.example.com";

Services.prefs.setBoolPref("security.tls.enable_kyber", true);
Services.prefs.setCharPref("network.dns.localDomains", retryDomains);
Services.prefs.setCharPref("network.dns.localDomains", [retryDomain]);
Services.prefs.setIntPref("network.http.speculative-parallel-limit", 0);

for (let i = 0; i < retryDomains.length; i++) {
// Get the number of xyber / x25519 callbacks prior to making the request
// ssl_grp_kem_xyber768d00 = 25497
// ssl_grp_ec_curve25519 = 29
let countOfXyber = handlerCount("/callback/25497");
let countOfX25519 = handlerCount("/callback/29");
let chan = makeChan(`https://${retryDomains[i]}:8443`);
let [, buf] = await channelOpenPromise(chan, CL_ALLOW_UNKNOWN_CL);
ok(buf);
// The server will make a xyber768d00 callback for the initial request, and
// then an x25519 callback for the retry. Both callback counts should
// increment by one.
equal(
handlerCount("/callback/25497"),
countOfXyber + 1,
"negotiated xyber768d00"
);
equal(
handlerCount("/callback/29"),
countOfX25519 + 1,
"negotiated x25519"
);
}
// Get the number of xyber / x25519 callbacks prior to making the request
// ssl_grp_kem_xyber768d00 = 25497
// ssl_grp_ec_curve25519 = 29
let countOfXyber = handlerCount("/callback/25497");
let countOfX25519 = handlerCount("/callback/29");
let chan = makeChan(`https://${retryDomain}:8443`);
let [, buf] = await channelOpenPromise(chan, CL_ALLOW_UNKNOWN_CL);
ok(buf);
// The server will make a xyber768d00 callback for the initial request, and
// then an x25519 callback for the retry. Both callback counts should
// increment by one.
equal(
handlerCount("/callback/25497"),
countOfXyber + 1,
"negotiated xyber768d00"
);
equal(handlerCount("/callback/29"), countOfX25519 + 1, "negotiated x25519");
if (!mozinfo.socketprocess_networking) {
// Bug 1824574
equal(
1,
await Glean.tls.xyberIntoleranceReason.PR_END_OF_FILE_ERROR.testGetValue(),
"PR_END_OF_FILE_ERROR telemetry accumulated"
);
equal(
1,
await Glean.tls.xyberIntoleranceReason.SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE.testGetValue(),
"SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE telemetry accumulated"
);
}
}
);

add_task(
{
skip_if: () => AppConstants.MOZ_SYSTEM_NSS,
},
async function testNoRetryXyber() {
const retryDomain = "xyber-alert-after-server-hello.example.com";

Services.prefs.setBoolPref("security.tls.enable_kyber", true);
Services.prefs.setCharPref("network.dns.localDomains", [retryDomain]);
Services.prefs.setIntPref("network.http.speculative-parallel-limit", 0);

// Get the number of xyber / x25519 / p256 callbacks prior to making the request
// ssl_grp_kem_xyber768d00 = 25497
// ssl_grp_ec_curve25519 = 29
let countOfXyber = handlerCount("/callback/25497");
let countOfX25519 = handlerCount("/callback/29");
let chan = makeChan(`https://${retryDomain}:8443`);
let [req] = await channelOpenPromise(chan, CL_EXPECT_FAILURE);
equal(req.status, 0x805a2f4d); // psm::GetXPCOMFromNSSError(SSL_ERROR_HANDSHAKE_FAILED)
// The server will make a xyber768d00 callback for the initial request and
// the client should not retry.
equal(
handlerCount("/callback/25497"),
countOfXyber + 1,
"negotiated xyber768d00"
);
equal(
handlerCount("/callback/29"),
countOfX25519,
"did not negotiate x25519"
);
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ const char* kHostZeroRttAlertUnexpected = "0rtt-alert-unexpected.example.com";
const char* kHostZeroRttAlertDowngrade = "0rtt-alert-downgrade.example.com";

const char* kHostXyberNetInterrupt = "xyber-net-interrupt.example.com";
const char* kHostXyberAlertUnexpected = "xyber-alert-unexpected.example.com";
const char* kHostXyberAlertAfterServerHello =
"xyber-alert-after-server-hello.example.com";

const char* kCertWildcard = "default-ee";

Expand All @@ -55,7 +56,7 @@ const FaultyServerHost sFaultyServerHosts[]{
{kHostZeroRttAlertUnexpected, kCertWildcard, ZeroRtt},
{kHostZeroRttAlertDowngrade, kCertWildcard, ZeroRtt},
{kHostXyberNetInterrupt, kCertWildcard, Xyber},
{kHostXyberAlertUnexpected, kCertWildcard, Xyber},
{kHostXyberAlertAfterServerHello, kCertWildcard, Xyber},
{nullptr, nullptr},
};

Expand Down Expand Up @@ -199,8 +200,8 @@ void SecretCallbackFailXyber(PRFileDesc* fd, PRUint16 epoch,
// The client will see this as a PR_END_OF_FILE / NS_ERROR_NET_INTERRUPT
// error.
ss->recordWriteCallback = FailingWriteCallback;
} else if (!strcmp(host->mHostName, kHostXyberAlertUnexpected)) {
SSL3_SendAlert(ss, alert_fatal, no_alert);
} else if (!strcmp(host->mHostName, kHostXyberAlertAfterServerHello)) {
SSL3_SendAlert(ss, alert_fatal, close_notify);
}
}
}
Expand Down

0 comments on commit b54645e

Please sign in to comment.