Proposed security fixes for Shibboleth 1.x (lenny)
Russ Allbery
rra at debian.org
Wed Sep 23 02:20:36 UTC 2009
Here is the diff for opensaml and shibboleth-sp for lenny. I also
backported the same fixes to the etch versions, which required manually
applying the patch for certificate naming. Hopefully I didn't break
anything.
I'm in the weird position of being able to test the etch fixes (at least
moderately; I haven't actually configured a server with PKIX verification,
so I'm not actually testing the certificate name matching, but I can
confirm the software runs) but not being able to test the lenny fixes
since we don't seem to have a lenny Shibboleth test environment already
set up. I'm going to push everything I have to the Alioth repositories in
case anyone else on this list is in a better position to test.
Here's the (small) opensaml patch:
diff --git a/debian/changelog b/debian/changelog
index 0d85a09..f471d39 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+opensaml (1.1.1-2+lenny1) UNRELEASED; urgency=high
+
+ * SECURITY: Correctly handle decoding of malformed URLs, closing a
+ possibly exploitable buffer overflow.
+ See <http://shibboleth.internet2.edu/secadv/secadv_20090826.txt>
+
+ -- Russ Allbery <rra at debian.org> Tue, 22 Sep 2009 16:13:40 -0700
+
opensaml (1.1.1-2) unstable; urgency=low
* Link the library against libdl since it uses dlopen and friends
diff --git a/saml/SAMLBrowserProfile.cpp b/saml/SAMLBrowserProfile.cpp
index 8aff9f2..3acf5d0 100644
--- a/saml/SAMLBrowserProfile.cpp
+++ b/saml/SAMLBrowserProfile.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright 2001-2005 Internet2
+ * Copyright 2001-2009 Internet2
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -511,7 +511,7 @@ BrowserProfile::CgiParse::url_decode(char *url)
for(x=0,y=0;url[y];++x,++y)
{
- if((url[x] = url[y]) == '%')
+ if((url[x] = url[y]) == '%' && isxdigit(url[y+1]) && isxdigit(url[y+2]))
{
url[x] = x2c(&url[y+1]);
y+=2;
and here is the more substantial shibboleth-sp patch:
diff --git a/debian/changelog b/debian/changelog
index 3643118..74c595b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,15 @@
+shibboleth-sp (1.3.1.dfsg1-3+lenny1) UNRELEASED; urgency=high
+
+ * SECURITY: Certificate subject names were incorrectly matched against
+ trusted "key names" when they contained nul characters. This affects
+ only Shibboleth deployments relying on the "PKIX" style of trust
+ validation, used in the absence of explicit certificate information in
+ the SAML metadata provided to the SP and reliance on certificate
+ authorities found in the <KeyAuthority> metadata extension element.
+ See <http://shibboleth.internet2.edu/secadv/secadv_20090817.txt>
+
+ -- Russ Allbery <rra at debian.org> Tue, 22 Sep 2009 16:18:11 -0700
+
shibboleth-sp (1.3.1.dfsg1-3) unstable; urgency=low
* Unlink the correct Apache configuration on package removal.
diff --git a/shib/ShibbolethTrust.cpp b/shib/ShibbolethTrust.cpp
index a032960..05f4709 100644
--- a/shib/ShibbolethTrust.cpp
+++ b/shib/ShibbolethTrust.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright 2001-2005 Internet2
+ * Copyright 2001-2009 Internet2
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -295,7 +295,6 @@ bool ShibbolethTrust::validate(void* certEE, const Iterator<void*>& certChain, c
auto_ptr<char> kn(toUTF8(role->getEntityDescriptor()->getId()));
keynames.push_back(kn.get());
- char buf[256];
X509* x=(X509*)certEE;
X509_NAME* subject=X509_get_subject_name(x);
if (subject) {
@@ -303,32 +302,31 @@ bool ShibbolethTrust::validate(void* certEE, const Iterator<void*>& certChain, c
// Seems that the way to do the compare is to write the X509_NAME into a BIO.
BIO* b = BIO_new(BIO_s_mem());
BIO* b2 = BIO_new(BIO_s_mem());
- BIO_set_mem_eof_return(b, 0);
- BIO_set_mem_eof_return(b2, 0);
// The flags give us LDAP order instead of X.500, with a comma separator.
int len=X509_NAME_print_ex(b,subject,0,XN_FLAG_RFC2253);
- string subjectstr,subjectstr2;
BIO_flush(b);
- while ((len = BIO_read(b, buf, 255)) > 0) {
- buf[len] = '\0';
- subjectstr+=buf;
- }
- if (log.isDebugEnabled())
- log.debugStream() << "certificate subject: " << subjectstr << logging::eol;
// The flags give us LDAP order instead of X.500, with a comma plus space separator.
len=X509_NAME_print_ex(b2,subject,0,XN_FLAG_RFC2253 + XN_FLAG_SEP_CPLUS_SPC - XN_FLAG_SEP_COMMA_PLUS);
BIO_flush(b2);
- while ((len = BIO_read(b2, buf, 255)) > 0) {
- buf[len] = '\0';
- subjectstr2+=buf;
+
+ BUF_MEM* bptr=NULL;
+ BUF_MEM* bptr2=NULL;
+ BIO_get_mem_ptr(b, &bptr);
+ BIO_get_mem_ptr(b2, &bptr2);
+
+ if (bptr && bptr->length > 0 && log.isDebugEnabled()) {
+ string subjectstr(bptr->data, bptr->length);
+ log.debug("certificate subject: %s", subjectstr.c_str());
}
// Check each keyname.
for (vector<string>::const_iterator n=keynames.begin(); n!=keynames.end(); n++) {
#ifdef HAVE_STRCASECMP
- if (!strcasecmp(n->c_str(),subjectstr.c_str()) || !strcasecmp(n->c_str(),subjectstr2.c_str())) {
+ if ((n->length() == bptr->length && !strncasecmp(n->c_str(), bptr->data, bptr->length)) ||
+ (n->length() == bptr2->length && !strncasecmp(n->c_str(), bptr2->data, bptr2->length))) {
#else
- if (!stricmp(n->c_str(),subjectstr.c_str()) || !stricmp(n->c_str(),subjectstr2.c_str())) {
+ if ((n->length() == bptr->length && !strnicmp(n->c_str(), bptr->data, bptr->length)) ||
+ (n->length() == bptr2->length && !strnicmp(n->c_str(), bptr2->data, bptr2->length))) {
#endif
log.debug("matched full subject DN to a key name (%s)", n->c_str());
checkName=false;
@@ -348,14 +346,13 @@ bool ShibbolethTrust::validate(void* certEE, const Iterator<void*>& certChain, c
if (check->type==GEN_DNS || check->type==GEN_URI) {
const char* altptr = (char*)ASN1_STRING_data(check->d.ia5);
const int altlen = ASN1_STRING_length(check->d.ia5);
-
for (vector<string>::const_iterator n=keynames.begin(); n!=keynames.end(); n++) {
#ifdef HAVE_STRCASECMP
- if ((check->type==GEN_DNS && !strncasecmp(altptr,n->c_str(),altlen))
+ if ((check->type==GEN_DNS && n->length()==altlen && !strncasecmp(altptr,n->c_str(),altlen))
#else
- if ((check->type==GEN_DNS && !strnicmp(altptr,n->c_str(),altlen))
+ if ((check->type==GEN_DNS && n->length()==altlen && !strnicmp(altptr,n->c_str(),altlen))
#endif
- || (check->type==GEN_URI && !strncmp(altptr,n->c_str(),altlen))) {
+ || (check->type==GEN_URI && n->length()==altlen && !strncmp(altptr,n->c_str(),altlen))) {
log.debug("matched DNS/URI subjectAltName to a key name (%s)", n->c_str());
checkName=false;
break;
@@ -368,27 +365,53 @@ bool ShibbolethTrust::validate(void* certEE, const Iterator<void*>& certChain, c
if (checkName) {
log.debug("unable to match subjectAltName, trying TLS CN match");
- memset(buf,0,sizeof(buf));
- if (X509_NAME_get_text_by_NID(subject,NID_commonName,buf,255)>0) {
+
+ // Fetch the last CN RDN.
+ char* peer_CN = NULL;
+ int j,i = -1;
+ while ((j=X509_NAME_get_index_by_NID(subject, NID_commonName, i)) >= 0)
+ i = j;
+ if (i >= 0) {
+ ASN1_STRING* tmp = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject, i));
+ // Copied in from libcurl.
+ /* In OpenSSL 0.9.7d and earlier, ASN1_STRING_to_UTF8 fails if the input
+ is already UTF-8 encoded. We check for this case and copy the raw
+ string manually to avoid the problem. */
+ if(tmp && ASN1_STRING_type(tmp) == V_ASN1_UTF8STRING) {
+ j = ASN1_STRING_length(tmp);
+ if(j >= 0) {
+ peer_CN = (char*)OPENSSL_malloc(j + 1);
+ memcpy(peer_CN, ASN1_STRING_data(tmp), j);
+ peer_CN[j] = '\0';
+ }
+ }
+ else /* not a UTF8 name */ {
+ j = ASN1_STRING_to_UTF8(reinterpret_cast<unsigned char**>(&peer_CN), tmp);
+ }
+
for (vector<string>::const_iterator n=keynames.begin(); n!=keynames.end(); n++) {
#ifdef HAVE_STRCASECMP
- if (!strcasecmp(buf,n->c_str())) {
+ if (n->length() == j && !strncasecmp(peer_CN, n->c_str(), j)) {
#else
- if (!stricmp(buf,n->c_str())) {
+ if (n->length() == j && !strnicmp(peer_CN, n->c_str(), j)) {
#endif
log.debug("matched subject CN to a key name (%s)", n->c_str());
checkName=false;
break;
}
}
+ if(peer_CN)
+ OPENSSL_free(peer_CN);
}
- else
+ else {
log.warn("no common name in certificate subject");
+ }
}
}
}
- else
+ else {
log.error("certificate has no subject?!");
+ }
}
if (checkName) {
diff --git a/xmlproviders/XMLTrust.cpp b/xmlproviders/XMLTrust.cpp
index 2618118..a45a429 100644
--- a/xmlproviders/XMLTrust.cpp
+++ b/xmlproviders/XMLTrust.cpp
@@ -1,5 +1,5 @@
/*
- * Copyright 2001-2005 Internet2
+ * Copyright 2001-2009 Internet2
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -454,7 +454,6 @@ bool XMLTrust::validate(void* certEE, const Iterator<void*>& certChain, const IR
auto_ptr<char> kn(toUTF8(role->getEntityDescriptor()->getId()));
keynames.push_back(kn.get());
- char buf[256];
X509* x=(X509*)certEE;
X509_NAME* subject=X509_get_subject_name(x);
if (subject) {
@@ -462,32 +461,31 @@ bool XMLTrust::validate(void* certEE, const Iterator<void*>& certChain, const IR
// Seems that the way to do the compare is to write the X509_NAME into a BIO.
BIO* b = BIO_new(BIO_s_mem());
BIO* b2 = BIO_new(BIO_s_mem());
- BIO_set_mem_eof_return(b, 0);
- BIO_set_mem_eof_return(b2, 0);
// The flags give us LDAP order instead of X.500, with a comma separator.
int len=X509_NAME_print_ex(b,subject,0,XN_FLAG_RFC2253);
- string subjectstr,subjectstr2;
BIO_flush(b);
- while ((len = BIO_read(b, buf, 255)) > 0) {
- buf[len] = '\0';
- subjectstr+=buf;
- }
- if (log.isDebugEnabled())
- log.debugStream() << "certificate subject: " << subjectstr << xmlproviders::logging::eol;
// The flags give us LDAP order instead of X.500, with a comma plus space separator.
len=X509_NAME_print_ex(b2,subject,0,XN_FLAG_RFC2253 + XN_FLAG_SEP_CPLUS_SPC - XN_FLAG_SEP_COMMA_PLUS);
BIO_flush(b2);
- while ((len = BIO_read(b2, buf, 255)) > 0) {
- buf[len] = '\0';
- subjectstr2+=buf;
+
+ BUF_MEM* bptr=NULL;
+ BUF_MEM* bptr2=NULL;
+ BIO_get_mem_ptr(b, &bptr);
+ BIO_get_mem_ptr(b2, &bptr2);
+
+ if (bptr && bptr->length > 0 && log.isDebugEnabled()) {
+ string subjectstr(bptr->data, bptr->length);
+ log.debug("certificate subject: %s", subjectstr.c_str());
}
// Check each keyname.
for (vector<string>::const_iterator n=keynames.begin(); n!=keynames.end(); n++) {
#ifdef HAVE_STRCASECMP
- if (!strcasecmp(n->c_str(),subjectstr.c_str()) || !strcasecmp(n->c_str(),subjectstr2.c_str())) {
+ if ((n->length() == bptr->length && !strncasecmp(n->c_str(), bptr->data, bptr->length)) ||
+ (n->length() == bptr2->length && !strncasecmp(n->c_str(), bptr2->data, bptr2->length))) {
#else
- if (!stricmp(n->c_str(),subjectstr.c_str()) || !stricmp(n->c_str(),subjectstr2.c_str())) {
+ if ((n->length() == bptr->length && !strnicmp(n->c_str(), bptr->data, bptr->length)) ||
+ (n->length() == bptr2->length && !strnicmp(n->c_str(), bptr2->data, bptr2->length))) {
#endif
log.debug("matched full subject DN to a key name (%s)", n->c_str());
checkName=false;
@@ -502,18 +500,18 @@ bool XMLTrust::validate(void* certEE, const Iterator<void*>& certChain, const IR
STACK_OF(GENERAL_NAME)* altnames=(STACK_OF(GENERAL_NAME)*)X509_get_ext_d2i(x, NID_subject_alt_name, NULL, NULL);
if (altnames) {
int numalts = sk_GENERAL_NAME_num(altnames);
- for (int an=0; !checkName && an<numalts; an++) {
+ for (int an=0; checkName && an<numalts; an++) {
const GENERAL_NAME* check = sk_GENERAL_NAME_value(altnames, an);
if (check->type==GEN_DNS || check->type==GEN_URI) {
const char* altptr = (char*)ASN1_STRING_data(check->d.ia5);
const int altlen = ASN1_STRING_length(check->d.ia5);
-
for (vector<string>::const_iterator n=keynames.begin(); n!=keynames.end(); n++) {
#ifdef HAVE_STRCASECMP
- if (!strncasecmp(altptr,n->c_str(),altlen)) {
+ if ((check->type==GEN_DNS && n->length()==altlen && !strncasecmp(altptr,n->c_str(),altlen))
#else
- if (!strnicmp(altptr,n->c_str(),altlen)) {
+ if ((check->type==GEN_DNS && n->length()==altlen && !strnicmp(altptr,n->c_str(),altlen))
#endif
+ || (check->type==GEN_URI && n->length()==altlen && !strncmp(altptr,n->c_str(),altlen))) {
log.debug("matched DNS/URI subjectAltName to a key name (%s)", n->c_str());
checkName=false;
break;
@@ -526,27 +524,53 @@ bool XMLTrust::validate(void* certEE, const Iterator<void*>& certChain, const IR
if (checkName) {
log.debug("unable to match subjectAltName, trying TLS CN match");
- memset(buf,0,sizeof(buf));
- if (X509_NAME_get_text_by_NID(subject,NID_commonName,buf,255)>0) {
+
+ // Fetch the last CN RDN.
+ char* peer_CN = NULL;
+ int j,i = -1;
+ while ((j=X509_NAME_get_index_by_NID(subject, NID_commonName, i)) >= 0)
+ i = j;
+ if (i >= 0) {
+ ASN1_STRING* tmp = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(subject, i));
+ // Copied in from libcurl.
+ /* In OpenSSL 0.9.7d and earlier, ASN1_STRING_to_UTF8 fails if the input
+ is already UTF-8 encoded. We check for this case and copy the raw
+ string manually to avoid the problem. */
+ if(tmp && ASN1_STRING_type(tmp) == V_ASN1_UTF8STRING) {
+ j = ASN1_STRING_length(tmp);
+ if(j >= 0) {
+ peer_CN = (char*)OPENSSL_malloc(j + 1);
+ memcpy(peer_CN, ASN1_STRING_data(tmp), j);
+ peer_CN[j] = '\0';
+ }
+ }
+ else /* not a UTF8 name */ {
+ j = ASN1_STRING_to_UTF8(reinterpret_cast<unsigned char**>(&peer_CN), tmp);
+ }
+
for (vector<string>::const_iterator n=keynames.begin(); n!=keynames.end(); n++) {
#ifdef HAVE_STRCASECMP
- if (!strcasecmp(buf,n->c_str())) {
+ if (n->length() == j && !strncasecmp(peer_CN, n->c_str(), j)) {
#else
- if (!stricmp(buf,n->c_str())) {
+ if (n->length() == j && !strnicmp(peer_CN, n->c_str(), j)) {
#endif
log.debug("matched subject CN to a key name (%s)", n->c_str());
checkName=false;
break;
}
}
+ if(peer_CN)
+ OPENSSL_free(peer_CN);
}
- else
+ else {
log.warn("no common name in certificate subject");
+ }
}
}
}
- else
+ else {
log.error("certificate has no subject?!");
+ }
}
if (checkName) {
--
Russ Allbery (rra at debian.org) <http://www.eyrie.org/~eagle/>
More information about the Pkg-shibboleth-devel
mailing list