[9] RFC: 8061798: Add support for TLS_FALLBACK_SCSV (RFC 7507)

[Florian Weimer]

On 06/16/2015 05:49 AM, Xuelei Fan wrote:
> On 6/15/2015 5:58 PM, Florian Weimer wrote:
>> On 06/03/2015 03:56 AM, Xuelei Fan wrote:
>>
>>> I can sponsor you for the specification update approval and changeset
>>> integration if needed.
>>
>> I'd really appreciate that.

Still applies, thanks.

>>> I still have a few comments about this fix:
>>>
>>> 1. TLS_FALLBACK_SCSV may still be miss-used as it is defined as a
>>> default enabled cipher suites.
>>
>> Are you sure about that?  The test case looks for the string "FALLBACK"
>> among the supported and enabled cipher suites, and it's not there.
>>
> Ah, I see your point.
> 
> You defined K_FALLBACK_SCSV as not allowed KeyExchange (line 343), and
> the cipher suite TLS_FALLBACK_SCSV as not allowed cipher suites (line
> 1097-1098) although it is in the block of the default enabled cipher
> suites code.
> 
> I would like to remove the two updates above as they are not actually
> allowed to be used.

If I do that, the code which actually reacts to TLS_FALLBACK_SCSV never
sees the SCSV because it has been filtered out before.  So I had to
leave this change in.

>> Do you have a pointer to similar code in OpenJDK?
> See the templates:
> jdk/test/javax/net/ssl/templates/SSLEngineTemplate.java
> jdk/test/javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java

With a lot of trial and error, I managed to write the EngineWrapper class.

I want to call particular attention to this bit:

    dummy.position(dummy.limit());
    networkOutput.clear();
    SSLHandshakeException deferred = null;
    try {
        wrapToScratch(dummy);
    } catch (SSLHandshakeException e) {
        // During handshakes, we may receive a deferred
        // exception from a task, but the engine still has
        // data to send.
        deferred = e;
        networkOutput.clear();
        try  {
            wrapToScratch(dummy);
        } catch (Exception e1) {
            // Rethrow original exception because it is
            // likely more informative.
            throw deferred;
        }
    }
    networkOutput.flip();
    writeFully(networkOutput);
    if (deferred != null)
        throw deferred;

This happens because the handshake runs exclusively on a task (not just
the bits that need blocking), and the way exceptions from tasks are
handled, they overtake the data being sent.  Without the above, the
handshake alert is never sent because the deferred exception from the
task hides the queued alert message.

Quite frankly, this smells of an SSLEngine bug.  (This is a separate
issue from the wrong exception class for handshake exceptions.)

>> The slightly awkward name is due to the fact that it's just a
>> flag send to the server, and it does not cause the TLS implementation to
>> perform the fallback on its own.  What about this?
>>
>>   public void setProtocolFallbackInProgress(boolean inProgress) {…}
>>   public boolean isProtocolFallbackInProgress() {…}
>>
> Sounds much better to me, except that the name is really long.

I would still prefer the long name because is it is quite explicit.  Its
length should not matter applications should not use this interface. :)

I've prepared a new webrev under:

  <http://cr.openjdk.java.net/~fweimer/8061798/webrev.03/>

The main addition is client support in SSLEngine.

-- 
Florian Weimer / Red Hat Product Security