718554 - SSL_ForceHandshake returns wrong positive results after sending pending data

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

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

This bug was caused by the patch for bug 676729.

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsecur.c&rev=1.53&root=/cvsroot#382

The call to ssl_SendSavedWriteData sets rv to a positive number (the number of bytes sent) when ssl_SendSavedWriteData succeeds. However, the code that handles the result of ssl3_GatherCompleteHandshake assumes that rv == SECFailure going into the block, and so fails to set rv = SECFailure when ssl3_GatherCompleteHandshake returns a negative result. ssl3_GatherCompleteHandshake often returns -1/PR_WOULD_BLOCK_ERROR.

Further, since SSL_ForceHandshake returns SECStatus, it should never return a value other than SECSuccess, SECFailure, or SECWouldBlock; we agreed in another bug that SSL_ForceHandshake would never return SECWouldBlock. 

Things that would have prevented this bug:

1. We should not initialize rv = SECFailure at the start of the function, because this masks a "variable was not initialized" warnings from the compiler that would have caught this bug.

2. We should have an automated test for this scenerio. (This bug was found in a new automated test I writing for bug 542832.)

3. We should use a block-scoped variable to store the result of ssl_SendSavedWriteData, like the gatherResult block-scoped variable that stores the result of ssl3_GatherCompleteHandshake.

4. The type of that local variable should be int, not SECStatus. SECStatus is an enum type, so we should not assign values to SECStatus variables other than SECFailure, SECWouldBlock, and SECSuccess. (A C compiler will not enforce this and most C compilers do not even have the option to warn about it; only C++ compilers issue diagnostics about this.)

Comment 1

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

Attachment: Testcase

Comment 2

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

Attachment: Fix [v1]

Without this patch, it is possible that SSL_ForceHandshake could return a SECSuccess (0) or a positive number such that a test for failure "!= SECFailure" or "< 0" would give the wrong result. In particular, I think SECSuccess could be returned if the EOF is reached on the connection; I think this condition may be under the control of an active MiTM. I am not sure to what extent this could be exploited. The internal (to libssl) state machine for the connection is not corrupted; only the application's understanding of the completion status of the handshake is affected. This could affect NPN connection coalescing, perhaps.

Comment 3

[Wan-Teh Chang]

Comment on attachment 589114 [details] [diff] [review]
Fix [v1]

r=wtc.  Thanks for the analysis and the fix.

Comment 4

[Wan-Teh Chang]

I extended bsmith's great analysis of the bug's risk in comment 2.

The call to ssl_SendSavedWriteData returns either -1 or a positive
number because the underlying write or send system call should return
-1 with EAGAIN/EWOULDBLOCK rather than returning 0 when it cannot
send anything from ss->pendingBuf.

So this bug won't affect a caller that tests for a successful
handshake completion by comparing the return value of SSL_ForceHandshake
with SECSuccess (0).

If the EOF is reached on the connection, the bug should cause
SSL_ForceHandshake to return a positive integer rather than
SECSuccess.

Comment 5

[Wan-Teh Chang]

I checked in the patch "Fix [v1]" on the NSS trunk (NSS 3.13.2).

Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.54; previous revision: 1.53
done

Comment 6

[Robert Relyea]

Comment on attachment 589114 [details] [diff] [review]
Fix [v1]

r+ rrelyea

Comment 7

[Robert Relyea]

Comment on attachment 589112 [details] [diff] [review]
Testcase

r+ assuming update to base loopback code.

bob