The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in 3.12.11

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

trunk
3.12.11

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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_*
Created attachment 545006 [details] [diff] [review]
Make SSL_OptionSetDefault and SSL_OptionGetDefault inspect environment
Attachment #545006 - Flags: review?(wtc)
This should go into 3.12.11 so that applications can use SSL_OptionSetDefault to properly control the new option from bug 665814.
Blocks: 665814
Target Milestone: --- → 3.12.11

Comment 4

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

Updated

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

Comment 7

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

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

Comment 11

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

6 years ago
Created attachment 548454 [details] [diff] [review]
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
Attachment #545006 - Attachment is obsolete: true
Attachment #545006 - Flags: superreview?(rrelyea)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Priority: -- → P2
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.