Closed Bug 1304923 Opened 3 years ago Closed 2 years ago

Display key exchange and signature type for TLS 1.3

Categories

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

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: mt, Assigned: franziskus)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

Bug 1304923 - adding key exchange group and signature scheme details to devtools view,
59 bytes, text/x-review-board-request
keeler
: review+
Honza
: review+
yulia
: review+
Details 
TLS 1.3 takes the key exchange and server certificate type out of the cipher suite definition.  That means reporting just the cipher suite leaves some critical information out.  We should report these on the UI.
Depends on: 1304924
Component: Security: UI → Security: PSM
Priority: -- → P2
Whiteboard: [psm-backlog]
Comment on attachment 8900692 [details]
Bug 1304923 - adding key exchange group and signature scheme details to devtools view,

https://reviewboard.mozilla.org/r/172134/#review177528

Looks good, but we need to do the versioning dance again in nsSSLStatus.

::: devtools/shared/webconsole/network-helper.js:623
(Diff revision 1)
>        }
>  
>        // Cipher suite.
>        info.cipherSuite = SSLStatus.cipherName;
>  
> +      // KEX group.

Maybe expand this abbreviation for those that don't know it.

::: security/manager/ssl/nsNSSCallbacks.cpp:848
(Diff revision 1)
> +      break;
> +    case ssl_grp_ec_curve25519:
> +      groupName = NS_LITERAL_CSTRING("x25519");
> +      break;
> +    case ssl_grp_none:
> +      groupName = NS_LITERAL_CSTRING("none");

Unless I'm missing it, "none" and "unknown group" aren't localized (and I'm assuming that it doesn't make much sense to localize the other group names). Maybe we could handle these by adding l10n strings on the front-end and special casing these two? (I'd rather not do the localization here, but I think it's important to do.)

::: security/manager/ssl/nsNSSCallbacks.cpp:867
(Diff revision 1)
> +    case ssl_grp_ffdhe_8192:
> +      groupName = NS_LITERAL_CSTRING("FF 8192");
> +      break;
> +    default:
> +      // This really shouldn't happen!
> +      groupName = NS_LITERAL_CSTRING("unknown group");

Let's add a `MOZ_ASSERT_UNREACHABLE` statement for debug builds.

::: security/manager/ssl/nsSSLStatus.cpp:208
(Diff revision 1)
>    if (streamFormatVersion >= 1) {
>      rv = aStream->Read16(&mCertificateTransparencyStatus);
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
> +  rv = aStream->ReadCString(mKeaGroup);

We need to handle this similarly to `mCertificateTransparencyStatus` - i.e. only read this field if `streamFormatVersion >= 2`.

::: security/manager/ssl/nsSSLStatus.cpp:218
(Diff revision 1)
>  
>  NS_IMETHODIMP
>  nsSSLStatus::Write(nsIObjectOutputStream* aStream)
>  {
>    // The current version of the binary stream format.
>    const uint8_t STREAM_FORMAT_VERSION = 1;

This should be incremented to 2.

::: security/manager/ssl/nsSSLStatus.cpp:254
(Diff revision 1)
>  
>    // Added in version 1.
>    rv = aStream->Write16(mCertificateTransparencyStatus);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = aStream->WriteStringZ(mKeaGroup.get());

Let's add a comment that this was added in version 2.
Attachment #8900692 - Flags: review?(dkeeler) → review-
Comment on attachment 8900692 [details]
Bug 1304923 - adding key exchange group and signature scheme details to devtools view,

https://reviewboard.mozilla.org/r/172134/#review178568

::: security/manager/ssl/nsNSSCallbacks.cpp:848
(Diff revision 1)
> +      break;
> +    case ssl_grp_ec_curve25519:
> +      groupName = NS_LITERAL_CSTRING("x25519");
> +      break;
> +    case ssl_grp_none:
> +      groupName = NS_LITERAL_CSTRING("none");

Agreed, I localised these two values.

::: security/manager/ssl/nsSSLStatus.cpp:208
(Diff revision 1)
>    if (streamFormatVersion >= 1) {
>      rv = aStream->Read16(&mCertificateTransparencyStatus);
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
> +  rv = aStream->ReadCString(mKeaGroup);

I figured there should be something like this but I wasn't sure how to handle it.
Assignee: nobody → franziskuskiefer
Priority: P2 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
Comment on attachment 8900692 [details]
Bug 1304923 - adding key exchange group and signature scheme details to devtools view,

https://reviewboard.mozilla.org/r/172134/#review178692

LGTM! Just a minor comment about handling cases we don't actually enable in Firefox.

::: security/manager/ssl/nsNSSCallbacks.cpp:856
(Diff revision 2)
> +      break;
> +    case ssl_grp_ffdhe_3072:
> +      groupName = NS_LITERAL_CSTRING("FF 3072");
> +      break;
> +    case ssl_grp_ffdhe_4096:
> +      groupName = NS_LITERAL_CSTRING("FF 4096");

This is minor, but it's probably worth saving a bit of space to not handle groups we don't enable ( https://dxr.mozilla.org/mozilla-central/rev/4caca1d0ba0e35cbe57a88493ebf162aa2cb3144/security/manager/ssl/nsNSSIOLayer.cpp#2487 ) (and I guess whatever NSS enables by default) (we should probably have a comment linking these two pieces of code).

::: security/manager/ssl/nsNSSCallbacks.cpp:916
(Diff revision 2)
> +      signatureName = NS_LITERAL_CSTRING("ED25519");
> +      break;
> +    case ssl_sig_ed448:
> +      signatureName = NS_LITERAL_CSTRING("ED448");
> +      break;
> +    case ssl_sig_dsa_sha1:

Same idea: https://dxr.mozilla.org/mozilla-central/rev/4caca1d0ba0e35cbe57a88493ebf162aa2cb3144/security/manager/ssl/nsNSSIOLayer.cpp#2407
Attachment #8900692 - Flags: review?(dkeeler) → review+
Attachment #8900692 - Flags: review?(yulia.startsev) → review?(odvarko)
Comment on attachment 8900692 [details]
Bug 1304923 - adding key exchange group and signature scheme details to devtools view,

https://reviewboard.mozilla.org/r/172134/#review179464

I reviewed only Network panel related files.
There are some typos, but otherwise it looks good.

R+ assuming my comments are resolved.


Thanks for working on this!

Honza

::: devtools/client/locales/en-US/netmonitor.properties:774
(Diff revision 2)
> +
> +# LOCALIZATION NOTE (netmonitor.security.keaGroup.none): This is the label
> +# displayed in the security tab describing the case when no group was used.
> +netmonitor.security.keaGroup.none=none
> +
> +# LOCALIZATION NOTE (netmonitor.security.keaGroup.unkown): This is the value

Typo: unkown => unknown

::: devtools/client/locales/en-US/netmonitor.properties:775
(Diff revision 2)
> +# LOCALIZATION NOTE (netmonitor.security.keaGroup.none): This is the label
> +# displayed in the security tab describing the case when no group was used.
> +netmonitor.security.keaGroup.none=none
> +
> +# LOCALIZATION NOTE (netmonitor.security.keaGroup.unkown): This is the value
> +# displayed in the security tab describing an unkown group.

Typo: unkown => unknown

::: devtools/client/locales/en-US/netmonitor.properties:784
(Diff revision 2)
> +# displayed in the security tab describing the signature scheme used by for
> +# the server certificate in this connection.
> +netmonitor.security.signatureScheme=Signature Scheme:
> +
> +# LOCALIZATION NOTE (netmonitor.security.signatureScheme): This is the label
> +# displayed in the security tab describing the case when no signature was used.

The localization note should use: netmonitor.security.signatureScheme.none

::: devtools/client/locales/en-US/netmonitor.properties:788
(Diff revision 2)
> +# LOCALIZATION NOTE (netmonitor.security.signatureScheme): This is the label
> +# displayed in the security tab describing the case when no signature was used.
> +netmonitor.security.signatureScheme.none=none
> +
> +# LOCALIZATION NOTE (netmonitor.security.signatureScheme): This is the value
> +# displayed in the security tab describing an unkown signature scheme.

The localization note should use:
netmonitor.security.signatureScheme.unknown
Attachment #8900692 - Flags: review?(odvarko) → review+
Comment on attachment 8900692 [details]
Bug 1304923 - adding key exchange group and signature scheme details to devtools view,

https://reviewboard.mozilla.org/r/172134/#review179572

I don't know the code so well since I work on the debugger. Honza probably has better comments regarding the network monitor. Also, it looks like you assigned my personal account (:yulia) and I wasnt able to submit the review with that one. Can you remove yulia as a reviewer? Thanks

::: devtools/client/locales/en-US/netmonitor.properties:768
(Diff revision 2)
>  netmonitor.security.cipherSuite=Cipher suite:
>  
> +# LOCALIZATION NOTE (netmonitor.security.keaGroup): This is the label displayed
> +# in the security tab describing the key exchange group suite used to secure
> +# this connection.
> +netmonitor.security.keaGroup=Key Exchange Group:

should this be keyGroup?

::: devtools/shared/webconsole/network-helper.js:628
(Diff revision 2)
>        info.cipherSuite = SSLStatus.cipherName;
>  
> +      // Key exchange group name.
> +      info.keaGroupName = SSLStatus.keaGroupName;
> +      // Localise two special values.
> +      if (info.keaGroupName == "none") {

nit: I tend to prefer `===` (identity equality) to `==` because `==` results in coercion. For example `false == 0 // true`

I am not sure what the standard is in network monitor
Attachment #8900692 - Flags: review?(ystartsev)
Comment on attachment 8900692 [details]
Bug 1304923 - adding key exchange group and signature scheme details to devtools view,

https://reviewboard.mozilla.org/r/172134/#review179892

::: devtools/client/locales/en-US/netmonitor.properties:768
(Diff revision 2)
>  netmonitor.security.cipherSuite=Cipher suite:
>  
> +# LOCALIZATION NOTE (netmonitor.security.keaGroup): This is the label displayed
> +# in the security tab describing the key exchange group suite used to secure
> +# this connection.
> +netmonitor.security.keaGroup=Key Exchange Group:

keaGroup is the name of the field we get from NSS. I wanted to keep it consistent.

::: devtools/client/locales/en-US/netmonitor.properties:774
(Diff revision 2)
> +
> +# LOCALIZATION NOTE (netmonitor.security.keaGroup.none): This is the label
> +# displayed in the security tab describing the case when no group was used.
> +netmonitor.security.keaGroup.none=none
> +
> +# LOCALIZATION NOTE (netmonitor.security.keaGroup.unkown): This is the value

typing is hard :/

::: devtools/client/locales/en-US/netmonitor.properties:784
(Diff revision 2)
> +# displayed in the security tab describing the signature scheme used by for
> +# the server certificate in this connection.
> +netmonitor.security.signatureScheme=Signature Scheme:
> +
> +# LOCALIZATION NOTE (netmonitor.security.signatureScheme): This is the label
> +# displayed in the security tab describing the case when no signature was used.

yep...

::: devtools/shared/webconsole/network-helper.js:628
(Diff revision 2)
>        info.cipherSuite = SSLStatus.cipherName;
>  
> +      // Key exchange group name.
> +      info.keaGroupName = SSLStatus.keaGroupName;
> +      // Localise two special values.
> +      if (info.keaGroupName == "none") {

I agree that `===` is safer, but I only see `==` in here. So I'll probably keep it at that.

::: security/manager/ssl/nsNSSCallbacks.cpp:856
(Diff revision 2)
> +      break;
> +    case ssl_grp_ffdhe_3072:
> +      groupName = NS_LITERAL_CSTRING("FF 3072");
> +      break;
> +    case ssl_grp_ffdhe_4096:
> +      groupName = NS_LITERAL_CSTRING("FF 4096");

Sure, NSS enables all FF groups by default. People only have to remember to touch this as well when changing it in nsNSSIOLayer (I'll add a comment there as well).
Pushed by franziskuskiefer@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/620c7f6df405
adding key exchange group and signature scheme details to devtools view, r=keeler, Honza
https://hg.mozilla.org/mozilla-central/rev/620c7f6df405
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
This is breaking my macOS stylo build. It looks like the problem is that getSignatureName does not have a case for ssl_sig_rsa_pkcs1_sha1, which is in the sEnabledSignatureSchemes array at http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/security/manager/ssl/nsNSSIOLayer.cpp#2418. It DOES have a case ssl_sig_rsa_pkcs1_sha1md5, which is not in the sEnabledSignatureSchemes array.
Flags: needinfo?(franziskuskiefer)
Depends on: 1396463
Sorry, my bad. I removed the wrong line. I pushed a bustage fix.
Flags: needinfo?(franziskuskiefer)
Depends on: 1396525
Comment on attachment 8900692 [details]
Bug 1304923 - adding key exchange group and signature scheme details to devtools view,

https://reviewboard.mozilla.org/r/172134/#review181074
Attachment #8900692 - Flags: review?(ystartsev) → review+
See Also: → 1378572
You need to log in before you can comment on or make changes to this bug.