297940 - incorrect values may be passed to variables if regular expressions return no value

Status: RESOLVED WORKSFORME

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

Bug 297928 fixed the problem about unreliable $1 for detaint_natural,
detaint_signed and trick_taint routines only. We have to fix this problem in the
rest of the code too.

Comment 1

[Christian Reis]

My initial analysis of the problem finds the following locations:

  * LoadTemplate in editclassifications.cgi
  * doGroupChanges in editgroups.cgi
  * bz_lock_tables in DB/Pg.pm
  * login in Auth/Login/WWW/Env.pm
  * editwhines.cgi (wtf is up with this code)
  * checksetup.pl around line 2448 (though this seems not to be a real problem)

Search.pm may be affected: I don't understand that code.

I couldn't find anywhere else that looks like a problem. A patch to fix at least
those problems would be very welcome.

Comment 2

[Frédéric Buclin]

what's the regexp you used? mine is "grep -rn -A 2 -B 2 --color '= $1' *" and I
get a full list of potential problems (I did not investigated).

Comment 3

[Christian Reis]

I just grepped for $1 and looked through the results one by one. It doesn't
actually occur so often.

Comment 4

[Christian Reis]

I realize I actually may have missed some spots for another fundamental
misunderstanding: if you have an re like /(a|b)?(.*)/ and the first item doesn't
match, $1 is also undefined. This generates some warnings, such as
Bugzilla::Search's SqlifyDate, on the line:

        if ($sign eq '+') { $amount = -$amount; }

That needs to be tested:

        if ($sign && $sign eq '+') { $amount = -$amount; }

Comment 5

[A. Karl Kornel]

The problem in Env.pm is completely destroying my access to New Charts, so I'm
splitting that bug off and fixing it now.

Comment 6

[A. Karl Kornel]

I'm creating a new bug to handle the noted problem with editwhines.

Comment 7

[u197037]

JIMHO
1. LoadTemplate in editclassifications.cgi is safe: there are no way to execute
this sub with user-modified or undef action value. Only pre-set ones.
2. editgroups.cgi - It's trivial to fix. Patch is on it's way...
3. editwhines.cgi - as well as #2 

Comment 8

[u197037]

Attachment: Simple patch for editgroups.cgi and editwhines.cgi

If I understand the problem right, simply adding "|| ''" will help to handle
undef $1 problem. Also, I've changed "=~ $1" to "= $1", but I'm not sure was it
right or not. Will also ask Erik for comments on last issue.

Comment 9

[u197037]

Comment on attachment 189893 [details] [diff] [review]
Simple patch for editgroups.cgi and editwhines.cgi

Oops... Missed already posted patch for bug 300831. Marking my one as obsolete.

Comment 10

[Max Kanat-Alexander]

justdave -- I think that this isn't a blocker, per se, unless we can prove that
it actually causes some specific bug.

Comment 11

[Dave Miller [:justdave]]

ok.

Comment 12

[Frédéric Buclin]

We are freezing the code for 3.0 in two weeks and we don't expect this bug to be fixed on time.

Comment 13

[Frédéric Buclin]

Specific problems will be filed separately.