Proposed security patch for xmltooling

Russ Allbery rra at debian.org
Tue Sep 22 21:01:45 UTC 2009


Here is what I currently have for xmltooling.  Scott, if you could look
this over when you get a chance and let me know if you think I got it all,
that would be great.

There were some changes that seemed to be related to UTF8 to UTF-8 naming
changes that I didn't pull up since I didn't think they were
security-related, but I'm a bit unsure on what patches went into the fix
for URL decoding, so I could have gotten that wrong.

diff --git a/debian/changelog b/debian/changelog
index abb1a3f..2f38c7c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,23 @@
+xmltooling (1.0-2+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>
+  * SECURITY: Correctly handle decoding of malformed URLs, closing a
+    possibly exploitable buffer overflow.
+    See <http://shibboleth.internet2.edu/secadv/secadv_20090826.txt>
+  * SECURITY: Correctly honor the "use" attribute of <KeyDescriptor> SAML
+    metadata to honor restrictions to signing or encryption.  This is a
+    partial fix; the complete fix also requires a new version of the
+    OpenSAML library.
+    See <http://shibboleth.internet2.edu/secadv/secadv_20090817a.txt>
+
+ -- Russ Allbery <rra at debian.org>  Thu, 17 Sep 2009 13:40:28 -0700
+
 xmltooling (1.0-2) unstable; urgency=low
 
   [ Ferenc Wagner ]
diff --git a/xmltooling/security/impl/AbstractPKIXTrustEngine.cpp b/xmltooling/security/impl/AbstractPKIXTrustEngine.cpp
index 2637cbb..c1845d9 100644
--- a/xmltooling/security/impl/AbstractPKIXTrustEngine.cpp
+++ b/xmltooling/security/impl/AbstractPKIXTrustEngine.cpp
@@ -1,5 +1,5 @@
 /*
- *  Copyright 2001-2007 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.
@@ -153,38 +153,37 @@ bool AbstractPKIXTrustEngine::checkEntityNames(
     for (vector<const Credential*>::const_iterator cred = creds.begin(); cred!=creds.end(); ++cred)
         trustednames.insert((*cred)->getKeyNames().begin(), (*cred)->getKeyNames().end());
 
-    char buf[256];
     X509_NAME* subject=X509_get_subject_name(certEE);
     if (subject) {
         // One way is a direct match to the subject DN.
         // 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;
+        X509_NAME_print_ex(b,subject,0,XN_FLAG_RFC2253);
         BIO_flush(b);
-        while ((len = BIO_read(b, buf, 255)) > 0) {
-            buf[len] = '\0';
-            subjectstr+=buf;
-        }
-        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);
+        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 (set<string>::const_iterator n=trustednames.begin(); n!=trustednames.end(); n++) {
+        for (set<string>::const_iterator n=trustednames.begin(); bptr && bptr2 && n!=trustednames.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());
                 BIO_free(b);
@@ -206,11 +205,11 @@ bool AbstractPKIXTrustEngine::checkEntityNames(
                     const int altlen = ASN1_STRING_length(check->d.ia5);
                     for (set<string>::const_iterator n=trustednames.begin(); n!=trustednames.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());
                             GENERAL_NAMES_free(altnames);
                             return true;
@@ -222,24 +221,52 @@ bool AbstractPKIXTrustEngine::checkEntityNames(
         GENERAL_NAMES_free(altnames);
             
         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 (set<string>::const_iterator n=trustednames.begin(); n!=trustednames.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());
+                    if(peer_CN)
+                        OPENSSL_free(peer_CN);
                     return true;
                 }
             }
+            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?!");
+    }
     
     return false;
 }
diff --git a/xmltooling/security/impl/InlineKeyResolver.cpp b/xmltooling/security/impl/InlineKeyResolver.cpp
index 57678c0..67d67cc 100644
--- a/xmltooling/security/impl/InlineKeyResolver.cpp
+++ b/xmltooling/security/impl/InlineKeyResolver.cpp
@@ -95,7 +95,7 @@ namespace xmltooling {
             return ret;
         }
         
-        const CredentialContext* getCredentialContext() const {
+        const CredentialContext* getCredentalContext() const {
             return m_credctx;
         }
 
diff --git a/xmltooling/util/URLEncoder.cpp b/xmltooling/util/URLEncoder.cpp
index 633a6f2..55a8187 100644
--- a/xmltooling/util/URLEncoder.cpp
+++ b/xmltooling/util/URLEncoder.cpp
@@ -1,5 +1,5 @@
 /*
- *  Copyright 2001-2007 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.
@@ -43,7 +43,7 @@ void URLEncoder::decode(char* s) const
 
     for(x=0,y=0;s[y];++x,++y)
     {
-        if((s[x] = s[y]) == '%')
+        if((s[x] = s[y]) == '%' && isxdigit(s[y+1]) && isxdigit(s[y+2]))
         {
             s[x] = x2c(&s[y+1]);
             y+=2;
diff --git a/xmltooling/util/URLEncoder.h b/xmltooling/util/URLEncoder.h
index d30f0f1..e662348 100644
--- a/xmltooling/util/URLEncoder.h
+++ b/xmltooling/util/URLEncoder.h
@@ -65,7 +65,7 @@ namespace xmltooling {
          * @return  true iff the character should be encoded 
          */
         virtual bool isBad(char ch) const {
-            static char badchars[]="=&/?:\"\\+<>#%{}|^~[]`;@";
+            static char badchars[]="=&/?:\"\\+<>#%{}|^~[],`;@";
             return (strchr(badchars,ch) || ch<=0x20 || ch>=0x7F);
         }
     };

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



More information about the Pkg-shibboleth-devel mailing list