Commit 0f39cc10 authored by Nick Mathewson's avatar Nick Mathewson 🤹
Browse files

Merge branch 'maint-0.4.3' into maint-0.4.4

parents 1af7f40d af08dad6

changes/bug33119

0 → 100644
+4 −0
Original line number Diff line number Diff line 
  o Major bugfixes (NSS):

    - Fix out-of-bound memory access in `tor_tls_cert_matches_key()` when Tor is

      compiled with NSS support. Fixes bug 33119; bugfix on 0.3.5.1-alpha. This

      issue is also tracked as TROVE-2020-001.

src/lib/tls/tortls_nss.c

+41 −6
Original line number Diff line number Diff line
@@ -726,23 +726,58 @@ MOCK_IMPL(int, 
tor_tls_cert_matches_key,(const tor_tls_t *tls,

                          const struct tor_x509_cert_t *cert))

{

  tor_assert(tls);

  tor_assert(cert);

  tor_assert(cert->cert);



  int rv = 0;



  CERTCertificate *peercert = SSL_PeerCertificate(tls->ssl);

  if (!peercert)

  tor_x509_cert_t *peercert = tor_tls_get_peer_cert((tor_tls_t *)tls);



  if (!peercert || !peercert->cert)

    goto done;

  CERTSubjectPublicKeyInfo *peer_info = &peercert->subjectPublicKeyInfo;



  CERTSubjectPublicKeyInfo *peer_info = &peercert->cert->subjectPublicKeyInfo;

  CERTSubjectPublicKeyInfo *cert_info = &cert->cert->subjectPublicKeyInfo;



  /* NSS stores the `len` field in bits, instead of bytes, for the

   * `subjectPublicKey` field in CERTSubjectPublicKeyInfo, but

   * `SECITEM_ItemsAreEqual()` compares the two bitstrings using a length field

   * defined in bytes.

   *

   * We convert the `len` field from bits to bytes, do our comparison with

   * `SECITEM_ItemsAreEqual()`, and reset the length field from bytes to bits

   * again.

   *

   * See also NSS's own implementation of `SECKEY_CopySubjectPublicKeyInfo()`

   * in seckey.c in the NSS source tree. This function also does the conversion

   * between bits and bytes.

   */

  const unsigned int peer_info_orig_len = peer_info->subjectPublicKey.len;

  const unsigned int cert_info_orig_len = cert_info->subjectPublicKey.len;



  /* We convert the length from bits to bytes, but instead of using NSS's

   * `DER_ConvertBitString()` macro on both of peer_info->subjectPublicKey and

   * cert_info->subjectPublicKey, we have to do the conversion explicitly since

   * both of the two subjectPublicKey fields are allowed to point to the same

   * memory address. Otherwise, the bits to bytes conversion would potentially

   * be applied twice, which would lead to us comparing too few of the bytes

   * when we call SECITEM_ItemsAreEqual(), which would be catastrophic.

   */

  peer_info->subjectPublicKey.len = ((peer_info_orig_len + 7) >> 3);

  cert_info->subjectPublicKey.len = ((cert_info_orig_len + 7) >> 3);



  rv = SECOID_CompareAlgorithmID(&peer_info->algorithm,

                                 &cert_info->algorithm) == 0 &&

       SECITEM_ItemsAreEqual(&peer_info->subjectPublicKey,

                             &cert_info->subjectPublicKey);



  /* Convert from bytes back to bits. */

  peer_info->subjectPublicKey.len = peer_info_orig_len;

  cert_info->subjectPublicKey.len = cert_info_orig_len;



 done:

  if (peercert)

    CERT_DestroyCertificate(peercert);

  tor_x509_cert_free(peercert);



  return rv;

}

src/test/test_tortls.c

+73 −0
Original line number Diff line number Diff line
@@ -105,6 +105,17 @@ const char* caCertString = "-----BEGIN CERTIFICATE-----\n" 
  "Yy1RT69d0rwYc5u/vnqODz1IjvT90smsrkBumGt791FAFeg=\n"

  "-----END CERTIFICATE-----\n";



static tor_x509_cert_t *fixed_x509_cert = NULL;

static tor_x509_cert_t *

get_peer_cert_mock_return_fixed(tor_tls_t *tls)

{

  (void)tls;

  if (fixed_x509_cert)

    return tor_x509_cert_dup(fixed_x509_cert);

  else

    return NULL;

}



tor_x509_cert_impl_t *

read_cert_from(const char *str)

{
@@ -513,6 +524,67 @@ test_tortls_verify(void *ignored) 
  crypto_pk_free(k);

}



static void

test_tortls_cert_matches_key(void *ignored)

{

  (void)ignored;



  tor_x509_cert_impl_t *cert1 = NULL,

                       *cert2 = NULL,

                       *cert3 = NULL,

                       *cert4 = NULL;

  tor_x509_cert_t *c1 = NULL, *c2 = NULL, *c3 = NULL, *c4 = NULL;

  crypto_pk_t *k1 = NULL, *k2 = NULL, *k3 = NULL;



  k1 = pk_generate(1);

  k2 = pk_generate(2);

  k3 = pk_generate(3);



  cert1 = tor_tls_create_certificate(k1, k2, "A", "B", 1000);

  cert2 = tor_tls_create_certificate(k1, k3, "C", "D", 1000);

  cert3 = tor_tls_create_certificate(k2, k3, "C", "D", 1000);

  cert4 = tor_tls_create_certificate(k3, k2, "E", "F", 1000);



  tt_assert(cert1 && cert2 && cert3 && cert4);



  c1 = tor_x509_cert_new(cert1); cert1 = NULL;

  c2 = tor_x509_cert_new(cert2); cert2 = NULL;

  c3 = tor_x509_cert_new(cert3); cert3 = NULL;

  c4 = tor_x509_cert_new(cert4); cert4 = NULL;



  tt_assert(c1 && c2 && c3 && c4);



  MOCK(tor_tls_get_peer_cert, get_peer_cert_mock_return_fixed);



  fixed_x509_cert = NULL;

  /* If the peer has no certificate, it shouldn't match anything. */

  tt_assert(! tor_tls_cert_matches_key(NULL, c1));

  tt_assert(! tor_tls_cert_matches_key(NULL, c2));

  tt_assert(! tor_tls_cert_matches_key(NULL, c3));

  tt_assert(! tor_tls_cert_matches_key(NULL, c4));

  fixed_x509_cert = c1;

  /* If the peer has a certificate, it should match every cert with the same

   * subject key. */

  tt_assert(tor_tls_cert_matches_key(NULL, c1));

  tt_assert(tor_tls_cert_matches_key(NULL, c2));

  tt_assert(! tor_tls_cert_matches_key(NULL, c3));

  tt_assert(! tor_tls_cert_matches_key(NULL, c4));



 done:

  tor_x509_cert_free(c1);

  tor_x509_cert_free(c2);

  tor_x509_cert_free(c3);

  tor_x509_cert_free(c4);

  if (cert1) tor_x509_cert_impl_free(cert1);

  if (cert2) tor_x509_cert_impl_free(cert2);

  if (cert3) tor_x509_cert_impl_free(cert3);

  if (cert4) tor_x509_cert_impl_free(cert4);

  crypto_pk_free(k1);

  crypto_pk_free(k2);

  crypto_pk_free(k3);

  UNMOCK(tor_tls_get_peer_cert);

}



#define LOCAL_TEST_CASE(name, flags)                            \

  { #name, test_tortls_##name, (flags|TT_FORK), NULL, NULL }
@@ -533,5 +605,6 @@ struct testcase_t tortls_tests[] = { 
  LOCAL_TEST_CASE(is_server, 0),

  LOCAL_TEST_CASE(bridge_init, TT_FORK),

  LOCAL_TEST_CASE(verify, TT_FORK),

  LOCAL_TEST_CASE(cert_matches_key, 0),

  END_OF_TESTCASES

};

src/test/test_tortls_openssl.c

+0 −70
Original line number Diff line number Diff line
@@ -475,75 +475,6 @@ fake_x509_free(X509 *cert) 
}

#endif /* !defined(OPENSSL_OPAQUE) */



static tor_x509_cert_t *fixed_x509_cert = NULL;

static tor_x509_cert_t *

get_peer_cert_mock_return_fixed(tor_tls_t *tls)

{

  (void)tls;

  if (fixed_x509_cert)

    return tor_x509_cert_dup(fixed_x509_cert);

  else

    return NULL;

}



static void

test_tortls_cert_matches_key(void *ignored)

{

  (void)ignored;



  X509 *cert1 = NULL, *cert2 = NULL, *cert3 = NULL, *cert4 = NULL;

  tor_x509_cert_t *c1 = NULL, *c2 = NULL, *c3 = NULL, *c4 = NULL;

  crypto_pk_t *k1 = NULL, *k2 = NULL, *k3 = NULL;



  k1 = pk_generate(1);

  k2 = pk_generate(2);

  k3 = pk_generate(3);



  cert1 = tor_tls_create_certificate(k1, k2, "A", "B", 1000);

  cert2 = tor_tls_create_certificate(k1, k3, "C", "D", 1000);

  cert3 = tor_tls_create_certificate(k2, k3, "C", "D", 1000);

  cert4 = tor_tls_create_certificate(k3, k2, "E", "F", 1000);



  tt_assert(cert1 && cert2 && cert3 && cert4);



  c1 = tor_x509_cert_new(cert1); cert1 = NULL;

  c2 = tor_x509_cert_new(cert2); cert2 = NULL;

  c3 = tor_x509_cert_new(cert3); cert3 = NULL;

  c4 = tor_x509_cert_new(cert4); cert4 = NULL;



  tt_assert(c1 && c2 && c3 && c4);



  MOCK(tor_tls_get_peer_cert, get_peer_cert_mock_return_fixed);



  fixed_x509_cert = NULL;

  /* If the peer has no certificate, it shouldn't match anything. */

  tt_assert(! tor_tls_cert_matches_key(NULL, c1));

  tt_assert(! tor_tls_cert_matches_key(NULL, c2));

  tt_assert(! tor_tls_cert_matches_key(NULL, c3));

  tt_assert(! tor_tls_cert_matches_key(NULL, c4));

  fixed_x509_cert = c1;

  /* If the peer has a certificate, it should match every cert with the same

   * subject key. */

  tt_assert(tor_tls_cert_matches_key(NULL, c1));

  tt_assert(tor_tls_cert_matches_key(NULL, c2));

  tt_assert(! tor_tls_cert_matches_key(NULL, c3));

  tt_assert(! tor_tls_cert_matches_key(NULL, c4));



 done:

  tor_x509_cert_free(c1);

  tor_x509_cert_free(c2);

  tor_x509_cert_free(c3);

  tor_x509_cert_free(c4);

  if (cert1) X509_free(cert1);

  if (cert2) X509_free(cert2);

  if (cert3) X509_free(cert3);

  if (cert4) X509_free(cert4);

  crypto_pk_free(k1);

  crypto_pk_free(k2);

  crypto_pk_free(k3);

  UNMOCK(tor_tls_get_peer_cert);

}



#ifndef OPENSSL_OPAQUE

static void

test_tortls_cert_get_key(void *ignored)
@@ -2275,7 +2206,6 @@ struct testcase_t tortls_openssl_tests[] = { 
  INTRUSIVE_TEST_CASE(get_error, TT_FORK),

  LOCAL_TEST_CASE(always_accept_verify_cb, 0),

  INTRUSIVE_TEST_CASE(x509_cert_free, 0),

  LOCAL_TEST_CASE(cert_matches_key, 0),

  INTRUSIVE_TEST_CASE(cert_get_key, 0),

  LOCAL_TEST_CASE(get_my_client_auth_key, TT_FORK),

  INTRUSIVE_TEST_CASE(get_ciphersuite_name, 0),