Last Comment Bug 87701 - Invalid username in bug changes echoed back without being escaped
: Invalid username in bug changes echoed back without being escaped
Status: RESOLVED FIXED
security
:
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: unspecified
: x86 Windows NT
: -- normal (vote)
: Bugzilla 2.14
Assigned To: Gervase Markham [:gerv]
: default-qa
:
Mentors:
: 93385 (view as bug list)
Depends on:
Blocks: 38852
  Show dependency treegraph
 
Reported: 2001-06-25 14:57 PDT by Jesse Ruderman
Modified: 2012-12-18 20:46 PST (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (529 bytes, patch)
2001-06-26 10:28 PDT, Gervase Markham [:gerv]
no flags Details | Diff | Splinter Review
Patch v.2 (581 bytes, patch)
2001-06-26 11:59 PDT, Gervase Markham [:gerv]
no flags Details | Diff | Splinter Review
Patch v.3 - is this what's wanted, then? (472 bytes, patch)
2001-06-28 04:35 PDT, Gervase Markham [:gerv]
no flags Details | Diff | Splinter Review
Patch v4 (795 bytes, patch)
2001-06-29 01:19 PDT, Dave Miller [:justdave] (justdave@bugzilla.org)
no flags Details | Diff | Splinter Review
Patch v.5 - expanded in scope (3.20 KB, patch)
2001-07-01 13:52 PDT, Gervase Markham [:gerv]
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2001-06-25 14:57:35 PDT
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] 2001-06-26 10:28:56 PDT
Created attachment 40096 [details] [diff] [review]
Patch
Comment 2 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-06-26 10:46:16 PDT
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] 2001-06-26 11:57:58 PDT
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] 2001-06-26 11:59:16 PDT
Created attachment 40117 [details] [diff] [review]
Patch v.2
Comment 5 Jacob Steenhagen 2001-06-26 19:27:00 PDT
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] 2001-06-27 00:37:08 PDT
> 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 2001-06-27 06:12:12 PDT
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] 2001-06-28 04:35:49 PDT
Created attachment 40452 [details] [diff] [review]
Patch v.3 - is this what's wanted, then?
Comment 9 Jacob Steenhagen 2001-06-28 06:20:38 PDT
Yes... that looks good.

r=jake on attachment 40452 [details] [diff] [review].
Comment 10 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-06-29 01:11:49 PDT
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] (justdave@bugzilla.org) 2001-06-29 01:19:40 PDT
Created attachment 40606 [details] [diff] [review]
Patch v4
Comment 12 Gervase Markham [:gerv] 2001-06-29 03:06:27 PDT
> 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] (justdave@bugzilla.org) 2001-06-29 03:25:22 PDT
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] 2001-06-29 06:05:54 PDT
Well, my patch has r= and you said they are the same, so I'll just check it in 
:-)

Gerv
Comment 15 Jesse Ruderman 2001-06-29 15:31:32 PDT
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] 2001-07-01 07:14:57 PDT
> 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] 2001-07-01 13:49:23 PDT
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] 2001-07-01 13:52:45 PDT
Created attachment 40833 [details] [diff] [review]
Patch v.5 - expanded in scope
Comment 19 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-07-04 00:08:29 PDT
r= justdave

checked in.
Comment 20 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-08-04 00:59:06 PDT
*** Bug 93385 has been marked as a duplicate of this bug. ***
Comment 21 Dave Miller [:justdave] (justdave@bugzilla.org) 2001-09-02 23:40:36 PDT
Moving to Bugzilla product

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