1483626 - Variable may be blank on the new certificate error strings

Status: RESOLVED FIXED

(Firefox :: Security, defect, P2)

Description

[YF (Yang)]

Attachment: siteCert.crt

Steps to reproduce:
1. Visit the https://www.txrjy.com/.
2. Click the Advanced or Continue button.


Actual results:
www.txrjy.com uses an invalid security certificate. The certificate is not trusted because the issuer certificate is unknown. The server might not be sending the appropriate intermediate certificates. An additional root certificate may need to be imported. The certificate is only valid for . Error code: SEC_ERROR_UNKNOWN_ISSUER

(certErrorMismatchSinglePrefix)


Expected results:
...
"The certificate is only valid for www.notexist.com."


Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=52c14d32d9812951c12d92b2594056fc15c5de80&tochange=617a25dfb30dcf0d64014ca92c987aa5bd3f6a4c

Comment 1

[Trisha Gupta [:trisha]]

Hey, I was working on this and noticed that input.data.certSubjectAltNames and subjectAltNames[0] are blank (shouldn't be), which is why this error is occurring. Does this mean something's wrong with the native code not providing correct data?

Comment 3

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

nsIX509Cert.subjectAltNames will be an empty string if the server's certificate doesn't have the subject alternative name extension present. It also doesn't do much validation of the entries in the certificate's subject alternative name extension if it is present. For example, if there were a dNSName entry that was just spaces or maybe even the empty string, that would show up as spaces or an empty string. I would treat that field as essentially untrusted.

Here's the backing code if it's helpful:

https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/security/manager/ssl/nsNSSCertificate.cpp#646 (this is what runs when the front-end accesses .subjectAltNames)

https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/security/manager/ssl/nsNSSCertificate.cpp#572 (this is called when an nsIX509Cert is initialized)

Comment 4

[Johann Hofmann [:johannh]]

Soo there is a case where the subjectAltName is just blank, that makes sense. Sounds to me like we should have a new string that leaves out the "The certificate is only valid for ..." part, like we had before bug 1415279. That should probably trigger if the alt name is null or an empty string after calling .trim().

Trisha, does that help you?

Comment 5

[Trisha Gupta [:trisha]]

Thanks a lot for your information. That really helps!

Comment 6

[Pascal Chevrel (relman team) -> :pascalc]

Updating tracking flags as we get closer to the 64 release.

Comment 7

[Johann Hofmann [:johannh]]

Hi Trisha, are you still looking into this bug or can I unassign you? :)

Thanks!

Comment 9

[Carolina Jimenez Gomez]

Attachment: Bug 1483626 - Checks if subjectAltNames has elements that are not empty string, and if it has them, they will be remove, preventing incomplete messages to show to the user. r=johannh

Comment 10

[Carolina Jimenez Gomez]

This is the output of doing a try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2d9beaa7129ec770a96138295deeb671033cdec

Comment 11

[Johann Hofmann [:johannh]]

Hey, that's probably due to removing the mochitest cert. See Phabricator :)

Comment 12

[Carolina Jimenez Gomez]

This is the output again... https://treeherder.mozilla.org/#/jobs?repo=try&revision=309cd8d487dabc2c0ee46531d234f1ce8de69a25

Comment 13

[Johann Hofmann [:johannh]]

That looks good to me! You will notice that some tests failed (the orange fields), but all of them seem to be intermittent (they have existing bugs filed for them and don't seem to be related to our test).

You should update the patch on phabricator and set checkin-needed.

Comment 14

[Natalia Csoregi [:nataliaCs]]

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

Comment 15

[Johann Hofmann [:johannh]]

You can do that with hg pull central --rebase :)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a09b3ceb8f72
Checks if subjectAltNames has elements that are not empty string, and if it has them, they will be remove, preventing incomplete r=johannh

Comment 17

[Razvan Maries]

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