462874 - Can not reliably wait for close_notify

Status: REOPENED

(NSS :: Libraries, defect, P2)

Description

[Kai Engert (:KaiE:)]

Miloslav Trmac, an engineer at Red Hat, reported the following in https://bugzilla.redhat.com/show_bug.cgi?id=468603

I propose we move the discussion over here, I'd like to avoid tweaking the deep internals of NSS' libSSL behavior, but instead see whether his proposed change makes sense in general, and can be accepted for cvs commit.

Below is what mitr said, quoting for the bugzilla mentioned aboved:


Description of problem:
I need to implement a "nested", temporary TLS session on a connection (running
unencrypted at first, then TLS, then again unencrypted).  I'm using
SSL_ImportFD(PR_ImportTCPSocket(dup(mysocket))) to create a NSS SSL socket.

Switching from unencrypted to encrypted works fine, but to switch back I need
to reliably detect when the TLS session has ended.

When any of the sides closes the SSL socket, nss sends a close_notify alert,
but it does not wait until it receives a close_notify alert from the other
side.

Ideally there should be an interface that lets me do this (see (man
SSL_shutdown) to see how OpenSSL handles it) - please consider this a feature
request.

Because the second unencrypted communication is unidirectional in my case, I
decided to PR_Close() the socket in the writer (and ignore any incoming
close_notify), and to PR_Recv() in the reader, which should return 0 upon
receiving the close_notify.

PR_Recv() actually processes the close_notify, but then waits for more data
anyway, consuming data from the unencrypted communication.

The attached patch fixes the receive side to stop reading data after receiving
a close_notify, which is sufficient for my current application - but please add
a general interface to reliably shut down the TLS session.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: proposed patch v1 (3 added lines, lots of context)

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I'm trying to understand the requirement here.  

Are you trying to have SSL terminate cleanly, but have the TCP 
connection continue in an unencrypted state?  

If so, that is a significant new requirement, and probably requires a 
greater change than the attached patch.  It would require having a way
to get the SSL protocol to go through all the motions of a PR_Close,
without closing the underlying socket.  That's not an entirely unreasonable 
request, IMO, but if that's what you're trying to accomplish, then this 
bug should be an RFE asking for that capability.  It would probably require
defining another settable SSL socket option.  

See also the large block comment about close_notify at 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsecur.c&rev=1.42&mark=999#999

Comment 3

[Miloslav Trmač]

Yes, that's what the application was trying to do.

(BTW, it turned out I don't need this functionality for my application after all.  So feel free to close this report if you don't think the functionality is likely to be useful.)

I'm not concerned with PR_Close() right now - dup() can take care of that - I'd just like the SSL layer on one side to read exactly data written by the SSL layer on the other side.

The block comment seems to describe only an implementation difficulty - the code can (in principle, perhaps the current organization makes it difficult) simply ignore ECONNRESET, and sending the close_notify in non-blocking mode is no worse than the current implementation.

Leaving all that aside, the specific submitted patch looks "obviously correct" to me and AFAICS it can not harm anything, and although it doesn't do everything I'd like, it makes some applications possible.  On the other hand, I don't understand SSL all that much and I might be missing something important.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Miloslav,
Does your application attempt to handshake as a client? or as a server?
Does dup() overcome the effects of a shutdown call on the underlying socket?

Comment 5

[Miloslav Trmač]

One as client, one as server :)  The one that was doing the PR_Read() was handshaking as a client.

Both the original file handle and the file handle returned by dup() are affected by PR_Shutdown().  PR_Close(), which I used, does not shut down the socket at all.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Are you willing to accept the limitation that ANY failure of the SSL 
handshake will leave the underlying socket in a state where the unencrypted 
connection cannot be resumed reliably (or perhaps, ever)?

Comment 7

[Miloslav Trmač]

Perfectly.  I was using the nested SSL session to authenticate the previous unencrypted context.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 346063 [details] [diff] [review]
proposed patch v1 (3 added lines, lots of context)


> 	rv = ssl3_HandleRecord(ss, &cText, &ss->gs.buf);
> 	if (rv < 0) {
> 	    return ss->recvdCloseNotify ? 0 : rv;
> 	}
>+	if (ss->recvdCloseNotify) {
>+	    return 0;
>+	}

I'm not opposed to this change in principle. 
You can rearrange it a little though, so that only one code path 
tests ss->recvdCloseNotify, but the results are the same.  
This will make the code smaller and easier to understand. 
Please do that.  When that is done correctly, I'll give it r+

Comment 9

[Miloslav Trmač]

Attachment: Stop PR_Recv() on close_notify (backed out)

Here is an updated patch.

Comment 10

[Miloslav Trmač]

Attachment: A test case

FWIW, I used this (with Fedora's python-nss) to test the behavior.

Correct output is:
'foo'
'bar'
'baz'

Incorrect output is:
'foo'
'bar'
''

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 346630 [details] [diff] [review]
Stop PR_Recv() on close_notify (backed out)

r=nelson
I'm willing to give this a try.  My only concern for it is that it might change behavior of some program that is presently working without this change, and that change might be a change for the worse.  But I'm willing to give it a try.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Checking in ssl3gthr.c; new revision: 1.8; previous revision: 1.7

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I backed out this patch, because all the Windows builds that began after
it was checked in failed their SSL tests.  The current hypothesis is that
somehow this patch keeps selfserv from shutting down, and selfserv keeps
holding on to the bound port, so all subsequent attempts to use it get 
EADDRINUSE.  It will be necessary for someone to login to the Windows 
Tinderbox systems and kill any leftover selfserv processes.  Then on the 
next build cycle, if everything goes back to green, that will confirm 
that this patch was the cause. 

It's strange that this is only an issue on Windows.  My own test builds 
on Windows with the Win95 flavor of NSPR all continue to pass all.sh,
so it may be related to the use of the WinNT flavor of NSPR, or to the 
use of the MinGW shell (I still use MKSNT).

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

I gather that Miloslav is not working on this any more...

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 346630 [details] [diff] [review]
Stop PR_Recv() on close_notify (backed out)

Marking this patch obsolete because it failed and was backed out