122418 - setting attachment status fails

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P1)

Description

[timeless]

Software error:
Attempted to send tainted string 'INSERT INTO attachstatuses (attach_id, statusid) VALUES (49, 1)' to the database at globals.pl line 216.

For help, please send mail to the webmaster (root@localhost), giving this error message and the time and date of the error.
-

cvs tip, w/ and w/o the patch to remove the old attachment system.
-
Administer Attachment Statuses
 
Name Description Sort Key Product Action(s)
needstesting needs testing 5 test Edit Delete
Create

Comment 1

[Myk Melez [:myk] [@mykmelez]]

I can't reproduce this on my tip install, but maybe I'm not doing something
taint-related correctly.  bbaetz?

Comment 2

[Bradley Baetz (:bbaetz)]

OK, got it. We check that the statuses are valid, but don't ever officially
detaint them. Patch coming, and taking

Comment 3

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 4

[Gervase Markham [:gerv]]

Comment on attachment 66978 [details] [diff] [review]
patch

r=gerv. I haven't reproduced the error, but the added code is, at worst,
harmless, and if bbaetz says it fixes the problem...

Gerv

Comment 5

[Christian Reis]

Comment on attachment 66978 [details] [diff] [review]
patch

damn. this bit me this morning. r=kiko

Comment 6

[Christian Reis]

*** Bug 123404 has been marked as a duplicate of this bug. ***

Comment 7

[Bradley Baetz (:bbaetz)]

Checked in.

Comment 8

[Gervase Markham [:gerv]]

I just hit a similar problem, the patch for which I rolled in over in bug
110012. Is it related?

--- attachment.cgi	20 Jan 2002 01:44:35 -0000	1.8
+++ attachment.cgi	4 Feb 2002 22:33:59 -0000
@@ -316,7 +316,8 @@
       || DisplayError("The attachment number of one of the attachments 
            you wanted to obsolete is invalid.") 
         && exit;
-  
+    trick_taint($attachid);
+    
     SendSQL("SELECT bug_id, isobsolete, description 
              FROM attachments WHERE attach_id = $attachid");
 

Gerv

Comment 9

[Bradley Baetz (:bbaetz)]

Yeah, but theres a slightly cleaner patch coming up.

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: patch for gerv's issue

Well, its actually a separate problem, but since when has that stopped us?

Comment 11

[Bradley Baetz (:bbaetz)]

Oh, and your patch was wrong, because that was the string detainting routine, so
you would have allowed arbitrary html....

Comment 12

[David D. Kilzer (ddk)]

Comment on attachment 67809 [details] [diff] [review]
patch for gerv's issue

r=ddk

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 67809 [details] [diff] [review]
patch for gerv's issue

r= justdave

why did this need a 2nd?  ;)

Comment 14

[Bradley Baetz (:bbaetz)]

Checked in