87701 - Invalid username in bug changes echoed back without being escaped

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Go to bug 33830.
2. Try to add <u>@netscape.com to the CC list.

Result: The name @netscape.com is not a valid username.
                 --------------------------------------

Expected: The name <u>@netscape.com is not a valid username.

This bug happens with all e-mail address fields, not just with CC.  I think it
affects the new-bug form as well as the form for modifying existing bugs.
This is a security hole. Fixing bug 26257 would make the security hole less
exploitable, but it would still be a bug.

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch

Comment 2

[Dave Miller [:justdave]]

This patch looks to me like it will put HTML escapes in the database...  not 
necessarily a good thing.  Based on the code surrounding the line you added I 
think the quoting actually needs to be done in globals.pl in the 
DBIDtoNameAndCheck() routine.

Comment 3

[Gervase Markham [:gerv]]

Yes, I can do it there. In that case, escapes wouldn't be (potentially) added to
the database unless ForceOK was true. Is that acceptable?

CGI.pl isn't available to globals.pl so I just copied the three lines.


Gerv

Comment 4

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Comment 5

[Jacob Steenhagen]

CGI.pl and globals.pl are both required in all of bugzilla's CGI's.

Also, I don't think this solves the problem at hand.  The CC ($name) is passed
to the DBNameToIdAndCheck() routine, but it is not returned, so quoting it there
will not have any effect.  What was done for other bugs of the nature is running
the variable through html_quote() before it is printed.

my $cc = html_quote($::FORM{newcc});
print "$cc doesn't exist";

Of course, this has to be done anywhere that user input is echoed in an error
message (as any other time).

Comment 6

[Gervase Markham [:gerv]]

> CGI.pl and globals.pl are both required in all of bugzilla's CGI's.

Well, I tried calling the html_quote function and it didn't like it...

>  so quoting it there will not have any effect. 

The error message which is the source of this bug report is there, and my code 
does correct that error message. If you are saying there's a problem with 
invalid characters getting into the DB, then this should be caught by 
emailregexp, shouldn't it?

Gerv

Comment 7

[Jacob Steenhagen]

My bad.  DBNameToIdAndCheck() also prints the error message, so quoting it there
does work.  Also, |$name = html_quote($name);| does work where you copied the
regular expressions from CGI.pl.

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.3 - is this what's wanted, then?

Comment 9

[Jacob Steenhagen]

Yes... that looks good.

r=jake on attachment 40452 [details] [diff] [review].

Comment 10

[Dave Miller [:justdave]]

Hmm, this has a r= on it and has been sitting here.  I'm glad it hasn't been 
checked in yet, because I'm still going to veto this one.  You're HTML-encoding 
data going to the database still.  DBname_to_id() is safe, because it's using 
SqlQuote() on the data before it goes to the database.  That'll prevent anything 
nasty from happening there.  So there's no need to encode the data prior to that 
point.

The next patch I'll post here in a minute should be a bit safer (it only encodes 
data getting sent back to the browser, and not data that's getting send to the 
database)

Comment 11

[Dave Miller [:justdave]]

Attachment: Patch v4

Comment 12

[Gervase Markham [:gerv]]

> You're HTML-encoding data going to the database still.

There's something wrong here. It's asking the DB whether <string> is a valid 
email address in the Bugzilla DB. (DBNameToIDAndCheck()). Who cares what 
<string> is? & < and > are not valid characters in an email address, as far as I 
am aware, so munging them to other non-valid characters is not a wrong thing to 
do. The match will fail anyway. 

Gerv

Comment 13

[Dave Miller [:justdave]]

ok, guess that's true...  unless $forceok is set, then it writes it.

Then again, the emailregexp should stop that. :-)

OK, I have no preference.  Someone besides me re-review this and pick one of the 
two.  Either one works :-)

Comment 14

[Gervase Markham [:gerv]]

Well, my patch has r= and you said they are the same, so I'll just check it in 
:-)

Gerv

Comment 15

[Jesse Ruderman]

I prefer Dave's patch, plus a check when you create a new account to disallows
<>& in the e-mail address.  Currently I can create an account with <> in the
e-mail address and then CC that account on a bug.  (See test bug 88448.)

Comment 16

[Gervase Markham [:gerv]]

> plus a check when you create a new account to disallows
> <>& in the e-mail address.

We don't do this already? If we don't, that's a separate (serious, 2.14) bug. 
This one can be fixed by either my patch or Dave's, which are totally 
equivalent.

Gerv

Comment 17

[Gervase Markham [:gerv]]

OK, new patch coming up. It does the following:
- Adds a regexp to match all illegal chars in an email address (list gathered   
  from http://javascript.internet.com/forms/check-email.html) to the 
  CheckEmailSyntax function. It will check that it matches the emailregexp param 
  and that it doesn't match any of those characters. 
- Adds an explanation to the error that this list of chars are not allowed.
- Simplifies the default emailregexp so it doesn't exclude spaces and commas, as 
  we are doing that in the new regexp.
- Change the emailregexpdesc to match.
- Fixes the original problem

Review me, please.

Gerv

Comment 18

[Gervase Markham [:gerv]]

Attachment: Patch v.5 - expanded in scope

Comment 19

[Dave Miller [:justdave]]

r= justdave

checked in.

Comment 20

[Dave Miller [:justdave]]

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

Comment 21

[Dave Miller [:justdave]]

Moving to Bugzilla product