Closed Bug 87701 Opened 23 years ago Closed 23 years ago

Invalid username in bug changes echoed back without being escaped


(Bugzilla :: Bugzilla-General, defect)

Windows NT
Not set



Bugzilla 2.14


(Reporter: jruderman, Assigned: gerv)



(Whiteboard: security)


(5 files)

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

Result: The name is not a valid username.

Expected: The name <u> 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.
Blocks: 38852
Whiteboard: security
Target Milestone: --- → Bugzilla 2.14
Attached patch PatchSplinter Review
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 in the 
DBIDtoNameAndCheck() routine.
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? isn't available to so I just copied the three lines.

Assignee: tara → gervase.markham
Attached patch Patch v.2Splinter Review and 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).
> and 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?

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
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 

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 
Attached patch Patch v4Splinter Review
Keywords: patch, review
> 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. 

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 :-)
Well, my patch has r= and you said they are the same, so I'll just check it in 

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.)
> 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 

OK, new patch coming up. It does the following:
- Adds a regexp to match all illegal chars in an email address (list gathered   
  from 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.

r= justdave

checked in.
Closed: 23 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.