Closed Bug 787433 Opened 12 years ago Closed 12 years ago

IRC component should return a proper CTCP VERSION response.

Categories

(Thunderbird :: Instant Messaging, defect)

15 Branch
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 18.0

People

(Reporter: skgsergio, Assigned: clokep)

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120824154833

Steps to reproduce:

Connected to an IRC server with Thunderbrid, and check the Thunderbird's response to a CTCP VERSION request.


Actual results:

Only returned the string "Thunderbird".


Expected results:

According to the IRC's RFC (http://www.irchelp.org/irchelp/rfc/) the client should return the client name and the version.

Sometimes bugs appear in the clients and Network Administrators must get rid of that buggy clients in order to ensure security of the Network users. One of the best "weapons" to detect the buggy versions is CTCP VERSION. If Thunderbird returns only the name of the client a bug on the client can result into a ban to all versions of the client.

I think that should be convenient to add the version number. At least that's my suggestion as a Network Administrator and as a future user of Thunderbird client that I found useful to have it at work.
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Sergio C. G. from comment #0)
> According to the IRC's RFC (http://www.irchelp.org/irchelp/rfc/) the client
> should return the client name and the version.
Yes, it does say that and I usually try very hard to abide by specifications. This was a design decision: I originally had it that way and changed it during a code review to prevent security/privacy leaks about what particular client is being used (with the thought being that if you know the exact client of another user you could more easily attack it).

I'm not sure I'm really qualified to decide whether this is an issue or not. To me it's essentially the same as the User-Agent, which does contain the version...

> Sometimes bugs appear in the clients and Network Administrators must get rid
> of that buggy clients in order to ensure security of the Network users. One
> of the best "weapons" to detect the buggy versions is CTCP VERSION. If
> Thunderbird returns only the name of the client a bug on the client can
> result into a ban to all versions of the client.
I understand the concept behind actually having the version number, yes. :) Does this actually happen in practice? Do most other clients show version numbers (Pidgin/Adium/Finch I believe just report "purple").

> I think that should be convenient to add the version number. At least that's
> my suggestion as a Network Administrator and as a future user of Thunderbird
> client that I found useful to have it at work.
Thanks, it's great to hear back from people running networks.

For fun, would this report the Gecko version or the application version?
Severity: normal → minor
(In reply to Patrick Cloke [:clokep] from comment #1)
> Yes, it does say that and I usually try very hard to abide by
> specifications. This was a design decision: I originally had it that way and
> changed it during a code review to prevent security/privacy leaks about what
> particular client is being used (with the thought being that if you know the
> exact client of another user you could more easily attack it).
>
> I'm not sure I'm really qualified to decide whether this is an issue or not.
> To me it's essentially the same as the User-Agent, which does contain the
> version...

For me is also the same as User-Agent, if the decision is to hide the version number, why not from the emails? Can appear an exploit for the email component and could be also exploited.

I've seen many IRC exploits and usually don't care about the version just try to trigger it.

> Does this actually happen in practice? Do most other clients show version
> numbers (Pidgin/Adium/Finch I believe just report "purple").

Most of the major clients and major engines show its version number but I had to download Adium and check it because I was not sure if libpurple does it  (is not a usual client for IRC) and I found that don't shows version number. I'll check if Pidgin does it on Monday... 

> Thanks, it's great to hear back from people running networks.

First I found it funny but then I thought that is a good feature.

> For fun, would this report the Gecko version or the application version?

Hmmm I think that it must report the version number that will change if there is a change in the IRC core and then a release. I think that application version should be enough.
(In reply to Sergio C. G. from comment #2)
> For me is also the same as User-Agent, if the decision is to hide the
> version number, why not from the emails? Can appear an exploit for the email
> component and could be also exploited.
I should probably say that this decision was made by Instantbird developers, not Mozilla developers. ;) So it should have no bearing on what is done for email.

> I've seen many IRC exploits and usually don't care about the version just
> try to trigger it.
> 
> > Does this actually happen in practice? Do most other clients show version
> > numbers (Pidgin/Adium/Finch I believe just report "purple").
> Most of the major clients and major engines show its version number but I
> had to download Adium and check it because I was not sure if libpurple does
> it  (is not a usual client for IRC) and I found that don't shows version
> number. I'll check if Pidgin does it on Monday... 
I know for a fact that libpurple does not: http://lxr.instantbird.org/pidgin2.6.3/source/libpurple/protocols/irc/parse.c#563

> > For fun, would this report the Gecko version or the application version?
> Hmmm I think that it must report the version number that will change if
> there is a change in the IRC core and then a release. I think that
> application version should be enough.
Probably makes sense since Instantbird and Thunderbird have differing versions of Gecko, but potentially the same version of chat/.

I'm debating this with some of the other developers, we'll see what happens...
Attached patch Instantbird patch (obsolete) — Splinter Review
This adds a version of "Instantbird 1.3a1pre" to Instantbird, it's also customizable per account (since that's something Mook had asked me for a long time ago and I felt like being a nice guy). I don't actually have a comm-central tree and __APP_VERSION__ doesn't seem to be used anywhere in c-c...does this exist in the build system?

If nothing else, we fall back to brandShortName (which is what is currently used), since you can't have an empty VERSION response.
Assignee: nobody → clokep
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #658340 - Flags: review?(aletheia2)
Comment on attachment 658340 [details] [diff] [review]
Instantbird patch

Why not just use nsIXULAppInfo.name and .version? Even simpler you may be able to use the Application object available to js.
(In reply to Patrick Cloke [:clokep] from comment #4)
> I don't actually have a
> comm-central tree and __APP_VERSION__ doesn't seem to be used anywhere in
> c-c...does this exist in the build system?

For Instantbird, it relies on:
DEFINES += -DAPP_VERSION="$(MOZ_APP_VERSION)"
in instantbird/app/Makefile.in

Thunderbird has the same define at http://mxr.mozilla.org/comm-central/source/mail/app/Makefile.in#40
(In reply to Mark Banner (:standard8) from comment #5)

> Why not just use nsIXULAppInfo.name and .version? Even simpler you may be
> able to use the Application object available to js.

The preprocessed default preference value is a nice solution because it lets the user customize the value but is updated automatically if the value is the same as the default one.

I don't think the Application object is available in Instantbird.
(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> (In reply to Mark Banner (:standard8) from comment #5)
> 
> > Why not just use nsIXULAppInfo.name and .version? Even simpler you may be
> > able to use the Application object available to js.
> 
> The preprocessed default preference value is a nice solution because it lets
> the user customize the value but is updated automatically if the value is
> the same as the default one.
This was the reasoning I chose to do it this way. I wouldn't be against using nsIXULAppInfo, however, and just checking to see if that pref exists (or maybe check only the account pref).
Comment on attachment 658340 [details] [diff] [review]
Instantbird patch

Looks good to me - though I do wonder what the use case for per-account version string overrides was ;)
Attachment #658340 - Flags: review?(aletheia2) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
Patch based on Standard8's suggestion.

This checks the version option of the account, if that's not found (or is empty) it will default to "Instantbird <version>" or "Thunderbird <version>". I also changed the username to use Services.appinfo.name, so these two sets of code are done in a similar way.
Attachment #658340 - Attachment is obsolete: true
Attachment #659072 - Flags: review?(aletheia2)
What's the use case for customizing this per-account? (afaik the user who you think requested it doesn't remember about why he would want that).
Isn't it slightly more useful to customize it for all accounts at once? + A global pref with a default value is discoverable for people playing with about:config.
The "version" account-specific pref isn't discoverable at all (except for people reading the code).
(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> What's the use case for customizing this per-account? (afaik the user who
> you think requested it doesn't remember about why he would want that).
> Isn't it slightly more useful to customize it for all accounts at once? + A
> global pref with a default value is discoverable for people playing with
> about:config.
> The "version" account-specific pref isn't discoverable at all (except for
> people reading the code).
I did it this way mostly to match the username code (which also isn't discoverable, by the way). It allows more fine grained control (per account), without being too complicated of checking both a global pref + a account pref.

Perhaps it is debatable whether it should even be customizable.
I think the only thing I could want to customize is "not send the version number", so I think just a boolean pref to send the client name only (ie the previous behavior) as opposed to the full client name+version would do.
Comment on attachment 659072 [details] [diff] [review]
Patch v2

I'll code that up this afternoon.
Attachment #659072 - Flags: review?(aletheia2)
Attached patch Simple patchSplinter Review
Simpler patch that doesn't give the user any options.
Attachment #659339 - Flags: review?(florian)
Attachment #659339 - Flags: review?(florian) → review+
Checked-in as http://hg.instantbird.org/instantbird/rev/46d0d1e824eb for Instantbird.

Adding checkin-needed for comm-central.
Keywords: checkin-needed
Attachment #659072 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/4316b336946c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
Using nsIXULAppInfo.name has the (unintended?) consequence of displaying "Thunderbird" for Daily and Earlybird builds too. Should we revert that change or is it ok?
(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> Using nsIXULAppInfo.name has the (unintended?) consequence of displaying
> "Thunderbird" for Daily and Earlybird builds too. Should we revert that
> change or is it ok?

I think that's fine as it will have the version number with it? Although is it reported as "Thunderbird 18" or is it "Thunderbird 18a1pre" or something like that? If it's the former, I'd think we'd want it to say something else...
I was actually talking about the username from irc.js, so no version number.

But for the CTCP version, I'm not sure it's ok to show "Thunderbird 18.0a1pre". Isn't the Thunderbird trademark reserved for releases and betas?
You need to log in before you can comment on or make changes to this bug.