Forum OpenACS Development: What a forking mess: noquote hell.

Collapse
Posted by Tom Jackson on

Boy, talk about missing out on something. Last night I loaded one of my own packages into OpenACS5.0.0b1a to see if it worked. Mostly good, seems I get an pg_atoi error for some reason.

Then I ventured into the package and tried adding stuff. All was fine until I started to notice results of the new noquote feature. Before I knew it, I had modified half a dozen pages, which I admit is easy to do. Then I started thinking of the consequences: I'm forking my code. I now need a 4.6.3 version and a 5.x version. If I enhance the package, I have to do so in two separate places, every time.

Then I thought: it couldn't be that bad right? So I read the porting guide to find that essentially every page has to be edited since all property tags need a noquote, and all include tags need to be looked at as well, in case they don't pass by reference.

So maybe I should just ask: is it possible to turn off noquote on a per package basis? Or did someone yell fork when I wasn't looking, or maybe in a language I didn't understand?

I'm not worried about my smallish 50 file packages, just the bigger 500 page one. Whatever problems noquote fixed, I doubt it was very big, or are there documented cases where disaster struck?

Collapse
Posted by Jeff Davis on
there is a place where you could turn it off (there was an exception for dotlrn for a while), although better might be to change 4.6.3 so ;noquote would be a noop. To be honest I don't think anyone has considered trying to maintain a single package to work on 4.6.3 and 5.0.

As for the security holes it fixes, I think XSS is reasonably important to guard against and noquote is the right way to fix it so I don't think we have made a mistake in making these changes.

Collapse
Posted by Tom Jackson on

It was my understanding that XSS was a large measure of social engineering, and a highly targeted exploit. Has anyone actually been a victum of XSS through OpenACS? But the problem with XSS is that it is so easy to dream up a way around any barriers. For instance on OpenACS.org, we know, or could guess who has admin rights for the site. If we try to post an exploit, the system will record the ip address where the data originated. Instead, a hacker could send an email, something like spam, which might be opened. If an image link src contained the request the hacker wanted to processed, an email application configured to load images might run the request with the user's privileges. I don't think XSS can be stopped unless you control the behavior of the users of your site (including the software they use and its configuration).

I'm interested in how you would disable noquote, but making it a noop in 4.6.3 might be good too.

Collapse
Posted by Dirk Gomez on
Tom, take a look here https://openacs.org/forums/message-view?message_id=154003

Should give you the idea.

As to has anyone been exploited through XSS? I wouldn't know, but do we want to wait?

It is the most frequently posted bug on any webappsecurity (or whatever) mailing list - has even surpassed SQL injection it seems - and doesn't require social engineering. It requires you to find a place where you can smuggle in mailicious HTML/JavaScript then you are set. Search around for XSS and see how the exploit works.

Collapse
Posted by Tom Jackson on

Dirk you may be interested to know that the X stands for 'cross'. That means the bad javascript or whatever could reside 'anywhere', like on another site or in an email. You just posted a 'link' above (which is a link to this page, so if you have another one going somewhere else, please post it), but what if, on that linked page, which is off site, I have constructed an image tag which does some work back here at OpenACS.org? I'm not sure how the patch affects this problem, which still exists, and is more likely.

Also, from what I can tell, we still spit out html entered by users, otherwise forums woundn't work very well. How is that safe? Because we filter _on the way in_. Just add 'nohtml' to variables that shouldn't contain html, where the database wouldn't otherwise barf on that type of input.

I've read, several times, the reasoning for using noquote. Mostly it was a rush job to get some new code out to a single client. There was no history, no legacy code to deal with. There were no other users to consider. But several other things were stated in the discussion: unquoted user strings work 95% of the time, and using noquote as a fix, breaks existing code. Instead of tracking down the 5% of the remaining problem, a quick fix was introduced, that would produce more visible errors.

Of course the main problem is that the noquote fix introduces its own version of a hard to find problem: over-quoting.

This whole problem is caused by sloppy development. Creating a list of how to avoid this under/over quote issue would be a good start. But XSS isn't going away any time soon, until a new HTML standard is drafted, and until all current browsers are removed from use.

Here's a simple example. I'm running 5.0.0b1a at 127.0.0.1:8000. In my home directory I create an html file which contains, among other things:


<img src="http://127.0.0.1:8000/admin/">

I haven't logged in in a while, so my session is expired on that server. When I load file:///home/tom/test.html, the access log gets two new entries:

127.0.0.1 - - [24/Dec/2003:09:56:03 -0800] "GET /admin/ HTTP/1.1" 302 364 "" "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.1) Gecko/20020830"
127.0.0.1 - - [24/Dec/2003:09:56:11 -0800] "GET /register/?return%5furl=http%3a%2f%2f127%2e0%2e0%2e1%3a8000%2fadmin%2f HTTP/1.1" 200 5964 "" "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.1) Gecko/20020830"

No javascript was used. The html page wasn't on the website, yet /admin/ was tried, and Mozilla even followed a redirect to login. Nice.

Now I login to the website using my browser, and reload the file, in another browser window here is the result:

127.0.0.1 - - [24/Dec/2003:10:02:08 -0800] "GET /admin/ HTTP/1.1" 200 3185 "" "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.1) Gecko/20020830"

Huh? I'm using the noquote patch, and yet I just fell victim to XSS. This is why I say it is social engineering. All you have to do is figure out how to get your victim to follow a link, or maybe just read your email, if they allow their email client to load images.

Collapse
Posted by Tom Jackson on

Jeff, I certainly don't want to maintain two packages, one for 4.6.3 and one for 5.x, but except for the noquote thing, all the packages I have written should work unchanged on both. I did see an pg_atoi error, which is probably a query that needs updating, but that is it. With every adp page changed due to property tag quoting, I would be forced to maintain two separate packages as long as I use 4.6.3. But maybe the suggestion is to completely abandon 4.6.3, mostly bug fixes and slight enhacements, and force users to upgrade to 5.0. But for custom vendor code, the hope would be you could copy the package over to 5.0 and have it work. I guess that is hoping to much.

Collapse
Posted by Don Baccus on
Well ... the time to post arguments against something like the adoption of the noquote solution is when we're arguing over whether or not to undertake it - not months later.  Sorry, you're too late, I can't see us going back at this point so I don't see much point in rehasing the issue.  Of course if you can convince enough OCT members of the wisdom of backing it out, we'll do it, but frankly I don't think you'll succeed at this point.  The TIP process at least ensures in the future we'll have very visible decision points for major changes so everyone who cares will know to make their opinion public.  But it wasn't in place when noquote was adopted so we made the decision more informally ... but openly, following discussion in the forums.

Why not just backport the noquote changes into the 4.6.3 templating system and have it be a no-op for all but your package?  I don't think this would be very difficult ...

Collapse
Posted by Dirk Gomez on
What you want is sloppy and risky, only to ensure backwards compatibility you
want OpenACS to be insecure by default.

Please read http://www.cert.org/advisories/CA-2000-02.html

The first fix for "Web Page Developers and Web Site Administrators" is "
Web Page Developers Should Recode Dynamically Generated Pages to Validate
Output" and that is exactly what we did: by default html tags are now quoted
- this is the security aspect.

The more noquote tags in your adp scripts, the less security, you (may have)
captured that quite right.

I don't understand your example though: you created an html file outside of
OpenACS and loaded it and are now wondering about the successfull page
request? This is beyond the scope of the noquote patch. What you describe is
cross-site request forgery and for GET requests OpenACS isn't secured against
that, for post requests it is if you use the form builder.

Have a look here to see what changed in OpenACS code:

http://cvs.openacs.org/cvs/openacs-4/packages/acs-templating/tcl/parse-procs.tcl?r1=1.14&r2=1.15

Now if you adhere to your own high coding standards you can use the noquote
sed oneliners and maybe be getting away with only handful of manual changes.

Collapse
Posted by Dirk Gomez on
Forget about get and post requests. Use formbuilder and you are secured against cross-site request forgeries.

Where is the security risk in the particular request you posted Tom? Only if register were a (potentially) malicious request you had a security issue.

It is true that we need to add session tokens to a bunch of pages. See here for a post https://openacs.org/forums/message-view?message_id=32884

Collapse
Posted by Tom Jackson on

So if formbuilder protects against XSS, what extra problem does the noquote thing handle?

My example above just shows that you don't need scripting, you just need an image tag, did I really need to delete data or modify my website to prove that to you? Point is, the request is made with my privileges.

And you are absolutely right: changes can be made to form processing to handle the problem. So what was the noquote for again, especially since we are validating input?

Collapse
Posted by Jeff Davis on
You don't want to say "nohtml" for plain text fields since that means you can't use < in them. Take the forum subject field for example, you should be able to have a tag in the subject, like "<blink> should be on the list of allowed tags", but not have to worry that you will have a blinking page as a consequence. Of course you could ad_quotehtml all the plain text fields and not have noquote but given how few places this was done correctly in 4.6.3 and before I am confident that OpenACS with noquote has far fewer quoting bugs than previous versions.

Also, something like a user contribution page which shows data from lots of different packages (some of which might be missing validation like you are talking about) are much safer with automatic html quoting.

It certainly was not hard to move most of the toolkit to noquote and where it's wrong, the fix is pretty obvious so I think the lack of backward compatibility is not a big deal at all (and if you submit a patch versus 4.6.3 to make ;noquote a noop we would be happy to apply it).

Collapse
Posted by Tom Jackson on

It sounds like the noop is a good idea, I'll try it out and post a TIP, or bug/feature. I'm usually less than clear when I write. To summarize, noquote isn't a bad idea, there are other good reasons to use it, mentioned in the original documents referenced above. Upgrading my packages is a good idea, and I had already done one before I started to think, not about doing the upgrade, but about maintaining sites that can't move to 5.0 right away, but might want upgrades to my packages.

Collapse
Posted by Dirk Gomez on
Right we do both now. Filtering input has been there forever and filtering output has been added now.
Collapse
Posted by Tom Jackson on

Dirk, from the CERT advisory:

 
* Web servers that dynamically generate pages based on unvalidated input 

The problem is unvalidated _input_. And Web Browsers.

I don't expect any changes from OpenACS, I think I started the thread with "Boy, talk about missing out on something..."

Thanks for the link to the changes. Seems easy enough to work with. My main concern is with the need to maintain two lines of development, but backporting looks like it might be a good choice.

Collapse
Posted by Dave Bauer on
To address Tom's concer on verifying input. One thing that seems to have been accepted as common practice is to construct URLs like so:

foo-delete?foo_id=$foo_id

instead of

foo-delete?[export_vars -sign foo_id]

Adding -sign to calls to export_vars, and adding :verify to ad_page_contract variables will allow for verification of input on pages that accept URL variables that are not forms. To make ad_page_contract work with signed variables add :verify to the query variable definition. I think a global -verify {} section added to ad_page_contract to verify all variables would be a good addition to provide a shortcut for this. Another option would be to create an acs kernel paramter to set all ad_page_contract variables to be verified by default (off by default for backwards compatibility.)

Ad_form already includes a form signature. I will have to look into the template::form calls to see if they also need to be called with a -sign parameter.

The biggest job to clean this up is fixing all the places in the toolkit where the url variables are explictly defined as in my first example.

Collapse
Posted by Dave Bauer on
To be clear only URLs that actually makea change on the server should be signed unless you have a very speicific applicaiton.

URLs with variables that read data shouldn't need to be signed.