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

[Xuelei Fan]

Hi Florian,

Sorry for the delay.

There are requirements to support version fall-back in client side for
JDK 8u/7u in practice.  As would require a non-api-update solution for
backporting.

I see your points about the disadvantage of putting TLS_FALLBACK_SCSV
into the regular cipher suite selector.  I think we may be able to
leverage the concept of "fallback": if the enabled protocol is the
highest supported version, the TLS_FALLBACK_SCSV cipher suite would not
be delivered in ClientHello message.  I think this would mitigate the
miss-use of this special cipher suite, and benefit from the backporting.

What do you think?

Thanks,
Xuelei

On 8/6/2015 4:45 AM, Florian Weimer wrote:
> 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.
>