Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: Snapshot
    • Fix Version/s: 4.0 Alpha 1
    • Component/s: HttpClient
    • Labels:
      None

      Description

      https should check CN of x509 cert

      Since we're essentially rolling our own "HttpsURLConnection", the checking provided by "javax.net.ssl.HostnameVerifier" is no longer in place.

      I have a patch I'm about to attach which caused both createSocket() methods on o.a.h.conn.ssl.SSLSocketFactory to blowup:

      test1: javax.net.ssl.SSLException: hostname in certificate didn't match: <vancity.com> != <www.vancity.com>
      test2: javax.net.ssl.SSLException: hostname in certificate didn't match: <vancity.com> != <www.vancity.com>

      Hopefully people agree that this is desirable.

      1. SSLSocketFactory_best.patch
        8 kB
        Julius Davies
      2. SSLSocketFactory_improved.patch
        7 kB
        Julius Davies
      3. SSLSocketFactory.patch
        5 kB
        Julius Davies

        Activity

        Hide
        Julius Davies added a comment -

        patch also includes fix of an NPE when using createSocket( Socket s ) [the method that upgrades pre-existing plain socket to SSLSocket].

        Not sure where to put my testing code - sorry I'm such a newb! Here's the testing code:

        public static void main( String[] args ) throws Exception {
        String host = args[ 0 ];
        SSLSocketFactory f = new SSLSocketFactory();
        HttpParams params = new DefaultHttpParams();
        try

        { Socket s = f.createSocket( args[ 0 ], 443, null, 0, params ); exercise( host, s ); s.close(); }

        catch ( Exception e )

        { System.out.println( "test1: " + e ); }

        try

        { Socket s = new Socket( args[ 0 ], 443 ); s = f.createSocket( s, args[ 0 ], 443, true ); exercise( host, s ); s.close(); }

        catch ( Exception e )

        { System.out.println( "test2: " + e ); }


        }

        private static void exercise( String host, Socket s ) throws Exception {
        InputStream in = s.getInputStream();
        OutputStream out = s.getOutputStream();
        out.write( ( "HEAD / HTTP/1.1\r\n" +
        "host: " + host + "\r\n\r\n" ).getBytes() );
        out.flush();

        for ( int i = 0; i < 64; i++ ) {
        int b = in.read();
        if ( b == -1 )

        { break; }

        else

        { System.out.print( (char) b ); }

        }
        System.out.println();
        }

        Show
        Julius Davies added a comment - patch also includes fix of an NPE when using createSocket( Socket s ) [the method that upgrades pre-existing plain socket to SSLSocket] . Not sure where to put my testing code - sorry I'm such a newb! Here's the testing code: public static void main( String[] args ) throws Exception { String host = args[ 0 ]; SSLSocketFactory f = new SSLSocketFactory(); HttpParams params = new DefaultHttpParams(); try { Socket s = f.createSocket( args[ 0 ], 443, null, 0, params ); exercise( host, s ); s.close(); } catch ( Exception e ) { System.out.println( "test1: " + e ); } try { Socket s = new Socket( args[ 0 ], 443 ); s = f.createSocket( s, args[ 0 ], 443, true ); exercise( host, s ); s.close(); } catch ( Exception e ) { System.out.println( "test2: " + e ); } } private static void exercise( String host, Socket s ) throws Exception { InputStream in = s.getInputStream(); OutputStream out = s.getOutputStream(); out.write( ( "HEAD / HTTP/1.1\r\n" + "host: " + host + "\r\n\r\n" ).getBytes() ); out.flush(); for ( int i = 0; i < 64; i++ ) { int b = in.read(); if ( b == -1 ) { break; } else { System.out.print( (char) b ); } } System.out.println(); }
        Hide
        Julius Davies added a comment -

        To account for a problem with IBM 1.4.x JVM's, I think we should also test against sslSocket.getSession() being null. If it is null, we should try to get the socket to blowup by calling socket.getInputStream().available().

        SSLSocket ssl = (SSLSocket) s;
        SSLSession session = ssl.getSession();
        if ( session == null )

        { // In our experience this only happens under IBM 1.4.x. // hopefully this will unearth the real problem: ssl.getInputStream().available(); }

        [Not sure how to deal with this 2nd patch. Do I upload a new patch containing both fixes? Sorry I'm such a newb!]

        Here's some background info:

        The IBM 1.4.x JVM, when acting as an SSL client, is quite picky about the certificate chain that the server presents. If the server includes some stray certificates in the chain, IBM will blowup.

        But it takes a little while to blowup:

        SSLSocket s = factory.createSocket( host, port );

        // okay, we're still okay

        SSLSession session = s.getSession();

        // still okay! No exceptions thrown! But session is null. Uh oh.

        InputStream in = s.getInputStream();

        // Still no exceptions thrown! Wow, IBM is a survivor.

        in.available();

        // ! * BOOM * !

        javax.net.ssl.SSLHandshakeException: bad certificate
        at com.ibm.jsse.bv.a(Unknown Source)
        at com.ibm.jsse.a.a(Unknown Source)
        at com.ibm.jsse.a.available(Unknown Source)

        Show
        Julius Davies added a comment - To account for a problem with IBM 1.4.x JVM's, I think we should also test against sslSocket.getSession() being null. If it is null, we should try to get the socket to blowup by calling socket.getInputStream().available(). SSLSocket ssl = (SSLSocket) s; SSLSession session = ssl.getSession(); if ( session == null ) { // In our experience this only happens under IBM 1.4.x. // hopefully this will unearth the real problem: ssl.getInputStream().available(); } [Not sure how to deal with this 2nd patch. Do I upload a new patch containing both fixes? Sorry I'm such a newb!] Here's some background info: The IBM 1.4.x JVM, when acting as an SSL client, is quite picky about the certificate chain that the server presents. If the server includes some stray certificates in the chain, IBM will blowup. But it takes a little while to blowup: SSLSocket s = factory.createSocket( host, port ); // okay, we're still okay SSLSession session = s.getSession(); // still okay! No exceptions thrown! But session is null. Uh oh. InputStream in = s.getInputStream(); // Still no exceptions thrown! Wow, IBM is a survivor. in.available(); // ! * BOOM * ! javax.net.ssl.SSLHandshakeException: bad certificate at com.ibm.jsse.bv.a(Unknown Source) at com.ibm.jsse.a.a(Unknown Source) at com.ibm.jsse.a.available(Unknown Source)
        Hide
        Oleg Kalnichevski added a comment -

        > Not sure how to deal with this 2nd patch. Do I upload a new patch containing both fixes?

        Yes, you do. That's the easiest way.

        Overall the patch looks good to me. I'll review it a little more thoroughly in the morning and commit the changes to SVN

        Many thanks for this contribution, Julius

        Oleg

        Show
        Oleg Kalnichevski added a comment - > Not sure how to deal with this 2nd patch. Do I upload a new patch containing both fixes? Yes, you do. That's the easiest way. Overall the patch looks good to me. I'll review it a little more thoroughly in the morning and commit the changes to SVN Many thanks for this contribution, Julius Oleg
        Hide
        Julius Davies added a comment -
        • Better formatting on the curly braces (e.g. "} else {" now on one line).
        • A few more comments.
        • Hostname verification no longer throws an NPE under IBM 1.4.2 when accessing a server that has spurious certificates in its chain. Instead it throws the underlying reason that's actually bothering IBM (IBM-1.4.x hates spurious certificates!).

        Here's some testing:
        =======================================
        Ability to deal with spurious certificates in chain.
        (I only tested on Linux)

        Succeeds:
        ------------------------
        IBM 1.5.0
        JRockit 1.4.2
        JRockit 1.5.0
        Sun 1.4.2
        Sun 1.5.0
        Sun 1.6.0-rc

        Fails:
        ------------------------
        IBM 1.4.2

        Show
        Julius Davies added a comment - Better formatting on the curly braces (e.g. "} else {" now on one line). A few more comments. Hostname verification no longer throws an NPE under IBM 1.4.2 when accessing a server that has spurious certificates in its chain. Instead it throws the underlying reason that's actually bothering IBM (IBM-1.4.x hates spurious certificates!). Here's some testing: ======================================= Ability to deal with spurious certificates in chain. (I only tested on Linux) Succeeds: ------------------------ IBM 1.5.0 JRockit 1.4.2 JRockit 1.5.0 Sun 1.4.2 Sun 1.5.0 Sun 1.6.0-rc Fails: ------------------------ IBM 1.4.2
        Hide
        Julius Davies added a comment -
        • I think this one is the best patch.
        • Okay, it's my first substantial patch to an apache project. I'm excited and I've gone insane.
        • Improved comments even more.
        • Attempted to deal with the [*.co.uk] wildcard problem. I think I've taken a decent first stab at the problem.
        Show
        Julius Davies added a comment - I think this one is the best patch. Okay, it's my first substantial patch to an apache project. I'm excited and I've gone insane. Improved comments even more. Attempted to deal with the [*.co.uk] wildcard problem. I think I've taken a decent first stab at the problem.
        Hide
        Oleg Kalnichevski added a comment -

        Patch (with some minor tweaks) checked in. Many thanks, Julius

        Now, since there appears to be many ways to skin a cat (I mean to check CN of x509 cert), we should provide a means to inject a case specific implementation of the CN verifier instead of trying to cover all possible scenarios with one implementation . I think we should come up with abstract interface to represent the process of CN verification and provide multiple implementations of it (lenient, strict, IBMJSSE specific). This should also allow for better unit testing of the CN verification logic. Speaking of which, some unit tests would be just awesome.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Patch (with some minor tweaks) checked in. Many thanks, Julius Now, since there appears to be many ways to skin a cat (I mean to check CN of x509 cert), we should provide a means to inject a case specific implementation of the CN verifier instead of trying to cover all possible scenarios with one implementation . I think we should come up with abstract interface to represent the process of CN verification and provide multiple implementations of it (lenient, strict, IBMJSSE specific). This should also allow for better unit testing of the CN verification logic. Speaking of which, some unit tests would be just awesome. Oleg
        Hide
        Martin van den Bemt added a comment -

        // The CN better have at least two dots if it wants wildcard action.
        + // (Hmmm... what about *.co.uk ??? Eeek! Something to think about
        + // on a rainy day, I guess.)

        According to your code.. If I am not mistaken .co.uk contains at least 2 dots...

        And officially (just had to deal with that issue), you need a wildcard certificate for every subdomain so :
        *.apache.org is a wildcard cerficiate for hostname.apache.org (just that level).
        If you want to have a valid certifacate for eg subhostname.hostname.apache.org, you need to get a *.subhostname.hostname.apache.org certificate for that seperately (although browsers seem to accept this stuff).

        I chose to have an hostname verifier setup that accepts above, but that is just used for staging and test environments (to prevent us from buyin a huge load of certificates)

        HTH..

        Mvgr,
        Martin

        Show
        Martin van den Bemt added a comment - // The CN better have at least two dots if it wants wildcard action. + // (Hmmm... what about *.co.uk ??? Eeek! Something to think about + // on a rainy day, I guess.) According to your code.. If I am not mistaken .co.uk contains at least 2 dots... And officially (just had to deal with that issue), you need a wildcard certificate for every subdomain so : *.apache.org is a wildcard cerficiate for hostname.apache.org (just that level). If you want to have a valid certifacate for eg subhostname.hostname.apache.org, you need to get a *.subhostname.hostname.apache.org certificate for that seperately (although browsers seem to accept this stuff). I chose to have an hostname verifier setup that accepts above, but that is just used for staging and test environments (to prevent us from buyin a huge load of certificates) HTH.. Mvgr, Martin
        Hide
        Julius Davies added a comment -

        HTTPCLIENT-614 will try to address Martin's concerns.

        This wiki entry has an interesting catalog of browser behaviour:

        http://wiki.cacert.org/wiki/WildcardCertificates

        • IE6 doesn't allow subdomains (so follows the RFC). *.apache.org does not match "a.b.apache.org".
        • Firefox/Mozilla allows subdomains (breaks RFC). *.apache.org DOES MATCH "a.b.apache.org"!
        • New versions of Konqueror (so Safari too?) allows subdomains (breaks RFC).
        • Operat allows subdomains (breaks RFC).

        I think I'll do some experimentation on my own and test some additional clients. I'll add my findings to cacert's very handy wiki! Curious about the following (but I'm lazy so I'm just going to stick to Linux):

        • wget
        • curl
        • java.net.URL on the following:
          1. Sun Java 1.3.1 + JSSE
          2. Sun Java 1.4.2
          3. Sun Java 5.0
          4. Sun Java 6.0
          5. IBM Java 1.4.2
          6. IBM Java 5.0
          7. JRockit Java 1.4.2
          8. JRockit Java 5.0
        Show
        Julius Davies added a comment - HTTPCLIENT-614 will try to address Martin's concerns. This wiki entry has an interesting catalog of browser behaviour: http://wiki.cacert.org/wiki/WildcardCertificates IE6 doesn't allow subdomains (so follows the RFC). *.apache.org does not match "a.b.apache.org". Firefox/Mozilla allows subdomains (breaks RFC). *.apache.org DOES MATCH "a.b.apache.org"! New versions of Konqueror (so Safari too?) allows subdomains (breaks RFC). Operat allows subdomains (breaks RFC). I think I'll do some experimentation on my own and test some additional clients. I'll add my findings to cacert's very handy wiki! Curious about the following (but I'm lazy so I'm just going to stick to Linux): wget curl java.net.URL on the following: 1. Sun Java 1.3.1 + JSSE 2. Sun Java 1.4.2 3. Sun Java 5.0 4. Sun Java 6.0 5. IBM Java 1.4.2 6. IBM Java 5.0 7. JRockit Java 1.4.2 8. JRockit Java 5.0
        Hide
        Martin van den Bemt added a comment -

        Sorry for the noise.. Completely missed issue 614..

        Show
        Martin van den Bemt added a comment - Sorry for the noise.. Completely missed issue 614..

          People

          • Assignee:
            Unassigned
            Reporter:
            Julius Davies
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development