Closed Bug 682244 Opened 8 years ago Closed 8 years ago
Auto-Update of CRLs not working with DD
.MM .YYYY date locale
User Agent: Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0 Build ID: 2011080400 Steps to reproduce: CLRs are (still) not being updated, which is quite security critical. More details can be found in bug #371522 but that bug is close fixed and the status is not changable. There is also no reaction from Mozilla developers there about the latest comments, that's why I have to open this new bug report.
This was something which was resolved over 2 years ago. If this is now a problem for you in Firefox 6, this is a regression. Please post steps to reproduce, a clearer description of the problem you are seeing, and provide a regression range if possible. Thanks
the point is that the reported problem was probably not corrected. Configure whatever CRL you want, wait for the day before the schedules auto-update day and see what happens. Nothing will happend unfortunately like you can see in the screenshot (https://bugzilla.mozilla.org/show_bug.cgi?id=371522#c23). You can also use the CRL URL that you see there to reproduce the problem. I'm not repeating all of the bug descriptions from bug #371522 because *this* bug is just there because #371522 cannot be reopened by ordinary bugzilla users.
Work will not continue on that bug because it is has been fixed long ago. I implore you to pull over pertinent information from that bug or to simply provide the required information to reproduce. We also need a regression window. Without that information in this bug, it will never be resolved.
I can confirm this partially (Gecko/20110828 Firefox 8.0a2): There seem's to be an off-by-one in the "x days before next update" function. I imported a CRL that has next update field: Next Update: Sep 6 21:03:09 2011 GMT Now I enable auto-update and specify "Update 1 day(s) before Next Update date". I'd expect this to update at Sep 5 21:03:09 2011 GMT, but it does not, it updates at Sep 6. I deleted the CRL, reimported and set auto-update to "Update 5 day(s) before Next Update date". As expected, it does NOT update at Sep 1 but at Sep 2 it does. This sounds like an off-by-one in the day calculation, might be easy to fix. It's unclear though if this is a regression or has been like that always.
Status: UNCONFIRMED → NEW
Ever confirmed: true
you might see a slightly different bug actually, can you please file a new bug for the off-by-one bug that you see? The initial report and the screen shot from https://bugzilla.mozilla.org/show_bug.cgi?id=371522#c23 show that the CRLs are not auto-updated at all.
Hm, I also tried the same steps with the CRL mentioned in bug 371522 screenshot attachment and besides the remark in comment 4, I cannot confirm the behavior. I imported the CRL, closed the browser and set a date after the next update field date, and auto-update status now says "OK". @ Bjoern: Can you reproduce this on Fx 6, and if so, could you post steps to reproduce so I can try and reproduce it?
(In reply to Christian Holler (:decoder) from comment #6) > Hm, I also tried the same steps with the CRL mentioned in bug 371522 > screenshot attachment and besides the remark in comment 4, I cannot confirm > the behavior. in any case you see a different bug in the same area, so please report it separately to not mix that issue up with the issue that people see here. If you do, please say which bug number that is so we can track changes from there, too. > @ Bjoern: Can you reproduce this on Fx 6, and if so, could you post steps to > reproduce so I can try and reproduce it? I've seen this in *all* Firefox releases (Linux 32bit build only) that I can remember. There is not much more I can tell you to help to reproduce this except for adding the CRL and provide the URL of it. Maybe other people seeing the CRLs-never-updated bug can tell which OS build they see that bug on, maybe it is related to the operations system in some way.
(In reply to Bjoern Jacke from comment #7) > in any case you see a different bug in the same area, so please report it > separately to not mix that issue up with the issue that people see here. If > you do, please say which bug number that is so we can track changes from > there, too. Bug 682830. > > I've seen this in *all* Firefox releases (Linux 32bit build only) that I can > remember. There is not much more I can tell you to help to reproduce this > except for adding the CRL and provide the URL of it. Maybe other people > seeing the CRLs-never-updated bug can tell which OS build they see that bug > on, maybe it is related to the operations system in some way. I tried this on Windows because it was the first VM that I had available, but I will try on Linux in a few and let you know about the results.
Just tried it on Linux with Fx6. Added the CRL with auto-update set to 1 day before next-update date (next-update date for the CACert CRL I have is Sept 5). Then I changed date to a date after the update date (Sept 7 in my case), started Firefox again and waited a bit (it does not seem to do the update at start up). After some minutes, the auto-update status field showed "OK".
and I can reproduce the not-updating on a Windows XP 32bit machine with a fresh Firefox 6 installation instantly. As the "next update time" is defined in the CRL itself it's a bit tricky to tell if it works or not because even on a manual update the real next-update-time is shown taken from the info inside the CRL. I simply used wireshark to sniff all the traffic to the CRL server. So after add the CRL with URL http://crl.cacert.org/revoke.crl and enabling the auto-update, I shut down the VM, sniff all the traffci to crl.cacert.org, set the clock of the Test VM 60 days in the future, start it up and start Firefox. Even after several minutes you see no traffic going to crl.cacert.org. Maybe the update only work if you (re)start your Firefox on the "next-update" day and it fails if Firefox is not being restarted or is just being (re)started some days later?
(In reply to Bjoern Jacke from comment #10) > and I can reproduce the not-updating on a Windows XP 32bit machine with a > fresh Firefox 6 installation instantly. As the "next update time" is defined > in the CRL itself it's a bit tricky to tell if it works or not because even > on a manual update the real next-update-time is shown taken from the info > inside the CRL. I simply used wireshark to sniff all the traffic to the CRL > server. So after add the CRL with URL http://crl.cacert.org/revoke.crl and > enabling the auto-update, I shut down the VM, sniff all the traffci to > crl.cacert.org, set the clock of the Test VM 60 days in the future, start it > up and start Firefox. Even after several minutes you see no traffic going to > crl.cacert.org. I will try to repeat your exact setup. Is the "auto-update status" field remaining empty for you or does it show OK anyways? > Maybe the update only work if you (re)start your Firefox on the > "next-update" day and it fails if Firefox is not being restarted or is just > being (re)started some days later? At least on Linux, I did not start at the exact date at all, only afterwards, and it worked. Will try on Windows again but I think the problem is elsewhere. I also used https not http for the CRL.
(In reply to Christian Holler (:decoder) from comment #11) > I will try to repeat your exact setup. Is the "auto-update status" field > remaining empty for you or does it show OK anyways? it remains empty > I also used https not http for the CRL. that doesn't make a difference for me.
(In reply to Bjoern Jacke from comment #12) > it remains empty I just tried this on Windows XP (32 bit) with a fresh install of Firefox 6, using the exact same CRL as you did (with HTTP), then closed FF and set the time to be November 2011. 3 minutes after opening Firefox again, my status field went to "OK". I did not check with Wireshark but given that your status remains empty and mine updates, that isn't necessary I guess. How long did you wait for the update to happen?
Status: NEW → UNCONFIRMED
Ever confirmed: false
(In reply to Christian Holler (:decoder) from comment #13) > I did not check with Wireshark but given that your > status remains empty and mine updates, that isn't necessary I guess. How > long did you wait for the update to happen? more than an hour with firefox running idle in the VM. wireshark showing *no* access to the CRL server at all, status field remaining empty.
I tried to use Auto-Update of CRL with several machines (WinXP, Win7) and different versions of Thunderbird and Firefox with the CRLs from CAcert; it never worked (status field remaining empty) Then I tried Knoppix5.1.1EN and it worked instantly, but Knoppix5.1.1DE doesn't. Also Knoppix6.7.0EN works, while Knoppix6.7.0DE doesn't. with EN, status field shows OK or an error with DE, status field remains empty Therefore this might be related to system or program language , so I would recommend to mention system and program language with the test results. WINXP DE Firefox 6.0.1 DE not working WIN7 DE Firefox 6.0.1 DE not working KNOPPIX 5.1.1 EN working: "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:188.8.131.52) Gecko/20061105 Iceape/1.0.6 (Debian-1.0.6-1)" "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:184.108.40.206) Gecko/20061220 Thunderbird/220.127.116.11" "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:18.104.22.168) Gecko/20061205 Iceweasel/22.214.171.124 (Debian-126.96.36.199+dfsg-1)" KNOPPIX 5.1.1 DE not working: "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:188.8.131.52) Gecko/20061105 Iceape/1.0.6 (Debian-1.0.6-1)" "Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:184.108.40.206) Gecko/20061220 Thunderbird/220.127.116.11" "Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:18.104.22.168) Gecko/20061205 Iceweasel/22.214.171.124 (Debian-126.96.36.199+dfsg-1)" KNOPPIX 6.7.0 DE not working: "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:188.8.131.52) Gecko/20110626 Icedove/3.1.11" "Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0 Iceweasel/5.0" KNOPPIX 6.7.0 EN working: "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:184.108.40.206) Gecko/20110626 Icedove/3.1.11" "Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0 Iceweasel/5.0"
@mozthun: Thank you for your detailed analysis! Indeed I was only trying to reproduce this issue with EN builds, not with DE builds. I will try this out either today or tomorrow and report back if I can confirm.
I finally traced it done to a single setting and Auto-Update worked the first time on this WinXP DE machine. The problem is the system date format; it's working with MM/DD/YYYY, but not with DD.MM.YYYY The value in security.crl.autoupdate.nextInstant... is saved with the date format of the system; a soon as you change nextInstant to the date format MM/DD/YYYY on a system with date format DD.MM.YYYY , CRL Auto-Update works. To reproduce this bug, set you system date format to DD.MM.YYYY (e.g. with German regional settings)
After setting system date format to DD.MM.YYYY (using german regional settings on Windows XP), I can confirm the behavior. With that locale, I get this value for nextInstant: security.crl.autoupdate.nextInstant.http://www.CAcert.org;11.09.2011 08:52:56 Even after setting my date to 15.09.2011 and waiting, nothing happens, no CRL update although auto-update is enabled.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Auto-Update of CRLs not working → Auto-Update of CRLs not working with DD.MM.YYYY date locale
I don't think this is a regression. The data in the nextInstant field should be stored as a time_t, storing a date string will never work (and probably never did with a locale that uses a different date format). I assume this feature never worked as intended on those locales. It is still unclear though how this will be fixed, as I was told there is some major work in progress that will replace this subsystem (and possibly others related to certificate management).
(In reply to Christian Holler (:decoder) from comment #19) > I don't think this is a regression. The data in the nextInstant field should > be stored as a time_t, storing a date string will never work Fully agree. Hard to understand why the code in nsCRLManager.cpp goes into all the contortions of converting a PRTime (PRInt64) value into a localized time string... http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsCRLManager.cpp&rev=&cvsroot=/cvsroot&mark=493-496#488 (Converting it back won't work for quite a few locales, I assume - see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/misc/prtime.c&rev=3.44&mark=951-967#945) > I assume this feature never worked as intended on those locales. Very likely. The code has basically been left untouched for about 9 years. > It is still unclear though how this will be fixed, as I was told there is > some major work in progress that will replace this subsystem (and possibly > others related to certificate management). What subsystem exactly? The CRLManager in PSM?
(In reply to Kaspar Brand from comment #21) > What subsystem exactly? The CRLManager in PSM? I think he is refering to the switch to libpkix for cert chain validation. The change to libpkix will not affect this.
Severity: critical → normal
(In reply to Brian Smith (:bsmith) from comment #22) > > I think he is refering to the switch to libpkix for cert chain validation. > The change to libpkix will not affect this. Yep that's what I meant. But kaie said this would also affect the update procedure for CRLs. @kaie: Can you give us some feedback here? Thanks :)
Assignee: nobody → nobody
Component: Security → Libraries
Product: Firefox → NSS
QA Contact: firefox → libraries
Version: 7 Branch → unspecified
Moving to PSM.
Assignee: nobody → nobody
Component: Libraries → Security: PSM
Product: NSS → Core
QA Contact: libraries → psm
Version: unspecified → Trunk
why don't you just close this bug as WORKSFORME if you don't care that people from non-English time zones should get CRL updates. Certificate Revocation List updates are nothing of importance, are they?
(In reply to Bjoern Jacke from comment #25) > why don't you just close this bug as WORKSFORME if you don't care that > people from non-English time zones should get CRL updates. Certificate > Revocation List updates are nothing of importance, are they? I don't see how your comment helps here. If you want to develop a patch yourself, go for it, post it here and you will get feedback on it. If you don't want to do that, then you will have to wait until someone else does so. Brian has already been assigned this bug, but I assume he is still very busy with other bugs that are considered even more critical right now.
The problem is that we use nsIDateTimeFormat::FormatPRExplodedTime to format the time to update the CRL. FormatPRExplodedTime ""performs a locale sensitive date formatting operation on the PRExplodedTime." However, this should be a locale-*INSENSITIVE* formatting operation that always formats the time in "1-Jan-1970 00:00:00 GMT" format. http://hg.mozilla.org/mozilla-central/annotate/9107c2de89d6/security/manager/ssl/src/nsCRLManager.cpp#l477
(In reply to Brian Smith (:bsmith) from comment #27) Thanks Brian. When I looked into your theory briefly I thought perhaps the formatting wasn't locale sensitive because we pass NULL for the locale, but that just selects the current locale. Would be nice if the API docs for FormatPRExplodedTime noted that.
Patch ready but I have a question re resetting the current value of the pref for affected users. Current changes: - Change nsCRLManager::ComputeNextAutoUpdateTime to format time using PR_FormatTime with a fixed format - uses GMT everywhere - Remove includes, dependencies for nsIDateTimeFormat in nsCRLManager.cpp Resetting pref: - Spoke with Brian earlier: he suggested resetting the value in the pref when it isn't parsed correctly. It looks like this should happen in nsNSSComponent::getParamsForNextCrlToDownload() which is called from DefineNextTimer() called at init/startup. Currently, if security.crl.autoupdate.nextInstant for a particular CRL cannot be parsed, it is just skipped ('continue'). I have change it so that if the value cannot be parsed, a time is setup for 30 secs from now (PR_Now() + CRL_AUTOUPDATE_DEFAULT_DELAY, set in DefineNextTimer()). Let me know if this is acceptable. It seems like the CRL(s) should be updated and the pref reset. There is a chance that the pref may be unparseable for another reason e.g. corruption, in which case timers will be firing every 30 secs. If this isn't acceptable, any suggestions?
I fail to understand why you're keeping a mechanism which is based on PR_FormatTime()/PR_ParseTimeString()... as has been suggested in comment 19, it would be much more reliable to simply use a time_t value (or PRTime, to be precise) for the nextInstant pref. Converting forth and back just makes it more error-prone.
Hi Kaspar, I was looking more at comment 27 and comment 28, which were discussing a quick fix in terms of locale. After discussion with Brian, we decided to remove locale altogether and just use PRFormat (to reduce some dependencies etc.). But that's just a description of a train of thought that started with the assumption that using a formatted date was the way to go, not necessarily the correct train out of the station. I'm happy either way - an advantage of using a formatted date could be easier debugging? But, like I said, I'm happy either way on this.
Steve, please re-post the patch with 8 lines of context and with the hg function names option turned on: [defaults] diff=-U 8 -p qdiff=-U 8 -p qnew = -U [diff] git = true showfunc = true unified = 8
Comment on attachment 591332 [details] [diff] [review] v1.0 patch Clearing review while we wait for updated patch
Sorry for that. I had those settings in my .hgrc but they didn't apply to 'export' apparently.
Comment on attachment 597580 [details] [diff] [review] v1.1 Fixed Diff format Review of attachment 597580 [details] [diff] [review]: ----------------------------------------------------------------- Kai has suggested that we shouldn't fix this bug because we should remove this feature after we add support for auto-fetching CRLs. However, I do not know if/when we will actually support auto-fetching of CRLs, and the fix seems pretty simple, so IMO we should just fix this. ::: security/manager/ssl/src/nsCRLManager.cpp @@ +472,4 @@ > } > } > > + int autoUpdateStrLenMax = 25; const int autoUpdateStrLenMax = 25; Document how you determined that 25 is the correct maximum buffer size. @@ +478,3 @@ > PR_ExplodeTime(tempTime, PR_GMTParameters, &explodedTime); > + // Format as "1-Jan-1970 00:00:00 GMT" > + if (0 == PR_FormatTime(nextAutoUpdateDate, autoUpdateStrLenMax, "%v %T GMT", PR_FormatTime is a deprecated function. It seems like a good idea to just format/parse the value as a number, as Kasper suggested, and then log the formatted version when logging is turned on (Perhaps just by printing the elements of the exploded time structure directly using PR_smprintf.
Attachment #597580 - Flags: superreview?(bsmith) → superreview-
Removed PRFormatTime; using PRTime type and storing as a string in prefs. No Int64 or PRTime type in prefs, so storing as a string. Another option would be to store as two int32s; strings seem simpler in terms of keeping the diff small and less invasive. Using nsCRT::atoll and PR_smprintf to convert between string and Int64. If there are better APIs, let me know and I'll update the diff.
Same as last patch, had skipped 'hg qref' before uploading.
Try Server Results: https://tbpl.mozilla.org/?tree=Try&rev=7ee71edcc534 As of this comment, all is green except for Android Opt jsreftest-1, which I've respun. It's fine on Android XUL Opt, and I don't expect it to be different for native.
Status: NEW → ASSIGNED
I would have prefered some comments in the code that explain why you assume your code works fine. After reading the code, if I understand correctly, anyone already having old CRL-update-preferences, and using this new code for the first time, would experience the following: - old pref is read, reading succeeds - code will attempt to use atoll to convert the pref string to PRTime Various contents for the old pref string are possible, for the various locales. It could either start with a "day of month number" or with a "month number" or with a "year number". It seems that won't be a problem, because the 64bit number of PRTime seems to never wrap until year 586912, if I calculated correctly? If that's true, ignoring that distant future, we can assume that PRNow will always be >= than these very small numbers that will be seen when the old pref strings starts with a numerical day, month or year. However, what about special scenario "zero"? Could the old date string ever start with a "day of week name" or with a "month name", which would result in a zero return value from atoll? If that's true, then all iterations through all the various CRLs would give the zero value. In this scenario, variable nearestUpdateTime would still have value zero after the loop. And in that scenario function getParamsForNextCrlToDownload returns a failure. What does this mean? If you can prove that the old pref string will never start with a character, then your patch seems OK to me. If you cannot rule that out, or if we want to prepare for the scenario that the result of atoll might be zero for whatever reason, then I propose you test your patch with that scenario and make sure it will still work. You might have to adjust that "if(nearestUpdateTime > 0)" check. Or, if my thinking is wrong, please explain why.
Kai, thanks for the review comments. I've come up with a list of possible return values that I want to run by you. So, nsCRT::atoll parses the first token in the string; I see 4 cases based on the type of token parsed: -1- Date number (year, month or day). Sample return values: 2012, 2013, 1-12, 1-31 -> All values far smaller than PR_Now() -2- Alpha char: returns 0; change to PR_Now() and force update. -3- Corrupted value (between epoch and PR_Now()): e.g. 0 - 1332280017 (Tue Mar 20, 2012, 2:46pm approx): autoupdate will happen -4- Corrupted value (larger than PR_Now()): no autoupdate Scenarios 1 and 2 seem most likely for users of CRL autoupdate, resulting in a forced autoupdate and restting the pref to use PRTime as an integer. Note that 2 would be a added condition to my previous patch, something like: if (tempTime == 0) tempTime = PR_Now(); .. just after the pref's value string is parsed. 3 would also trigger an autoupdate (timer set for now + CRL_AUTOUPDATE_DEFAULT_DELAY=30s, right?), and the pref will be given a PRTime value for next update; an unflagged recovery from corruption. Update for 4 will be scheduled according to the value returned (assuming I'm reading DefineNextTimer() correctly); an unflagged corruption with no recovery. I don't know how likely corruption would be, or how much you care about it, but I wanted to mention the possibilities here, specifically the fact that there would be no flagging of the error or the recovery in case 3. Seems to me that this is a corner case we can ignore, but I want to check with you before continuing.
Updated patch adds error handling for pref string with a time format starting with alpha characters. Also adds comment based on scenarios identified in comment 40. Tested by manually changing the pref's value to non-numeric characters. CRL is updated and pref reset to PRTime-based value.
Comment on attachment 608059 [details] [diff] [review] v2.2 Deal with time string to integer = 0 Thank you for the changes, looks good to me, r=kaie
Attachment #608059 - Flags: review?(kaie) → review+
I think we don't need to worry about scenario 4.
Thanks Kai. FYI... Try push: http://hg.mozilla.org/try/pushloghtml?changeset=02ad8e67b8a0 Results page: https://tbpl.mozilla.org/?tree=Try&rev=02ad8e67b8a0 Repo updated before push.
Update on try: Please ignore the job in comment 44; it pushed an unrelated revision, not written by me. Updated job results are here: https://tbpl.mozilla.org/?tree=Try&rev=261986253f0b All seems to pass except Android Reftest J1, which I've respun a couple of times with no success. I doubt it's related to this patch, and I notice other people are seeing it as well.
Comment on attachment 608059 [details] [diff] [review] v2.2 Deal with time string to integer = 0 Review of attachment 608059 [details] [diff] [review]: ----------------------------------------------------------------- I see no problems with this approach. I appreciate the logging to make debugging easier.
Attachment #608059 - Flags: superreview?(bsmith) → superreview+
Thanks Brian and Kai. https://hg.mozilla.org/integration/mozilla-inbound/rev/5904fd174eec
Please can you set the target milestone when landing on inbound, along the lines of http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-when-pushing :-) https://hg.mozilla.org/mozilla-central/rev/5904fd174eec
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Sorry Ed! Missed that one on my final todo list. Thanks!
You need to log in before you can comment on or make changes to this bug.