300831 - editwhines twice uses $1 without checking for regex match

Status: RESOLVED FIXED

(Bugzilla :: Whining, defect)

Description

[A. Karl Kornel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)

In editwhines.cgi, around line 240 and immediately after, two regexes were used.
 After each regex the variable $1 is used, but nothing is done to be sure that
the regexes match before using $1.

Reproducible: Didn't try

Steps to Reproduce:




See bug 297940 for more information.

Comment 1

[A. Karl Kornel]

Attachment: Patch v1

For MAILTO_USER, throw an error if the regex doesn't match.  For MAILTO_GROUP,
also throw an error if the regex doesn't match.

Requesting review from joel as suggested by LpSolit.

Comment 2

[Max Kanat-Alexander]

Not blocking unless you can prove it actually causes a bug. (Although I agree
it's good cleanup.)

Comment 3

[u197037]

Karl, As I understand, original code were replasing original $mailto with
results of the match. While your version is passing it as is if matched.
So, I afraid there should be also something like:

if ($mailto =~ /^[0-9a-z_\-\.]+$/i) {
    $mailto = $1; # $1 is definitely not undef here due to above check
    ...

Or, resembling original code, 
$mailto =~ /.../;
$mailto = $1 || '';
if ($mailto ne '') {
    ...

Am I right?

Comment 4

[Marc Schumann [:Wurblzap]]

Blocking a blocker (bug 297940), so this should be a blocker.

Alternatively, make bug 297940 not a blocker any more :)

Comment 5

[A. Karl Kornel]

(In reply to comment #3)
> Karl, As I understand, original code were replasing original $mailto with
> results of the match. While your version is passing it as is if matched.

Well, if the match ever fails an error is thrown and execution stops, which is
fine.  Unfortunately, as you note, a good match doesn't erase the taint on
$mailto.  I'll update the patches appropriately.

Comment 6

[A. Karl Kornel]

Attachment: Patch v2

Modification of attachment 189366 [details] [diff] [review] with respect to comment 5.

In the cases where the match passes, but the tainted $mailto is still used,
replace $mailto with $1.

Moving up review request.

Comment 7

[A. Karl Kornel]

Comment on attachment 189366 [details] [diff] [review]
Patch v1

Removing review requests from obsolete attachments.

Comment 8

[A. Karl Kornel]

Attachment: Patch v2

Comment 9

[Max Kanat-Alexander]

We made bug 297940 no longer a 2.20 blocker, so this is no longer a blocker either.

Comment 10

[Frédéric Buclin]

tip:

Checking in editwhines.cgi;
/cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v  <--  editwhines.cgi
new revision: 1.10; previous revision: 1.9
done

2.20rc2:

Checking in editwhines.cgi;
/cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v  <--  editwhines.cgi
new revision: 1.8.2.1; previous revision: 1.8
done