Invalid username in bug changes echoed back without being escaped

RESOLVED FIXED in Bugzilla 2.14

Status

()

Bugzilla
Bugzilla-General
RESOLVED FIXED
16 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: gerv)

Tracking

unspecified
Bugzilla 2.14
x86
Windows NT

Details

(Whiteboard: security)

Attachments

(5 attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Updated

16 years ago
Blocks: 38852
Whiteboard: security
Target Milestone: --- → Bugzilla 2.14
(Assignee)

Comment 1

16 years ago
Created attachment 40096 [details] [diff] [review]
Patch
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.
(Assignee)

Comment 3

16 years ago
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
Assignee: tara → gervase.markham
(Assignee)

Comment 4

16 years ago
Created attachment 40117 [details] [diff] [review]
Patch v.2

Comment 5

16 years ago
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).
(Assignee)

Comment 6

16 years ago
> 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

16 years ago
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.
(Assignee)

Comment 8

16 years ago
Created attachment 40452 [details] [diff] [review]
Patch v.3 - is this what's wanted, then?

Comment 9

16 years ago
Yes... that looks good.

r=jake on attachment 40452 [details] [diff] [review].
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)
Created attachment 40606 [details] [diff] [review]
Patch v4
Keywords: patch, review
(Assignee)

Comment 12

16 years ago
> 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
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 :-)
(Assignee)

Comment 14

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

Gerv
(Reporter)

Comment 15

16 years ago
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.)
(Assignee)

Comment 16

16 years ago
> 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
(Assignee)

Comment 17

16 years ago
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
(Assignee)

Comment 18

16 years ago
Created attachment 40833 [details] [diff] [review]
Patch v.5 - expanded in scope
r= justdave

checked in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
*** Bug 93385 has been marked as a duplicate of this bug. ***
Moving to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: Bugzilla 2.13 → unspecified
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.