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

[Xuelei Fan]

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.
> 
>> 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.

>> Some applications may just get all supported cipher suites and enable
>> them all for a certain circumstance.  For example:
>>
>>     String[] suites = sslSocket.getSupportedCipherSuites();
>>     sslSocket.setEnabledCipherSuites(suites);
>>
>> As would result in unexpected behavior as you concerned about this protocol.
> 
> I should add further tests to cover this kind of misuse.
> 
>> What do you think if TLS_FALLBACK_SCSV is not defined as a supported
>> (enabled) cipher suite?  I mean SSLSocket.getSupportedCipherSuites()
>> would not return TLS_FALLBACK_SCSV in any cases.
> 
> Right, that's the best behavior.
> 
>> 2. SSLEngineImpl
>> Better to update SSLEngineImpl.java in this fix as what you did for
>> SSLSocketImpl.java
> 
> I've been trying to write a test case which basically does this
> 
>    InputStream    \                 / InputStream
>                    >-- SSLEngine --<
>    OutputStream   /                 \ OutputStream
> 
> Essentially reimplementing SSLSocket on top of Socket, using SSLEngine.
> 
> 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

> It would also be
> helpful to people who try to implement more complicated variants of
> STARTTLS.
> 
> SSLEngine appears to be quite difficult to use directly.
> 
It's an AIO programming style.  Quite different from socket programming.

>> 3. src/java.base/share/classes/javax/net/ssl/SSLParameters.java
>> ------------
>>  472      * <b>Note:</b> Performing protocol downgrades outside the TLS
>>  473      * protocol can introduce security vulnerabilities.  The
>>  474      * built-in version negotiation mechanism of the TLS protocol
>>  475      * should be used instead of explicit protocol downgrades.
>>  476      * See section 4 of RFC 7507 for additional advice.
>>
>> Sounds like it is a part of "implNote".
> 
> Not as far as I understand the scope of “implNote”.  The note above
> describes expected application behavior (“call this method if you
> implement your own fallback”).  It's not an implementation detail of the
> TLS impleemntation.
> 
I see.

>>
>> ------------
>>  478      * @implNote
>>  479      * The default JSSE provider uses the TLS_FALLBACK_SCSV signaling
>>  480      * ciphersuite value to indicate protocol fallback, as specified
>>  481      * in RFC 7507.
>>
>> As this is a public spec, any JSSE provider should follow this spec.
>> I'd like to say:
>>    "A JSSE provider should use the TLS_FALLBACK_SCSV ..."
> 
> Fine, we can update the spec if the mechanism changes due to TLS 1.3
> interop issues.
> 
>> ------------
>>  491     public final void setIndicateProtocolFallback(
>>                  boolean indicateFallback) {
>>  506     public final boolean getIndicateProtocolFallback()
>>
>> "setIndicate" does not sound like instinctive enough to me.  What do you
>> think:
>>         void indicateProtocolFallback(boolean useProtocolFallback)
>>         boolean isProtocolFallback()
>> or
>>         void setUseProtocolFallback(boolean useProtocolFallback)
>>         boolean getUseProtocolFallback()
> 
> I deliberately used a setter and getter because I thought these
> standard.
My preference, too.

> 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.

What do you think:
    void setUseProtocolFallbackMode(boolean useProtocolFallbackMode)
    boolean getUseProtocolFallbackMode()

;-) Only three letters shorter.

Or
    void setProtocolFallbackMode(boolean useProtocolFallbackMode)
    boolean getProtocolFallbackMode()

Thanks,
Xuelei