165221 - Apostrophes are not properly handled in e-mail addresses

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect, P1)

Description

[Tim O'Connor]

My proper e-mail address is Tim_O'[e-mail]. RFC822 is perfectly
happy to allow me to have that apostrophe in my e-mail address but SQL is not.
So I attempted to work around SQL by doubling the apostrophe. All's well and my
account is accepted. I do not, however, receive any e-mail. Why? Because there
is no e-mail account Tim_O''[e-mail].

When taking an e-mail address from SQL Bugzilla should check for escaped
characters (like the double apostrophe) and correct them.

I did not, by the way, try escaping the apostrophe with a back-slash as that's
not normal SQL.

Comment 1

[Bradley Baetz (:bbaetz)]

JayPee - this is yours:

sub ValidateNewUser {
 ...
}

   SendSQL("SELECT eventdata FROM tokens WHERE tokentype = 'emailold' 
                AND eventdata like '%:$username' 
                 OR eventdata like '$username:%'");

you cannot do that...

What if the username had a : in it, anyway?

Why is createaccount.cgi trick_tainting the given email addr?

I don't think that this can cause problems, but its a BadIdea to have this
reaching the db anyway...

Comment 2

[J. Paul Reed [:preed]]

Yeah, this looks like a security bug to me.

Accepting, and cc-ing the usual suspects.

And as I told bbaetz in IRC: "I'm on it."

Comment 3

[J. Paul Reed [:preed]]

Did some preliminary research on this: looks like it's a regression from bug
23067. ValidateNewUser was created to prevent registration of old email
addresses that could be used to hijack an account or prevent the original user
from reverting to their old address (I *think*).

Fixing this is gonna be somewhat of a PITA (or rather, is going to involve more
than just the quoting issue) because ':' is used as a delimiter in the token
table. "Oops."

More on this as it develops...

Comment 4

[J. Paul Reed [:preed]]

Attachment: Initial thinking on this

Here's an initial version of the patch on this; untested, but I'll do that
shortly.

As bbaetz and I found out on IRC ":" is ok because CGI.pl:CheckEmailSyntax()
refuses all addresses with ":" (and a bunch of other things, including ", [,
and ], but NOT ', ironically).

Comment 5

[J. Paul Reed [:preed]]

Attachment: Take 2

That last patch was crap, but this is pure *gold*, I tells ya!

Patched a 2.16-release installation and created an account with "'" in it; got
the initial account email. Logged in. World rejoiced.

Comment 6

[Joel Peshkin]

Comment on attachment 97131 [details] [diff] [review]
Take 2

globals.pl : the sqlunameOld and New variables don;t match their declarations 
(U vs u)

May as well fix and attach a fresh patch

Comment 7

[J. Paul Reed [:preed]]

Attachment: Take 3: REALLY gold

Stupid typos.

That other one was fool's gold... *this* one is *real* gold.

Comment 8

[Bradley Baetz (:bbaetz)]

Comment on attachment 97134 [details] [diff] [review]
Take 3: REALLY gold

If the username contains a % sign, then because this uses LIKE, that will be
included in the query.

ditto for _

Comment 9

[J. Paul Reed [:preed]]

Attachment: Take 4?

I have nothing witty to type this late at night.

As bbaetz and I discussed on IRC, there are some real quoting issues with using
SQL's like; % is valid in email, as is _.

The methodology here is split the string into two using substrings, and check
that way. bbaetz wondered out loud if this would impact perf because we're no
longer using indices to help, but that field isn't indexed anyway, so...

Comment 10

[J. Paul Reed [:preed]]

Attachment: Take 5

That's it... I'm not working on this anymore tonight... my stress headache is
making me useless.

Last patch had a missing ) and a couple of other mysql stupidities I forgot to
correct.

Comment 11

[Joel Peshkin]

Comment on attachment 97141 [details] [diff] [review]
Take 5

r=joel

(Alternative to SUBSTRING(LOCATE())...

SUBSTRING_INDEX(str,delim,count)

Comment 12

[Matthew Tuck [:CodeMachine]]

I'm not totally up to speed with this ... do I need to viciously slap JayPee
with a copy of the developers' guide or is this a long-standing issue?  And who
reviewed it if the former?

Comment 13

[Matthew Tuck [:CodeMachine]]

I thought %% or \% or something was the a way of escaping wildcards in LIKE?

Comment 14

[Bradley Baetz (:bbaetz)]

It wasn't JP; it was zeroj (I got confused)

I thikn the problem was assuming that checking the email syntax was enough to
trick taint it

The % thing is separate to this bug, but it was easier to fix it at the same
time than to keep it as it was.

\% can be use, but then you have to escape the \ to, and....

SUBSTRING_INDEX is not db portable, I think (neither is LEFT or RIGHT)

I'll review this later today, hopefully

Comment 15

[Matthew Tuck [:CodeMachine]]

Then zeroj needs to be told about this.  And we still need to know who let it
through.

Comment 16

[Bradley Baetz (:bbaetz)]

Comment on attachment 97141 [details] [diff] [review]
Take 5

The trick_taint was added for the fix for bug 136506, according to cvs, because
perl 5.005 was showing taint errors (bug 136506 is probably why people with
perl 5.6.1 didn't see it)

Anyway, r=bbaetz if you add a comment explaining the fun function usage

Comment 17

[J. Paul Reed [:preed]]

I'll add some comments.

So, bbaetz: does the trick_taint have to go back in for 5.005 support?

Comment 18

[J. Paul Reed [:preed]]

re: comment 15:

This is a regression caused by bug 23067, the huge 'allow users to change their
email addr' patch.

It was written by [e-mail] (John Vandenberg) (cc'ing him now) and r='ed
(at various points in time) by gerv, myk, and justdave.

justdave gave it the final 2xr= on 3/31/2002 (but it was given a previous r=myk
and 2r=justdave) and checked it in on 4/1/2002:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=Bugzilla&branch=&branchtype=match&dir=&file=&filetype=match&who=justdave%25syndicomm.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=3%2F29%2F2002+23%3A00%3A00&maxdate=4%2F2%2F2002&cvsroot=%2Fcvsroot

Comment 19

[J. Paul Reed [:preed]]

Ok... read a bit more on bug 136506, and it looks like the problem was just with
ValidateNewUser(), which calls SqlQuote() now, which de-taintifies things.

Gonna check this in.

Comment 20

[J. Paul Reed [:preed]]

Checked in on the trunk...

Checking in createaccount.cgi;
/cvsroot/mozilla/webtools/bugzilla/createaccount.cgi,v  <--  createaccount.cgi
new revision: 1.25; previous revision: 1.24
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.197; previous revision: 1.196
done

... and the 2.16 branch:

Checking in createaccount.cgi;
/cvsroot/mozilla/webtools/bugzilla/createaccount.cgi,v  <--  createaccount.cgi
new revision: 1.21.2.3; previous revision: 1.21.2.2
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.169.2.10; previous revision: 1.169.2.9
done

I'll wait for a verified before removing the security bit.

Comment 21

[J. Paul Reed [:preed]]

Attachment: Checked in patch

Patch that was actually checked in; it's just Take 5 + comments requested by
bbeatz; adding now to make myk's life easier in patching bmo.

Comment 22

[J. Paul Reed [:preed]]

Removing security bit for publication in status report.

Comment 23

[Bradley Baetz (:bbaetz)]

Readding security bit - 2.16 is not out yet.

Comment 24

[Dave Miller [:justdave]]

Security announcement is posted.  Removing the security bit.

Comment 25

[Dave Miller [:justdave]]

*** Bug 183315 has been marked as a duplicate of this bug. ***

Comment 26

[Frédéric Buclin]

The patch also landed on the 2.16 branch -> TM 2.16.

Comment 27

[PBM]

As per comments in bug 415658, does this need to be reopened?

Comment 28

[Gervase Markham [:gerv]]

No, we don't need to reopen this one, we need to resolve bug 415658. :-)

Gerv