697910 - ssl3_HandleHandshake does not correctly handle SECWouldBlock from ssl3_HandleHandshakeMessage when the handshake message spans multiple TLS records

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #542832 +++

With the patch from Bug 542832, Patrick McManus from found that he is unable to connect to https://www.cotendo.com/ with libssl reporting an SSL_ERROR_RX_UNEXPECTED_CERTIFICATE error. This server splits its Certificate message into multiple records. ssl3_HandleHandshake calls ssl3_HandleCertificate and ssl3_HandleCertificate returns SECWouldBlock, ssl3_HandleHandshake leaves the old Certificate message in ss->ssl3.hs.msg_body. Consequently, the next time we try to step through the SSL state machine, we see the Certificate message in ss->ssl3.hs.msg_body and wrongly call ssl3_HandleCertificate again to parse it. ssl3_HandleCertificate then fails with the SSL_ERROR_RX_UNEXPECTED_CERTIFICATE error code.

The problematic code is here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.152&root=/cvsroot#8820

Nicely, the problematic code is labeled "XXX This appears to be wrong."

I have split this into a separate bug from bug 542832 because it might affect other (existing) functionality too.

Comment 1

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

Attachment: Handle second case where ssl3_HandleHandshakeMessage returns SECWouldBlock similarly to the way we handle the first case

There are two cases where ssl3_HandleHandshake calls ssl3_HandleHandshakeMessage.

In the first case, we have an entire handshake message in the buffer passed to us in the parameter. We avoid doing a copy by passing the part of that input buffer to ssl3_HandleHandshakeMessage directly.

In the second case, we call ssl3_HandleHandshakeMessage once we've constructed an entire handshake message into ss->ssl3.hs.msg_body.

After we call ssl3_HandleHandshakeMessage, we need to remember that we already processed the message in ss->ssl3.hs.msg_body. We do this by resetting ss->ssl3.hs.msg_body.len to zero. We currently don't do this in the SECWouldBlock case, so will call ssl3_HandleHandshakeMessage on the same message again when we restart the handshake.

In both cases, we also need to reset ss->ssl3.hs.msg_len and ssl3.hs.header_bytes to zero so that we parse the next message correctly. This is something we did in both cases for SECSuccess but only in the second case for SECWouldBlock.

We could reset all these variables to zero in all cases, including SECFailure. However, I chose not to update them in the SECFailure case, because it will make debugging easier when you set a breakpoint on the "if (rv == SECFailure)" bodies.

The effective change is actually smaller than it looks because I reordered two of the assignments, to make it easier to see the similarity between this second case and the first case. There is only one difference between the two cases: In the first case, we advance buf->buf (and decrease buf->len) past the processed message. In the second case, we truncate ss->ssl3.hs.msg_body to an empty buffer, basically discarding the message. Otherwise, the cases are almost exactly the same.

I spent (too much) time trying to factor out the commonality between these two cases into a single code path. However, the code was much harder to understand than this change. 

We may have been able to detect this bug sooner if the assertion:
     PORT_Assert(ss->ssl3.hs.msg_body.len <= ss->ssl3.hs.msg_len)
had instead been written:
     PORT_Assert(ss->ssl3.hs.msg_body.len <= ss->ssl3.hs.msg_len);

The new stronger assertion is saying that, if we have something in ss->ssl3.hs.msg_body, then it must be a partial message. This is a valid assertion because, if it were a full message then we would have called ssl3_HandleHandshakeMessage on it already, and after doing so we shouldn't have kept a message in ss->ssl3.hs.msg_body any longer.

Comment 2

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

> This is something we did in both cases for SECSuccess
> but only in the second case for SECWouldBlock.

:sigh: This is something we did in both cases for SECSuccess but we only did it in the FIRST case (not the second case) for SECWouldBlock.

Comment 3

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

(In reply to Brian Smith (:bsmith) from comment #2)
> We may have been able to detect this bug sooner if the assertion:
>      PORT_Assert(ss->ssl3.hs.msg_body.len <= ss->ssl3.hs.msg_len)
> had instead been written:
>      PORT_Assert(ss->ssl3.hs.msg_body.len <= ss->ssl3.hs.msg_len);

:double-sigh:

instead been written:
     PORT_Assert(ss->ssl3.hs.msg_body.len < ss->ssl3.hs.msg_len);

My patch makes this change.

Comment 4

[Robert Relyea]

Comment on attachment 570381 [details] [diff] [review]
Handle second case where ssl3_HandleHandshakeMessage returns SECWouldBlock similarly to the way we handle the first case

r+ rrelyea

Comment 5

[Wan-Teh Chang]

Attachment: wtc's patch

I noticed this problem while reviewing Eric Rescorla's DTLS patch.
My patch differs from Brian's patch as follows:
- I didn't notice the problem with that PORT_Assert.  I will take
  a look at that tomorrow.
- I copied the code from "40 lines before/above this one" exactly,
  whereas Brian's version is arguably clearer.  I think it's
  important to keep the two copies the same.

In the debugger, I confirmed that this code path is never take in
practice.  So the code from "40 lines before/above this one" should
definitely be considered as the model.

Comment 6

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

I will update my patch to take Wan-Teh's feedback into consideration. I would like to keep the PORT_Assert change and the movement of the assignment:
   
    ss->ssl3.hs.msg_len = 0;

from my original patch, because this makes things clearer and more symmetric with the "40 lines above" case.

Comment 7

[Kai Engert (:KaiE:)]

Given comments 5 and 6, should keyword checkin-needed be removed?

Comment 8

[Wan-Teh Chang]

Attachment: Handle second case where ssl3_HandleHandshakeMessage returns SECWouldBlock similarly to the way we handle the first case, v2

Brian: I kept your PORT_Assert change.  I don't understand why you
want to move the assignment:
   
    ss->ssl3.hs.msg_len = 0;

because the original code matched the "40 lines above" case exactly.
So I only took your whitespace change to the assignment.

Re: if (rv != SECSuccess) vs. if (rv == SECWouldBlock) when SECFailure
has already been handled:

They are equivalent, and I think "if (rv == SECWouldBlock)" is clearer
and doesn't need a comment to clarify.  I went with "if (rv != SECSuccess)"
to match the "40 lines above" case exactly.  If you prefer
"if (rv == SECWouldBlock)", please change both cases.

I suspect "if (rv != SECSuccess)" may have been intentional for speed.
Old-school system programmers often try to compare with 0 because many
processors have branch instructions based on zero/nonzero status.  This
is why NSS often checks for failure by testing rv != SECSuccess rather
than rv == SECFailure.

Patch checked in on the NSS trunk (NSS 3.13.5).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.174; previous revision: 1.173
done