1391506 - Defining new flag values for the tlsFlags attribute (e.g., max version, fallback limit, enabling AltServerHello)

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Comment 1

[Sajjad Arshad]

Attachment: define-tls-flags-values.patch

In this patch, we are adding flags for setting max version and fallback limit, and enable AltServerHello. Here is the link to Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=658b60cdf0150763d9d2e28cec227aea87125014&selectedJob=124042718

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8898591 [details] [diff] [review]
define-tls-flags-values.patch

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

your flags are designed to be opaque - so don't define them in a public IDL or you will spend your life supporting the ABI. (stick them in a psm h file or at least some new idl perhaps named nsITLSInternal.idl

Comment 3

[Sajjad Arshad]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> Comment on attachment 8898591 [details] [diff] [review]
> define-tls-flags-values.patch
> 
> Review of attachment 8898591 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> your flags are designed to be opaque - so don't define them in a public IDL
> or you will spend your life supporting the ABI. (stick them in a psm h file
> or at least some new idl perhaps named nsITLSInternal.idl

That's a good point. I will remove these IDL definitions because they won't be permanent.

I was also wondering if you know of any existing unit test that I can use as a template to test these new flag values?

Thanks,

Comment 4

[Patrick McManus [:mcmanus]]

I don't know of a relevant test.. but that's more the land of where psm and keeler might help.

Comment 5

[Sajjad Arshad]

Attachment: define-tls-flags-values.patch

The issues were resolved, and the new Try push was completed without errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff6f390175f555e6c7564ff2772291c78faa6aab&selectedJob=124266834

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8899061 [details] [diff] [review]
define-tls-flags-values.patch

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

Unfortunately, I don't think this approach will work for the TLS intolerance fallback settings. I'm not sure what the best approach would be, though. We might end up needing to restructure how the nsSSLIOLayerHelpers objects work. First, though, I would start by expanding nsITLSServerSocket so we can actually test the features we're looking to enable here.

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +1681,5 @@
>    // see nsNSSComponent::setEnabledTLSVersions for pref handling rules
>    uint32_t limit = Preferences::GetUint("security.tls.version.fallback-limit",
>                                          3); // 3 = TLS 1.2
> +
> +  // set fallback limit if it is set in the tls flags

We need to document the possible values and effects of the tls flags in one centralized location.

@@ +1683,5 @@
>                                          3); // 3 = TLS 1.2
> +
> +  // set fallback limit if it is set in the tls flags
> +  uint32_t tlsFlagsFallbackLimit = (mTlsFlags & (7 << 3)) >> 3;
> +  if (tlsFlagsFallbackLimit)

style nit: always use braces around conditional bodies

@@ +2464,5 @@
> +    case 4:
> +      range.max = SSL_LIBRARY_VERSION_TLS_1_3;
> +      break;
> +  }
> +  

nit: there are unnecessary space characters on this line

@@ +2605,5 @@
>    PRStatus stat;
>  
> +  SharedSSLState* sharedState = nullptr;
> +
> +  if (providerTlsFlags)

nit: braces

@@ +2606,5 @@
>  
> +  SharedSSLState* sharedState = nullptr;
> +
> +  if (providerTlsFlags)
> +    sharedState = new SharedSSLState(providerTlsFlags);

This is a memory leak. I also don't think this approach will work. If we're creating a new SharedSSLState for each new socket, we're not actually sharing any state. I could be wrong, but things like TLS intolerance fallback won't work if custom tlsFlags are set, because the backend will never see that previous attempts failed (i.e. information from rememberIntolerantAtVersion won't be remembered, and adjustForTLSIntolerance will have no effect because there's no remembered information). It might be best to write a few tests that exercise the functionality you're looking to implement to confirm or disprove this assumption. We might need to expand nsITLSServerSocket ( https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsITLSServerSocket.idl ) to have an additional callback so that we can simulate various server behaviors such as TLS version intolerance (e.g. add a user-settable callback in TLSServerSocket::AuthCertificateHook like in TLSServerConnectionInfo::HandshakeCallback - see https://dxr.mozilla.org/mozilla-central/source/netwerk/base/TLSServerSocket.cpp ).

Comment 7

[Sajjad Arshad]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)
> Comment on attachment 8899061 [details] [diff] [review]
> define-tls-flags-values.patch
> 
> Review of attachment 8899061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately, I don't think this approach will work for the TLS intolerance
> fallback settings. I'm not sure what the best approach would be, though. We
> might end up needing to restructure how the nsSSLIOLayerHelpers objects
> work. First, though, I would start by expanding nsITLSServerSocket so we can
> actually test the features we're looking to enable here.
> 
> ::: security/manager/ssl/nsNSSIOLayer.cpp
> @@ +1681,5 @@
> >    // see nsNSSComponent::setEnabledTLSVersions for pref handling rules
> >    uint32_t limit = Preferences::GetUint("security.tls.version.fallback-limit",
> >                                          3); // 3 = TLS 1.2
> > +
> > +  // set fallback limit if it is set in the tls flags
> 
> We need to document the possible values and effects of the tls flags in one
> centralized location.
> 
> @@ +1683,5 @@
> >                                          3); // 3 = TLS 1.2
> > +
> > +  // set fallback limit if it is set in the tls flags
> > +  uint32_t tlsFlagsFallbackLimit = (mTlsFlags & (7 << 3)) >> 3;
> > +  if (tlsFlagsFallbackLimit)
> 
> style nit: always use braces around conditional bodies
> 
> @@ +2464,5 @@
> > +    case 4:
> > +      range.max = SSL_LIBRARY_VERSION_TLS_1_3;
> > +      break;
> > +  }
> > +  
> 
> nit: there are unnecessary space characters on this line
> 
> @@ +2605,5 @@
> >    PRStatus stat;
> >  
> > +  SharedSSLState* sharedState = nullptr;
> > +
> > +  if (providerTlsFlags)
> 
> nit: braces
> 
> @@ +2606,5 @@
> >  
> > +  SharedSSLState* sharedState = nullptr;
> > +
> > +  if (providerTlsFlags)
> > +    sharedState = new SharedSSLState(providerTlsFlags);
> 
> This is a memory leak. I also don't think this approach will work. If we're
> creating a new SharedSSLState for each new socket, we're not actually
> sharing any state. I could be wrong, but things like TLS intolerance
> fallback won't work if custom tlsFlags are set, because the backend will
> never see that previous attempts failed (i.e. information from
> rememberIntolerantAtVersion won't be remembered, and adjustForTLSIntolerance
> will have no effect because there's no remembered information). It might be
> best to write a few tests that exercise the functionality you're looking to
> implement to confirm or disprove this assumption. We might need to expand
> nsITLSServerSocket (
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/
> nsITLSServerSocket.idl ) to have an additional callback so that we can
> simulate various server behaviors such as TLS version intolerance (e.g. add
> a user-settable callback in TLSServerSocket::AuthCertificateHook like in
> TLSServerConnectionInfo::HandshakeCallback - see
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/TLSServerSocket.
> cpp ).

Thanks for your comprehensive feedback.

I totally agree that we have to extend the TLSServerSocket for testing purposes, and that is something I am working on right now. In general, the state would remain shared between all the TLS connections that are not using tlsFlags which contains all the user's browsing connections. We allocated a new SharedSSLState whenever we are setting the tlsFlags which only happens rarely by ourselves in the add-on for the experiments.

Basically, both rememberIntolerantAtVersion and adjustForTLSIntolerance are called on the same object, so the information will be remembered. I tested this with a server I created, and I am quite sure this approach is working. Indeed we have to think of a way to prevent the memory leak you mentioned.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Sajjad Arshad from comment #7)
> Basically, both rememberIntolerantAtVersion and adjustForTLSIntolerance are
> called on the same object, so the information will be remembered. I tested
> this with a server I created, and I am quite sure this approach is working.
> Indeed we have to think of a way to prevent the memory leak you mentioned.

Hmmm - I guess necko is reusing the underlying objects in the case of retries? In any case, we would probably still want to re-use information from previous connections when making completely new ones.

Comment 9

[Sajjad Arshad]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> (In reply to Sajjad Arshad from comment #7)
> > Basically, both rememberIntolerantAtVersion and adjustForTLSIntolerance are
> > called on the same object, so the information will be remembered. I tested
> > this with a server I created, and I am quite sure this approach is working.
> > Indeed we have to think of a way to prevent the memory leak you mentioned.
> 
> Hmmm - I guess necko is reusing the underlying objects in the case of
> retries? In any case, we would probably still want to re-use information
> from previous connections when making completely new ones.

I will investigate this to get to the bottom of it. One interesting point is that the whole purpose of introducing the tlsFlags was to isolate our experiments and minimize/remove side-effects to user's browsing. So, actually it is an ideal thing that this information won't be available when making completely new connection.

Aside from this, I was wondering when you are suggesting for putting our new flag constants?

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8899061 [details] [diff] [review]
define-tls-flags-values.patch

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

keeler is your best reviewer for PSM - though NI me with any questions I can help with

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Sajjad Arshad from comment #9)
> Aside from this, I was wondering when you are suggesting for putting our new
> flag constants?

I'm not sure I understand the question. If you're asking where in the code a good place to document the flag values and effects would be, maybe nsNSSIOLayer.cpp or nsNSSIOLayer.h?

Comment 12

[Patrick McManus [:mcmanus]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
>
> Hmmm - I guess necko is reusing the underlying objects in the case of
> retries?

yes, that's the magic PR_CONNECT_RESET_ERROR bit under if (wantRetry).. 

> In any case, we would probably still want to re-use information
> from previous connections when making completely new ones.

This is the interesting question. As noted in comment 9, we really want these to be isolated.. but brand new ones seem wrong too - they should probably clone the default one and then throw it away.

Comment 13

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #12)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> >
> 
> This is the interesting question. As noted in comment 9, we really want
> these to be isolated.. but brand new ones seem wrong too - they should
> probably clone the default one and then throw it away.

I'm going to go back on this and say the way the patch was doing this (new version) was basically right (modulo mem leak). The idea here is basically to be able to test various permutations of the stack independent of the user's state (such as their existing intolerance list). I'll make the documentation clear on that point.

Comment 14

[Patrick McManus [:mcmanus]]

Attachment: Bug 1391506 - Creating max version, fallback limit, and alt server hello flag values for the tlsFlags

Review commit: https://reviewboard.mozilla.org/r/173970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173970/

Comment 15

[Patrick McManus [:mcmanus]]

Attachment: Bug 1391506 - documentation, whitespace, braces, etc..

Review commit: https://reviewboard.mozilla.org/r/173972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173972/

Comment 16

[Patrick McManus [:mcmanus]]

Attachment: Bug 1391506 - per socket intolerance structure

Review commit: https://reviewboard.mozilla.org/r/173974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173974/

Comment 17

[Patrick McManus [:mcmanus]]

Attachment: Bug 1391506 - test for tls_flags max version

Review commit: https://reviewboard.mozilla.org/r/173976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173976/

Comment 18

[Patrick McManus [:mcmanus]]

Comment on attachment 8902395 [details]
Bug 1391506 - test for tls_flags max version

https://reviewboard.mozilla.org/r/173976/#review179168

I structured this as a series of incremental patches on top of the original one.. I can squish it into 2 for landing if you like, but it would be nice to retain the original author

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8902392 [details]
Bug 1391506 - Creating max version, fallback limit, and alt server hello flag values for the tlsFlags

https://reviewboard.mozilla.org/r/173970/#review179216

::: security/manager/ssl/SharedSSLState.h:23
(Diff revision 1)
>  
>  class SharedSSLState {
>  public:
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SharedSSLState)
> -  SharedSSLState();
> +  SharedSSLState() : SharedSSLState(0) {}
> +  explicit SharedSSLState(uint32_t aTlsFlags);

I didn't actually try this out until now, but it looks like we can simplify this to just one explicit constructor with a default value:

`explicit SharedSSLState(uint32_t aTlsFlags = 0);`

::: security/manager/ssl/nsNSSIOLayer.h:172
(Diff revision 1)
>  
>  class nsSSLIOLayerHelpers
>  {
>  public:
> -  nsSSLIOLayerHelpers();
> +  nsSSLIOLayerHelpers() : nsSSLIOLayerHelpers(0) {}
> +  explicit nsSSLIOLayerHelpers(uint32_t aTlsFlags);

Same idea with only having one constructor.

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8902393 [details]
Bug 1391506 - documentation, whitespace, braces, etc..

https://reviewboard.mozilla.org/r/173972/#review179180

::: security/manager/ssl/nsNSSIOLayer.cpp:70
(Diff revision 1)
> +// but provide a mechanism for direct TLS manipulation when experimenting with new
> +// features in the scope of a single socket. They do not create a persistent ABI.
> +//
> +// Use of these flags creates a new 'sharedSSLState' so existing states for intolerance
> +// are not carried to sockets that use these flags (and intolerance they discover
> +// does not impact other normal sockets not using the flags.

nit: closing ')'

::: security/manager/ssl/nsNSSIOLayer.cpp:1725
(Diff revision 1)
>    uint32_t limit = Preferences::GetUint("security.tls.version.fallback-limit",
>                                          3); // 3 = TLS 1.2
>  
>    // set fallback limit if it is set in the tls flags
> -  uint32_t tlsFlagsFallbackLimit = (mTlsFlags & (7 << 3)) >> 3;
> -  if (tlsFlagsFallbackLimit)
> +  uint32_t tlsFlagsFallbackLimit = getTLSProviderFlagFallbackLimit(mTlsFlags);
> +    

nit: some trailing whitespace

Comment 21

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8902394 [details]
Bug 1391506 - per socket intolerance structure

https://reviewboard.mozilla.org/r/173974/#review179186

::: security/manager/ssl/SharedSSLState.cpp:129
(Diff revision 1)
>  , mOCSPStaplingEnabled(false)
>  , mOCSPMustStapleEnabled(false)
>  {
>    mIOLayerHelpers.Init();
> +  if (!aTlsFlags) { // the per socket flags don't need memory
> +    mClientAuthRemember = new nsClientAuthRememberService();

SharedSSLState::GetClientAuthRememberService returns mClientAuthRemember. We should probably add an assertion/null-check combo where this gets called to ensure we don't accidentally a null pointer (looks like the only unchecked calls are in nsClientAuthRememberService::ClearAllRememberedDecisions, which shouldn't ever be called on one of these special SharedSSLStates, but still).

::: security/manager/ssl/nsNSSIOLayer.h:123
(Diff revision 1)
>      mSSLVersionUsed = version;
>    }
>  
>    void SetMACAlgorithmUsed(int16_t mac) { mMACAlgorithmUsed = mac; }
>  
> +  void SetSharedOwningReference(mozilla::psm::SharedSSLState *ref);

nit: * to the left

::: security/manager/ssl/nsNSSIOLayer.h:168
(Diff revision 1)
>    uint32_t mProviderTlsFlags;
>    mozilla::TimeStamp mSocketCreationTimestamp;
>    uint64_t mPlaintextBytesRead;
>  
>    nsCOMPtr<nsIX509Cert> mClientCert;
> +  RefPtr<mozilla::psm::SharedSSLState> mOwningSharedRef;

We should probably document what this is for and the expectations around it.

::: security/manager/ssl/nsNSSIOLayer.cpp:703
(Diff revision 1)
>  {
>    return mSharedState;
>  }
>  
> +void
> +nsNSSSocketInfo::SetSharedOwningReference(SharedSSLState *aRef)

nit: * to the left

::: security/manager/ssl/nsNSSIOLayer.cpp:2672
(Diff revision 1)
>    PRStatus stat;
>  
>    SharedSSLState* sharedState = nullptr;
> -
> +  RefPtr<SharedSSLState> allocatedState;
>    if (providerTlsFlags) {
> -    sharedState = new SharedSSLState(providerTlsFlags);
> +    allocatedState = new SharedSSLState(providerTlsFlags);  

nit: trailing whitespace

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8902395 [details]
Bug 1391506 - test for tls_flags max version

https://reviewboard.mozilla.org/r/173976/#review179192

Like I mentioned on slack, I think we should check these in squashed so we don't put a misleading patch in mercurial history. I'm investigating whether or not we can use the SNI callback to simulate TLS version intolerance. We can implement that in a separate bug if it pans out.

::: netwerk/test/unit/test_tls_flags.js:161
(Diff revision 1)
> +                     Ci.nsICertOverrideService.ERROR_MISMATCH;
> +  certOverrideService.rememberValidityOverride(hostname, port, cert,
> +                                               overrideBits, true);
> +}
> +
> +function startClient(port, tlsFlags, expectSuccess) {

I imagine we should also check the expected negotiated TLS version, right?

::: netwerk/test/unit/test_tls_flags.js:194
(Diff revision 1)
> +  do_print("TEST 1");
> +  let server = startServer(cert,
> +                           Ci.nsITLSClientStatus.TLS_VERSION_1_1,
> +                           Ci.nsITLSClientStatus.TLS_VERSION_1_3);
> +  storeCertOverride(server.port, cert);
> +  await startClient(server.port, 0x44, true /*should succeed*/);

Add a comment that this also does the alt hello? (Or was setting that bit not intended?)

Comment 23

[Patrick McManus [:mcmanus]]

Comment on attachment 8902392 [details]
Bug 1391506 - Creating max version, fallback limit, and alt server hello flag values for the tlsFlags

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173970/diff/1-2/

Comment 24

[Patrick McManus [:mcmanus]]

> 
> Add a comment that this also does the alt hello? (Or was setting that bit
> not intended?)

that was a mistake (I was hand testing it). thanks.

Comment 25

[Patrick McManus [:mcmanus]]

Comment on attachment 8902392 [details]
Bug 1391506 - Creating max version, fallback limit, and alt server hello flag values for the tlsFlags

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173970/diff/2-3/

Comment 26

[Patrick McManus [:mcmanus]]

Comment on attachment 8902392 [details]
Bug 1391506 - Creating max version, fallback limit, and alt server hello flag values for the tlsFlags

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173970/diff/3-4/

Comment 27

[Patrick McManus [:mcmanus]]

Comment on attachment 8902392 [details]
Bug 1391506 - Creating max version, fallback limit, and alt server hello flag values for the tlsFlags

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173970/diff/4-5/

Comment 28

[Patrick McManus [:mcmanus]]

Comment on attachment 8902392 [details]
Bug 1391506 - Creating max version, fallback limit, and alt server hello flag values for the tlsFlags

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173970/diff/5-6/

Comment 29

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9629720453fb -d 1e922b64ff15: rebasing 416726:9629720453fb "Bug 1391506 - Creating max version, fallback limit, and alt server hello flag values for the tlsFlags r=keeler" (tip)
merging security/manager/ssl/nsNSSIOLayer.cpp
merging security/manager/ssl/nsNSSIOLayer.h
warning: conflicts while merging security/manager/ssl/nsNSSIOLayer.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 30

[Patrick McManus [:mcmanus]]

Comment on attachment 8902392 [details]
Bug 1391506 - Creating max version, fallback limit, and alt server hello flag values for the tlsFlags

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173970/diff/6-7/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/537aeafd49ec
Creating max version, fallback limit, and alt server hello flag values for the tlsFlags r=keeler

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/537aeafd49ec