Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My app communicates with a server over an internal network through https. The SSL certificate on this server is listed for the host as its external host name. I want to accept this certificate, but I don't want to accept ANY certificate as many workarounds I've seen seem to do. I would also like to have it so that later on, in the strong possibility that his app is accessing a different server with a correct SSL certificate, that I don't have to change it. I have it working as I would like it seems, but I am very unsure of myself when it comes to implementing my own methods to override a library used for secure communications, and I am not very strong in my knowledge of SSL communications. Here is the code I have, basically I would like input on security loopholes and its effectiveness at doing what I need.

HttpClient httpclient = new DefaultHttpClient();
SSLSocketFactory ssf = SSLSocketFactory.getSocketFactory();
ssf.setHostnameVerifier( new X509HostnameVerifier() {

   @Override
   public void verify( String host, String[] cns, String[] subjectAlts )
         throws SSLException
   {
   }

   @Override
   public void verify( String host, X509Certificate cert ) throws SSLException
   {               
   }

   @Override
   public void verify( String host, SSLSocket ssl ) throws IOException
   {                 
      boolean foundHost = false;
      for( javax.security.cert.X509Certificate cert : ssl.getSession().getPeerCertificateChain() )
      {
         String[] tokens = cert.getSubjectDN().getName().split( "," );
         for( String token : tokens )
         {
            String[] keyVal = token.split( "=" );

            if( keyVal.length > 1 && keyVal[0].equals( "CN" ) )
            {
               foundHost = keyVal[1].equals( context.getString( R.string.CertHostName ) );

               if(!foundHost)
               {
                foundHost = keyVal[1].equals( host );
               }
            }

            if(foundHost)
            {
               break;
            }      
         }
         if(foundHost)
         {
            break;
         }
      }

      if(!foundHost)
      {
         throw new IOException( "Mismatched host in SSL certificate" );
      }
   }

   @Override
   public boolean verify( String host, SSLSession session )
   {
      return false;
   }
} );
httpclient.getConnectionManager().getSchemeRegistry().unregister( "https" );
httpclient.getConnectionManager().getSchemeRegistry().register( new Scheme( "https", ssf, 443 ) );

Basically, I found out which method to override through some log statements to see which one was being used in the anonymous inner X509HostnameVerifier class, and hopefully handled it appropriately. Input would be appreciated.

share|improve this question
    
Since certificate authorities will stop issuing certificates with internal hostnames, I see your options as: a) Implement this workaround; b) Always use the public hostname and interface, or c) Deploy split DNS such that from inside your network, the public hostname resolves to an internal IP address. I believe options (b) and (c) are less hackish. –  200_success Nov 21 '13 at 3:41
add comment

2 Answers

There are a few items inhere I would suggest can be improved (assuming the security aspects of the code are valid - i.e. legal disclaimer about 'safe' and SSL means speak to someone who's more of a security expert than me).

First, I assume you are using the apache-commons based HttpClient code.... this is fine, but it would have been useful to show the import statements ;-).

Regardless. There are three things I would change.

First, whenever you do an anonymous class implementation and it has more than 1 method that needs to be overridden, then I recommend that you create a full class implementation for it. In this case, your code should have a (perhaps nested) class that implements the

private static final class AlternateHostnameVerifier implements X509HostnameVerifier {
    ....
}

then your code becomes:

HttpClient httpclient = new DefaultHttpClient();
SSLSocketFactory ssf = SSLSocketFactory.getSocketFactory();
ssf.setHostnameVerifier(new AlternateHostnameVerifier());
httpclient.getConnectionManager().getSchemeRegistry().unregister( "https" );
httpclient.getConnectionManager().getSchemeRegistry().register( new Scheme( "https", ssf, 443 ) );

This is a much better way to show the critical logic of your program, and still make it readable.

Next up, in your new AlternateHostnameVerifier class, I would recommend you implement all the interface methods fully. Leaving two of the methods as stub methods is only a short-term solution. In the future the apache-commons implementation may change and call one of the other verify methods, and then you have a sudden, unexplained bug. Specifically throwing a UnsupportedOperationException is a much better alternative than leaving it blank.

Finally, I have a preference for methods that can to simply 'return' when they succeed rather than trying to break out of loops and keep track of variables outside of the loops.

In this case, you have foundHost which tracks the state. you can eliminate this variable if you just 'return' each time you set it to true, and otherwise always throw the exception.

So, you can rewrite the code as:

@Override
public void verify( String host, SSLSocket ssl ) throws IOException {                 
  for( javax.security.cert.X509Certificate cert : ssl.getSession().getPeerCertificateChain() ) {
     String[] tokens = cert.getSubjectDN().getName().split( "," );
     for( String token : tokens ) {
        String[] keyVal = token.split( "=" , 2);

        if( keyVal.length > 1 && keyVal[0].equals( "CN" ) ) {
           if (keyVal[1].equals( host ) || keyVal[1].equals( context.getString( R.string.CertHostName ) )) {
               // our CN matches the Android's certificate host.
               return;
           }
        }
     }
  }
  throw new IOException( "Mismatched host in SSL certificate" );
}
share|improve this answer
    
+1 - yet another nice review, keep it up! –  Mat's Mug Nov 21 '13 at 2:13
    
Definitely should have gone with the nested class. Coding standards where I work frown upon a return breaking out of several levels or returning before the end of the method, but I'm sure it would be fine in the cleaned up way presented here. Thanks! –  Drake Clarris Nov 21 '13 at 13:33
add comment

Instead of implementing your X509HostnameVerifier from scratch, extend org.apache.http.conn.ssl.AbstractVerifier, which does most of that work for you. All you have to do is override .verify(String host, String[] cns, String[] subjectAlts, boolean stringWithSubDomains().

share|improve this answer
    
Very nice, I wasn't aware of that. –  Drake Clarris Nov 21 '13 at 13:30
    
Well duh, I guess I don't know the HttpClient API well enough. Good catch. –  rolfl Nov 21 '13 at 14:06
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.