1177430 - Remove assertion !ss->xtnData.sniNameArr in ssl_DestroySocketContents

Status: RESOLVED FIXED

(NSS :: Test, defect)

Description

[Kai Engert [:KaiE:]]

Function ssl_DestroySocketContents has code to cleanup ss->xtnData.sniNameArr, but in debug mode, it doesn't allow that array to be non-null.

That's a problem with our SSL protocol tests (ssl_gtests). Because some tests send a bad sequence of messages, the array may be non-null on destruction.

We should handle bad peers gracefully, even in debug mode.

I suggest to remove the assertion.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: 1177430.patch

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8626189 [details] [diff] [review]
1177430.patch

I wouldn't worry about the comment.

Comment 3

[Wan-Teh Chang]

Comment on attachment 8626189 [details] [diff] [review]
1177430.patch

Review of attachment 8626189 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. I agree that the comment can be omitted because it
is clear that ssl_DestroySocketContents must be able to
destroy the contents of a socket in a partially inconsistent
state.

Comment 4

[Wan-Teh Chang]

Kai, Martin:

I studied the code related to ss->xtnData.sniNameArr. That assertion
in ssl_DestroySocketContents seems to be intended to ensure that
ssl3_HandleClientHello frees ss->xtnData.sniNameArr as soon as it is
finished with ss->xtnData.sniNameArr. If we want to preserve the
spirit of that assertion, we can move the assertion to the beginning
of ssl3_SendServerHello.

Here are the code snippets that support my proposal:

   8217         /* Clean up sni name array */
   8218         if (ssl3_ExtensionNegotiated(ss, ssl_server_name_xtn) &&
   8219             ss->xtnData.sniNameArr) {
   8220             PORT_Free(ss->xtnData.sniNameArr);
   8221             ss->xtnData.sniNameArr = NULL;
   8222             ss->xtnData.sniNameArrSize = 0;
   8223         }
   8224
   8225         ssl_GetXmitBufLock(ss); haveXmitBufLock = PR_TRUE;
   8226
   8227         rv = ssl3_SendServerHello(ss);
   8228         if (rv != SECSuccess) {
   8229             errCode = PORT_GetError();
   8230             goto loser;
   8231         }

and

   8387         /* Free sniNameArr. The data that each SECItem in the array
   8388          * points into is the data from the input buffer "b". It will
   8389          * not be available outside the scope of this or it's child
   8390          * functions.*/
   8391         if (ss->xtnData.sniNameArr) {
   8392             PORT_Free(ss->xtnData.sniNameArr);
   8393             ss->xtnData.sniNameArr = NULL;
   8394             ss->xtnData.sniNameArrSize = 0;
   8395         }
   8396         if (ret <= SSL_SNI_SEND_ALERT) {
   8397             /* desc and errCode should be set. */
   8398             goto alert_loser;
   8399         }
   8400     }
   8401 #ifndef SSL_SNI_ALLOW_NAME_CHANGE_2HS
   8402     else if (ss->firstHsDone) {
   8403         /* Check that we don't have the name is current spec
   8404          * if this extension was not negotiated on the 2d hs. */
   8405         PRBool passed = PR_TRUE;
   8406         ssl_GetSpecReadLock(ss);  /*******************************/
   8407         if (ss->ssl3.cwSpec->srvVirtName.data) {
   8408             passed = PR_FALSE;
   8409         }
   8410         ssl_ReleaseSpecReadLock(ss);  /***************************/
   8411         if (!passed) {
   8412             errCode = SSL_ERROR_UNRECOGNIZED_NAME_ALERT;
   8413             desc = handshake_failure;
   8414             goto alert_loser;
   8415         }
   8416     }
   8417 #endif
   8418
   8419     sid = ssl3_NewSessionID(ss, PR_TRUE);
   8420     if (sid == NULL) {
   8421         errCode = PORT_GetError();
   8422         goto loser;     /* memory error is set. */
   8423     }
   8424     ss->sec.ci.sid = sid;
   8425
   8426     ss->ssl3.hs.isResuming = PR_FALSE;
   8427     ssl_GetXmitBufLock(ss);
   8428     rv = ssl3_SendServerHelloSequence(ss);
   8429     ssl_ReleaseXmitBufLock(ss);
   8430     if (rv != SECSuccess) {
   8431         errCode = PORT_GetError();
   8432         goto loser;
   8433     }

are the only two places (both in ssl3_HandleClientHello) besides
ssl_DestroySocketContents where we free ss->xtnData.sniNameArr:

http://mxr.mozilla.org/nss/search?string=sniNameArr%2B%3D%2BNULL

Comment 5

[Eric Rescorla (:ekr)]

Kai,

Can we land this patch, either with or without Wan-Teh's suggestion? I am running into this in some other code.

Comment 6

[Kai Engert [:KaiE:]]

Checked in without comment.
https://hg.mozilla.org/projects/nss/rev/0b8056579bd9

If you'd like to restore the assertion at a different place, we'd need to test that the new assertion indeed works and doesn't break, and I didn't have time to do that right now.

Please suggest a follow-up patch if you want to add a new assertion, and we can test it when someone has time.

Comment 7

[Eric Rescorla (:ekr)]

Thanks for fixing this. This will be fine for my purposes.