Proposed security fixes for Shibboleth 1.x (lenny)

Russ Allbery rra at debian.org
Thu Sep 24 23:01:56 UTC 2009


"Scott Cantor" <cantor.2 at osu.edu> writes:
> Scott Cantor wrote on 2009-09-23:

>> I think you're missing a set of changes to one of the source files in
>> shib-target (possibly shib-target.cpp?) with a bunch of copies of the
>> URL fix. The code's a mess. It's in one spot in the new version, but
>> not here.

> Correcting myself, I think there was also a copy in adfs/adfs.cpp

> Did I say it was a mess?

> I went and looked for the rev, it's here:
> http://svn.middleware.georgetown.edu/view/cpp-sp?view=rev&revision=3110

Thank you!  I did indeed miss those -- you'd warned that it was in
multiple places, but I saw the SSL cert fix in multiple places and forgot
to check for the URL encoding fix.  I did a grep through the source tree,
just to double-check, and can't find any others.

Here's the new proposed patch set.

diff -u opensaml-1.1.1/debian/changelog opensaml-1.1.1/debian/changelog
--- opensaml-1.1.1/debian/changelog
+++ opensaml-1.1.1/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
only in patch2:
unchanged:
--- opensaml-1.1.1.orig/saml/SAMLBrowserProfile.cpp
+++ opensaml-1.1.1/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 @@
 
     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;

diff -u shibboleth-sp-1.3.1.dfsg1/debian/changelog shibboleth-sp-1.3.1.dfsg1/debian/changelog
--- shibboleth-sp-1.3.1.dfsg1/debian/changelog
+++ shibboleth-sp-1.3.1.dfsg1/debian/changelog
@@ -1,3 +1,18 @@
+shibboleth-sp (1.3.1.dfsg1-3+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>
+  * 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 -u shibboleth-sp-1.3.1.dfsg1/xmlproviders/XMLTrust.cpp shibboleth-sp-1.3.1.dfsg1/xmlproviders/XMLTrust.cpp
--- shibboleth-sp-1.3.1.dfsg1/xmlproviders/XMLTrust.cpp
+++ shibboleth-sp-1.3.1.dfsg1/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 @@
         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 @@
             // 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 @@
                 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 @@
                 
                 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 -u shibboleth-sp-1.3.1.dfsg1/shib/ShibbolethTrust.cpp shibboleth-sp-1.3.1.dfsg1/shib/ShibbolethTrust.cpp
--- shibboleth-sp-1.3.1.dfsg1/shib/ShibbolethTrust.cpp
+++ shibboleth-sp-1.3.1.dfsg1/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 @@
         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 @@
             // 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 @@
                         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 @@
                 
                 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) {
only in patch2:
unchanged:
--- shibboleth-sp-1.3.1.dfsg1.orig/adfs/adfs.cpp
+++ shibboleth-sp-1.3.1.dfsg1/adfs/adfs.cpp
@@ -241,7 +241,7 @@
 
     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;
only in patch2:
unchanged:
--- shibboleth-sp-1.3.1.dfsg1.orig/shib-target/shib-handlers.cpp
+++ shibboleth-sp-1.3.1.dfsg1/shib-target/shib-handlers.cpp
@@ -522,7 +522,7 @@
 
     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;


-- 
Russ Allbery (rra at debian.org)               <http://www.eyrie.org/~eagle/>



More information about the Pkg-shibboleth-devel mailing list