Closed Bug 817596 Opened 7 years ago Closed 6 years ago

Unable to parse nonce values from "jabber.ubuntu-fr.org"

Categories

(Chat Core :: XMPP, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rom3o, Assigned: trouble)

References

Details

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121024073032

Steps to reproduce:

I wanted to test the cat with my thunderbird 2 email accounts while both using the xmpp protocol: facebook-chat and jabber.

I found it necessary to use the account login as a profile name, which I did.
No problem for facebook. Unfortunately I haven't managed to connect to jabber.

I then discovered that it was necessary to add a security exception in thunderbird to connect to jabber, what I did (see attached image). However I don't have the option to designate the certificate as safe (the option is grayed out).



Actual results:

In the end, I keep getting the following error message in the window: "Error: Authentication Failed."

Here's the console error displayed by each failure: 
Horodatage : 03/12/2012 16:01:38
Erreur : Error decoding: nonce="XXXXXXXXXX"
Fichier Source : resource:///modules/xmpp-session.jsm
Ligne : 335
Code Source :
xmpp-session

XXXXXXXXXX is different for each test:
"eLIW35lY5QwC3sUiI/m3uhWJagWVFjzT0DikkoQppV8="
"COlMoGXZJDp4MjRLDpes+zlZ9dT1DZHD6ZDBZHLYLMQ="
"Uy5nCD+5GfYprxz2d4ki6UTJXL1tY7HIiLnx5OdsrMw="


Expected results:

1) the connection would be established. (logical behavior of an instant messaging software)

2) The software should have asked me to add a security exception unless I have to do it manually.


---Note that the connection is no problem in other programs such as Pidgin. This one asks for confirmation to the approval of the safety certificate then connects.
So the reason why we can't connect here is that the nonce values sent by the server during the DIGEST-MD5 authentication dialog contains the = character.

Our current parsing code is:
 88       let e = elem.split("=");
 89       if (e.length != 2)
 90         throw "Error decoding: " + elem;

The line should look like:
nonce=value

So we fail if there's an '=' character in the value.

The specs at http://tools.ietf.org/html/rfc2831 says:
        nonce             = "nonce" "=" <"> nonce-value <">
        nonce-value       = qdstr-val
(at http://tools.ietf.org/html/rfc2831#section-2.1.1)
      qdstr-val      = *( qdtext | quoted-pair )
      qdtext         = <any TEXT except <">>
      quoted-pair    = "\" CHAR
(at http://tools.ietf.org/html/rfc2831#section-7.2)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Can't connect to network messaging "jabber.ubuntu-fr.org" → Unable to parse nonce values from "jabber.ubuntu-fr.org"
Attached patch Untested patch (obsolete) — Splinter Review
I think this is all that is necessary.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #688513 - Flags: review?(florian)
Comment on attachment 688513 [details] [diff] [review]
Untested patch

So I also think this is all we need to fix the case reported in comment 0.

This wouldn't handle the case of "blahblah\"blahblah" that is possible if I'm reading the spec correctly.

I'm also a bit confused that the Digest SASL spec seems to only treat double quotes as a special character, when we also handle single quotes. I wonder if that's also a bug.

Nit: The "split" variable name doesn't seem great. For some reason I would expect that variable to contain an array, rather than an index.
Comment on attachment 688513 [details] [diff] [review]
Untested patch

IRC discussion in #instantbird just after comment 3:
13:02:08 * clokep isn't sure if his patch was r- or r+ then... ;)
13:02:22 - flo-retina: I'm not sure either
13:02:30 - flo-retina: should we handle \ escaping?
13:02:38 - flo-retina: should we remove the single quotes from the regexp?
13:03:16 - flo-retina: another way to say it is that I don't know if we are trying to fix connecting to the server that was reported to be incompatible with us (in that case the patch is good I think) or if we are trying to respect the spec (in that case more work is needed)
13:04:17 - clokep: We should probably fix everything we see that's wrong, but I was hesitant to change a lot without having the problem server around...
13:04:35 - flo-retina: well, the server hostname is given in the bug report ;)
13:05:02 - flo-retina: and you don't need to have an account to receive a challenge from the server
13:05:25 - flo-retina: hmm, if we unescape \, we should reescape it later, shouldn't we?
13:05:45 - flo-retina: or should we just change the regexp to not match " if there's an \ before it?
13:06:09 - flo-retina: that would keep the escaped string, and we would resend it escaped
13:06:25 - clokep: I'd say change the regexp to not match if there's a \ before it.
13:06:41 - flo-retina: hmm, we should check how that string is used
13:06:52 - flo-retina: if we use it while computing something (likely) we need to unescape
13:07:02 - clokep: I can take another look tonight.
Attachment #688513 - Flags: review?(florian) → review-
Duplicate of this bug: 923154
Attached patch Patch v2 (obsolete) — Splinter Review
This patch gets my jabber.org account to connect again.
Attachment #688513 - Attachment is obsolete: true
Attachment #813988 - Flags: review?
Attachment #813988 - Flags: review? → review?(florian)
Comment on attachment 813988 [details] [diff] [review]
Patch v2

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

Sorry for the delayed review!

::: chat/protocols/xmpp/xmpp-authmechs.jsm
@@ +92,5 @@
>          throw "Error decoding: " + elem;
>  
> +      // Replace any " or ' not immediately preceded by a \.
> +      data[elem.slice(0, index)] =
> +        elem.slice(index + 1).replace(/(^|[^\\])"|'/g, "$1");

This regexp is confusing, and I'm not convinced it does what you want.

After reading again the spec I quoted in comment 1, I think we should look at the first and last character. If they are both " or ' we should drop them. Then we should unescape the rest of the string.

The part that isn't clear to me is why we are messing with the ' character, but I suspect it was because 'some servers use this character even though it's not in the spec'.
Attachment #813988 - Flags: review?(florian) → review-
Here is a slightly different patch, based on Patrick Cloke's patch. It improves unquoting, and it fixes another parsing issue I have with our in-house Jabber server: Elements of the challenge are not separated by "," but by ", " (note the extra space).
Comment on attachment 8345227 [details] [diff] [review]
alternate suggested patch

I think you wanted to set review for Florian on this.
Attachment #8345227 - Flags: review?(florian)
What about a solution for this problem (for the official release)?
Thunderbird 15.0 introduced Multi-Channel Chat with XMPP, Twitter and IRC und since 17.0.x most people are unable to use their XMPP account, especially in combination with huge XMPP services like jabber.org. For many months one of the functionalities of Thunderbird is broken, so I don't understand why the importance of this ticket is only "normal", it should be "high", the problem has to be fixed until the next release of Thunderbird, hasn't it?
(In reply to Thomas from comment #10)
> I don't understand why the importance of this ticket is only "normal", it should be "high"

As this already has a patch awaiting review, I don't see why you are concerned about the importance rating, which does not usually affect the amount of attention a bug will receive anyway.
Component: Instant Messaging → XMPP
OS: Windows 7 → All
Product: Thunderbird → Chat Core
Hardware: x86_64 → All
Version: 17 → trunk
Comment on attachment 8345227 [details] [diff] [review]
alternate suggested patch

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

This seems fine, thanks for the patch! And I'm really sorry about the delay here :-/.

I just found one nit, but we can handle it while doing the checkin, you don't have to submit a new patch.

::: modules/xmpp-authmechs.jsm.orig
@@ +91,5 @@
>          throw "Error decoding: " + elem;
>  
> +      // Unquote
> +      data[elem.slice(0, index)] =
> +        elem.slice(index + 1).replace(/^["']|["']$/g,"").replace(/\\(.)/g, "$1");

Coding style nit: we usually have a space after the ',' separating function parameters.

The first regexp here would accept a string wrapped in mismatched single and double quotes, but a server sending us such a broken string would likely be quite broken; I don't think this could actually happen, and even if it did, it likely wouldn't have any consequence other than the authentication failing.
Attachment #8345227 - Flags: review?(florian) → review+
Could we also expand the "// Unquote" a bit? The replace followed by replace is a bit confusing to me. :)
https://hg.mozilla.org/comm-central/rev/a3378f94e260

Thanks again for the patch, and sorry it took me so long to take care of it.
Assignee: clokep → trouble
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Attachment #813988 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.