Skip to content
GitLab
Projects
Groups
Snippets
/
Help
Help
Support
Community forum
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in
Toggle navigation
Menu
Open sidebar
David Goulet
Tor
Commits
7142f3e4
Commit
7142f3e4
authored
Jul 09, 2020
by
Nick Mathewson
🌻
Browse files
Merge branch 'trove_2020_001_035' into maint-0.3.5
parents
3e08dd9d
7b2d1070
Changes
4
Hide whitespace changes
Inline
Side-by-side
changes/bug33119
0 → 100644
View file @
7142f3e4
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
View file @
7142f3e4
...
...
@@ -713,23 +713,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
View file @
7142f3e4
...
...
@@ -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
View file @
7142f3e4
...
...
@@ -479,75 +479,6 @@ fake_x509_free(X509 *cert)
}
#endif
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
)
...
...
@@ -2279,7 +2210,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
),
...
...
Write
Preview
Supports
Markdown
0%
Try again
or
attach a new file
.
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment