Forum OpenACS Q&A: Thanks for ETP 2

Collapse
Posted by Lachlan Myers on
It's great!

I've just moved from 4.6 to 5.1 (I wanted to re-organise stuff so I migrated content for static stuff rather than upgrade) and the whole task was cut and paste simple!

A note for anyone having HTML code problems - you may need to modify the Kernel parameter (under /acs-admin) Antispam -> AllowedAttribute to include your preferred attributes.

There are possible security implications, but its worth it for such as "src" - useful for "img" tags.

I've allowed the following

href src target align valign width height cellspacing border cellpadding vspace hspace alt
Anyone know of security issues with these?
Collapse
2: Re: Thanks for ETP 2 (response to 1)
Posted by Jeff Davis on
If you allow img src, a malicious user could have a link
like <img src="http://example.com/acs-admin/grant-sitewide-admin?user_id=X">
in a forum post or comment, which when someone with sitewide admin visited the post would automatically grant user X admin.

I would say that is a security hole.

Collapse
3: Security issues with HREF? (response to 1)
Posted by Staffan Hansson on
Jeff, I can see how adding SRC to the allowed attributes in the Antispam kernel parameter will open up a security hole, and I won't do that. But I'd really like to allow HREF in the users' postings. How bad would that be? Are there any known security risks involved in allowing HREF that I should take into consideration? It seems the HREF attribute isn't fully trusted, since it isn't allowed by default in the toolkit - and still most sites, including this one, allow links in user postings.
Collapse
Posted by Jeff Davis on
Collapse
5: Re: Thanks for ETP 2 (response to 1)
Posted by Ben Koot on
Playing devils advocate...

From an end user perspective this discussion doesn't make much sense. We allow all kind's of html manipulation trhoughout the system, but on essential pages like the personal workspace, effectively "welcome to OACS", an environment that should allow maxium user friendliness, html formatting is regarded a security risk. I fail to see the rationale.

Why can't we have a simple default setting for all text fields thoughout the toolkit, allowing text, fixed width, html. If it's safe for a weblog posting it should be safe for a bio in the public user profile also.

It looks like we overlooked making the personal workpace a pleasant and easy to use environement (most users are not intersted in technological environments), that each user can configure as he/she likes. This looks a serious flaw in userability. Adding additional links to essential parts of site should also be a default option.

A personal workspace should be an area that is created by users of the system, not the way the developers would like people to use the system!. Simple restrictions like formatting the personal profile don't make sence.

If I use ETP I can create whatever I like, so why not give us the same freedom trhoughout the toolkit. It would also reduce postings in the bugtracker. Please correct me if I am wrong.

Cheers
Ben

Collapse
6: Re: Thanks for ETP 2 (response to 5)
Posted by Jeff Davis on
Ben, you are talking about something entirely different. The issue with the bio being html vs. not is unrelated to this; for the bio it's simply that no one has taken the time to make the bio allow mixed formatting (richtext, html, plain). The technical consideration here is that for every field you want to change to allow mixed formatting like that you need to carry around the "format" field as well.

I think it's important to fix but it's really pretty unrelated to the issue of what html is safe to allow through versus not.

I think good security is not something we should sacrifice for ease of use (at least not by default); look at the reputation phpNuke et al have for security laxness. Much of it springs from a history of these sorts of exploits. Ultimately it's something that, if we want OpenACS to be acceptable for anything other than personal sites, we need to take care not to permit.

Collapse
Posted by Staffan Hansson on
Thanks Jeff, I've decided to take my chances and allow HREF. Malicious HREFs don't seem quite as deceptive as their SRC counterparts - if they don't contain javascript, that is, in which case they can be disguised into looking harmless in the browser. So no javascript allowed.

I've read through the interesting pages you linked to, and think I've gotten the general picture. Now, since the various articles specifically refer to Apache, PHP, and just about everything except the OpenACS environment, I was wondering how vulnerable our particular toolkit is to dirty tricks like XSS (cross site scripting), cookie theft, etc. What are our strengths and weaknesses in this respect? Apart from security being constantly in mind during development, has some überhacker in the community done a comprehensive analysis of the code base specifically looking for security holes? What's the perceived security status?

Collapse
Posted by Tom Jackson on

A lot of issues would go away if you only allowed POSTed data for form submission. Clickable links which allow for drastic action easily allow abuse.

Another issue is the permission context of a user. It is built up permission by permission, and admins can do everything without question. Allowing admins to browse the site, or read their email with their admin account makes it difficult to protect against XSS.

Most if not all these attacks could be eliminated by requiring some type of human acknowledgement of insert/update/delete actions, such as reading an image. It may annoy your users, but they are starting getting used to it. Personally I think it is quite drastic to nullify many of the benefits of the web and community collaboration by disallowing image links, although I think we will live without frame/iframe/object tags.

Collapse
9: Re: Thanks for ETP 2 (response to 1)
Posted by Ola Hansson on
Regarding image links - what if we could verify somehow that the src attribute is really pointing to an image and then allow it through. Is it possible and would it help? The check would only have to be performed if the image is on the server the tag is posted on ...

Oh, well. Just thinking out loud.

Collapse
10: Re: Thanks for ETP 2 (response to 1)
Posted by Andrei Popov on

How about style and class?

Also, in image insertion: are we talking about external images or the ones that are hosted inside the community, within Photo Album or, say, File Storage? Wouldn't it make sense to allow them from a "trusted" source?

Lastly, I suppose it could be possible to fetch an "image" in a neutral/sandbox way, verify that it indeed is an image, then a allow the post -- but that would slow the process down quite a bit. One can just as well post a link to an image instead... But wouldn't that result in the same dangers though?

Collapse
11: Re: Thanks for ETP 2 (response to 10)
Posted by Jeff Davis on
style has the same problem as img in that you could have a background image in the style attr which fetched a page that took some action. The reason you worry about img src is that unless you have a really paranoid admin, images are loaded automatically when the page is loaded, whereas a malicious href still needs to be clicked on. Of course if you are a bad guy you could always just have an innocuous looking href do a redirect to the action page as easily as put the malicious link in line.

class I think would be safe if a malicious user can't control the css.

Tom's right that we should be checking for POST for actions, although I think even better is to provide a signed nonce which would be something like "user_id,session_id,form_name,server_secret" hashed which could be used to validate that the POST came from a form actually served to the given user.

Maybe I will look at adding a "nonce" type for form builder which could be used to do this.

Collapse
12: Re: Thanks for ETP 2 (response to 11)
Posted by Jeff Davis on
Oh, and for anyone interested in the "make someone read an image to do action X" stuff here is a link to the captcha project.
Collapse
13: Re: Thanks for ETP 2 (response to 1)
Posted by Ben Koot on
Understood Dave, Thanks for the explanation. Sorry I bothered.

Cheers
Ben

Collapse
14: Re: Thanks for ETP 2 (response to 1)
Posted by Ola Hansson on
As I hinted above, I wanted to find a way to allow images to be posted without opening up known security holes. This is what I did:

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?

Collapse
15: Re: Thanks for ETP 2 (response to 1)
Posted by Ola Hansson on
Oh, and I noticed that a default install from HEAD allows these tags: * B I P A LI (* meaning all tags are allowed)

Perhaps not that good a default ...

Collapse
16: signed nonce (response to 11)
Posted by Andrew Piskorski on
Jeff, on using a "signed nonce" to verify that form submissions haven't been tampered with, doesn't OpenACS already have facilities to do that sort of thing? I was pretty sure it did...
Collapse
17: Re: signed nonce (response to 16)
Posted by Jeff Davis on
Andrew, it does have facilities for signing things, but there is nothing in place to insure that the signed variable sent back is the signed variable sent to the browser.

In ad_form for example the key is signed but the signature says:

<input type="hidden" 
   name="__key_signature" 
   value="851 0 2D43063DA4AFDC46574E81151ED48C64939CF3AC" />
the 851 is the token_id and 0 is the expire time (does not expire) and the hash is:
ns_sha1 "$value$token_id$expire_time$secret_token"
Now, all you need to do to forge this is to get some other form to sign an integer (any form which lets you enter an integer which is subsequently served back signed would work) since there is nothing there tied to the session or a shared secret or anything like that.

Some ways to fix this would include having a shared secret (a random string in a session cookie for example) or a server side secret which is session specific (pick a token_id for the session but don't send it in the request).

One other thing I realized in looking at this is that as far as I can tell, we don't ever expire tokens which is probably a mistake. We should probably have a token lifetime parameter and sweep expired tokens. We should have

Collapse
18: Proposed fix for "src" (response to 1)
Posted by Lachlan Myers on
Ola,
    Your solution for the "src" attribute looks useful; and better than leaving it wide open. My images are all local, and  reference the file with extention directly, so your solution would be good for me.

There is probably a wider design solution, but its not one I feel qualified to get involved with. In the meantime, I'd be grateful if you can make the patch available, pls?

Collapse
19: Re: Thanks for ETP 2 (response to 1)
Posted by Ola Hansson on
Lachlan,

I posted the entire patch above, so you should be able to "view source" in the browser and copy it to a patch file, "patch-src", for instance.

Then "cd" to /web/yourserver/packages and try to apply the patch with "patch -p0 <patch-src" or something like that (and restart of course).

Currently, external images will get through too but that may not be such a good idea(?) considering the possibility that an abuser redirects such a request to an admin action on your server, as Jeff said above.

I'm sure this patch is by no means waterproof but we could add to the concept as we learn more about this aspect of XSS/CSS ...

Collapse
20: Patch - thanks, but help! (response to 1)
Posted by Lachlan Myers on
Ola,

Thanks - first time I've used patch :/. I tried your suggestion, with result as below.
From my v5.1, I had to get the revision 1.37 of the text procs. I'll do it as a cut and paste, but do you have any suggestions about what I was doing wrong?

$ patch --verbose -p0 <~/patch-src
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|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
--------------------------
Patching file acs-tcl/tcl/text-html-procs.tcl using Plan A...
Hunk #1 FAILED at 702.
1 out of 1 hunk FAILED -- saving rejects to file acs-tcl/tcl/text-html-procs.tcl.rej
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|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
--------------------------
Patching file acs-templating/tcl/data-procs.tcl using Plan A...
Hunk #1 FAILED at 110.
1 out of 1 hunk FAILED -- saving rejects to file acs-templating/tcl/data-procs.tcl.rej
done
Collapse
21: Re: Re: Thanks for ETP 2 (response to 14)
Posted by Pavel Boghita on
I don't know how to create/use a patch, so I have edited my files manually...

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'"
}
}