Skip to content

Commit

Permalink
Bug 1689228 - Minor ECH -09 fixes for interop testing, fuzzing. r=mt
Browse files Browse the repository at this point in the history 
A few minor ECH -09 fixes for interop testing and fuzzing:
- selfserv now takes a PKCS8 keypair for ECH. This is more maintainable and significantly
  less terrible than parsing the ECHConfigs and cobbling one together within selfserv
  (e.g. we can support other KEMs without modifying the server).
- Get rid of the newline character in tstclnt retry_configs output.
- Fuzzer fixes in tls13_HandleHrrCookie:
 - We shouldn't use internal_error when PK11_HPKE_ImportContext fails. Cookies are
   unprotected in fuzzer mode, so this can be expected to occur.
 - Only restore the application token when recovering hash state, otherwise the
   copy could happen twice, leaking one of the allocations.

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

--HG--
extra : moz-landing-system : lando
  • Loading branch information
Kevin Jacobs committed Jan 31, 2021
1 parent 722c105 commit 4df976d
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 41 deletions.
36 changes: 9 additions & 27 deletions cmd/selfserv/selfserv.c
View file
Expand Up @@ -1968,12 +1968,14 @@ configureEchWithData(PRFileDesc *model_sock)
{
/* The input should be a Base64-encoded ECHKey struct:
* struct {
* opaque sk<0..2^16-1>;
* ECHConfig config<0..2^16>; // draft-ietf-tls-esni-09
* opaque pkcs8_ech_keypair<0..2^16-1>;
* ECHConfigs configs<0..2^16>; // draft-ietf-tls-esni-09
* } ECHKey;
*
* This is not a standardized format, rather it's designed for
* interoperability with https://github.com/xvzcf/tls-interop-runner.
* It is the user's responsibility to ensure that the PKCS8 keypair
* corresponds to the public key embedded in the ECHConfigs.
*/

#define REMAINING_BYTES(rdr, buf) \
Expand All @@ -1984,9 +1986,9 @@ configureEchWithData(PRFileDesc *model_sock)
unsigned char *reader;
PK11SlotInfo *slot = NULL;
SECItem *decoded = NULL;
SECItem *pkcs8Key = NULL;
SECKEYPublicKey *pk = NULL;
SECKEYPrivateKey *sk = NULL;
SECItem pkcs8Key = { siBuffer, NULL, 0 };

decoded = NSSBase64_DecodeBuffer(NULL, NULL, echParamsStr, PORT_Strlen(echParamsStr));
if (!decoded || decoded->len < 2) {
Expand All @@ -2001,32 +2003,14 @@ configureEchWithData(PRFileDesc *model_sock)
errWarn("Bad ECHParams encoding");
goto loser;
}
/* Importing a raw KEM private key is generally awful,
* however since we only support X25519, we can hardcode
* all the OID data. */
const PRUint8 pkcs8Start[] = { 0x30, 0x67, 0x02, 0x01, 0x00, 0x30, 0x14, 0x06,
0x07, 0x2A, 0x86, 0x48, 0xCE, 0x3D, 0x02, 0x01,
0x06, 0x09, 0x2B, 0x06, 0x01, 0x04, 0x01, 0xDA,
0x47, 0x0F, 0x01, 0x04, 0x4C, 0x30, 0x4A, 0x02,
0x01, 0x01, 0x04, 0x20 };
const PRUint8 pkcs8End[] = { 0xA1, 0x23, 0x03, 0x21, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00 };
pkcs8Key = SECITEM_AllocItem(NULL, NULL, sizeof(pkcs8Start) + len + sizeof(pkcs8End));
if (!pkcs8Key) {
goto loser;
}
PORT_Memcpy(pkcs8Key->data, pkcs8Start, sizeof(pkcs8Start));
PORT_Memcpy(&pkcs8Key->data[sizeof(pkcs8Start)], reader, len);
PORT_Memcpy(&pkcs8Key->data[sizeof(pkcs8Start) + len], pkcs8End, sizeof(pkcs8End));
pkcs8Key.data = reader;
pkcs8Key.len = len;
reader += len;

/* Convert the key bytes to key handles */
slot = PK11_GetInternalKeySlot();
rv = PK11_ImportDERPrivateKeyInfoAndReturnKey(
slot, pkcs8Key, NULL, NULL, PR_FALSE, PR_FALSE, KU_ALL, &sk, NULL);
slot, &pkcs8Key, NULL, NULL, PR_FALSE, PR_FALSE, KU_ALL, &sk, NULL);
if (rv != SECSuccess || !sk) {
errWarn("ECH key import failed");
goto loser;
Expand All @@ -2037,7 +2021,7 @@ configureEchWithData(PRFileDesc *model_sock)
goto loser;
}

/* Remainder is the ECHConfig. */
/* Remainder is the ECHConfigs. */
rv = SSL_SetServerEchConfigs(model_sock, pk, sk, reader,
REMAINING_BYTES(reader, decoded));
if (rv != SECSuccess) {
Expand All @@ -2048,7 +2032,6 @@ configureEchWithData(PRFileDesc *model_sock)
PK11_FreeSlot(slot);
SECKEY_DestroyPrivateKey(sk);
SECKEY_DestroyPublicKey(pk);
SECITEM_FreeItem(pkcs8Key, PR_TRUE);
SECITEM_FreeItem(decoded, PR_TRUE);
return SECSuccess;
loser:
Expand All @@ -2057,7 +2040,6 @@ configureEchWithData(PRFileDesc *model_sock)
}
SECKEY_DestroyPrivateKey(sk);
SECKEY_DestroyPublicKey(pk);
SECITEM_FreeItem(pkcs8Key, PR_TRUE);
SECITEM_FreeItem(decoded, PR_TRUE);
return SECFailure;
}
Expand Down
5 changes: 5 additions & 0 deletions cmd/tstclnt/tstclnt.c
View file
Expand Up @@ -1279,6 +1279,11 @@ printEchRetryConfigs(PRFileDesc *s)
return SECFailure;
}

// Remove the newline characters that NSSBase64_EncodeItem unhelpfully inserts.
char *newline = strstr(retriesBase64, "\r\n");
if (newline) {
memmove(newline, newline + 2, strlen(newline + 2) + 1);
}
fprintf(stderr, "Received ECH retry_configs: \n%s\n", retriesBase64);
PORT_Free(retriesBase64);
SECITEM_FreeItem(&retries, PR_FALSE);
Expand Down
28 changes: 15 additions & 13 deletions lib/ssl/tls13hashstate.c
View file
Expand Up @@ -131,7 +131,8 @@ tls13_MakeHrrCookie(sslSocket *ss, const sslNamedGroupDef *selectedGroup,

/* Given a cookie and cookieLen, decrypt and parse, returning
* any values that were requested via the "previous_" params. If
* recoverHashState is true, the transcript state is recovered */
* recoverState is true, the transcript state and application
* token are restored. */
SECStatus
tls13_HandleHrrCookie(sslSocket *ss,
unsigned char *cookie, unsigned int cookieLen,
Expand All @@ -142,7 +143,7 @@ tls13_HandleHrrCookie(sslSocket *ss,
HpkeAeadId *previousEchAeadId,
SECItem *previousEchConfigId,
HpkeContext **previousEchHpkeCtx,
PRBool recoverHashState)
PRBool recoverState)
{
SECStatus rv;
unsigned char plaintext[1024];
Expand Down Expand Up @@ -217,29 +218,30 @@ tls13_HandleHrrCookie(sslSocket *ss,
}

/* Application token. */
PORT_Assert(ss->xtnData.applicationToken.len == 0);
rv = sslRead_ReadNumber(&reader, 2, &appTokenLen);
if (rv != SECSuccess) {
FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, illegal_parameter);
return SECFailure;
}
if (SECITEM_AllocItem(NULL, &ss->xtnData.applicationToken,
appTokenLen) == NULL) {
FATAL_ERROR(ss, PORT_GetError(), internal_error);
return SECFailure;
}
ss->xtnData.applicationToken.len = appTokenLen;
sslReadBuffer appTokenReader = { 0 };
rv = sslRead_Read(&reader, appTokenLen, &appTokenReader);
if (rv != SECSuccess) {
FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, illegal_parameter);
return SECFailure;
}
PORT_Assert(appTokenReader.len == appTokenLen);
PORT_Memcpy(ss->xtnData.applicationToken.data, appTokenReader.buf, appTokenLen);

/* The remainder is the hash. */
if (recoverHashState) {
if (recoverState) {
PORT_Assert(ss->xtnData.applicationToken.len == 0);
if (SECITEM_AllocItem(NULL, &ss->xtnData.applicationToken,
appTokenLen) == NULL) {
FATAL_ERROR(ss, PORT_GetError(), internal_error);
return SECFailure;
}
PORT_Memcpy(ss->xtnData.applicationToken.data, appTokenReader.buf, appTokenLen);
ss->xtnData.applicationToken.len = appTokenLen;

/* The remainder is the hash. */
unsigned int hashLen = SSL_READER_REMAINING(&reader);
if (hashLen != tls13_GetHashSize(ss)) {
FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_CLIENT_HELLO, illegal_parameter);
Expand Down Expand Up @@ -279,7 +281,7 @@ tls13_HandleHrrCookie(sslSocket *ss,
echHpkeBuf.len };
hpkeContext = PK11_HPKE_ImportContext(&hpkeItem, NULL);
if (!hpkeContext) {
FATAL_ERROR(ss, PORT_GetError(), internal_error);
FATAL_ERROR(ss, PORT_GetError(), illegal_parameter);
return SECFailure;
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/ssl/tls13hashstate.h
View file
Expand Up @@ -26,5 +26,5 @@ SECStatus tls13_HandleHrrCookie(sslSocket *ss,
HpkeAeadId *previousEchAeadId,
SECItem *previousEchConfigId,
HpkeContext **previousEchHpkeCtx,
PRBool recoverHashState);
PRBool recoverState);
#endif

0 comments on commit 4df976d

Please sign in to comment.