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

RESOLVED FIXED in 3.14

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Elio Maldonado, Assigned: Elio Maldonado)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 10 obsolete attachments)

36.99 KB, patch
Details | Diff | Splinter Review
15.03 KB, patch
Wan-Teh Chang
: review-
Details | Diff | Splinter Review
40.18 KB, patch
Elio Maldonado
: review+
Details | Diff | Splinter Review
23.31 KB, patch
Details | Diff | Splinter Review
9.23 KB, patch
Robert Relyea
: review+
Details | Diff | Splinter Review
9.87 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
(Assignee)

Comment 1

6 years ago
Created attachment 614880 [details] [diff] [review]
Disable ssl pkcs11 layer bypass at build ti

Enabled by a build-time environment variable.
Assignee: nobody → emaldona
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.
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
Created attachment 614978 [details] [diff] [review]
Gets rid of sslbypass

Comment 5

6 years ago
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
Attachment #614880 - Flags: review+
(Assignee)

Comment 6

6 years ago
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
(Assignee)

Comment 7

6 years ago
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

6 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 616794 [details] [diff] [review]
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.
(Assignee)

Comment 10

6 years ago
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
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
Created attachment 616851 [details] [diff] [review]
Prevent users from enabllinf bypass
Attachment #614880 - Attachment is obsolete: true
Attachment #614978 - Attachment is obsolete: true
Attachment #616794 - Attachment is obsolete: true
Attachment #616851 - Flags: review?(wtc)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 13

6 years ago
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

6 years ago
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.
Attachment #616851 - Flags: review?(wtc) → review+

Comment 15

6 years ago
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.
(Assignee)

Comment 16

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
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.
(Assignee)

Comment 18

6 years ago
Created attachment 618347 [details] [diff] [review]
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.
Attachment #616851 - Attachment is obsolete: true
Attachment #618347 - Flags: review+
(Assignee)

Comment 19

6 years ago
Created attachment 618349 [details] [diff] [review]
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.
Attachment #618349 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #618347 - Flags: review+ → review?(wtc)
(Assignee)

Updated

6 years ago
Attachment #618349 - Flags: review+ → review?(wtc)
(Assignee)

Comment 20

6 years ago
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?
(Assignee)

Updated

6 years ago
Attachment #618347 - Flags: superreview?(rrelyea)
(Assignee)

Updated

6 years ago
Attachment #618349 - Flags: superreview?(rrelyea)

Comment 21

6 years ago
>  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

6 years ago
> 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
(Assignee)

Comment 23

6 years ago
Created attachment 623742 [details] [diff] [review]
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.
Attachment #618347 - Attachment is obsolete: true
Attachment #618349 - Attachment is obsolete: true
Attachment #618347 - Flags: superreview?(rrelyea)
Attachment #618347 - Flags: review?(wtc)
Attachment #618349 - Flags: superreview?(rrelyea)
Attachment #618349 - Flags: review?(wtc)
Attachment #623742 - Flags: review?(rrelyea)
(Assignee)

Updated

6 years ago
Attachment #623742 - Attachment description: Prevent user from enabling bypass, preserves ABI if needed, not be checked in → Prevent user from enabling bypass, preserves ABI if needed, not innteded to be checked in
(Assignee)

Comment 24

6 years ago
Created attachment 623743 [details] [diff] [review]
Prevent user from enabling bypass, preserves ABI if needed, TRUNK version
Attachment #623743 - Flags: review?(rrelyea)
(Assignee)

Comment 25

6 years ago
Created attachment 623751 [details] [diff] [review]
Removes bypass code unconditionally

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

Updated

6 years ago
Attachment #623743 - Flags: review?(wtc)
(Assignee)

Updated

6 years ago
Attachment #623742 - Attachment description: Prevent user from enabling bypass, preserves ABI if needed, not innteded to be checked in → Prevent user from enabling bypass, preserves ABI if needed, BRANCH not to be checked in

Comment 26

6 years ago
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?
Attachment #623743 - Flags: review?(wtc) → review-
(Assignee)

Comment 27

6 years ago
I fully agree with the suggested changes. I can also get rid of the changes to the tools.
(Assignee)

Comment 28

6 years ago
Created attachment 624553 [details] [diff] [review]
Prevent user from enabling bypass
Attachment #623742 - Attachment is obsolete: true
Attachment #623743 - Attachment is obsolete: true
Attachment #623742 - Flags: review?(rrelyea)
Attachment #623743 - Flags: review?(rrelyea)
Attachment #624553 - Flags: review?(wtc)
(Assignee)

Updated

6 years ago
Target Milestone: --- → 3.14

Comment 29

6 years ago
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.
Attachment #624553 - Flags: review?(wtc) → review-
(Assignee)

Comment 30

6 years ago
(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.
(Assignee)

Comment 31

6 years ago
Created attachment 625144 [details] [diff] [review]
Prevent user from enabling bypass
Attachment #624553 - Attachment is obsolete: true
Attachment #625144 - Flags: review?(wtc)
(Assignee)

Comment 32

6 years ago
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.
Attachment #625144 - Flags: review?(rrelyea)

Comment 33

6 years ago
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.
(Assignee)

Comment 34

6 years ago
Created attachment 630691 [details] [diff] [review]
Prevent user from enabling bypass - address comment 33 requests
Attachment #625144 - Attachment is obsolete: true
Attachment #625144 - Flags: review?(wtc)
Attachment #625144 - Flags: review?(rrelyea)
Attachment #630691 - Flags: review?(wtc)

Comment 35

6 years ago
Created attachment 630755 [details] [diff] [review]
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

6 years ago
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.)
Attachment #630691 - Flags: review?(wtc) → review-
(Assignee)

Comment 37

6 years ago
(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.
(Assignee)

Comment 38

6 years ago
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.
Attachment #630755 - Flags: review+

Comment 39

6 years ago
Elio: you are welcome to check in the complete patch
because you have been working on this bug.  Thanks.
(Assignee)

Comment 40

6 years ago
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
(Assignee)

Comment 41

6 years ago
Created attachment 631794 [details] [diff] [review]
The patch as it was checked in - on account for recent code movement
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 42

5 years ago
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.
(Assignee)

Comment 43

5 years ago
Created attachment 643026 [details] [diff] [review]
Fix way we conditionalize per request on comment 21
Attachment #643026 - Flags: review?(rrelyea)

Comment 44

5 years ago
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

5 years ago
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

5 years ago
> 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

5 years ago
(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

5 years ago
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
Attachment #643026 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 49

5 years ago
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
(Assignee)

Comment 50

5 years ago
Created attachment 647030 [details] [diff] [review]
Reviewed patch ammended per Bob's request

Patch as it was checked in to the trunk.
(Assignee)

Updated

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All

Comment 51

5 years ago
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

5 years ago
The bypass code is compiled in the default NSS build and still works.
(Assignee)

Updated

4 years ago
Blocks: 360426
You need to log in before you can comment on or make changes to this bug.