Forum OpenACS Q&A: Re: Thanks for ETP 2
I allowed the "src" attribute (via the kernel parameter) but added an additional security check in ad_html_security_check validating that the src value actually is an image (by letting ns_guesstype check that it is of mime type "image/*"). This entails that the location of the image must include the file extension (.jpg, .gif, etc.) which unfortunately is not currently the case with thumbnails in Photo Album.
Adding "src" to the allowed attributes should no longer invite XSS attacks, I hope ...
However, I discovered that form-builder elements of type "text" (e.g., text and textarea widgets), such as those in the Forums package, were not being security checked at all(!). So another change I made was to make the "text" datatype validation proc, too, call ad_html_security_check - just like the "richtext" datatype does.
Even though we're going to upgrade all the toolkit's forms to use "richtext" sooner or later it might not hurt to patch this wide open hole up in the meantime.
Here are the proposed changes:
Index: acs-tcl/tcl/text-html-procs.tcl
===================================================================
RCS file: /cvsroot/openacs-4/packages/acs-tcl/tcl/text-html-procs.tcl,v
retrieving revision 1.37
diff -u -r1.37 text-html-procs.tcl
--- acs-tcl/tcl/text-html-procs.tcl 20 Apr 2004 21:13:04 -0000 1.37
+++ acs-tcl/tcl/text-html-procs.tcl 11 May 2004 18:14:51 -0000
@@ -702,6 +702,16 @@
if { ![info exists allowed_attribute($attr_name)] && ![info exists allowed_attribute(*)] } {
return "The attribute '$attr_name' is not allowed for $tagname tags"
}
+
+ if { [string equal [string tolower $attr_name] "src"] } {
+ set url $attr_value
+ set guessed_mime_type [ns_guesstype $url]
+
+ if { ![string equal -length 5 $guessed_mime_type "image"] } {
+ return "The attribute '$attr_name' is only allowed if its value has a valid image mime type (image/*).
+ You have a '$attr_name' attribute in there with the value '$attr_value', which has a '$guessed_mime_type' mime type."
+ }
+ }
if { ![string equal [string tolower $attr_name] "style"] } {
if { [regexp {^\s*([^\s:]+):} $attr_value match protocol] } {
Index: acs-templating/tcl/data-procs.tcl
===================================================================
RCS file: /cvsroot/openacs-4/packages/acs-templating/tcl/data-procs.tcl,v
retrieving revision 1.12
diff -u -r1.12 data-procs.tcl
--- acs-templating/tcl/data-procs.tcl 11 Dec 2003 21:39:57 -0000 1.12
+++ acs-templating/tcl/data-procs.tcl 11 May 2004 18:15:47 -0000
@@ -110,8 +110,21 @@
ad_proc -public template::data::validate::text { value_ref message_ref } {
- # anything is valid for text
- return 1
+ upvar 2 $message_ref message $value_ref contents
+
+ # HTML *needs* to be security checked but since with the "text" data type
+ # there is no way of knowing whether the user wanted the contents to be
+ # HTML or plain text we'll have to validate plain text as well, although
+ # malicious HTML code couldn't hurt in that case.
+ # This means, example XSS code will unnecessarily be refused ...
+
+ set check_result [ad_html_security_check $contents]
+ if { ![empty_string_p $check_result] } {
+ set message $check_result
+ return 0
+ }
+
+ return 1
}
ad_proc -public template::data::validate::string { value_ref message_ref } {
Comments are welcome.
Should this be TIP'ed?
for whoever may be interested, in order to check whether the url is internal or not (and return an error if the url is external) I have appended Ola's addition to /packages/acs-tcl/tcl/text-html-procs.tcl with this:
else {
if {![string match /* $url]} {
return "The attribute '$attr_name' is only allowed if
the source is internal. You have a '$attr_name' attribute
in there with the source '$url'"
}
}
