Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

align X509_VERIFY_PARAM host and email behavior with OpenSSL #1062

Merged
merged 1 commit into from
Jun 28, 2023
Merged

align X509_VERIFY_PARAM host and email behavior with OpenSSL #1062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions crypto/x509/x509_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1730,21 +1730,20 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) {
test.incorrect_value_len));
}));

// Passing zero as the length, unlike OpenSSL, should trigger an error and
// should cause verification to fail.
ASSERT_EQ(X509_V_ERR_INVALID_CALL,
// AWS-LC supports passing zero as the length for host and email for
// backwards compatibility with OpenSSL.
ASSERT_EQ(X509_V_OK,
Verify(leaf.get(), {root.get()}, {}, empty_crls, 0,
[&test](X509_VERIFY_PARAM *param) {
ASSERT_FALSE(test.func(param, test.correct_value, 0));
ASSERT_TRUE(test.func(param, test.correct_value, 0));
}));

// Passing an empty value should be an error when setting and should cause
// verification to fail.
ASSERT_EQ(X509_V_ERR_INVALID_CALL,
Verify(leaf.get(), {root.get()}, {}, empty_crls, 0,
[&test](X509_VERIFY_PARAM *param) {
ASSERT_FALSE(test.func(param, nullptr, 0));
}));
// AWS-LC allows an empty value with zero as the length for backwards
// compatibility with OpenSSL.
ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {}, empty_crls, 0,
[&test](X509_VERIFY_PARAM *param) {
ASSERT_TRUE(test.func(param, nullptr, 0));
}));

// Passing a value with embedded NULs should also be an error and should
// also cause verification to fail.
Expand All @@ -1755,6 +1754,16 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) {
}));
}

// |X509_VERIFY_PARAM_set1_host| has additional strange behavior.

// AWS-LC/OpenSSL allows an empty value with a non-zero length for backwards
// compatibility with OpenSSL. We do not recommend this behavior.
ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {}, empty_crls, 0,
[](X509_VERIFY_PARAM *param) {
ASSERT_TRUE(X509_VERIFY_PARAM_set1_host(
param, nullptr, strlen(kHostname)));
}));

// IP addresses work slightly differently:

// The correct value should still work.
Expand Down
68 changes: 59 additions & 9 deletions crypto/x509/x509_vpm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,11 @@ static int int_x509_param_set_hosts(X509_VERIFY_PARAM *param, int mode,
const char *name, size_t namelen) {
char *copy;

if (name == NULL || namelen == 0) {
// Unlike OpenSSL, we reject trying to set or add an empty name.
return 0;
// Setting 0 to automatically detect the length of |name| is an OpenSSL quirk
// that AWS-LC isn't keen on supporting. However, consumers often assume
// OpenSSL semantics from AWS-LC, so it's supported in this case.
if (name != NULL && namelen == 0) {
samuel40791765 marked this conversation as resolved.
Show resolved Hide resolved
namelen = strlen(name);
}

// Refuse names with embedded NUL bytes.
Expand All @@ -95,6 +97,12 @@ static int int_x509_param_set_hosts(X509_VERIFY_PARAM *param, int mode,
string_stack_free(param->hosts);
param->hosts = NULL;
}
// OpenSSL returns 1 when trying to set or add an empty name. This is also a
// quirk that AWS-LC isn't keen on supporting, but we maintain for backwards
// compatibility.
if (name == NULL || namelen == 0) {
return 1;
}

copy = OPENSSL_strndup(name, namelen);
if (copy == NULL) {
Expand Down Expand Up @@ -313,8 +321,50 @@ int X509_VERIFY_PARAM_set1(X509_VERIFY_PARAM *to,
return ret;
}

static int int_x509_param_set1(char **pdest, size_t *pdestlen, const char *src,
size_t srclen) {
static int int_x509_param_set1_email(char **pdest, size_t *pdestlen,
const char *src, size_t srclen) {
void *tmp;
if (src != NULL) {
// Setting |srclen| to 0 to automatically detect the length of |src| is an
// OpenSSL quirk that AWS-LC isn't keen on supporting. However, consumers
// often assume OpenSSL semantics from AWS-LC, so it's supported in this
// case.
if (srclen == 0) {
srclen = strlen(src);
}
andrewhop marked this conversation as resolved.
Show resolved Hide resolved

tmp = OPENSSL_memdup(src, srclen);
if (tmp == NULL) {
return 0;
}
} else {
// This allows an empty string to disable previously configured checks.
// This is an OpenSSL quirk that AWS-LC isn't keen on supporting. However,
// consumers often assume OpenSSL semantics from AWS-LC, so it's supported
// in this case.
tmp = NULL;
srclen = 0;
}

if (*pdest != NULL) {
OPENSSL_free(*pdest);
}
*pdest = tmp;
if (pdestlen != NULL) {
*pdestlen = srclen;
}
return 1;
}

// IP addresses work slightly differently, so we use another function to
// differentiate from emails. |X509_VERIFY_PARAM_set1_ip| takes a const
// unsigned char*, instead of a const char*, so the same strlen logic that was
// being used is not quite suitable here.
// We keep the original behavior that BoringSSL left, but only for IP addresses.
// We can align the behavior with |int_x509_param_set1_email| like OpenSSL has
// been doing if needed.
andrewhop marked this conversation as resolved.
Show resolved Hide resolved
static int int_x509_param_set1_ip(unsigned char **pdest, size_t *pdestlen,
const unsigned char *src, size_t srclen) {
void *tmp;
if (src == NULL || srclen == 0) {
// Unlike OpenSSL, we do not allow an empty string to disable previously
Expand Down Expand Up @@ -455,7 +505,8 @@ char *X509_VERIFY_PARAM_get0_peername(X509_VERIFY_PARAM *param) {
int X509_VERIFY_PARAM_set1_email(X509_VERIFY_PARAM *param, const char *email,
size_t emaillen) {
if (OPENSSL_memchr(email, '\0', emaillen) != NULL ||
!int_x509_param_set1(&param->email, &param->emaillen, email, emaillen)) {
!int_x509_param_set1_email(&param->email, &param->emaillen, email,
emaillen)) {
param->poison = 1;
return 0;
}
Expand All @@ -465,9 +516,8 @@ int X509_VERIFY_PARAM_set1_email(X509_VERIFY_PARAM *param, const char *email,

int X509_VERIFY_PARAM_set1_ip(X509_VERIFY_PARAM *param, const unsigned char *ip,
size_t iplen) {
if ((iplen != 4 && iplen != 16) ||
!int_x509_param_set1((char **)&param->ip, &param->iplen, (char *)ip,
iplen)) {
if ((iplen != 0 && iplen != 4 && iplen != 16) ||
!int_x509_param_set1_ip(&param->ip, &param->iplen, ip, iplen)) {
param->poison = 1;
return 0;
}
Expand Down
32 changes: 32 additions & 0 deletions include/openssl/x509.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2923,21 +2923,53 @@ OPENSSL_EXPORT int X509_VERIFY_PARAM_add0_policy(X509_VERIFY_PARAM *param,
OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_policies(
X509_VERIFY_PARAM *param, const STACK_OF(ASN1_OBJECT) *policies);


// X509_VERIFY_PARAM_set1_host sets the expected DNS hostname to |name| and
// clears any previously specified hostname. If |name| is NULL or empty, the
// list of hostnames is cleared and name checks are not performed on the peer
// certificate.
// |namelen| should be set to the length of |name|. It may be zero if |name| is
// NUL-terminated, but this is only maintained for backwards compatibility with
// OpenSSL.
OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_host(X509_VERIFY_PARAM *param,
const char *name,
size_t namelen);

// X509_VERIFY_PARAM_add1_host adds |name| as an additional DNS hostname
// reference identifier that can match the peer's certificate.
// Any previous names set via |X509_VERIFY_PARAM_set1_host| or
// |X509_VERIFY_PARAM_add1_host| are retained, no change is made if |name| is
// NULL or empty. When multiple names are configured, the peer is considered
// verified when any name matches.
// |namelen| should be set to the length of |name|. It may be zero if |name| is
// NUL-terminated, but this is only maintained for backwards compatibility with
// OpenSSL.
OPENSSL_EXPORT int X509_VERIFY_PARAM_add1_host(X509_VERIFY_PARAM *param,
const char *name,
size_t namelen);
OPENSSL_EXPORT void X509_VERIFY_PARAM_set_hostflags(X509_VERIFY_PARAM *param,
unsigned int flags);
OPENSSL_EXPORT char *X509_VERIFY_PARAM_get0_peername(X509_VERIFY_PARAM *);

// X509_VERIFY_PARAM_set1_email sets the expected RFC822 email address to
// |email|.
// |emaillen| should be set to the length of |email|. It may be zero if |email|
// is NUL-terminated, but this is only maintained for backwards compatibility
// with OpenSSL.
OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_email(X509_VERIFY_PARAM *param,
const char *email,
size_t emaillen);

// X509_VERIFY_PARAM_set1_ip sets the expected IP address to |ip|. |iplen| MUST
// be set to the length of |email|.
OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_ip(X509_VERIFY_PARAM *param,
const unsigned char *ip,
size_t iplen);

// X509_VERIFY_PARAM_set1_ip_asc sets the expected IP address to |ipasc|.
// |ipasc| MUST be a NUL-terminal ASCII string: dotted decimal quad for IPv4 and
// colon-separated hexadecimal for IPv6. The condensed "::" notation is
// supported for IPv6 addresses.
OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_ip_asc(X509_VERIFY_PARAM *param,
const char *ipasc);

Expand Down