Closed Bug 622332 Opened 10 years ago Closed 7 years ago

Show certificate sha256 fingerprint

Categories

(Core Graveyard :: Security: UI, enhancement)

x86_64
Linux
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla32

People

(Reporter: hanno, Assigned: Cykesiopka)

Details

(Whiteboard: [psm-feedback][psm-cert-manager])

Attachments

(4 files, 5 obsolete files)

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)
Whiteboard: [psm-feedback][psm-cert-manager]
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.
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.
(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
Attached patch bug622332_v0.patch (obsolete) — Splinter Review
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.
Attachment #8420615 - Flags: feedback?(dkeeler)
Attached image Bug 622332 v0 Cert Viewer Screenshot.png (obsolete) —
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
Attachment #8420615 - Flags: feedback?(dkeeler) → feedback+
(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
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.
Attached image sha256certviewer-30.png
See image.
(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?
(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...?
Attached patch bug622332_v1.patch (obsolete) — Splinter Review
+ 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
Attachment #8420615 - Attachment is obsolete: true
Attachment #8422869 - Flags: review?(dkeeler)
Attachment #8420616 - Attachment is obsolete: true
 (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.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
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.
Attachment #8422869 - Flags: review?(dkeeler)
(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.
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.
Attached patch bug622332_v2.patch (obsolete) — Splinter Review
+ 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.
Attachment #8422869 - Attachment is obsolete: true
Attachment #8423597 - Flags: review?(dkeeler)
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
Attachment #8423597 - Flags: review?(dkeeler) → review+
(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].
For reference this is how the Android Cert Details page looks like with this patch on a tablet.
Attached patch bug622332_v3.patch (obsolete) — Splinter Review
+ Addresses review comments from Comment 19

Does this need additional review from peers in Toolkit etc?
Attachment #8423597 - Attachment is obsolete: true
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.
(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.
Keywords: checkin-needed
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!
Keywords: checkin-needed
(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.
(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!
+ 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+.
Attachment #8424251 - Attachment is obsolete: true
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f2a5edce0a57
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.