Closed Bug 1355977 Opened 3 years ago Closed 2 years ago

Viewing the validity dates for a TLS certificate shows two time zones

Categories

(Core :: Security, defect, P5)

52 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: mdunphy, Assigned: jorgk-bmo)

References

Details

(Keywords: regression)

Attachments

(4 files)

Attached image Screenshot of the 'bug'
Visit a site with a working TLS cert, eg, https://bugzilla.mozilla.org
View the certificate, go to the Details tab, and then to the Validity leaf
Check the dates under "Not Before" and "Not After"

The date is shown in the current time zone, and again below in parentheses in GMT. However there are two time zones listed in the parentheses version:

Sun 25 Oct 2015 05:00:00 PM PST
(Mon 26 Oct 2015 12:00:00 AM PST GMT)  <-- Should not have PST here


Version: Firefox 52.0 (64-bit)  via Fedora 24 (firefox-52.0.2-2.fc24.x86_64)
Severity: trivial → normal
Component: Page Info Window → Security
Product: Firefox → Core
I can reproduce this on Linux and OS X but not Windows. It looks like this may have been caused by bug 1301640 or something related. Henri, can you have a look?
Blocks: 1301640
Flags: needinfo?(hsivonen)
I can verify that. It seems like it may be a regression from bug 1351427 since I believe that cert date/times are from C++.

Jorg, any chance you saw anything like that in Tb?

Seems like related to http://searchfox.org/mozilla-central/source/security/manager/ssl/nsNSSCertValidity.cpp and maybe can be simplified now.
(or even migrated to JS!)
Flags: needinfo?(jorgk)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Looking in TB on Windows I see this:
27 February 2016, 01:00:00 AM
(27 February 2016, 12:00:00 AM GMT)
That of course follows the operating settings now. However, the timezone is not part of the OS pattern on Windows.
The reporter seems to be on Linux and we know that Linux appends the timezone as part of the OS pattern for the long format, see comment #1.

So the problem reported is that the we call the C++ formatter, it retrieves the pattern from the OS, on Linux that includes a timezone, and then GMT gets appended:
https://dxr.mozilla.org/mozilla-central/rev/c697e756f738ce37abc56f31bfbc48f55625d617/intl/locale/DateTimeFormat.cpp#199

So the fix would be to strip any timezone from the pattern in case GMT is appended, about here:
https://dxr.mozilla.org/mozilla-central/rev/c697e756f738ce37abc56f31bfbc48f55625d617/intl/locale/DateTimeFormat.cpp#212

Hard for me to do since I'm on Windows. It's a few lines: You need to detect that timezone information is requested in the pattern, then remove it.

You want to have a go? I can give you feedback ;-)
Flags: needinfo?(jorgk)
Flags: needinfo?(hsivonen)
Hmm, no idea how that can be in FF 52 (!!) (Version: Firefox 52.0 (64-bit) via Fedora 24 (firefox-52.0.2-2.fc24.x86_64)) since bug 1351427 landed on FF 55 last week.

You should test this on a Nightly on Mac or Linux first. FF 52 predates the ICU work in bug 1301640 which landed on mozilla53.

Chances are that that behaved like we behave now: get the OS formatted time, append GMT, doh!
Oh, you're saying the GMT gets added at nsNSSCertHelper.cpp#1697. So my analysis in comment #3 was about 70% wrong :-(

I my defence I have to say that the reporter supplied a picture taken with FF 52 and that may or may not match what the current code does.

So does the current code show |12:00:00 AM PST GMT| or does it actually show GMT twice? Do you have a Mac or Linux? In comment #2 you meant to say "confirm", not "verify" right? So you must have seen it on some system.

If you show me what it looks like currently, I can think about it more. The |12:00:00 AM PST| is already wrong since the system should have delivered GMT, not PST.
It does show it twice.

Attaching a screenshot from my linux nightly from today.
That's good news, so the time we format already contains GMT and not some other timezone.

So the fix is in nsNSSCertHelper.cpp#1697:
- text.AppendLiteral(" GMT)");
+ // Append "GMT" if it's not already added by the formatter
+ // since the OS pattern contained a timezone (Mac and Linux).
+ if (tempString.Find(" GMT") != kNotFound)
+   text.AppendLiteral(" GMT)");

Untested and might not compile, so just use the "right" 'Find' function.

As I said, on Windows I can't test it. Since this was broken upto FF 52, and maybe worked better during 53 and 54, this can't be our highest priority bug ;-)
The bug no longer seems to be valid my install of FF55.0.3 does not seem to have the bug
The bug only happens on Linux, your screenshot doesn't look like Linux.
I'm now on FF 54 via Fedora 24 (firefox-54.0-2.fc24.x86_64) and the cert viewer no longer shows two time zones. Rather it shows no time zone and GMT. For the bugzilla.mozilla.org cert:

Not Before:
October 25, 2015 at 5:00:00 PM
(October 26, 2015 at 12:00:00 AM GMT)

Not After:
November 7, 2017 at 4:00:00 AM
(November 7, 2017 at 12:00:00 PM GMT)

That seems closer to the right behaviour. Should the first line have a time zone ("PST" or "GMT-7"), or, is it supposed to be implicit that it's using the local time zone?
Looks like the time zone is not shown. If it were shown, you'd get:
October 25, 2015 at 5:00:00 PM PST
(October 26, 2015 at 12:00:00 AM PST GMT)
I trust Zibi's screenshot from comment #7.
Ah, you are right. Having gotten with the times, F55 on Fedora 26 (firefox-55.0.2-3.fc26.x86_64) is back to the two-time-zones behaviour:

October 25, 2015, 5:00:00 PM GMT-7
(October 26, 2015, 12:00:00 AM GMT GMT)
Blocks: 1351427
Keywords: regression
Priority: -- → P5
Attached patch 1355977.patchSplinter Review
Time to fix this. I've booted my Linux box ;-)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8943078 - Flags: review?(gandalf)
Comment on attachment 8943078 [details] [diff] [review]
1355977.patch

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

lgtm! Thanks for the patch :)
Attachment #8943078 - Flags: review?(gandalf) → review+
Comment on attachment 8943078 [details] [diff] [review]
1355977.patch

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

::: security/manager/ssl/nsNSSCertHelper.cpp
@@ +1676,5 @@
>  
>    text.Append(tempString);
> +  // Append "GMT" if it's not already added by the formatter
> +  // since the OS pattern contained a timezone (Mac and Linux).
> +  if (tempString.Find(" GMT") == kNotFound) {

Would it make sense to Find from the end of the string (RFind)?

@@ +1679,5 @@
> +  // since the OS pattern contained a timezone (Mac and Linux).
> +  if (tempString.Find(" GMT") == kNotFound) {
> +    text.AppendLiteral(" GMT)");
> +  } else {
> +    text.Append(')');

I would append the ) unconditionally for both cases after the if().
Is it still needed to Add single char strings via Append(), not AppendLiteral?
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd5dc9d8245
don't show GMT twice in certificate details. r=zibi
Keywords: checkin-needed
(In reply to :aceman from comment #16)
> Would it make sense to Find from the end of the string (RFind)?
Well, since " GMT" is contained at most once, it makes no difference.

> > +    text.Append(')');
> I would append the ) unconditionally for both cases after the if().
That's a matter of taste. I see no reason for appending " GMT" and then appending ')'.

> Is it still needed to Add single char strings via Append(), not
> AppendLiteral?
Not needed, but more efficient. In C-C we switched all since character appends from AppendLiteral() to Append(), see bug 1402750, https://hg.mozilla.org/comm-central/rev/15ac102a32ac.
https://hg.mozilla.org/mozilla-central/rev/9fd5dc9d8245
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.