Closed Bug 668001 Opened 13 years ago Closed 13 years ago

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


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: briansmith, Assigned: briansmith)




(1 file, 1 obsolete file)

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.
Also, SSL_OptionGetDefault will return the wrong results until a socket is created.
Assignee: nobody → bsmith
Summary: libssl: SSL_OptionSetDefault do not override environment variables like SSLBYPASS and NSS_* → libssl: SSL_OptionSetDefault and SSL_OptionGetDefault do not account for environment variables like SSLBYPASS and NSS_*
This should go into 3.12.11 so that applications can use SSL_OptionSetDefault to properly control the new option from bug 665814.
Target Milestone: --- → 3.12.11
I believe in yesterday's conference call, Brian explained why this is indeed needed, so we're waiting for a review from Wan-Teh.
Attachment #545006 - Flags: superreview?(rrelyea)
Attachment #545006 - Attachment is patch: true
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".
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.
I don't know where the precedence is documented.  My expected
precedence is as follows, from high to low:

  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

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
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

Nit: please add a short comment to document what this
function does.
Attachment #545006 - Flags: review?(wtc) → review+
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 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. :)
Attachment #545006 - Flags: review+
(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:,969-970#964

There is such code only for the SSLFORCELOCKS environment variable.
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

Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision:; previous revision:
Attachment #545006 - Attachment is obsolete: true
Attachment #545006 - Flags: superreview?(rrelyea)
Closed: 13 years ago
Priority: -- → P2
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.