117614 - Undefined subroutine &main::detaint_natural called at editusers.cgi line 739.

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P1)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

While using the current 2.14.1 branch, remove or add a user to a group and you
receive that message.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

IMO this should be a 2.14.1 release blocker if someone can confirm this.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

sub detaint_natural was added in globals.pl in rev 1.124bbaetz%cs.mcgill.caOct
23 2001 in bug 97469 which the branch didn't pick up.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

And the calls (two of them) in editusers.cgi were added in bug 108821: Prevent
users with any blessgroupset privileges from blessing any group set, which the
branch DID pick up.

1.29 myk%mozilla.org Nov 7 2001

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

long_list.cgi also has a call to detaint_natural.

Fix for bug 109690: long_list.cgi was not validating that the bug ID parameter
was actually a number.
Patch by Brad Baetz
r= justdave, gerv

Comment 5

[Bradley Baetz (:bbaetz)]

Sigh. Yeah, port detaint_natural to the branch - these security bugs are why it 
exists. I'm surprised we didn't catch this sooner.

(BTW, why is this a security-sensitive bug? Of course, thats the only reason I 
noticed it while on holiday :)

Comment 6

[Dave Miller [:justdave]]

Because the summaries of a couple of the existing security bugs were mentioned
in previous comments.

Comment 7

[David Lawrence [:dkl]]

I am holding putting out a Red Hat Bugzilla Errata update pending fix for this
bug.

Comment 8

[David D. Kilzer (ddk)]

Attachment: Patch v.1

Picking up detaint_natural() and trick_taint() from patch for
bug 97469 (attachment 54138 [details] [diff] [review]).  Patches only globals.pl and
processmail.

globals.pl
  - Add attribution for Bradley Baetz
  - Pick up additional $ENV deletion
  - Replace sub detaint_string() with sub trick_taint() and
    sub detaint_natural()
  - Changes use of detaint_string() to trick_taint()

processmail
  - Add attribution for Bradley Baetz [not in original patch]
  - Change detaint_string() to trick_taint()
  - Add two calls to detaint_natural()
  - Add call to detaint_natural() in foreach loop

I recommend that you look at the original patch when reviewing
this one!

Comment 9

[Jacob Steenhagen]

Is there anything in 2.14.1 that wants to use trick_taint()?  If not, I'd
suggest simply adding detaint_natural() and leaving my poorly named
detaint_string() alone for this version :)

2.16 will, obviously, have the correctly named routine.

Comment 10

[David D. Kilzer (ddk)]

All instances of detaint_string() were replaced in the original patch
with either detaint_natural() or trick_taint() (which, I just noticed,
is named poorly--it should be trick_detaint() or detaint_trick()).

I grepped for other uses of detaint_string(), and there were none
outside the original patch, so I figured I would get rid of it in
favor of the newer functions.

I focused on only grabbing the "taint"-related changes from that
patch.  The rest of the changes were left out.

Comment 11

[Jacob Steenhagen]

I understand that you're trying to make it match what the original patch was,
but my goal is to keep the security patch (to version 2.14.1) as small as
possible.  To that end, I'd rather see just the addition of detaint_natural()
than all the changes needed in processmail, etc. to rename detaint_string() to
trick_taint().

Also, trick_taint() is the correct name for the long term as the only reason to
call that routine is to literally trick the tainting mechenism to think
something is safe (because you know there's no way it can't be) even though its
logic tells it that the string isn't safe.

Comment 12

[David D. Kilzer (ddk)]

Attachment: Patch v.2

The kindler, gentler version of Patch v.1 (attachment 63263 [details] [diff] [review]) with
only the detaint_natural() subroutine added as well as the
attribution line for Bradley Baetz.  Obsoleted first patch.

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 63274 [details] [diff] [review]
Patch v.2

r=justdave

Comment 14

[David Lawrence [:dkl]]

Comment on attachment 63274 [details] [diff] [review]
Patch v.2

Looks good. Works as expected.

Comment 15

[David Lawrence [:dkl]]

Patch for fix checked in 2001/01/03 on -rBUGZILLA-2_14_1-BRANCH. Resolving as FIXED.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Opening security bugs for which fixes have appeared in official bugzilla
release.  As per justdave and his posse.