745281 - Provide the option of disabling SSL PKCS #11 bypass at build time

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Elio Maldonado]

The environment variable SSLBYPASS when set allows NSS SSL/TLS to bypass the PKCS #11 layer. The user is advised to not set this variable if FIPS is enabled. SSL PKCS #11 bypass is a feature that that not all downstream distributions have no interest in and actually do not support. Such is the case with Red Hat Enterprise Linux and Fedora. We should provide the ability for downstream package maintainers to disable the feature at build time.

Comment 1

[Elio Maldonado]

Attachment: Disable ssl pkcs11 layer bypass at build ti

Enabled by a build-time environment variable.

Comment 2

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

I support the idea, and I understand why you wrote the patch the way you did. But, this makes libssl even harder to read and maintain that before. We should just remove the bypass code completely. That would also simplify the TLS 1.2 implementation.

Comment 3

[Elio Maldonado]

(In reply to Brian Smith (:bsmith) from comment #2)
> did. But, this makes libssl even harder to read and maintain that before. We
Yes, it does make reading the code. I didn't aare remove sslbypass, at least not for 3.13.

> should just remove the bypass code completely. That would also simplify the
> TLS 1.2 implementation.
I'll attach such a patch next in the hope that for 3.14 we will enjoy such freedom.

Comment 4

[Elio Maldonado]

Attachment: Gets rid of sslbypass

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 614880 [details] [diff] [review]
Disable ssl pkcs11 layer bypass at build ti

>@@ -1001,8 +1001,17 @@ ssl3_ComputeCommonKeyHash(PRUint8 * hash
>     SECStatus     rv 		= SECSuccess;
> 
>     if (bypassPKCS11) {
>+#ifdef NOSSLBYPASS
>+     /* We shouldn't be here. SSL_OptionSet should have returned an error preventing it. */
>+     PR_ASSERT(!bypassPKCS11);
>+     PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
>+     return SECFailure;
>+     /* Reset bypassPKCS11 to false and recursevily call ourselves. */
>+     /*return ssl3_ComputeCommonKeyHash(hashBuf, bufLen, hashes, PR_TRUE);*/

consider to remove the disabled call to ssl3_ComputeCommonKeyHash (and comment) if not used


>@@ -1790,6 +1799,12 @@ ssl3_InitPendingCipherSpec(sslSocket *ss
> 	}
>     }
>     if (ss->opt.bypassPKCS11 && pwSpec->msItem.len && pwSpec->msItem.data) {
>+#ifdef NOSSLBYPASS
>+	/* Enabling bypassPKCS11 should have been prevented in SSL_OptionSet */ 
>+	PORT_Assert(ss->opt.bypassPKCS11 == PR_FALSE);

PORT_Assert( !ss->opt.bypassPKCS11 );


r=kaie

Comment 6

[Elio Maldonado]

Found an error
if [ -n "${NSS_NOSSLBYPASS}" ]; then
	echo "${SCRIPTNAME}: bypass not supported."
else
SERVER_OPTIONS="-B -s"
end
should be
if [ -n "${NSS_NOSSLBYPASS}" ]; then
		echo "${SCRIPTNAME}: bypass not supported."
		else
		SERVER_OPTIONS="-B -s"
		fi

Comment 7

[Elio Maldonado]

Comment on attachment 614880 [details] [diff] [review]
Disable ssl pkcs11 layer bypass at build ti


>RCS file: /cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v
.....
>+                else
>+                	SERVER_OPTIONS="-B -s"
>+		  end <------------------------------ should be fi
>                 ;;
>             "fips")
>                 SERVER_OPTIONS=

Comment 8

[Wan-Teh Chang]

The C preprocessor macro and environment variable names should say
"NO PKCS11 BYPASS" rather than "NO SSL BYPASS".

I suggest adding underscores (_) to the macro and environment variable
names.  "NOSSLBYPASS" is not easy to read.

Comment 9

[Elio Maldonado]

Attachment: The patch as was applied in the stable branch

This version incorporates Kai's and Wan-Teh's requests and the needed fix to tests/ssl.sh Elio pointed out.

Comment 10

[Elio Maldonado]

Upon consultation with Wan-Teh Chang it's best to revert the previous commit.

Reversing the previous commit on the branch:
Checking in mozilla/security/nss/lib/ssl/config.mk;
/cvsroot/mozilla/security/nss/lib/ssl/config.mk,v  <--  config.mk
new revision: 1.31.6.2; previous revision: 1.31.6.1
done
Checking in mozilla/security/nss/lib/ssl/derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.13.2.2; previous revision: 1.13.2.1
done
Checking in mozilla/security/nss/lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.167.2.2; previous revision: 1.167.2.1
done
Checking in mozilla/security/nss/lib/ssl/ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.22.2.2; previous revision: 1.22.2.1
done
Checking in mozilla/security/nss/lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.82.2.3; previous revision: 1.82.2.2
done
Checking in mozilla/security/nss/tests/ssl/ssl.sh;
/cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v  <--  ssl.sh
new revision: 1.109.2.2; previous revision: 1.109.2.1
done

Comment 11

[Elio Maldonado]

I quote Wan-Teh's criticism and advise,
>The reason I don't like the SSL PKCS #11 bypass code is that it complicates
>the code.  Your patch nests C preprocessor #if with C if, complicating the
>code even more.
Yes, and is the same complaint Brain had and other would.

>If all you need is to prevent NSS users from enabling the bypass mode, then
>you can simply modify SSL_OptionSet, SSL_OptionSetDefault, and SSL_CanBypass
>to fail.
Thank you, that's music to my ears. Less it better. Will attach such a patch next.

Comment 12

[Elio Maldonado]

Attachment: Prevent users from enabllinf bypass

Comment 13

[Elio Maldonado]

Comment on attachment 614978 [details] [diff] [review]
Gets rid of sslbypass

I have obsoleted this patch here, but a patch like this belongs in Bug 692686.

Comment 14

[Wan-Teh Chang]

Comment on attachment 616851 [details] [diff] [review]
Prevent users from enabllinf bypass

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

r=wtc.  I suggest two changes.

::: ./mozilla/security/nss/lib/ssl/derive.c
@@ +589,5 @@
>                PRBool *pcanbypass, void *pwArg)
> +{
> +#ifdef NO_PKCS11_BYPASS
> +    *pcanbypass = PR_FALSE;
> +    return SECSuccess;

Reading the comments for SSL_CanBypass, I am not sure whether
we should return SECSuccess or SECFailure here.  This is a minor
issue though.

We should make sure pcanbypass is not a null pointer:

    if (!pcanbypass) {
        PORT_SetError(SEC_ERROR_INVALID_ARGS);
        return SECFailure;
    }

::: ./mozilla/security/nss/lib/ssl/sslsock.c
@@ +689,5 @@
>  	    rv = SECFailure;
>  	} else {
>              if (PR_FALSE != on) {
> +#ifdef NO_PKCS11_BYPASS
> +                ss->opt.bypassPKCS11   = PR_FALSE;

I think it's better to do
       PORT_SetError(PR_NOT_IMPLEMENTED_ERROR);
       rv = SECFailure;
in this case.  Similarly for SSL_OptionSetDefault.

Alternatively, we can change SSL_BypassSetup() to be

static PRStatus SSL_BypassSetup(void)
{
#ifdef NO_PKCS11_BYPASS
    PORT_SetError(PR_NOT_IMPLEMENTED_ERROR);
    return PR_FAILURE;
#else
    return PR_CallOnce(&setupBypassOnce, &SSL_BypassRegisterShutdown);
#endif
}

Then SSL_OptionSet and SSL_OptionSetDefault will automatically
do the right thing.

Comment 15

[Wan-Teh Chang]

Comment on attachment 616851 [details] [diff] [review]
Prevent users from enabllinf bypass

Elio: I looked at how the NSS test programs use SSL_CanBypass
and SSL_BYPASS_PKCS11.

selfserv exits with an error status if SSL_CanBypass returns
SECFailure.  This implies SSL_CanBypass needs to return SECSuccess
when NO_PKCS11_BYPASS is defined.

tstclnt, strclnt, and selfserv all exit with an error status if
the SSL_OptionSet call to enable SSL_BYPASS_PKCS11 fails.  This
means we need to either change our test suite to not run these
SSL test programs in bypass mode, or make SSL_OptionSet lie and
return SECSuccess (what your current patch does).

What is your opinion?  It would be nice to get a second opinion on this.

Comment 16

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #15)
> selfserv exits with an error status if SSL_CanBypass returns
> SECFailure.  This implies SSL_CanBypass needs to return SECSuccess
> when NO_PKCS11_BYPASS is defined.
> 
> tstclnt, strclnt, and selfserv all exit with an error status if
> the SSL_OptionSet call to enable SSL_BYPASS_PKCS11 fails.  This
> means we need to either change our test suite to not run these
> SSL test programs in bypass mode, or make SSL_OptionSet lie and
> return SECSuccess (what your current patch does).

> 
> What is your opinion?  It would be nice to get a second opinion on this.

We should not change the library code to lie, that's a no-no. If need be we change the tools which I admit is a bit of work. Changing the test scripts to be smart about having bypass disabled or not is preferable. I have tested the reduced as well as the fuller patch, that one with the ugly #ifdef's in ssl3sock.c and ssl3ext.c that huts our eyes, with this change to mozilla/security/nss/tests/ssl/ssl.sh
===================================================================
RCS file: /cvsroot/mozilla/security/nss/tests/ssl/ssl.sh,v
retrieving revision 1.109.2.2
diff -u -p -r1.109.2.2 ssl.sh
--- ./mozilla/security/nss/tests/ssl/ssl.sh	20 Apr 2012 00:37:54 -0000	1.109.2.2
+++ ./mozilla/security/nss/tests/ssl/ssl.sh	20 Apr 2012 22:17:06 -0000
@@ -958,7 +958,11 @@ ssl_run_tests()
                 SERVER_OPTIONS=
                 ;;
             "bypass")
+                if [ -n $NSS_NO_PKCS11_BYPASS ]; then
+                    SERVER_OPTIONS=
+                else
                 SERVER_OPTIONS="-B -s"
+                fi
                 ;;
             "fips")
and similarly for CLIENT_OPTIONS.
That makes fips_normal and normal_fips suites effectively normal_normal. I admit, it's a quick and dirty trick so I didn't have to muck around with the test code and scripts until later.

Comment 17

[Elio Maldonado]

The reduced patch is okay for the current stable 3-13 branch as well as for currently shipping versions of RHEL and Fedora but it doesn't meet all my needs for the next major version of RHEL and fedora-18.  I failed to mention that this bug comes also result of my efforts to build nss (the higher layers) without any softoken, freebl, or util sources in the tree, we package those separately as you may recall. We also the need to keep the higher layers from digging inside the crypto boundary. For f-18 I need the full patch. Yes, those ugly parts you and Brain rightfully complain about. I'll try to make it not much worst that it's already :-) Until we can do the true bypass code removal on 3.14 or later along with your other SSL cleanup.

Comment 18

[Elio Maldonado]

Attachment: Prevent users from enabing bypass (BRANCH)

Incorporates wtc's and suggestions, the tools now query the error for PR_NOT_IMPLEMENTED_ERROR so they don't report an error if that is the case. This is the essential patch.

Comment 19

[Elio Maldonado]

Attachment: Suplemmentary patch to compile out bypass code

Lot's of #ifdef's but they shouldn't make following the code much worst that it is already :-) Please notice the comment on ssl3_ComputeRecordMAC.

Comment 20

[Elio Maldonado]

Regarding that FIXME comment on ssl3conc.c on not being able to do the PORT_Assert(!ss->opt.bypassPKCS11); as I would like due to the detected memory leak, I experimented and I can activate the assertion when I replaced MD5 with SAH1 in the sslstress.txt. Why is this so?

Comment 21

[Robert Relyea]

>  quote Wan-Teh's criticism and advise,
>> The reason I don't like the SSL PKCS #11 bypass code is that it complicates
>> the code.  Your patch nests C preprocessor #if with C if, complicating the
>> code even more.
> Yes, and is the same complaint Brain had and other would.

While I agree with the sentament, The ifdef's for the BYPASS case are basically around existing C if statements:

original code:
    if (bypass) {
       do something
    } else  {
       do something else
    }

Elio's patch has
    if (bypass) {
#ifdef NO_BYPASS
         assert()
#else
         do something
#end
    } else {
         do something else
    }


Elio, I would prefer something like:

#ifndef NO_BYPASS
    if (bypass) {
        do something
    } else
#endif
    {
       do something else
    }
I don't really think we need to even look at the bypass flag and assert. Just simply make the code go way.

In either case the complication is having bypass at all.

bob

Comment 22

[Robert Relyea]

> We should not change the library code to lie, that's a no-no.

I disagree. Whatever happens in the upstream patch, you will need to return SECSuccess in RHEL because you can't change the API in RHEL releases.

You can remove Bypass because is doesn't actually change the semantics, only the timing. Also all your asserts need to go way, and setting Bypass has to silently succeed.

If wtc thinks we should do this, and I suggest you should do this, and you will be required to do this in RHEL, it's probably best to do this in your suggested upstream patch, no?;).

bob

Comment 23

[Elio Maldonado]

Attachment: Prevent user from enabling bypass, preserves ABI if needed, BRANCH not to be checked in

This patch is based on the stable branch sources and is identical the the one I have submitted for fedora downstream. It isn't intended to be checked in, one for the trunk coming next.

Comment 24

[Elio Maldonado]

Attachment: Prevent user from enabling bypass, preserves ABI if needed, TRUNK version

Comment 25

[Elio Maldonado]

Attachment: Removes bypass code unconditionally

This is what many would prefer. It properly belongs on Bug 692686.

Comment 26

[Wan-Teh Chang]

Comment on attachment 623743 [details] [diff] [review]
Prevent user from enabling bypass, preserves ABI if needed, TRUNK version

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

Elio: thank you for the new patch.  I suggest some changes
below.

High-level comments:

1. I think we should just make the relevant function lie
in a self-consistent way (to the extent possible), as we
did to other deprecated features.  This will minimize the
changes to NSS-based programs, including our own tests.

2. I suggest removing NSS_PRESERVE_ABI/PRESERVE_ABI, partly
because of the above suggestion, and partly because ABI
means something different.  (You're really referring to
preserving behavior, not ABI.)

::: ./mozilla/security/nss/lib/ssl/config.mk
@@ +8,5 @@
>  endif
>  
> +ifdef NSS_NO_PKCS11_BYPASS
> +DEFINES += -DNO_PKCS11_BYPASS
> +ifdef NSS_PRESERVE_ABI

This should not be nested inside ifdef NSS_NO_PKCS11_BYPASS.

Also, you didn't document the meaning of the PRESERVE_ABI macro.
I suggest that we not add the PRESERVE_ABI macro.

::: ./mozilla/security/nss/lib/ssl/derive.c
@@ +563,5 @@
> +        PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +        return SECFailure;
> +    }
> +#ifdef PRESERVE_ABI
> +    return PR_SUCCESS;

This should be SECSuccess, and the #else part should be SECFailure.

::: ./mozilla/security/nss/lib/ssl/ssl3con.c
@@ +969,5 @@
>      if (bypassPKCS11) {
>  	MD5_HashBuf (hashes->md5, hashBuf, bufLen);
>  	SHA1_HashBuf(hashes->sha, hashBuf, bufLen);
>      } else {
> +#endif

I suggest you use Bob's suggestion and replace
    } else {
with
    } else
#endif
    {

This allows you to avoid the change below:
#ifndef NO_PKCS11_BYPASS
    }
#endif

@@ +1775,5 @@
>  	}
>      } else if (pwSpec->master_secret) {
> +#else
> +    if (pwSpec->master_secret) {
> +#endif

This probably can use a similar trick:
    } else
#endif
    if (pwSpec->master_secret) {

::: ./mozilla/security/nss/lib/ssl/ssl3ext.c
@@ +695,5 @@
>  	    &mac_key_pkcs11);
>      }
> +#else
> +    rv = ssl3_GetSessionTicketKeysPKCS11(ss, &aes_key_pkcs11, &mac_key_pkcs11);
> +#endif

Use Bob's trick here?

Comment 27

[Elio Maldonado]

I fully agree with the suggested changes. I can also get rid of the changes to the tools.

Comment 28

[Elio Maldonado]

Attachment: Prevent user from enabling bypass

Comment 29

[Wan-Teh Chang]

Comment on attachment 624553 [details] [diff] [review]
Prevent user from enabling bypass

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

I suggest some changes below.

::: ./mozilla/security/nss/lib/ssl/derive.c
@@ +563,5 @@
> +        PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +        return SECFailure;
> +    }
> +    /* Guarantee binary compatibility */
> +    *pcanbypass = PR_TRUE;

I think we should set *pcanbypass to PR_FALSE, which
means we cannot bypass.

Returning SECSuccess should be enough to guarantee
binary compatibility.

::: ./mozilla/security/nss/lib/ssl/ssl3con.c
@@ +971,5 @@
>  	SHA1_HashBuf(hashes->sha, hashBuf, bufLen);
>      } else {
> +#else
> +    {
> +#endif

This is one way to do it.  Another way is to change

    } else {

to

    } else
#endif
    {

@@ +1916,2 @@
>      if (! spec->bypassCiphers) {
> +#endif

I think you can do this as follows:

    if (! spec->bypassCiphers) {

        ...

    } else {
#ifndef NO_PKCS11_BYPASS
        /* bypass version */
        ...
#endif
    }

This assumes spec->bypassCiphers is always false if
NO_PKCS11_BYPASS is defined.

@@ +3317,5 @@
>  	ss->ssl3.hs.messages.len = 0;
>  	MD5_Begin((MD5Context *)ss->ssl3.hs.md5_cx);
>  	SHA1_Begin((SHA1Context *)ss->ssl3.hs.sha_cx);
>      } else {
> +#endif

You should use the same trick here.

@@ +3352,5 @@
>  	PORT_Assert(!ss->ssl3.hs.messages.buf && !ss->ssl3.hs.messages.space);
>  	ss->ssl3.hs.messages.buf = NULL;
>  	ss->ssl3.hs.messages.space = 0;
>      } else {
> +#endif

Use the trick here.

@@ +3791,5 @@
>  #undef shacx
>      } else {
> +#else
> +    {
> +#endif /* notdef NO_PKCS11_BYPASS */

Consider doing

    } else
#endif
    {

@@ +7792,4 @@
>  	rv = ssl3_InitPendingCipherSpec(ss,  NULL);
>      } else {
>  double_bypass:
> +#endif

Can we use the trick here?  The double_bypass: label may make
it impossible.

::: ./mozilla/security/nss/lib/ssl/ssl3ext.c
@@ +690,5 @@
>      if (ss->opt.bypassPKCS11) {
>  	rv = ssl3_GetSessionTicketKeys(&aes_key, &aes_key_length,
>  	    &mac_key, &mac_key_length);
>      } else {
> +#endif

Use the trick here.

@@ +864,1 @@
>      if (ss->opt.bypassPKCS11) {

Do this block as follows:
    if (ss->opt.bypassPKCS11) {
#ifndef NO_PKCS11_BYPASS
        ...
#endif
    } else {
        ...
    }

if ss->opt.bypassPKCS11 is always false when NO_PKCS11_BYPASS
is defined.

If this works, please make the same change to every
    if (ss->opt.bypassPKCS11) {
    } else {
    }
statement in this file.

@@ +910,5 @@
>  	    sizeof(computed_mac));
>      } else {
> +#else
> +    {
> +#endif

Consider doing this:

    } else
#endif
    {

::: ./mozilla/security/nss/lib/ssl/sslsock.c
@@ +550,5 @@
>  static PRStatus SSL_BypassSetup(void)
>  {
> +#ifdef NO_PKCS11_BYPASS
> +    /* Guarantee binary compatibility */
> +	return PR_SUCCESS;

Indent by only four spaces.

Comment 30

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #29)

> I think we should set *pcanbypass to PR_FALSE, which
> means we cannot bypass.
> 
> Returning SECSuccess should be enough to guarantee
> binary compatibility.

That's good news. It works and gets rid of one white lie.

> 
> ::: ./mozilla/security/nss/lib/ssl/ssl3con.c
> @@ +971,5 @@
> >  	SHA1_HashBuf(hashes->sha, hashBuf, bufLen);
> >      } else {
> > +#else
> > +    {
> > +#endif
> 
> This is one way to do it.  Another way is to change
> 
>     } else {
> 
> to
> 
>     } else
> #endif
>     {
I like as it gets rid of the #else. The less we have to read the better.
> 
> @@ +1916,2 @@
> >      if (! spec->bypassCiphers) {
> > +#endif
> 
> I think you can do this as follows:
> 
>     if (! spec->bypassCiphers) {
> 
>         ...
> 
>     } else {
> #ifndef NO_PKCS11_BYPASS
>         /* bypass version */
>         ...
> #endif
>     }
> 
> This assumes spec->bypassCiphers is always false if
> NO_PKCS11_BYPASS is defined.
And I have verified this with all those assertions I use to have. Reduces the amount of conditionals we have to read.
> 
> @@ +3317,5 @@
> >  	ss->ssl3.hs.messages.len = 0;
> >  	MD5_Begin((MD5Context *)ss->ssl3.hs.md5_cx);
> >  	SHA1_Begin((SHA1Context *)ss->ssl3.hs.sha_cx);
> >      } else {
> > +#endif
> 
> You should use the same trick here.
> 
> @@ +3352,5 @@
> >  	PORT_Assert(!ss->ssl3.hs.messages.buf && !ss->ssl3.hs.messages.space);
> >  	ss->ssl3.hs.messages.buf = NULL;
> >  	ss->ssl3.hs.messages.space = 0;
> >      } else {
> > +#endif
> 
> Use the trick here.
> 
> @@ +3791,5 @@
> >  #undef shacx
> >      } else {
> > +#else
> > +    {
> > +#endif /* notdef NO_PKCS11_BYPASS */
> 
> Consider doing
> 
>     } else
> #endif
>     {
> 
> @@ +7792,4 @@
> >  	rv = ssl3_InitPendingCipherSpec(ss,  NULL);
> >      } else {
> >  double_bypass:
> > +#endif
> 
> Can we use the trick here?  The double_bypass: label may make
> it impossible.
Actually I can workaround the problem by conditionalizing the label.
I
 ...
 rv = ssl3_InitPendingCipherSpec(ss,  NULL);
#endif
   } else {
#ifndef NSS_PKCS11_BYPASS
double_bypass:
#endif

I know that it is a bit ugly but it is preferable to what I have now which is
...
#ifndef NSS_PKCS11_BYPASS
}
#endif
over thirty lines below in the code and made the code hard to read.

> 
> ::: ./mozilla/security/nss/lib/ssl/ssl3ext.c
> @@ +690,5 @@
> >      if (ss->opt.bypassPKCS11) {
> >  	rv = ssl3_GetSessionTicketKeys(&aes_key, &aes_key_length,
> >  	    &mac_key, &mac_key_length);
> >      } else {
> > +#endif
> 
> Use the trick here.
> 
> @@ +864,1 @@
> >      if (ss->opt.bypassPKCS11) {
> 
> Do this block as follows:
>     if (ss->opt.bypassPKCS11) {
> #ifndef NO_PKCS11_BYPASS
>         ...
> #endif
>     } else {
>         ...
>     }
> 
> if ss->opt.bypassPKCS11 is always false when NO_PKCS11_BYPASS
> is defined.
> 
> If this works, please make the same change to every
>     if (ss->opt.bypassPKCS11) {
>     } else {
>     }
> statement in this file.
Yes, and I hope you don't mind if I make about making similar changes on ssl3con.c as well.
Will do it where it buys me readability.

> @@ +910,5 @@
> >  	    sizeof(computed_mac));
> >      } else {
> > +#else
> > +    {
> > +#endif
> 
> Consider doing this:
> 
>     } else
> #endif
>     {
> 
> ::: ./mozilla/security/nss/lib/ssl/sslsock.c
> @@ +550,5 @@
> >  static PRStatus SSL_BypassSetup(void)
> >  {
> > +#ifdef NO_PKCS11_BYPASS
> > +    /* Guarantee binary compatibility */
> > +	return PR_SUCCESS;
> 
> Indent by only four spaces.
Will do.

Comment 31

[Elio Maldonado]

Attachment: Prevent user from enabling bypass

Comment 32

[Elio Maldonado]

Comment on attachment 625144 [details] [diff] [review]
Prevent user from enabling bypass

After a lot of very good suggestions this bug has gotten silent. Adding Bob in case Wan-Teh is tied up with too many reviews.

Comment 33

[Wan-Teh Chang]

Comment on attachment 625144 [details] [diff] [review]
Prevent user from enabling bypass

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

Hi Elio,

This is very close now.  Could you try the changes I suggested below?
Thanks.

::: ./mozilla/security/nss/lib/ssl/ssl3con.c
@@ +1757,4 @@
>  	    goto done;  /* err code set by ssl3_DeriveMasterSecret */
>  	}
>      }
> +#ifndef NO_PKCS11_BYPASS

Can you move this inside
    if (ss->opt.bypassPKCS11 && pwSpec->msItem.len && pwSpec->msItem.data) {
?

@@ +1772,5 @@
>  	if (rv == SECSuccess) {
>  	    rv = ssl3_InitPendingContextsBypass(ss);
>  	}
> +    } else
> +#endif

Can you move this up?

@@ +3126,4 @@
>  	return rv;
>      }
>      if (ss->opt.bypassPKCS11) {
> +#ifndef NO_PKCS11_BYPASS

You can move #ifndef NO_PKCS11_BYPASS to include the
    if (ss->opt.bypassPKCS11) {
and the closing curly brace
    }

::: ./mozilla/security/nss/lib/ssl/ssl3ext.c
@@ +1324,4 @@
>  	    sid->keaKeyBits = parsed_session_ticket->keaKeyBits;
>  
>  	    /* Copy master secret. */
> +#ifndef NO_PKCS11_BYPASS

I believe this change is not necessary.

Comment 34

[Elio Maldonado]

Attachment: Prevent user from enabling bypass - address comment 33 requests

Comment 35

[Wan-Teh Chang]

Attachment: Complete patch

Hi Elio,

I decided to take a closer look at your patch by applying
it to a source tree and building it.  With this patch, I
can build lib/ssl without libfreebl.a and blapi.h.

I also fixed compiler warnings about unused variables and
unused functions, and commented out the code that sets
the default value of the bypassPKCS11 option based on the
SSLBYPASS environment variable.

Note that sslsnce.c calls SHA256_HashBuf, which is bad.
I replaced it with a HASH_HashBuf call, inside an #ifdef.
Perhaps we should always call HASH_HashBuf.

Please compare your latest patch with this patch and
combine the two patches.  Thanks.

Comment 36

[Wan-Teh Chang]

Comment on attachment 630691 [details] [diff] [review]
Prevent user from enabling bypass - address comment 33 requests

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

This patch contains unrelated changes to selfserv.c and manifest.mn.

Below I describe a better solution I came up with for one particular place.

::: ./mozilla/security/nss/lib/ssl/ssl3con.c
@@ +970,5 @@
>  	MD5_HashBuf (hashes->md5, hashBuf, bufLen);
>  	SHA1_HashBuf(hashes->sha, hashBuf, bufLen);
> +    } else
> +#endif
> +    {

In my patch, I decided to handle this as follows:

    if (bypassPKCS11) {
#ifndef NO_PKCS11_BYPASS
        MD5_HashBuf (hashes->md5, hashBuf, bufLen);
        SHA1_HashBuf(hashes->sha, hashBuf, bufLen);
#endif
    } else {

This is safe because bypassPKCS11 is always false in the
NO_PKCS11_BYPASS build.  (I verified it, but you may want
to doublecheck it.)

Comment 37

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #36)
> Comment on attachment 630691 [details] [diff] [review]
> Prevent user from enabling bypass - address comment 33 requests
> 
> Review of attachment 630691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch contains unrelated changes to selfserv.c and manifest.mn.
I forgot to remove those changes are which meant for fedora only. By the way, they should look familiar to you as they are the temporary workaround for fedora you wrote.
> 
> Below I describe a better solution I came up with for one particular place.
> 
> ::: ./mozilla/security/nss/lib/ssl/ssl3con.c
> @@ +970,5 @@
> >  	MD5_HashBuf (hashes->md5, hashBuf, bufLen);
> >  	SHA1_HashBuf(hashes->sha, hashBuf, bufLen);
> > +    } else
> > +#endif
> > +    {
> 
> In my patch, I decided to handle this as follows:
> 
>     if (bypassPKCS11) {
> #ifndef NO_PKCS11_BYPASS
>         MD5_HashBuf (hashes->md5, hashBuf, bufLen);
>         SHA1_HashBuf(hashes->sha, hashBuf, bufLen);
> #endif
>     } else {
> 
> This is safe because bypassPKCS11 is always false in the
> NO_PKCS11_BYPASS build.  (I verified it, but you may want
> to doublecheck it.)

I was going to suggest this very same change. I have actually tested it with bypass allowed and disabled. On to the comparison of patches.

Comment 38

[Elio Maldonado]

Comment on attachment 630755 [details] [diff] [review]
Complete patch

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

r+ for all the additions by wtc to complete the patch plus the additional modification mentioned in his last comment.

Comment 39

[Wan-Teh Chang]

Elio: you are welcome to check in the complete patch
because you have been working on this bug.  Thanks.

Comment 40

[Elio Maldonado]

Chcked in to trunk:
Checking in mozilla/security/nss/lib/ssl/Makefile;
/cvsroot/mozilla/security/nss/lib/ssl/Makefile,v  <--  Makefile
new revision: 1.11; previous revision: 1.10
done
Checking in mozilla/security/nss/lib/ssl/config.mk;
/cvsroot/mozilla/security/nss/lib/ssl/config.mk,v  <--  config.mk
new revision: 1.33; previous revision: 1.32
done
Checking in mozilla/security/nss/lib/ssl/derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/security/nss/lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.183; previous revision: 1.182
done
Checking in mozilla/security/nss/lib/ssl/ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.29; previous revision: 1.28
done
Checking in mozilla/security/nss/lib/ssl/ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.25; previous revision: 1.24
done
Checking in mozilla/security/nss/lib/ssl/sslsnce.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsnce.c,v  <--  sslsnce.c
new revision: 1.62; previous revision: 1.61
done
Checking in mozilla/security/nss/lib/ssl/sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.91; previous revision: 1.90
done

Comment 41

[Elio Maldonado]

Attachment: The patch as it was checked in - on account for recent code movement

Comment 42

[Elio Maldonado]

Reopening as not all review requests have been met.

On Comment 21 Bob expressed is preference forb doing the conditionalization as follows:
#ifndef NO_PKCS11_BYPASS
    if (bypass) {
        do something
    } else
#endif
    {
       do something else
    }
... and recently he strongly reiterated that preference in a downstream fedora bug.
Additionally, in Comment 26 Wan_Teh endorsed Bob's suggestion. Somehow with all
the other changes that request was not fullfilled. A patch for it is next.

Comment 43

[Elio Maldonado]

Attachment: Fix way we conditionalize per request on comment 21

Comment 44

[Robert Relyea]

The reason I prefer

#ifdef NO_PKCS11_BYPASS
   if (bypass) {
     do something
   } else
#endif
   {

is that is is less brittle to mistakes in turning bypass on. If somehow the bypass flag is flipped on we will have very weird failures if we have code that reads:

   if (bypass) {
#ifdef NO_PKCS11_BYPASS
        do something
#endif
   } else {

Comment 45

[Wan-Teh Chang]

Comment on attachment 643026 [details] [diff] [review]
Fix way we conditionalize per request on comment 21

Note: I endorsed Bob's suggestion before I discovered
that we can do

   if (bypass) {
#ifdef NO_PKCS11_BYPASS
        do something
#endif
   } else {

to nest "if" and "#if" cleanly.  The original form

#ifdef NO_PKCS11_BYPASS
   if (bypass) {
     do something
   } else
#endif
   {

is harder to read.  I think the current form makes the
right trade-off.

Comment 46

[Robert Relyea]

> is harder to read. 

I disagree. I  think the are both quite easy to read.

> I think the current form makes the right trade-off.

I strongly disagree.

1) leaving the if in the code is just asking for trouble. It will be a debugging nightmare if bypass every gets set.
2) it leave more code and what should be non-op if. It makes the code much harder to review for correctness.

The are both relatively easy to read. In both cases there is a single if. What is more I view the later case actually dangerous to leave in the code, which is why I rejected the earliest patch Elio provided (which is actually a little bit safer because it asserted if bypass is set.

bob

Comment 47

[Robert Relyea]

(See comment 21. Why would I think the current form proposed by wtc would be OK if I rejected the form in comment 21?).

bob

Comment 48

[Robert Relyea]

Comment on attachment 643026 [details] [diff] [review]
Fix way we conditionalize per request on comment 21

r+

But I'd like you to fix one case:

in ssl3_ComputeRecordMac, the code is structures as follows:

if (!bypass) {
    do something
} else {
    long do something else
}

In this case

if (!bypass) {
    do something;
} else {
#ifdef NO_BYPASS
    long do something else
#endif
}

and

if (!bypass) {
   do something;
}
#ifdef NO_BYPASS
else {
   long do something else
}
#endif 

are functionally the same. The following would be more readable to future readers of the code (though it will generate a bigger diff):

#ifdef NO_BYPASS
if (bypass) {
   long do something else
} else
#endif
{
  do something
}

Note that both the sense of the if and the imbedded bodies have been swapped.

r+ because the current patch is an improvement and the only remaining problem is pre-existing.

bob

Comment 49

[Elio Maldonado]

Checked in to trunk
Checking in mozilla/security/nss/lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.186; previous revision: 1.185
done
Checking in mozilla/security/nss/lib/ssl/ssl3ext.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ext.c,v  <--  ssl3ext.c
new revision: 1.27; previous revision: 1.26
done

Comment 50

[Elio Maldonado]

Attachment: Reviewed patch ammended per Bob's request

Patch as it was checked in to the trunk.

Comment 51

[Julien Pierre]

Does this change mean that RH & Google are building and testing NSS only without bypass ?

Are we confident that the bypass code still works, or is this something Oracle is likely to find out only after we try it ?

Comment 52

[Wan-Teh Chang]

The bypass code is compiled in the default NSS build and still works.