668001 - libssl: SSL_OptionSetDefault and SSL_OptionGetDefault do not account for environment variables like SSLBYPASS and NSS_*

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

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

Brian wrote:
> With SSLBYPASS, the environment variable is checked the first time a socket
> is created. That means that if the application called SSL_OptionSetDefault
> before creating a socket, the environment variable will override the
> explicit call to SSL_OptionSetDefault. ...
> ... I will file a separate bug
> about more clearly documenting the advantages/disadvantages of using
> SSL_OptionSetDefault as it related to environment variables.

Wan-Teh wrote:
Thank you for the explanation.  This is the bug I feared.
SSL_OptionSetDefault should override environment variables.  We should
fix this bug for all environment variables with a separate patch.

Comment 1

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

Also, SSL_OptionGetDefault will return the wrong results until a socket is created.

Comment 2

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

Attachment: Make SSL_OptionSetDefault and SSL_OptionGetDefault inspect environment

Comment 3

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

This should go into 3.12.11 so that applications can use SSL_OptionSetDefault to properly control the new option from bug 665814.

Comment 4

[Kai Engert (:KaiE:)]

I believe in yesterday's conference call, Brian explained why this is indeed needed, so we're waiting for a review from Wan-Teh.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I object.  The documented behavior of these environment variables is that they
override all application behavior.  IMO, the environment variables should trump all other calls.  I've always envisioned them this way.  They exist to allow users to change the behaviors of applications, even those that call "...SetDefault".

Comment 6

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

Nelson, we cannot make the environment variables trump SSL_OptionSet because that would break backward compatibility.

I am happy to let them continue to override SSL_OptionSetDefault. I can just change Gecko to always use SSL_OptionSet and never SSL_OptionSetDefault. (I would like to make this change anyway.) But, SSL_OptionSet must continue to work the same way it does now.

Comment 7

[Wan-Teh Chang]

I don't know where the precedence is documented.  My expected
precedence is as follows, from high to low:

  SSL_OptionSet
  SSL_OptionSetDefault
  environment variables
  initializers for the static variable 'ssl_defaults'

Environment variables change the defaults of the NSS library.
Applications should then be able to change an option to the
desired value.

I believe this is what the current libSSL code intends to do,
but it does that imperfectly; the side effect of an
SSL_OptionSetDefault call before the first ssl_Socket call is
discarded.

To implement the behavior Nelson described in comment 5,
SSL_OptionSetDefault needs to honor the defaults specified
by environment variables, but this is only done for the
SSL_NO_LOCKS option using the SSLFORCELOCKS environmet
variable.  It is not done for the other three environment
variables Nelson added: SSLBYPASS, NSS_SSL_ENABLE_RENEGOTIATION,
and NSS_SSL_REQUIRE_SAFE_NEGOTIATION.

Comment 8

[Wan-Teh Chang]

Comment on attachment 545006 [details] [diff] [review]
Make SSL_OptionSetDefault and SSL_OptionGetDefault inspect environment

r=wtc.  This patch is fine by me, but please wait for Nelson
and Bob's review.

Please make a reasonable effort to follow Nelson and Bob's
distinctive coding styles when modifying their code.  For
example, Nelson often aligns the variable names in variable
declarations, etc.

> /* forward declarations. */
> static sslSocket *ssl_NewSocket(PRBool makeLocks);
> static SECStatus  ssl_MakeLocks(sslSocket *ss);
>+static void ssl_SetDefaultsFromEnvironment();
> static PRStatus   ssl_PushIOLayer(sslSocket *ns, PRFileDesc *stack, 
>                                   PRDescIdentity id);

So, here you should align ssl_SetDefaultsFromEnvironment
with the other function names.

>+static void
>+ssl_SetDefaultsFromEnvironment()

Nit: please add a short comment to document what this
function does.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 6:
Perhaps I was unclear.  
I meant that environment variables override SetDefault.
(Did I not write that?) 

I do NOT propose that they override OptionSet.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 545006 [details] [diff] [review]
Make SSL_OptionSetDefault and SSL_OptionGetDefault inspect environment

I agree in principle with this patch.  The code looks good, too. :)

Comment 11

[Wan-Teh Chang]

(In reply to comment #9)
>
> I meant that environment variables override SetDefault.

Nelson, I am still confused.  Brian's patch enables SSL_OptionSetDefault
to override environment variables.

For an environment to override SSL_OptionSetDefault, we need code like this:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsock.c&rev=1.70&mark=964,969-970#964

There is such code only for the SSLFORCELOCKS environment variable.

Comment 12

[Wan-Teh Chang]

Attachment: Make SSL_OptionSetDefault and SSL_OptionGetDefault inspect environment, v2

I edited Brian's patch (add the "void" argument list,
required for C) and checked it in on the NSS trunk (NSS 3.13)
and NSS_3_12_BRANCH (NSS 3.12.11).

Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.71; previous revision: 1.70
done

Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.67.2.3; previous revision: 1.67.2.2
done