Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix renegotiation with Client Auth (1.1.0) #1983

Conversation

mattcaswell
Copy link
Member

Checklist
  • tests are added or updated
  • CLA is signed
Description of change

#1920 describes a problem where a non-client auth reneg handshake that occurs after a client-auth handshake will end up with the client sending a Certificate message when it is not supposed to.

There is another related bug where, in the same situation, the server expects a Certificate message even when it hasn't requested one!

These two bugs together mean that OpenSSL will happily talk to the same version of itself in this scenario, but nothing else.

In adding tests for this I found a third bug in the implementation of SSL_VERIFY_CLIENT_ONCE. The documentation for this flag says:

      B<Server mode:> only request a client certificate on the initial TLS/SSL
      handshake. Do not ask for a client certificate again in case of a
      renegotiation. This flag must be used together with SSL_VERIFY_PEER.
    
      B<Client mode:> ignored

But the implementation actually did nothing. After the server sends its
ServerKeyExchange message, the code was checking s->session->peer to see if
it is NULL. If it was set then it did not ask for another client
certificate. However s->session->peer will only be set in the event of a
resumption, but a ServerKeyExchange message is only sent in the event of a
full handshake (i.e. no resumption).

The documentation suggests that the original intention was for this to
have an effect on renegotiation, and resumption doesn't come into it.

The fix is to properly check for renegotiation, not whether there is already
a client certificate in the session.

As far as I can tell this has been broken for a long time. I actually wondered whether we would be better off just removing it. But as it turns out I need to use it for my tests to work so I fixed it anyway!!

I also fixed a bug in TLSProxy where zero length messages weren't being recorded properly.

This is the 1.1.0 version of #1982

Fixes #1920

The flag SSL_VERIFY_CLIENT_ONCE is documented as follows:

  B<Server mode:> only request a client certificate on the initial TLS/SSL
  handshake. Do not ask for a client certificate again in case of a
  renegotiation. This flag must be used together with SSL_VERIFY_PEER.

  B<Client mode:> ignored

But the implementation actually did nothing. After the server sends its
ServerKeyExchange message, the code was checking s->session->peer to see if
it is NULL. If it was set then it did not ask for another client
certificate. However s->session->peer will only be set in the event of a
resumption, but a ServerKeyExchange message is only sent in the event of a
full handshake (i.e. no resumption).

The documentation suggests that the original intention was for this to
have an effect on renegotiation, and resumption doesn't come into it.

The fix is to properly check for renegotiation, not whether there is already
a client certificate in the session.

As far as I can tell this has been broken for a *long* time.
In a non client-auth renegotiation where the original handshake *was*
client auth, then the client will send a Certificate message anyway
resulting in a connection failure.
In a non client-auth renegotiation where the original handshake *was*
client auth, then the server will expect the client to send a Certificate
message anyway resulting in a connection failure.
@mattcaswell
Copy link
Member Author

Updated this to have the same reneg test as #1982 for consistency reasons.

@levitte
Copy link
Member

levitte commented Jan 12, 2017

Is this still current?

@mattcaswell
Copy link
Member Author

Yes - its still current.

levitte pushed a commit that referenced this pull request Jan 23, 2017
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #1983)
levitte pushed a commit that referenced this pull request Jan 23, 2017
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #1983)
levitte pushed a commit that referenced this pull request Jan 23, 2017
Repeat for various handshake types

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #1983)
levitte pushed a commit that referenced this pull request Jan 23, 2017
The flag SSL_VERIFY_CLIENT_ONCE is documented as follows:

  B<Server mode:> only request a client certificate on the initial TLS/SSL
  handshake. Do not ask for a client certificate again in case of a
  renegotiation. This flag must be used together with SSL_VERIFY_PEER.

  B<Client mode:> ignored

But the implementation actually did nothing. After the server sends its
ServerKeyExchange message, the code was checking s->session->peer to see if
it is NULL. If it was set then it did not ask for another client
certificate. However s->session->peer will only be set in the event of a
resumption, but a ServerKeyExchange message is only sent in the event of a
full handshake (i.e. no resumption).

The documentation suggests that the original intention was for this to
have an effect on renegotiation, and resumption doesn't come into it.

The fix is to properly check for renegotiation, not whether there is already
a client certificate in the session.

As far as I can tell this has been broken for a *long* time.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #1983)
levitte pushed a commit that referenced this pull request Jan 23, 2017
In a non client-auth renegotiation where the original handshake *was*
client auth, then the client will send a Certificate message anyway
resulting in a connection failure.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #1983)
levitte pushed a commit that referenced this pull request Jan 23, 2017
In a non client-auth renegotiation where the original handshake *was*
client auth, then the server will expect the client to send a Certificate
message anyway resulting in a connection failure.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #1983)
levitte pushed a commit that referenced this pull request Jan 23, 2017
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #1983)
@mattcaswell
Copy link
Member Author

Pushed. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants