577140 - libssl does not ignore HelloRequests correctly

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P5)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Firstly, to the TLS specs, "The one message that is not bound by these ordering rules is the HelloRequest message, which can be sent at any time, but which SHOULD be ignored by the client if it arrives in the middle of a handshake."

Secondly, when renegotiation is disabled, libssl should just ignore a HelloRequest instead of treating it like an error. According to the TLS specs, "This message MAY be ignored by client if it does not wish to renegotiate a session, or the client may, if it wishes, respond with a no_renegotiation alert." Also, "This message is always a warning."

http://tools.ietf.org/html/rfc5246#section-7.4
http://tools.ietf.org/html/rfc5246#section-7.4.1.1
http://tools.ietf.org/html/rfc4346#section-7.4
http://tools.ietf.org/html/rfc4346#section-7.4.1.1
http://tools.ietf.org/html/rfc2246#section-7.4
http://tools.ietf.org/html/rfc2246#section-7.4.1.1


The relevant code is:

    if (ss->ssl3.hs.ws == wait_server_hello)
	return SECSuccess;
    if (ss->ssl3.hs.ws != idle_handshake || ss->sec.isServer) {
	(void)SSL3_SendAlert(ss, alert_fatal, unexpected_message);
	PORT_SetError(SSL_ERROR_RX_UNEXPECTED_HELLO_REQUEST);
	return SECFailure;
    }
    if (ss->opt.enableRenegotiation == SSL_RENEGOTIATE_NEVER) {
	ssl_GetXmitBufLock(ss);
	rv = SSL3_SendAlert(ss, alert_warning, no_renegotiation);
	ssl_ReleaseXmitBufLock(ss);
	PORT_SetError(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED);
	return SECFailure;
    }

which should become:

    if (ss->ssl3.hs.ws != idle_handshake)
	return SECSuccess;
    if (ss->opt.enableRenegotiation == SSL_RENEGOTIATE_NEVER) {
	ssl_GetXmitBufLock(ss);
	rv = SSL3_SendAlert(ss, alert_warning, no_renegotiation);
	ssl_ReleaseXmitBufLock(ss);
	return rv;
    }

Note that the ss->sec.isServer check isn't needed because that is already handled by ssl3_HandleHandshakeMessage.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

How do you get SHOULD from MAY?
The RFC clearly says MAY do one, or may do the other.  
libssl does one of the two.  
The alert avoids accusations of improperly ignoring the request.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

AFAICT, the intent is to allow the connection to continue as if the request was never made if the client chooses not to renegotiate.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Agreed that the warning response should not be disruptive, and that the 
connection should continune, much as if the request had been ignored. 
The only quibble here concerns whether sending the warning alert is desirable.  
I believe it is.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I agree. Note that the formatting of the code got messed up, but the replacement code *does* continue to send the alert. I guess the bug summary's wording of "ignore HelloRequest" could have been clearer.