CSRF Exploit fix

Reviews
Diff

- Download Diff

Information
Submitter:	Bogdan-Cristian Tătăroiu
Repository:	infoarena
Branch:
Bugs:
Depends On:
Reviewers
Groups:	hackers
People:

Description

Revisions 910, part of 912, 915, 916, 927, 928, 932, 933

Testing Done

Change Summary:

- Fixed bug which made blog page actions not work.
- Differentiate between nontextblock controllers (such as account) and special textblock controllers (such as blog). In the previous patch, images could not be attached from blog pages.

I think this should be the final change :)

-- Bogdan

Description:

~		Revisions 910, part of 912, 915, 916, 927, 928 :-"
	~	Revisions 910, part of 912, 915, 916, 927, 928, 932, 933

Diff:

Revision 2 (+166 -104)

Show changes

	/trunk/common/string.php
	/trunk/www/config.php
	/trunk/www/index.php
	/trunk/www/controllers/job_detail.php
	/trunk/www/controllers/monitor.php
	/trunk/www/controllers/projector.php
	/trunk/www/controllers/resetpass.php
	/trunk/www/wiki/MyTextile.php

I've been thinking about this for a while. My musings go beyond CSRF exploits, notably to XSS exploits also. I also took the time to rant about browsers. Please bear with me.

I think we can split the discussion in three parts:

1) Never change server state on GET requests. (A more fancy way of saying it, courtesy of HTTP/1.1 RFC2616 Section 9, is "GET requests should be idempotent.")
This is standard HTTP practice but we don't always obey. For example, we should never logout or delete wiki pages (!!!) by simply following a link. All such controllers must be guarded against GET requests.

This small fix will get rid of various nasty exploits, such as embedding an image with source '/logout' or writing a dishonest link such as
'Naked pictures of me!':problema/adunare?action=attach-del&file=...

As you can see, the problem is not that we allow people to create links to "unsafe" actions. Such actions should not exist in the first place.

2) Don't host (i.e., serve) active content... or at least try not to. It may surprise you, but this is so hopeless that it breaks my heart and makes me cry.

"Active" content basically means whatever is not 100% completey static and safe. Famous examples: shockwave flash, javascript, vbscript, and java applets. However, PDF files were also rather unsafe until Adobe fixed their plugin.

In general, there are many browser plugins out there that run active content, and the usual security policy, if any, is that the container has access to the website that it was downloaded from. One way to exploit this: create your own website, say evil.com, embed a SWF file hosted on infoarena.ro as wiki page attachment. Then, lure infoarena admins to visit evil.com. Your SWF can start sending malicious POST requests to infoarena.ro using admin privileges. Bummer!

Of course, you only have to write proper HTTP headers (Content-type: application/octet-stream; Content-disposition: attachment; force-download, etc.)
to solve this. That's great! Except browsers are stupid and broken. Some IEs will ignore your force-download directives for <EMBED> or <OBJECT> tags.

In the end, your only hope to avoid XSS exploits is to not host active content at all. One way to do this is to only allow certain MIME types when people upload attachments. That's great, and it kind of works! However, you've just entered the land of MIME magic idiosyncrasies! It turns out lots of image formats allow you to embed arbitrary blobs. For example, you can embed a full fledged java applet (JAR) inside a GIF image. Your Java run-time is smart enough to ignore gazillions of petabytes at the beginning of your file until it finds a JAR header. Then it starts reading the applet. Yey! Ask for "GIFAR attack" from your favourite search engine. The same goes for some IE versions. Try serving a "Content-type: text/plain" file that starts with "<html>". IE really likes chewing HTML, and will parse whatever walks and quacks like HTML, regardless of your MIME type indications.

Should I mention charset encoding attacks? I'll stop short of it for the moment.

It seems rather to completely avoid CSRF/XSS attacks. However, we could surely use some decent protection along the lines of my rant. I think content-type restrictions and content-disposition headers would do just fine. Additionally, we could ban attachment downloads with HTTP Referer outside infoarena domains; I think we already do that.

All else failing, in backup we trust.

3) Write a decent url-mapper, maybe inspired by RoR's Routes module (http://wiki.rubyonrails.org/rails/pages/Routes). PHP has some kind of callback mechanism, and anonymous functions, so we could use those. The URL mapper should be unit-testable and should allow one to deny requests if they don't match the expected HTTP method (GET/POST).

I only write this because you tried to move some stuff from index.php. I figured you are interested in a better design than the current one. :)

The following line 3 annotations are rather useless. It's clear we need a completely different fix for this. :)

Bogdan-Cristian Tătăroiu Oct. 24, 2008, 4:18 a.m.

Regarding point 2: I'm not sure if you looked over http://reviewboard.infoarena.ro/r/11/. That patch also adds a safe_only option to attachment download which only delivers the attachment to the browser if it's mime type is safe (jpeg, gif or plain text for example). So now in textile, the image tag only points to a safe attachment download. That coupled with proper html filtering (http://reviewboard.infoarena.ro/r/10/) should not allow users to do anything bad :). Html filtering doesn't work at all on live and it didn't work since revision 696 :). I know it wasn't disabled intentionally, the textile _tokenize function has a pointless regexp for nested html tags (something like <txt <txt ... /> />) that match all html tags and separtes them from the rest of the text. That regexp didn't actually work in the original source code anyway because of a badly placed comment :).

From what I searched on the gifar thing a while ago I saw that for a java applet to be actually run it needed to be called in a <applet> tag, since the gifar is a gif with a jar(zip) at the end. I don't think it will run by default, but I didn't look too deep into this issue and I could be wrong :).

With these three patches combined I don't really see what a bad user could do.

Cristian George Strat Oct. 24, 2008, 5:08 a.m.

> With these three patches combined I don't really see what a bad user could do.

I think there are a few bad things one could do. For example, how about this piece of valid Textile code?

'Free beer!':http://infoarena.ro/sandbox/utf8?action=delete

...and then luring admins into clicking the link?


Regarding point (2) and the other changesets, here's a safe text/plain attachment:

http://infoarena.ro/sandbox?action=download&file=x.txt

FF3 and Opera9.5 act as expected, they display a harmless plain text. However, IE6 chews it as HTML and runs the Javascript inside it. :(   Try it.

Bogdan-Cristian Tătăroiu Oct. 24, 2008, 6:47 a.m.

Mkay, guess we'll remove "text/plain" from the safe list (at least for IE users)...

Regarding point 1: From what I could see, the only actions that modify stuff in GET requests are textblock "delete", "restore" and "attach-del". I'll implement a simple confirm page for them and it should be ok. I don't really know what to do with "logout".

Regarding point 3: I moved the list of directly mapping controllers in www/config.php (can you suggest a better place? :) ) since I needed a superset of it in www/wiki/MyTextile.php. I rewritten a bit of index.php to make it more organized. It seemed cleaner this way :) If we make the changes mentioned at point 1, then IA_CONTROLLERS probably won't be necessary and could be reverted back to index.php. I'll look into that Routes module, but a new url-mapper is not really important for this patch.

Anything else that should be changed?

Cristian George Strat Oct. 24, 2008, 8:19 a.m.

Yup, remove text/plain completely.

I'm not sure what else needs POST guard, maybe attachment-rename? Oh, and I don't think it's necessary to implement confirmation pages. Actually,    confirmation pages would suck   , as far as usability is concerned. I think it's easier to just issue POST requests using Javascript. You can do something like this: http://mentaljetsam.wordpress.com/2008/06/02/using-javascript-to-post-data-between-pages/
As you can see, you can use the same technique for /logout also.

Regarding IA_CONTROLLERS, I'd rather have it reverted, especially since it won't be necessary. I agree with you it's beyond the scope of this patch to implement a nice URL mapper.


Don't worry if you're not comfortable with Javascript. I can help with that, and I can make sure it's portable across browsers.

/trunk/common/string.php (Diff revision 2)

Evil space. :)

/trunk/www/config.php (Diff revision 2)

I don't think these belong here, in www/config.php.

/trunk/www/wiki/MyTextile.php (Diff revision 2)

Evil tab.  :)

Change Summary:

Reverted changes to index.php, moved IA_CONTROLLERS back to index.php. Remove IA_NONTEXTBLOCK_CONTROLLERS check in MyTextile.php

-- Bogdan

Diff:

Revision 3 (+28 -8)

Show changes

	/trunk/www/controllers/job_detail.php
	/trunk/www/controllers/monitor.php
	/trunk/www/controllers/projector.php
	/trunk/www/controllers/resetpass.php
	/trunk/www/wiki/MyTextile.php

Change Summary:

Ahem, put common/string.php back in diff :-"

Diff:

Revision 4 (+41 -8)

Show changes

	/trunk/common/string.php
	/trunk/www/controllers/job_detail.php
	/trunk/www/controllers/monitor.php
	/trunk/www/controllers/projector.php
	/trunk/www/controllers/resetpass.php
	/trunk/www/wiki/MyTextile.php

/trunk/common/string.php (Diff revision 4)

I suspect starts_with("john", 0) evaluates true, courtesy of PHP type coercion.

This is bad behaviour since it's hard to make sure you're always passing strings, rather than integers or bools.

Either coerce rterm to string or log fatal error message.

/trunk/common/string.php (Diff revision 4)

Same as above.

ends_with("john", 0) probably evaluates true.

/trunk/www/wiki/MyTextile.php (Diff revision 4)

This comment line seems out of place now. Maybe it should precede the if (!external-image) code path below.

/trunk/www/wiki/MyTextile.php (Diff revision 4)

just plot/

/trunk/www/wiki/MyTextile.php (Diff revision 4)

This code path is about rewriting `src` such that it points to the image resampling controller. It seems a bit misleading to have it depend on !$allow. Either $allow is a bad name or ignore $allow completely when branching.

This is more about coding style, not correctness.

/trunk/www/wiki/MyTextile.php (Diff revision 4)

.   :)

Diff:

Revision 5 (+41 -7)

Show changes

	/trunk/common/string.php
	/trunk/www/controllers/job_detail.php
	/trunk/www/controllers/monitor.php
	/trunk/www/controllers/projector.php
	/trunk/www/controllers/resetpass.php
	/trunk/www/wiki/MyTextile.php

Ship it!

Great!

You have a pending review.

Review Board 2.5.13.1

Screenshots

Files

Change Summary:

Description:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Diff: