622332 - Show certificate sha256 fingerprint

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, enhancement)

Description

[Hanno Boeck]

Currently, firefox only shows the certificate fingerprints in sha1 and md5.
Both Opera and Chromium show sha1 and sha256 (haven't checked IE), I'd suggest to do the same. (although sha1 should be completely deprecated on the long term, it should probably stay there for some time for compatibility reasons - I think md5 can go away)

Comment 1

[Sean Leonard]

I would like to second this UI enhancement.

Independent of this bug (which was forwarded to me by Kaspar Brand), I wrote a very brief extension called "SHA-256 Certificate Viewer":
https://www.seantek.com/sha256certviewer/

It overlays the UI so that the SHA-256 fingerprint appears first. (The term "SHA-256" instead of "SHA256" is intentional.)

Unfortunately, I wrote it really quickly (less than 40 minutes). In that time I could not figure out how to break the text nicely across multiple lines while preserving click-and-copy functionality (where all the text can be selected in one click for an easy copy). So, please take the particular UI with a grain of salt.

Ideally in my opinion, the fingerprint should look like this:
 971BE57D 7AAFF3C3 B25324DA 3443C90C
 2EE7976B 63403724 CC90976C A4D7CEC1

I recognize this format diverges from the tried-and-true Mozilla format 00:11:22:33..., but the :-separated string becomes impossible to read once it extends out to 64 hex characters on a single line (or even a line broken into two parts).

I would also like to advocate click-and-copy technology with custom menu items. By "click-and-copy", I mean:
<label clickSelectsAll="true" readonly="true" "style: -moz-binding: [textbox]" ... />

The custom menu items would allow the user to copy the hash in different formats.

Comment 2

[Sean Leonard]

Just to mention, I would be happy to contribute the code in the SHA-256 Certificate Viewer to Mozilla assuming everyone's okay with whatever ends up being the UI design here.

Comment 3

[Jan Bevers]

(In reply to Sean Leonard from comment #2)
> Just to mention, I would be happy to contribute the code in the SHA-256
> Certificate Viewer to Mozilla assuming everyone's okay with whatever ends up
> being the UI design here.

Hi Sean,

I'm trying to get the SHA-256 fingerprint in my custom app which uses geckofx with xulrunner. How did you manage to get the value, did you make any change to xulrunner?

Thx
Jan

Comment 4

[:Cykesiopka]

Attachment: bug622332_v0.patch

The UI still needs work (the hash isn't split evenly between the two lines by default at least), so just asking for feedback.

This patch uses the format introduced by Bug 932116, so copy & paste and context menus should work.

Comment 5

[:Cykesiopka]

Attachment: Bug 622332 v0 Cert Viewer Screenshot.png

Comment 6

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

Comment on attachment 8420615 [details] [diff] [review]
bug622332_v0.patch

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

This looks great!
It would definitely be nice to have the fingerprint wrap to two equal lines - maybe some CSS would handle that.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +997,5 @@
>    return NS_ERROR_FAILURE;
>  }
>  
>  NS_IMETHODIMP
> +nsNSSCertificate::GetSha256Fingerprint(nsAString& aSha256Fingerprint)

Since this is so similar to the other two fingerprint functions, it would be great to factor out the common code and then just do:
return GetFingerprint(aSha256Fingerprint, SEC_OID_SHA256);

@@ +1013,5 @@
> +    return rv;
> +  }
> +
> +  char* fpStr = CERT_Hexify(const_cast<SECItem*>(&digest.get()), 1);
> +  if (fpStr) {

nit: handle the exceptional case here, instead of the common one
This, in other words:
if (!fpStr) {
  return NS_ERROR_FAILURE;
}

// otherwise continue with the successful case

Comment 7

[Sean Leonard]

(In reply to David Keeler (:keeler) from comment #6)
> Comment on attachment 8420615 [details] [diff] [review]
> bug622332_v0.patch
> 
> Review of attachment 8420615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great!
> It would definitely be nice to have the fingerprint wrap to two equal lines
> - maybe some CSS would handle that.
> 

What, you mean like this?

https://www.seantek.com/sha256certviewer/

In my code, this was accomplished without modifying nsIX509Cert. Please, do not modify it.

-Sean

Comment 8

[Hanno Boeck]

Just my two bits, I think the SHA256 hash should replace the MD5 hash and not accompany it. Certificate hashes are one of the cases where collission problems really matter, so I think it creates a false sense of security to show the MD5 hash at all.

Comment 9

[Sean Leonard]

Attachment: sha256certviewer-30.png

See image.

Comment 10

[:Cykesiopka]

(In reply to Sean Leonard from comment #7)
> In my code, this was accomplished without modifying nsIX509Cert. Please, do
> not modify it.

Sorry, I'm not sure why modifying nsIX509Cert would be bad... Could you explain?

Comment 11

[:Cykesiopka]

(In reply to Hanno Boeck from comment #8)
> Just my two bits, I think the SHA256 hash should replace the MD5 hash and
> not accompany it. Certificate hashes are one of the cases where collission
> problems really matter, so I think it creates a false sense of security to
> show the MD5 hash at all.

In terms of removing it from the Cert Viewer UI, sure.

Dunno about removing it from the IDL though; that may introduce compat issues...?

Comment 12

[:Cykesiopka]

Attachment: bug622332_v1.patch

+ Factor out SHA common code into GetShaFingerprint() (mozilla::psm::Digest doesn't seem to support MD5)
+ Handle the exceptional |fpStr| case in if block, instead of the common case

+ Make some styling changes to have SHA-256 fingerprint wrap to two equal lines (seems to work, but improvements welcome)
+ Remove display of MD5 fingerprint

Comment 13

[:Cykesiopka]

Attachment: Bug 622332 v1 Cert Viewer Screenshot.png

Comment 14

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

 (In reply to Cykesiopka from comment #11)
> Dunno about removing it from the IDL though; that may introduce compat
> issues...?

Please also remove md5Fingerprint from the IDL and from the implementation. It will cause compatibility issues but we do not guarantee compatibility and also it will motivate the addons that use the md5Fingerprint to update to use the sha256 fingerprint.

By the way, I think there's a better UI possible: Instead of showing the fingerprints, we could have an input field that the user can type or paste the fingerprint they expect the cert to have. Then we can show a O/X indicator based on whether the fingerprint patches. But, that could happen in a separate bug.

Comment 15

[:Cykesiopka]

Comment on attachment 8422869 [details] [diff] [review]
bug622332_v1.patch

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #14)
> Please also remove md5Fingerprint from the IDL and from the implementation.
> It will cause compatibility issues but we do not guarantee compatibility and
> also it will motivate the addons that use the md5Fingerprint to update to
> use the sha256 fingerprint.

OK, I'll remove it then.

Comment 16

[Sean Leonard]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #14)
>  (In reply to Cykesiopka from comment #11)
> > Dunno about removing it from the IDL though; that may introduce compat
> > issues...?
> 
> Please also remove md5Fingerprint from the IDL and from the implementation.
> It will cause compatibility issues but we do not guarantee compatibility and

Please do not remove the md5Fingerprint. The issue is not whether applications depend on that particular function, but rather that a change in the IDL is extremely disruptive to binary components. If you must, please create a new interface such as nsIX509Cert4 or some such, and change the return of md5Fingerprint to NS_ERROR_NOT_IMPLEMENTED.

Better yet, you could just use well-known techniques to compute the SHA-256 fingerprint from the cert itself (as my extension shows), without needing to manipulate nsIX509Cert.

Thanks.

Comment 17

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

A long time ago, interfaces were frozen. This meant we had to add a completely new interface every time something changed (this is how we ended up with nsIX509Cert, nsIX509Cert2, and nsIX509Cert3). However, interfaces are no longer frozen and we are no longer doing this sort of thing. This does affect addons, and particularly binary addons, but we've decided this is the cost of progress. For quite some time, MD5 has been considered unfit for security purposes. Removing uses of it increases the security of our users. We should definitely remove md5Fingerprint here.

Comment 18

[:Cykesiopka]

Attachment: bug622332_v2.patch

+ Remove md5fingerprint from IDL
+ Update Android Cert Details viewer as well
+ Change tests that depend on md5fingerprint

I can separate the patch into multiple ones if that is preferred.

Comment 19

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

Comment on attachment 8423597 [details] [diff] [review]
bug622332_v2.patch

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

If you can figure out how to get the certificate dialog to show up on android, I think it would be a good idea to make sure that still works as expected (it looks like it should be fine).
I have a couple of nits, but no real concerns.
Thanks for working on this!

::: security/manager/pki/resources/content/viewCertDetails.xul
@@ +87,5 @@
>            <spacer/>
>          </row>
>          <row>
> +          <label value="&certmgr.certdetail.sha256fingerprint;"/>
> +          <hbox>

Why does this need to be wrapped in an hbox? Is that to get it to use two lines?

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1008,5 @@
> +  aFingerprint.Truncate();
> +  Digest digest;
> +  nsresult rv = digest.DigestBuf(aHashAlg, mCert->derCert.data,
> +                                 mCert->derCert.len);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

I don't think the NS_WARN_IF is necessary here.

@@ +1012,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  char* fpStr = CERT_Hexify(const_cast<SECItem*>(&digest.get()), 1);

Let's document that CERT_Hexify's second argument is an int that is interpreted as a boolean.

@@ +1017,5 @@
> +  if (!fpStr) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  aFingerprint = NS_ConvertASCIItoUTF16(fpStr);

I think you can just do "aFingerprint.AssignASCII(fpStr);" here.

@@ +1025,5 @@
> +
> +NS_IMETHODIMP
> +nsNSSCertificate::GetSha256Fingerprint(nsAString& aSha256Fingerprint)
> +{
> +  return nsNSSCertificate::GetShaFingerprint(aSha256Fingerprint, SEC_OID_SHA256);

nit: "nsNSSCertificate::" shouldn't be necessary here

@@ +1032,4 @@
>  NS_IMETHODIMP
>  nsNSSCertificate::GetSha1Fingerprint(nsAString& _sha1Fingerprint)
>  {
> +  return nsNSSCertificate::GetShaFingerprint(_sha1Fingerprint, SEC_OID_SHA1);

same here

::: security/manager/ssl/src/nsNSSCertificate.h
@@ +64,5 @@
>    virtual void virtualDestroyNSSReference();
>    void destructorSafeDestroyNSSReference();
>    bool InitFromDER(char* certDER, int derLen);  // return false on failure
>  
> +  nsresult GetShaFingerprint(nsAString& aFingerprint, SECOidTag aHashAlg);

While it's true that only SHA digests are supported right now, I think there's a more descriptive name for this function we could use. Maybe "GetCertificateDigest"? Or "GetCertificateHash"?

::: security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
@@ +168,5 @@
>    return NS_ERROR_NOT_IMPLEMENTED;
>  }
>  
>  NS_IMETHODIMP
> +nsNSSCertificateFakeTransport::GetSha1Fingerprint(nsAString &_sha1Fingerprint)

nit: while you're changing this line, you could move the & to next to nsAString and call the parameter aSha1Fingerprint

Comment 20

[:Cykesiopka]

(In reply to David Keeler (:keeler) [needinfo? is a good way to get my attention] from comment #19)
> If you can figure out how to get the certificate dialog to show up on
> android, I think it would be a good idea to make sure that still works as
> expected (it looks like it should be fine).

I've tested it and it works like expected.

> ::: security/manager/pki/resources/content/viewCertDetails.xul
> @@ +87,5 @@
> >            <spacer/>
> >          </row>
> >          <row>
> > +          <label value="&certmgr.certdetail.sha256fingerprint;"/>
> > +          <hbox>
> 
> Why does this need to be wrapped in an hbox? Is that to get it to use two
> lines?

Yes. Leaving the hbox out makes the UI look like attachment 8420616 [details].

Comment 21

[:Cykesiopka]

Attachment: Bug 622332 v3 Android Cert Details Screenshot.png

For reference this is how the Android Cert Details page looks like with this patch on a tablet.

Comment 22

[:Cykesiopka]

Attachment: bug622332_v3.patch

+ Addresses review comments from Comment 19

Does this need additional review from peers in Toolkit etc?

Comment 23

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

The toolkit changes are test-only and very simple, so I don't think additional review is necessary there. I feel similarly about the android changes, but they're not test-only. I think it's probably fine, but if you want to be extra-sure, feel free to get additional review.

Comment 24

[:Cykesiopka]

(In reply to David Keeler (:keeler) [needinfo? is a good way to get my attention] from comment #23)
> The toolkit changes are test-only and very simple, so I don't think
> additional review is necessary there. I feel similarly about the android
> changes, but they're not test-only. I think it's probably fine, but if you
> want to be extra-sure, feel free to get additional review.

If you're fine with it, so am I.

Comment 25

[:Cykesiopka]

Oops, just read https://groups.google.com/forum/#!topic/mozilla.dev.platform/9w0-Bh_3vVI

Could someone push this to Try for me? (Or vouch for me if/when I request Try access I guess...)

Thanks!

Comment 26

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

(In reply to Cykesiopka from comment #25)
> Could someone push this to Try for me? (Or vouch for me if/when I request
> Try access I guess...)

Please request Level 1 commit access (see bug 607661 for an example of what to do) and I will vouch for you.

Comment 27

[:Cykesiopka]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #26)
> (In reply to Cykesiopka from comment #25)
> > Could someone push this to Try for me? (Or vouch for me if/when I request
> > Try access I guess...)
> 
> Please request Level 1 commit access (see bug 607661 for an example of what
> to do) and I will vouch for you.

I've requested Level 1 commit access in Bug 1013752, thanks!

Comment 28

[:Cykesiopka]

Attachment: bug622332_v4.patch

+ Make styling more robust
 
I discovered that the previous cols="47" approach doesn't work on my Windows machine due to font size differences from my Ubuntu VM - this new approach fixes that, and handles non-default chrome UI font sizes better as well.

Carrying forward r+.

Comment 29

[:Cykesiopka]

And here's a link to a Try run for v3 (v4 is just styling changes, so I'm guessing the results would be the same):
  https://tbpl.mozilla.org/?tree=Try&rev=1d96320ef9bc

Everything is green, but I apologise in advance if I ran too little/too much tests.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a5edce0a57

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/f2a5edce0a57