1524323 - inconsistent message and error code when a cert has serious issues in addition to being expired or being for the wrong host

Status: RESOLVED FIXED

(Firefox :: Security, defect, P3)

Description

[Julien Cristau [:jcristau]]

I've got a server with a self-signed cert that expired on August 23, 2017 (it's on a private network behind a vpn, so can't easily share it). When I visit that site, the error page's "Advanced" information says:

Websites prove their identity via certificates, which are valid for a set time period. The certificate for XXX appears to be expired.

Error code: MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT

Clicking the error code does show the corresponding "The certificate is not trusted because it is self-signed." text. So it seems there's some inconsistency there in what we're displaying.

Seeing the same thing in 66 and 67 at the moment; for comparison, 65.0 says:

XXX uses an invalid security certificate.

The certificate is not trusted because it is self-signed.
The certificate expired on August 23, 2017, 2:00 AM. The current time is January 31, 2019, 6:15 PM.

Error code: SEC_ERROR_UNKNOWN_ISSUER

Comment 1

[Johann Hofmann [:johannh]]

Yeah, we should fix this, though it's a bit of an edge case.

Comment 2

[Mellina Y.]

Hi Johann,

Could I try to solve this issue?
I'd probably need some guidance on where the cert code files are located and a test website (although I can try to find this in the web).

Thanks!

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Change the status for beta to have the same as nightly and release.
For more information, please visit auto_nag documentation.

Comment 7

[Jayati Shrivastava]

Hi Johannh! I would like to work on this bug. May I know what needs to be done here?

Comment 8

[Jayati Shrivastava]

What error message needs to be shown instead?

Comment 9

[Johann Hofmann [:johannh]]

Answered on Matrix:

To be honest I haven't really looked at the root cause of this bug. The idea is that it shouldn't show a weird mix of both errors, but only one of them? (ideally we'd show a list of issues but that probably takes it a little too far)

Comment 11

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] - On leave until October, slow/no response on BMO, DM me if urgent from comment #9)

Answered on Matrix:

To be honest I haven't really looked at the root cause of this bug. The idea is that it shouldn't show a weird mix of both errors, but only one of them? (ideally we'd show a list of issues but that probably takes it a little too far)

The root cause here is that the code in setTechnicalDetailsOnCertError uses a setL10NLabel helper (aaaaaargh who capitalized that N) which overwrites previous values unless some additional param is explicitly set to false. Most of the error content gets set in a giant switch statement, so this doesn't matter. But for validity timing issues and domain mismatches, we use separate if conditions so they overwrite things added in the previous switch statement. That shouldn't happen.

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1524323 - fix advanced error messages for certificate error pages regarding certs broken in several ways, r?prathiksha!,april!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5d51370ce39
fix advanced error messages for certificate error pages regarding certs broken in several ways, r=prathiksha,April

Comment 14

[Noemi Erli[:noemi_erli]]

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

Comment 16

[sandwichs]

I believe the chosen solution is incorrect.
Why are you second-guessing the error code and implement your own error-priority logic?

The root cause here is that the code in setTechnicalDetailsOnCertError uses a setL10NLabel helper (aaaaaargh who capitalized that N) which overwrites previous values unless some additional param is explicitly set to false. Most of the error content gets set in a giant switch statement, so this doesn't matter. But for validity timing issues and domain mismatches, we use separate if conditions so they overwrite things added in the previous switch statement. That shouldn't happen.

Do you believe all errors should be shown, just like it always used to be before Quantum?
I personally learned to rely on these errors being available, but I will understand if you claim it will overwhelm the users.

Comment 17

[:Gijs (he/him)]

(In reply to sandwichs from comment #16)

I believe the chosen solution is incorrect.

You've not said what you think is incorrect about it. What are we doing vs. what do you think we should be doing?

Comment 18

[sandwichs]

Oops, sorry.
The proposed solution is second-guessing the error code. If you want to show only one error, it should be case on the error code.

But really I think you should restore the original and intended behavior that shows all errors.
After all, showing all of them is why these errors are handled separate IFs instead of being part of the big Case statement above the IFs.

I found a screenshot of a pre-quantum firefox, e.g. before this issue was introduced: https://discourse-prod-uploads-81679984178418.s3.dualstack.us-west-2.amazonaws.com/original/3X/6/b/6b596e4b09225b5fe9f87b152b5deae92b15480b.png

Thank you for considering!

Comment 19

[:Gijs (he/him)]

(In reply to sandwichs from comment #18)

Oops, sorry.
The proposed solution is second-guessing the error code. If you want to show only one error, it should be case on the error code.

I disagree. The patch that landed is based on information provided by the network layer that is higher level than the error code, like "is the cert untrusted", which can have a number of different causes (ie error codes), and then specialcases error codes. Put differently, if the error codes were making good use of different bits in a single bitspace, we could have implemented this by checking for the result of an AND with different filtering flags (ie if all errors along the lines of "we don't trust this cert" had bit 5 set, and all the time validity ones had bit 3 set, or whatever).

We're trusting the network layer to provide error information where these bools and the error code are consistent. TBH, if that isn't the case, both before and after the patch the code would have had worse problems.

The advantage of this approach is that if new error codes are added, they automatically get categorized correctly without having to adjust the front-end, instead of having to manually update the frontend for every error code that is added (and potentially doing the Wrong Thing if a new error code is unknown, or having to show a less descriptive error ("we don't accept this cert, but can't tell you why")).

There's also the fact that a single error code cannot convey the fact that the cert has multiple issues, and so using only the error code then delegates the prioritization of what we show the user (or what fixes we try to suggest / apply) to that single error code provided by the networking/tls layer, when it should properly be with the consumer of that information.

But really I think you should restore the original and intended behavior that shows all errors.
After all, showing all of them is why these errors are handled separate IFs instead of being part of the big Case statement above the IFs.

I found a screenshot of a pre-quantum firefox, e.g. before this issue was introduced: https://discourse-prod-uploads-81679984178418.s3.dualstack.us-west-2.amazonaws.com/original/3X/6/b/6b596e4b09225b5fe9f87b152b5deae92b15480b.png

Thank you for considering!

As you suspected in comment #16, I'm not convinced that showing 3 different problems with the cert at once is going to be a better user experience for the average person than only showing the most important one...

Comment 20

[sandwichs]

Being able to see all problems was an useful feature that will be hard to replace.
But I guess the users won't care and code quality is not my problem, so I give up.