Last Comment Bug 650522 - Can not login on Mail.ru and qiwi.ru in Firefox 3.6.17/3.5.19
: Can not login on Mail.ru and qiwi.ru in Firefox 3.6.17/3.5.19
Status: RESOLVED FIXED
: regression, top100, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: unspecified
: All All
: -- blocker (vote)
: ---
Assigned To: :Ehsan Akhgari
:
:
Mentors:
http://forum.mozilla-russia.org/viewt...
Depends on: 667087
Blocks: CVE-2011-2362
  Show dependency treegraph
 
Reported: 2011-04-16 11:53 PDT by Alexander L. Slovesnik
Modified: 2011-06-28 10:47 PDT (History)
16 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.18+
.18-fixed
.19+
.19-fixed


Attachments
1.9.2 patch (3.10 KB, patch)
2011-04-18 14:25 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
1.9.2 patch (4.86 KB, patch)
2011-04-18 15:07 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
1.9.1 patch (3.85 KB, patch)
2011-04-18 15:08 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Trunk patch (test only) (1.12 KB, patch)
2011-04-18 15:27 PDT, :Ehsan Akhgari
dwitte: review+
Details | Diff | Splinter Review
1.9.2 patch (26.96 KB, patch)
2011-06-06 16:27 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
1.9.1 patch (6.41 KB, patch)
2011-06-06 16:29 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
1.9.2 patch (6.41 KB, patch)
2011-06-06 16:30 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
1.9.2 patch (4.87 KB, patch)
2011-06-06 16:46 PDT, :Ehsan Akhgari
dwitte: review+
christian: approval1.9.2.18+
Details | Diff | Splinter Review

Description Alexander L. Slovesnik 2011-04-16 11:53:29 PDT
I've got a report that users can not login on Mail.ru and qiwi.ru with 3.5.19 and 3.6.17 builds ( ftp://ftp.mozilla.org/pub/firefox/nightly/3.6.17-candidates/build2/win32/ru/Firefox%20Setup%203.6.17.exe and ftp://ftp.mozilla.org/pub/firefox/nightly/3.5.19-candidates/build1/win32/ru/Firefox%20Setup%203.5.19.exe ). It somehow connected with changes in cookies handling in these builds.
I verified that with Firefox 3.6.17 candidate build I cannot login on Mail.ru web-interface. It works in Firefox 3.6.16. 
I've looked at logs and found only one changeset that change something about cookies - http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b48d65b67c9

Mail.ru is largest e-mail provider in Russia (more than 50 millions visitor per month). IMHO this should block.
If someone needs test account in Mail.ru, I will be happy to provide one.
Comment 1 [:Aleksej] 2011-04-16 12:48:56 PDT
Reproduced with:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); ru; rv:1.9.2.17) Gecko/20110414 Firefox/3.6.17

WFM with:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); ru; rv:1.9.2.14) Gecko/20110218 Firefox/3.6.14
Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Comment 2 [:Aleksej] 2011-04-16 12:49:38 PDT
(In reply to comment #1)
> Reproduced with:
(on mail.ru)
Comment 5 :Ehsan Akhgari 2011-04-17 14:30:31 PDT
:(

Posting about this to release-drivers.
Comment 6 :Ehsan Akhgari 2011-04-17 14:31:32 PDT
Alexander: could you please email me  the info for a test account on mail.ru?
Comment 7 Alexander L. Slovesnik 2011-04-17 14:41:35 PDT
(In reply to comment #6)
> Alexander: could you please email me  the info for a test account on mail.ru?

Sent.
Comment 8 Daniel Veditz [:dveditz] 2011-04-18 10:12:25 PDT
Firefox 4.0 works on mail.ru? It contains a fix for the same bug which is why we thought it was safe, but maybe the branch version has a specific problem.
Comment 9 Alexander L. Slovesnik 2011-04-18 10:30:49 PDT
(In reply to comment #8)
> Firefox 4.0 works on mail.ru? It contains a fix for the same bug which is why
> we thought it was safe, but maybe the branch version has a specific problem.
According to comment 1 it works on 4.0.1.
Comment 11 :Ehsan Akhgari 2011-04-18 11:59:17 PDT
(In reply to comment #8)
> Firefox 4.0 works on mail.ru? It contains a fix for the same bug which is why
> we thought it was safe, but maybe the branch version has a specific problem.

The mozilla-central patch and the branch patches were totally different.  This is only a problem on stable branches.  I'm trying to figure out why...
Comment 12 :Ehsan Akhgari 2011-04-18 14:20:19 PDT
So, I have a seemingly safe fix at hand, which I'll attach shortly.

Daniel recommends us that for 1.9.2.17 and 1.9.1.19, we should probably just back out bug 616264, and try to get it in the next time, along with this fix.  I'll do the necessary backouts.
Comment 13 :Ehsan Akhgari 2011-04-18 14:25:36 PDT
Created attachment 526830 [details] [diff] [review]
1.9.2 patch
Comment 14 :Ehsan Akhgari 2011-04-18 15:07:04 PDT
Created attachment 526842 [details] [diff] [review]
1.9.2 patch

Actually, it's best to add the correct length checking too, to make sure that nextDot[1] is not past our string buffer.
Comment 15 :Ehsan Akhgari 2011-04-18 15:08:14 PDT
Created attachment 526843 [details] [diff] [review]
1.9.1 patch

Mostly the same as the 1.9.2 patch, but applying on 1.9.1, and also making sure that the test passes (since NetUtils doesn't exist on 1.9.1).
Comment 16 :Ehsan Akhgari 2011-04-18 15:21:16 PDT
This can block the next dot-release.
Comment 17 :Ehsan Akhgari 2011-04-18 15:27:46 PDT
Created attachment 526847 [details] [diff] [review]
Trunk patch (test only)

The lesson that trunk can take from this is the test case, I guess (since this bug doesn't exist on trunk).
Comment 18 Al Billings [:abillings] 2011-04-18 15:44:22 PDT
I assume we're planning on rebuilding 1.9.2, at least, for this.
Comment 19 :Ehsan Akhgari 2011-04-18 18:55:15 PDT
(In reply to comment #18)
> I assume we're planning on rebuilding 1.9.2, at least, for this.

Christian told me that we are, yes.  I've landed the backouts earlier today, FWIW.
Comment 20 David Chan [:dchan] 2011-04-18 21:38:02 PDT
Comment on attachment 526842 [details] [diff] [review]
1.9.2 patch

Is 
(nextDot < end && *(nextDot + 1) == '\0')
required in the conditional check?

This evaluates true if the character after a '.' is '\0' or the second character after the initial '.' is '\0'. For the former case, I believe strchr() handles it, due to returning NULL after not finding '.' . 

The latter only applies in the first iteration of the loop. There shouldn't be a problem if the string is something like ".\0." since *currentDot will evaluate to FALSE in the ?: . There is a small difference in that the loop iterates one more time with nextDot = nsnull.
Comment 21 Al Billings [:abillings] 2011-04-19 13:45:39 PDT
I see you landed this on 1.9.1 as well, Dan.
Comment 22 Alexey Gladkov 2011-04-19 14:45:07 PDT
I confirm this bug with firefox-3.6.17 (linux)
Comment 23 :Ehsan Akhgari 2011-04-19 16:25:55 PDT
(In reply to comment #20)
> Comment on attachment 526842 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=526842
> 1.9.2 patch
> 
> Is 
> (nextDot < end && *(nextDot + 1) == '\0')
> required in the conditional check?
> 
> This evaluates true if the character after a '.' is '\0' or the second
> character after the initial '.' is '\0'. For the former case, I believe
> strchr() handles it, due to returning NULL after not finding '.' . 

I added |nextDot < end| because otherwise *(nextDot+1) could read memory not part of the string.  What this check effectively tests is whether there is a null character after nextDot.
Comment 24 David Chan [:dchan] 2011-04-19 17:35:32 PDT
(In reply to comment #23)
> (In reply to comment #20)
> > Comment on attachment 526842 [details] [diff] [review]
>
> I added |nextDot < end| because otherwise *(nextDot+1) could read memory not
> part of the string.  What this check effectively tests is whether there is a
> null character after nextDot.

Thanks for the clarification
Comment 25 :Ehsan Akhgari 2011-04-19 17:38:23 PDT
David pinged me about this on IRC, which caused me to document why I added the length checks in these patches.

Originally I ported the first version of the patch (without the |nextDot < end| checks) to 1.9.1, and I saw an existing test under extensions/cookie failing.  I investigated that under the debugger, and the values of currentDot and nextDot were in such a way that made this length check necessary.  I don't remember the exact details at all, and unfortunately I don't have my 1.9.{1,2} object directories around any more, but if you're curious, you can follow my footsteps as I laid out here and see what was breaking under 1.9.1, which caused me to add the length checks.
Comment 26 :Ehsan Akhgari 2011-04-19 17:45:04 PDT
(In reply to comment #25)
> David pinged me about this on IRC, which caused me to document why I added the
> length checks in these patches.

And here's the rest of the story, courtesy of David!


dchan
grr curiosity got the best of me
dchan
looking at 1.9.1 code
ehsan
heh
dchan
there appears to be a path for fileURIs
dchan
where an empty host can get by
ehsan
this rings a bell
ehsan
yeah, maybe it was a file URI
dchan
then you insert . in front
dchan
currentDot = '.'
dchan
nextDot = '\0'
dchan
nextDot + 1 off the string
ehsan
yes
ehsan
which would fail http://mxr.mozilla.org/mozilla1.9.1/source/extensions/cookie/test/unit/test_bug526789.js#65
ehsan
:)
Comment 27 Daniel Veditz [:dveditz] 2011-04-19 22:41:30 PDT
blocking 1.9.2/1.9.1 per comment 19, and fixed by backout of bug 616264

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1a8da3d914c3 (default)
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fab6db5c327d (relbranch)
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9e0d3f6ee9af (default)
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b2756532de0f (relbranch)

This regression bug is "fixed". It would be best to track the "fix bug 616264 correctly" patch in that other bug since that's not how we're fixing the regression. If for some reason you need new patches we can put those in the other bug, otherwise no need to duplicate them.
Comment 28 :Ehsan Akhgari 2011-04-20 11:39:39 PDT
(In reply to comment #27)
> This regression bug is "fixed". It would be best to track the "fix bug 616264
> correctly" patch in that other bug since that's not how we're fixing the
> regression. If for some reason you need new patches we can put those in the
> other bug, otherwise no need to duplicate them.

I'll keep my patches in this bug, and will just apply them on top of the patches in bug 616264 when they get reviewed.  This will make Dan's job as the reviewer much easier.
Comment 29 Al Billings [:abillings] 2011-04-21 16:47:57 PDT
I've verified this fix in build 2 of 1.9.1.19 and build 3 of 1.9.2.17 (and against earlier builds) using a mail.ru account that I created for myself.
Comment 30 :Ehsan Akhgari 2011-05-24 18:09:41 PDT
Dan: ping?
Comment 31 dwitte@gmail.com 2011-05-25 12:53:18 PDT
Pong. :( I'll take a look at this on Sunday!
Comment 32 dwitte@gmail.com 2011-06-04 23:22:27 PDT
Comment on attachment 526847 [details] [diff] [review]
Trunk patch (test only)

r=dwitte
Comment 33 dwitte@gmail.com 2011-06-05 00:10:36 PDT
Comment on attachment 526842 [details] [diff] [review]
1.9.2 patch

>diff --git a/netwerk/cookie/src/nsCookieService.cpp b/netwerk/cookie/src/nsCookieService.cpp
>--- a/netwerk/cookie/src/nsCookieService.cpp
>+++ b/netwerk/cookie/src/nsCookieService.cpp
>@@ -1284,16 +1284,17 @@ nsCookieService::GetCookieInternal(nsIUR

>   const char *currentDot = hostFromURI.get();
>   const char *nextDot = currentDot + 1;
>+  const char *end = currentDot + (hostFromURI.Length() - 1);

So for a string 'foo', 'end' now points to the last character 'o'...

>@@ -1338,17 +1339,17 @@ nsCookieService::GetCookieInternal(nsIUR

>-    if (!nextDot || *(nextDot + 1) == '.')
>+    if (!nextDot || (nextDot < end && *(nextDot + 1) == '\0'))
>       break;

Which means the test 'nextDot < end && *(nextDot + 1) == '\0'' will never evaluate true.

I think you either want 'end' to be pointing to the null, or use an '<=' test here? Since the intent of this test is to break out if there's a '.' at the end of the string; but not if 'nextDot' actually is the end of the string, which happens for file:// URIs.

Does that sound right?
Comment 34 :Ehsan Akhgari 2011-06-06 16:26:44 PDT
Yes, that makes sense.  I'll submit new patches in a moment.  Sorry for my delay.
Comment 35 :Ehsan Akhgari 2011-06-06 16:27:38 PDT
Created attachment 537679 [details] [diff] [review]
1.9.2 patch
Comment 36 :Ehsan Akhgari 2011-06-06 16:29:05 PDT
Created attachment 537680 [details] [diff] [review]
1.9.1 patch
Comment 37 :Ehsan Akhgari 2011-06-06 16:30:47 PDT
Created attachment 537681 [details] [diff] [review]
1.9.2 patch

This time it's the right patch!
Comment 38 :Ehsan Akhgari 2011-06-06 16:32:58 PDT
Comment on attachment 537680 [details] [diff] [review]
1.9.1 patch

So long, 1.9.1!  You were a cute little stable branch.  Your memory will stay with us.
Comment 39 :Ehsan Akhgari 2011-06-06 16:37:30 PDT
Trunk test case pushed to cedar: http://hg.mozilla.org/projects/cedar/rev/53abbc401888
Comment 40 :Ehsan Akhgari 2011-06-06 16:46:21 PDT
Created attachment 537686 [details] [diff] [review]
1.9.2 patch

Without the unnecessary cruft.
Comment 41 dwitte@gmail.com 2011-06-06 16:47:42 PDT
Comment on attachment 537686 [details] [diff] [review]
1.9.2 patch

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

Ship it!
Comment 42 :Ehsan Akhgari 2011-06-06 18:45:41 PDT
Landed for 1.9.2.18:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb935ffe5ff1
Comment 43 Mounir Lamouri (:mounir) 2011-06-07 03:50:40 PDT
Test case in m-c:
http://hg.mozilla.org/mozilla-central/rev/53abbc401888

Note You need to log in before you can comment on or make changes to this bug.