file_open is not safe and performs too many useless syscalls

Hi Florent,

Would you elaborate on the security implications you are referring to (given that these calls to open() are coming from zipfile.is_zipfile()), in order to evaluate the importance of this bug report?

I can see a lot of room for optimization in this behavior too, indeed.

Thanks,

I am not a security expert, but it looks like a Directory traversal vulnerability.
http://en.wikipedia.org/wiki/Directory_traversal_attack

You could image a specially crafted "/.zip" or "/home.zip" file which includes a python module which matches an openerp file name, and contain malicious code.

To be sincere, I am not really concerned about the security implications.
Since we run regression tests very often, we try to track down some performance issues which slow down our tests.

I will probably propose a patch to improve the file_open utility a little bit.

On 02/07/2012 08:01 PM, Florent wrote:
> You could image a specially crafted "/.zip" or "/home.zip" file which
> includes a python module which matches an openerp file name, and contain
> malicious code.

Yes that is possible indeed, but it seems it would require some very exotic
conditions to be exploitable - or a situation where the attacker has full
control on the disk space from which OpenERP is running (in which case there
are much easier methods to attack the system, obviously).

> To be sincere, I am not really concerned about the security implications.

Exactly, I'm also much more concerned by the number of useless I/O syscalls
that are triggered by this logic. This stems from 2 things:

1. zipped modules currently have a higher precedence than extracted versions,
which I think is a legacy behavior - and probably a bad choice
2. the code trying to locate the zipped modules escapes the "chroot" of the
addons path, which makes no sense - modules are not supposed to be loaded
outside of it

> I will probably propose a patch to improve the file_open utility a
> little bit.

Thanks. As you seem to be working on the second issue, I will probably push a
fix for the first issue in parallel, so be careful because this could cause a
number of conflicts in your own work.
I think the behavior must be changed to give precedence to extracted/directory
versions of modules. Firstly, this the default case and also the most common
case, so it is a sane choice for the highest priority (performance-wise).
Secondly, keeping a zipped module next to an extracted module with the same
name is not a documented/supported feature. Thirdly, even if you were relying
on this features, having the extracted version take precedence is more
practical (you could simply extract the zip and try a quick fix in the
extracted version - much harder the other way around)

Well the reason for zip before unzipped might be because OpenERP was proposing to "Import module" as a .ZIP file in 4.2 and 5.0.

And in this case the assumption is that the imported ZIP module should take precedence on the installed (not zipped) module.
However I agree that it is deprecated now, and the behaviour you propose looks more sane.

I did not know you were already working on a patch.
The merge proposal covers the point 2 and also the other bug 928507.
I refactored the utility while preserving as much compatibility as possible (and keeping an eye on Win32, even if I don't run it).

It is very easy to adapt it to cover point 1 too (loading from directory first, from zip second).

On 02/08/2012 06:51 PM, Florent wrote:
> Well the reason for zip before unzipped might be because OpenERP was
> proposing to "Import module" as a .ZIP file in 4.2 and 5.0.
>
> And in this case the assumption is that the imported ZIP module should take
> precedence on the installed (not zipped) module.
> However I agree that it is deprecated now, and the behaviour you propose
> looks more sane.

Thank your for the feedback. It also seems to me that even with the zip upload
case supported, allowing to upload a zip file with the same name as an existing
module is going to be confusing.. whichever precedence we choose may not be the
one the user expects (e.g importing by mistake a zip with the same name as an
existing module)

> I did not know you were already working on a patch.
> The merge proposal covers the point 2 and also the other bug 928507.
>
> I refactored the utility while preserving as much compatibility as possible
> (and keeping an eye on Win32, even if I don't run it).

Thanks, that's very much appreciated!

> It is very easy to adapt it to cover point 1 too (loading from directory
> first, from zip second).

Excellent, thanks for merging my changes in your branch!
I have a few remaining things to check and then we can do the final merge of
your work: there are a few pathological cases during module installation where
we explicitly call tools.file_open on an ambiguous path, still causing a great
number of useless zip-related i/o syscalls before eventually finding the
regular file somewhere else.

This bug has a patch and a merge-proposal since Feb 2012.
The merge-proposal is reviewed and approved by Olivier (thanks).

What else?

Is there any reason not to merge it?

Florent's patch was merged in trunk (7.0) at revision 4386 revision-id: <email address hidden>

Thanks for the excellent contribution!

PS: Don't expect dramatic performance improvement, the syscalls were only consuming a negligible amount of time because the filesystem cache will hold the negative dentries in memory. Therefore each directory visit did not result in an expensive filesystem call, but in most cases a direct hit in the FS cache.
PPS: There was nothing holding back the actual merge, except resource contention in our R&D department. Sorry for the delay!

thank you!

At least it fixed a bug with pathinfo=True and it moved the FS import before the zip import.
And the most important is that it improves the overall code quality by small steps :-)