Commit c4f25ce0 authored by Adam Langley's avatar Adam Langley Committed by Adam Langley
Browse files

Work around yaSSL bug.

yaSSL has a couple of bugs in their DH client implementation. This
change works around the worst of the two.

Firstly, they expect the the DH public value to be the same length as
the prime. This change pads the public value as needed to ensure this.

Secondly, although they handle the first byte of the shared key being
zero, they don't handle the case of the second, third, etc bytes being
zero. So whenever that happens the handshake fails. I don't think that
there's anything that we can do about that one.

Change-Id: I789c9e5739f19449473305d59fe5c3fb9b4a6167
Reviewed-on: https://boringssl-review.googlesource.com/6578

Reviewed-by: default avatarDavid Benjamin <[email protected]>
Reviewed-by: default avatarAdam Langley <[email protected]>
parent c5eb4676
......@@ -1221,6 +1221,9 @@ int ssl3_send_server_key_exchange(SSL *s) {
int n;
CERT *cert;
BIGNUM *r[4];
/* r_pad_bytes[i] contains the number of zero padding bytes that need to
* precede |r[i]| when serialising it. */
unsigned r_pad_bytes[4] = {0};
int nr[4];
BUF_MEM *buf;
EVP_MD_CTX md_ctx;
......@@ -1292,6 +1295,10 @@ int ssl3_send_server_key_exchange(SSL *s) {
r[0] = dh->p;
r[1] = dh->g;
r[2] = dh->pub_key;
/* Due to a bug in yaSSL, the public key must be zero padded to the size
* of the prime. */
assert(BN_num_bytes(dh->pub_key) <= BN_num_bytes(dh->p));
r_pad_bytes[2] = BN_num_bytes(dh->p) - BN_num_bytes(dh->pub_key);
} else if (alg_k & SSL_kECDHE) {
/* Determine the curve to use. */
int nid = NID_undef;
......@@ -1378,7 +1385,7 @@ int ssl3_send_server_key_exchange(SSL *s) {
}
for (i = 0; i < 4 && r[i] != NULL; i++) {
nr[i] = BN_num_bytes(r[i]);
nr[i] = BN_num_bytes(r[i]) + r_pad_bytes[i];
n += 2 + nr[i];
}
......@@ -1390,7 +1397,10 @@ int ssl3_send_server_key_exchange(SSL *s) {
for (i = 0; i < 4 && r[i] != NULL; i++) {
s2n(nr[i], p);
BN_bn2bin(r[i], p);
if (!BN_bn2bin_padded(p, nr[i], r[i])) {
OPENSSL_PUT_ERROR(SSL, ERR_LIB_BN);
goto err;
}
p += nr[i];
}
......
......@@ -727,6 +727,24 @@ static ScopedSSL_CTX SetupCtx(const TestConfig *config) {
}
ScopedDH dh(DH_get_2048_256(NULL));
if (config->use_sparse_dh_prime) {
// This prime number is 2^1024 + 643 – a value just above a power of two.
// Because of its form, values modulo it are essentially certain to be one
// byte shorter. This is used to test padding of these values.
if (BN_hex2bn(
&dh->p,
"1000000000000000000000000000000000000000000000000000000000000000"
"0000000000000000000000000000000000000000000000000000000000000000"
"0000000000000000000000000000000000000000000000000000000000000000"
"0000000000000000000000000000000000000000000000000000000000000028"
"3") == 0 ||
!BN_set_word(dh->g, 2)) {
return nullptr;
}
dh->priv_length = 0;
}
if (!dh || !SSL_CTX_set_tmp_dh(ssl_ctx.get(), dh.get())) {
return nullptr;
}
......
......@@ -787,6 +787,11 @@ type ProtocolBugs struct {
// HelloRequest handshake message to be sent before each application
// data record. This only makes sense for a server.
SendHelloRequestBeforeEveryAppDataRecord bool
// RequireDHPublicValueLen causes a fatal error if the length (in
// bytes) of the server's Diffie-Hellman public value is not equal to
// this.
RequireDHPublicValueLen int
}
func (c *Config) serverInit() {
......
......@@ -15,6 +15,7 @@ import (
"crypto/x509"
"encoding/asn1"
"errors"
"fmt"
"io"
"math/big"
)
......@@ -652,6 +653,10 @@ func (ka *dheKeyAgreement) processServerKeyExchange(config *Config, clientHello
return errServerKeyExchange
}
if l := config.Bugs.RequireDHPublicValueLen; l != 0 && l != yLen {
return fmt.Errorf("RequireDHPublicValueLen set to %d, but server's public value was %d bytes on the wire and %d bytes if minimal", l, yLen, (ka.yTheirs.BitLen()+7)/8)
}
sig := k
serverDHParams := skx.key[:len(skx.key)-len(sig)]
......
......@@ -2185,6 +2185,20 @@ func addCipherSuiteTests() {
expectedError: ":DH_P_TOO_LONG:",
})
// This test ensures that Diffie-Hellman public values are padded with
// zeros so that they're the same length as the prime. This is to avoid
// hitting a bug in yaSSL.
testCases = append(testCases, testCase{
testType: serverTest,
name: "DHPublicValuePadded",
config: Config{
CipherSuites: []uint16{TLS_DHE_RSA_WITH_AES_128_GCM_SHA256},
Bugs: ProtocolBugs{
RequireDHPublicValueLen: (1025 + 7) / 8,
},
},
flags: []string{"-use-sparse-dh-prime"},
})
// versionSpecificCiphersTest specifies a test for the TLS 1.0 and TLS
// 1.1 specific cipher suite settings. A server is setup with the given
......
......@@ -99,6 +99,7 @@ const Flag<bool> kBoolFlags[] = {
{ "-renegotiate-ignore", &TestConfig::renegotiate_ignore },
{ "-disable-npn", &TestConfig::disable_npn },
{ "-p384-only", &TestConfig::p384_only },
{ "-use-sparse-dh-prime", &TestConfig::use_sparse_dh_prime },
};
const Flag<std::string> kStringFlags[] = {
......
......@@ -102,6 +102,7 @@ struct TestConfig {
bool disable_npn = false;
int expect_server_key_exchange_hash = 0;
bool p384_only = false;
bool use_sparse_dh_prime = false;
};
bool ParseConfig(int argc, char **argv, TestConfig *out_config);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment