Security: Rewrite the extension hook system

elbekko · 2008-07-05 18:43:27

Fair enough. If there's a performant alternative, I'm all up for it

Mediator · 2008-07-05 21:36:57

sesser wrote:

The security difference is that secure applications should not dynamically evaluate any code. By using a hook system that is based on eval() you encourage the usage of eval(). This might lead to the use of eval() within extensions. And this eventually leads to security problems.

If you don't use eval you dicourage the use of it in third party extensions? I hardly see that as being realistic, if an extension creator wants to use eval they will probably use eval whether upstream uses it or not.

sesser wrote:

You are not including an arbitrary file. You are only including files containing hook data. get_hook() can ensure that only SANE filenames are possible.

Basically you are saying that include is safe because you would make sure the filenames are sanitized. However, in an eval based system, the data to be eval should be from a trusted source also, which fluxbb has done a decent job of achieving with their current implementation. Either way you have to run some kind of security check to ensure malicious code is not executed, I fail to see the difference in the two systems if an eval based system is implemented correctly

orlandu63 · 2008-07-05 22:23:59

Includes are cleaner

sirena · 2008-07-05 22:43:05

Vote+1 for stefan's proposal. His experience suggests to me that his idea has to be considered seriously.

It is also great to be getting his input into FluxBB at the development stage, IMHO. Please don't scare him away

MattF · 2008-07-05 23:28:18

Mediator wrote:

If you don't use eval you dicourage the use of it in third party extensions? I hardly see that as being realistic, if an extension creator wants to use eval they will probably use eval whether upstream uses it or not.

That doesn't alter the basic point he was making of security.

Mediator wrote:

sesser wrote:
You are not including an arbitrary file. You are only including files containing hook data. get_hook() can ensure that only SANE filenames are possible.
Basically you are saying that include is safe because you would make sure the filenames are sanitized. However, in an eval based system, the data to be eval should be from a trusted source also, which fluxbb has done a decent job of achieving with their current implementation. Either way you have to run some kind of security check to ensure malicious code is not executed, I fail to see the difference in the two systems if an eval based system is implemented correctly

The final portion there is the relevant one: 'if an eval based system is implemented correctly'. Btw, there is no such thing as a truly trusted source. Anything which isn't hardcoded, (and even then only if you know for a fact you have made no mistakes), cannot be trusted.

It is a fairly moot point provided everything is secure and sanitisation is perfect. However, perfection can never be guaranteed.

Felix · 2008-07-05 23:32:52

Well, out of question we don't want to scare anyone away Especially with his experience.
And I already stated, that I'd like to have the includes instead of eval.

But I also understand the developers. The system is done and it is working.
And now there is a "new" user, who "wants" to change their "whole" extension system.
And tempts them with security audits... (yes, I exxagerated)

Josha Eichorn - Using eval() in PHP
Just throwing another piece into the crowd

Mediator · 2008-07-05 23:33:53

MattF:
Which is pretty much exactly the point I am making, either way has security risks, and each can be as secure as the other provided your implementation is good, which FluxBBs is. Dynamically including files poses many of the same risks using eval does. So security should not be used as a reason to make the change. Performance on the otherhand might be a reason, or a reason not to.

Connor · 2008-07-05 23:43:38

tbh, changing it isn't much of an issue, since its not terribly huge to do, and it doesn't affect the way extensions are written, also, I don't think we were ever completely happen with using eval() for whatever reasons, and the reason we didn't use include was we were worried about the speed issues of it, which may not really matter due to opcode caching.

This is not just a security issue.

sesser · 2008-07-05 23:44:43

Mediator wrote:

If you don't use eval you dicourage the use of it in third party extensions? I hardly see that as being realistic, if an extension creator wants to use eval they will probably use eval whether upstream uses it or not.

The opposite of to not encourage is not to discourage.
Of course you cannot stop any extension writer from shooting himself. However extension writers tend to copy the coding style of upstream. What they don't see they cannot copy.

Mediator wrote:

Either way you have to run some kind of security check to ensure malicious code is not executed, I fail to see the difference in the two systems if an eval based system is implemented correctly

I will only repeat myself but... In case an application uses eval() it dynamically evaluates code. This means arbitrary code can be executed if a) there is an injection into eval(), b) there is a bug that allows overwriting the variable that is evaluated()

This means a wrongly used extract() (or any other kind of variable overwrite vulnerability) in an extension directly leads to arbitrary code execution.

The include method on the other hand is only vulnerable to a file overwrite. An attacker needs to be able to overwrite the content of the cache file to execute code(). This kind of vulnerability is a) more unlikely and b) because of other cache files already exist, FluxBB can already be attacked this way.

This means using the eval() method has a larger attack surface.

Mediator · 2008-07-05 23:56:49

sesser wrote:

The opposite of to not encourage is not to discourage.
Of course you cannot stop any extension writer from shooting himself. However extension writers tend to copy the coding style of upstream. What they don't see they cannot copy.

I agree, but I very much doubt it would ever be an issue for this circumstance.

sesser wrote:

I will only repeat myself but... In case an application uses eval() it dynamically evaluates code. This means arbitrary code can be executed if a) there is an injection into eval(), b) there is a bug that allows overwriting the variable that is evaluated()
This means a wrongly used extract() (or any other kind of variable overwrite vulnerability) in an extension directly leads to arbitrary code execution.
The include method on the other hand is only vulnerable to a file overwrite. An attacker needs to be able to overwrite the content of the cache file to execute code(). This kind of vulnerability is a) more unlikely and b) because of other cache files already exist, FluxBB can already be attacked this way.
This means using the eval() method has a larger attack surface.

You are being highly illogical. The most obviously method of overwriting the dynamically executed code would be someone changes the hook code in the database through sql injection in an extension. SQL injection is atleast 90% more likely to occur in third party extensions than exploitation of a variable overwrite issue. In which case using cached files solved absolutly nothing.

MattF · 2008-07-06 00:11:47

Mediator wrote:

MattF:
Which is pretty much exactly the point I am making, either way has security risks, and each can be as secure as the other provided your implementation is good,

I'll rephrase the point then. If one has a crap security implementation, (hypothetically), which is most secure on its own merit alone?

Mediator · 2008-07-06 00:13:43

How does a hypothetical situation apply to the changes suggested in this post.

Stay on topic please?

MattF · 2008-07-06 00:17:25

Mediator wrote:

How does a hypothetical situation apply to the changes suggested in this post.
Stay on topic please?

The situation applies perfectly *ANYTIME* a security consideration is concerned. Most security considerations are purely hypothetical as they are designed around to make sure they should never achieve actuality.

Now, your answer to my previous question is?

Mediator · 2008-07-06 00:50:08

FluxBB's implementation to the hooks system using eval poses no realistically higher threat than using file includes.

Smartys · 2008-07-06 00:52:12

It poses a *slightly* higher threat, in theory. At least that's how I see it.
The major argument is not security, but is rather memory consumption and speed (when used with an opcode cache).

Mediator · 2008-07-06 01:03:55

Alrighty, well this is my stance its completely up to the developers as to whether or not the changes get implemented. I am all for the changes being made, but I think they should be made for the right reasons, not hyped up security threats which have essentially no chance of ever occuring with the way eval is being used in this situation. The major security risk, exists in both implementations.

MattF · 2008-07-06 01:19:49

Mediator wrote:

I am all for the changes being made, but I think they should be made for the right reasons, not hyped up security threats which have essentially no chance of ever occuring with the way eval is being used in this situation. The major security risk, exists in both implementations.

Security was never actually the be all and end all point. It is merely one consideration, nothing more. I was merely pointing out that your opinion regarding the security aspect was flawed. Anything and everything has risk. That does not mean that everything is equal, however.

Btw, your direct answer dodging abilities would make many a politician proud.

MattF · 2008-07-06 01:44:28

Almost forgot to mention. Granted, my opinion on the subject will count for little, but I am also on the list of people who would prefer an alternative option to the use of eval, if possible.

sesser · 2008-07-06 08:48:17

Mediator wrote:

You are being highly illogical. The most obviously method of overwriting the dynamically executed code would be someone changes the hook code in the database through sql injection in an extension. SQL injection is atleast 90% more likely to occur in third party extensions than exploitation of a variable overwrite issue. In which case using cached files solved absolutly nothing.

Sorry but here you are wrong. The majority of people will run FluxBB in combination with a MySQL server. In MySQL only one query per call is possible. This means unless the SQL injection is in an UPDATE/INSERT statement of the extension table, there is no way to get data into it. It is highly unlikely that an extension modifies the extension table itself. Therefore the existance of an SQL injection is not equal to direct remote code execution.

If you actually use PostgresSQL then you are damned anyway, because current FluxBB contains SQL injection in the core...

MattF · 2008-07-06 14:43:04

sesser wrote:

If you actually use PostgresSQL then you are damned anyway, because current FluxBB contains SQL injection in the core...

Is that just 1.3 related, or 1.2* also?

Jérémie · 2008-07-07 11:44:05

I don't care one way or another, but please remember that most people don't use opcode cache.

orlandu63 · 2008-07-08 23:24:46

Jérémie wrote:

I don't care one way or another, but please remember that most people don't use opcode cache.

Source?

Jérémie · 2008-07-08 23:44:20

Easy: show me 3 low cost but good shared host, hosting at least 50K+ sites, that have a working opcode cache for those sites.

MattF · 2008-07-08 23:47:20

Jérémie wrote:

Easy: show me 3 low cost but good shared host, hosting at least 50K+ sites, that have a working opcode cache for those sites.

You can add admins who refuse to install all the shite that these things need to be compiled to that list too. (At least that's why I gave up on trying to install them). So, by that token, I would assume there are more like I.

orlandu63 · 2008-07-08 23:54:15

Why should I do the work? Show me 5 that don't.

Forums

#26 2008-07-05 18:43:27

Re: Security: Rewrite the extension hook system

#27 2008-07-05 21:36:57

Re: Security: Rewrite the extension hook system

#28 2008-07-05 22:23:59

Re: Security: Rewrite the extension hook system

#29 2008-07-05 22:43:05

Re: Security: Rewrite the extension hook system

#30 2008-07-05 23:28:18

Re: Security: Rewrite the extension hook system

#31 2008-07-05 23:32:52

Re: Security: Rewrite the extension hook system

#32 2008-07-05 23:33:53

Re: Security: Rewrite the extension hook system

#33 2008-07-05 23:43:38

Re: Security: Rewrite the extension hook system

#34 2008-07-05 23:44:43

Re: Security: Rewrite the extension hook system

#35 2008-07-05 23:56:49

Re: Security: Rewrite the extension hook system

#36 2008-07-06 00:11:47

Re: Security: Rewrite the extension hook system

#37 2008-07-06 00:13:43

Re: Security: Rewrite the extension hook system

#38 2008-07-06 00:17:25

Re: Security: Rewrite the extension hook system

#39 2008-07-06 00:50:08

Re: Security: Rewrite the extension hook system

#40 2008-07-06 00:52:12

Re: Security: Rewrite the extension hook system

#41 2008-07-06 01:03:55

Re: Security: Rewrite the extension hook system

#42 2008-07-06 01:19:49

Re: Security: Rewrite the extension hook system

#43 2008-07-06 01:44:28

Re: Security: Rewrite the extension hook system

#44 2008-07-06 08:48:17

Re: Security: Rewrite the extension hook system

#45 2008-07-06 14:43:04

Re: Security: Rewrite the extension hook system

#46 2008-07-07 11:44:05

Re: Security: Rewrite the extension hook system

#47 2008-07-08 23:24:46

Re: Security: Rewrite the extension hook system

#48 2008-07-08 23:44:20

Re: Security: Rewrite the extension hook system

#49 2008-07-08 23:47:20

Re: Security: Rewrite the extension hook system

#50 2008-07-08 23:54:15

Re: Security: Rewrite the extension hook system

Board footer