From b852b51e57eceb79a306b6b58ac333eb8c151eb2 Mon Sep 17 00:00:00 2001
From: roadrunner2 <ronald@innovation.ch>
Date: Sun, 1 Oct 2017 13:10:14 -0700
Subject: [PATCH] subject alt name parsing fixes (#140)

* Fix formatting of IPv6 addresses.

They now correctly show as 1234:abcd:... instead of 18::52::171::205::...

* Fix subjectAltName parsing to always generate GeneralNames.

It now handles arbitrary lists of names and always produces a GeneralNames
as required by the definition. Additionally, leading and trailing whitespace
around separators and labels is properly removed.

This fixes #134.
---
 .../org/jruby/ext/openssl/X509Extension.java  | 15 +++--
 .../ext/openssl/X509ExtensionFactory.java     | 67 +++++++++----------
 src/test/ruby/x509/test_x509ext.rb            | 38 +++++++++--
 3 files changed, 72 insertions(+), 48 deletions(-)

diff --git a/src/main/java/org/jruby/ext/openssl/X509Extension.java b/src/main/java/org/jruby/ext/openssl/X509Extension.java
index 3c9c3fa2..5f8bf8c8 100644
--- a/src/main/java/org/jruby/ext/openssl/X509Extension.java
+++ b/src/main/java/org/jruby/ext/openssl/X509Extension.java
@@ -661,11 +661,16 @@ private static boolean formatGeneralName(final GeneralName name, final ByteList
                 append(':');
             final byte[] ip = ((ASN1OctetString) name.getName()).getOctets();
             int len = ip.length; boolean ip4 = len == 4;
-            for ( int i = 0; i < ip.length; i++ ) {
-                out.append( ConvertBytes.intToCharBytes( ((int) ip[i]) & 0xff ) );
-                if ( i != len - 1 ) {
-                    if ( ip4 ) out.append('.');
-                    else out.append(':').append(':');
+            if ( ip4 ) {
+                for ( int i = 0; i < ip.length; i++ ) {
+                    out.append( ConvertBytes.intToCharBytes( ((int) ip[i]) & 0xff ) );
+                    if ( i != len - 1 ) out.append('.');
+                }
+            }
+            else {
+                for ( int i = 0; i < ip.length; i += 2 ) {
+                    out.append( ConvertBytes.intToHexBytes( ((ip[i] & 0xff) << 8 | (ip[i+1] & 0xff)) ) );
+                    if ( i != len - 2 ) out.append(':');
                 }
             }
             break;
diff --git a/src/main/java/org/jruby/ext/openssl/X509ExtensionFactory.java b/src/main/java/org/jruby/ext/openssl/X509ExtensionFactory.java
index cf691704..52459cd8 100644
--- a/src/main/java/org/jruby/ext/openssl/X509ExtensionFactory.java
+++ b/src/main/java/org/jruby/ext/openssl/X509ExtensionFactory.java
@@ -490,69 +490,62 @@ private ASN1Encodable parseIssuerAltName(final ThreadContext context, final Stri
 
     private static final String DNS_ = "DNS:";
     private static final String DNS_Name_ = "DNS Name:";
+    private static final String IP_ = "IP:";
+    private static final String IP_Address_ = "IP Address:";
     private static final String URI_ = "URI:";
     private static final String RID_ = "RID:";
     private static final String email_ = "email:";
     private static final String dirName_ = "dirName:";
     private static final String otherName_ = "otherName:";
 
-    private static ASN1Encodable parseSubjectAltName(final String valuex) throws IOException {
+    private static GeneralNames parseSubjectAltName(final String valuex) throws IOException {
+        final String[] vals = valuex.split("(?<!(^|[^\\\\])((\\\\\\\\)?\\\\\\\\)?\\\\),");   // allow up to three levels of escaping of ','
+        final GeneralName[] names = new GeneralName[vals.length];
+        for ( int i = 0; i < vals.length; i++ ) {
+            names[i] = parseGeneralName(vals[i].replaceAll("\\\\([,\\\\])", "$1").trim());
+        }
+        return new GeneralNames(names);
+    }
+
+    private static GeneralName parseGeneralName(final String valuex) throws IOException {
         if ( valuex.startsWith(DNS_) ) {
-            final String[] vals = valuex.split(",");
-            final GeneralName[] names = new GeneralName[vals.length];
-            for ( int i = 0; i < vals.length; i++ ) {
-                final String dns = vals[i].substring(DNS_.length());
-                names[i] = new GeneralName(GeneralName.dNSName, dns);
-            }
-            return new GeneralNames(names);
+            final String dns = valuex.substring(DNS_.length()).trim();
+            return new GeneralName(GeneralName.dNSName, dns);
         }
         if ( valuex.startsWith(DNS_Name_) ) {
-            final String dns = valuex.substring(DNS_Name_.length());
+            final String dns = valuex.substring(DNS_Name_.length()).trim();
             return new GeneralName(GeneralName.dNSName, dns);
         }
         if ( valuex.startsWith(URI_) ) {
-            final String uri = valuex.substring(URI_.length());
+            final String uri = valuex.substring(URI_.length()).trim();
             return new GeneralName(GeneralName.uniformResourceIdentifier, uri);
         }
         if ( valuex.startsWith(RID_) ) {
-            final String rid = valuex.substring(RID_.length());
+            final String rid = valuex.substring(RID_.length()).trim();
             return new GeneralName(GeneralName.registeredID, rid);
         }
         if ( valuex.startsWith(email_) ) {
-            final String[] vals = valuex.split(",");
-            final GeneralName[] names = new GeneralName[vals.length];
-            for ( int i = 0; i < vals.length; i++ ) {
-                if (vals[i].startsWith(email_)) {
-                    String mail = vals[i].substring(email_.length());
-                    names[i] = new GeneralName(GeneralName.rfc822Name, mail);
-                }
-                else {
-                    ASN1Encodable name = parseSubjectAltName(vals[i]);
-                    names[i] = name instanceof GeneralNames ? ((GeneralNames) name).getNames()[0] : (GeneralName) name;
-                }
-            }
-            return new GeneralNames(names);
-        }
-        if ( valuex.startsWith("IP:") || valuex.startsWith("IP Address:") ) {
-            final int idx = valuex.charAt(2) == ':' ? 3 : 11;
-            String[] vals = valuex.substring(idx).split("\\.|::");
-            final byte[] ip = new byte[vals.length];
-            for ( int i = 0; i < vals.length; i++ ) {
-                ip[i] = (byte) (Integer.parseInt(vals[i]) & 0xff);
-            }
-            return new GeneralName(GeneralName.iPAddress, new DEROctetString(ip));
+            String mail = valuex.substring(email_.length()).trim();
+            return new GeneralName(GeneralName.rfc822Name, mail);
+        }
+        if ( valuex.startsWith(IP_) ) {
+            final String ip = valuex.substring(IP_.length()).trim();
+            return new GeneralName(GeneralName.iPAddress, ip);
+        }
+        if ( valuex.startsWith(IP_Address_) ) {
+            final String ip = valuex.substring(IP_Address_.length()).trim();
+            return new GeneralName(GeneralName.iPAddress, ip);
         }
         if ( valuex.startsWith("other") ) { // otherName || othername
-            final String other = valuex.substring(otherName_.length());
+            final String other = valuex.substring(otherName_.length()).trim();
             return new GeneralName(GeneralName.otherName, other);
         }
         if ( valuex.startsWith("dir") ) { // dirName || dirname
-            final String dir = valuex.substring(dirName_.length());
+            final String dir = valuex.substring(dirName_.length()).trim();
             return new GeneralName(GeneralName.directoryName, dir);
         }
 
-        throw new IOException("could not parse SubjectAltName: " + valuex);
-
+        throw new IOException("could not parse SubjectAltName part: " + valuex);
     }
 
     private DEROctetString parseSubjectKeyIdentifier(final ThreadContext context, final String oid, final String valuex) {
diff --git a/src/test/ruby/x509/test_x509ext.rb b/src/test/ruby/x509/test_x509ext.rb
index 8b1812af..10f48d01 100644
--- a/src/test/ruby/x509/test_x509ext.rb
+++ b/src/test/ruby/x509/test_x509ext.rb
@@ -151,12 +151,38 @@ def test_subject_alt_name_sign_to_pem
   end
 
   def test_subject_alt_name_sequence
+    tests = [
+        {
+            :input => "email:foo@bar.com,DNS:a.b.com,email:baz@bar.com",
+            :output => "email:foo@bar.com, DNS:a.b.com, email:baz@bar.com",
+            :der => "0,\x06\x03U\x1D\x11\x04%0#\x81\vfoo@bar.com\x82\aa.b.com\x81\vbaz@bar.com",
+        },
+        {
+            :input => "DNS:a.b.com, email:foo@bar.com",
+            :der => "0\x1f\x06\x03U\x1d\x11\x04\x180\x16\x82\x07a.b.com\x81\x0bfoo@bar.com",
+        },
+        {
+            :input => "URI:https://a.b.com/, DNS:a.b.com",
+            :der => "0$\x06\x03U\x1d\x11\x04\x1d0\x1b\x86\x10https://a.b.com/\x82\x07a.b.com",
+        },
+        {
+            :input => "IP:1.2.3.4,IP: fe80::12:345:5678, email:foo@bar.com, dirName: CN=John Doe+CN=Doe\\\\\\, John\\,O=Acme",
+            :output => "IP:1.2.3.4, IP:fe80:0:0:0:0:12:345:5678, email:foo@bar.com, DirName:CN=John Doe+CN=Doe\\, John,O=Acme",
+            :der => "0f\x06\x03U\x1d\x11\x04_0]\x87\x04\x01\x02\x03\x04\x87\x10\xfe\x80\x00\x00\x00\x00\x00\x00\x00\x00\x00\x12\x03EVx\x81\x0bfoo@bar.com\xa46041#0\x0f\x06\x03U\x04\x03\x0c\x08John Doe0\x10\x06\x03U\x04\x03\x0c\x09Doe, John1\x0d0\x0b\x06\x03U\x04\x0a\x0c\x04Acme"
+        },
+        {
+            :input => "RID:1.3.6.1.3.100.200",
+            :der => "0\x12\x06\x03U\x1d\x11\x04\x0b0\x09\x88\x07+\x06\x01\x03d\x81H",
+        },
+    ]
+
     extensions = OpenSSL::X509::ExtensionFactory.new
-    ext = extensions.create_extension("subjectAltName", "email:foo@bar.com,DNS:a.b.com,email:baz@bar.com")
-    assert_equal 'subjectAltName', ext.oid
-    assert_equal 'email:foo@bar.com, DNS:a.b.com, email:baz@bar.com', ext.value
-    mri_der = "0,\x06\x03U\x1D\x11\x04%0#\x81\vfoo@bar.com\x82\aa.b.com\x81\vbaz@bar.com"
-    assert_equal mri_der, ext.to_der
+    tests.each { |test|
+        ext = extensions.create_extension("subjectAltName", test[:input])
+        assert_equal 'subjectAltName', ext.oid
+        assert_equal (test[:output] || test[:input]), ext.value
+        assert_equal test[:der], ext.to_der
+    }
   end
 
   def subject_alt_name(domains)
@@ -165,4 +191,4 @@ def subject_alt_name(domains)
   end
   private :subject_alt_name
 
-end
\ No newline at end of file
+end