Please upgrade here. These earlier versions are no longer being updated and have security issues.
HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.
usability and security concerns over extensive glob() calls in Vanilla
codegrunt
New
@Todd @Tim
Howdy. I have been working on a custom theme for Vanilla with the goal to get as much as I can into Smarty templates. Being fairly new to Vanilla I am still coming to grips with the logic flow. This means that it is not always obvious what file is actually being called to create a given page due to the fact that the default themes do not use actual templates for the most part (they use inline PHP code or rely on the core code's view). During this process I was experiencing what I thought was odd caching but upon further investigation turns out to be the result of an (IMHO) unusual design decision by the Vanilla team.
Within Vanilla, theme files (and a lot of other things as well looking at the code) are searched for by using PHP's "glob()" function. For theme files, all of the following are identical and will be read in as a template by Vanilla and parsed either via Smarty or in some cases passed directly to the PHP interpreter:
index.tpl
index.this_is_not_a_template
index.jpg
index.gif
index.foobar
The first problem with this is that it is highly counter-intuitive and unpredictable. In my case, my code editor creates backup files with a "~" suffix and these were being read in by Vanilla in some cases which caused a lot of head scratching and confusion until I discovered what was actually happening.
The second problem is a security one. Essentially, using glob() so wantonly allows malicious code to be embedded within safe looking file extensions. For example, a GIF could be created with PHP code embedded within it but after the GIF header so it appears like a proper image and then stored as "themes/themename/views/discussions/index.gif". This will be parsed through the PHP interpreter and unless you are well versed with the Vanilla codebase, it would be far from obvious where the malicious code was being loaded from.
I have had to do enough codebase cleanups for clients to know that this is a real concern. It is extremely common for hosting accounts that have been compromised to have backdoors setup in non-obvious locations and Vanilla's glob() usage has created a new vector for this.
Looking at the code I am still not clear why glob() was chosen other than perhaps to avoid having to put in explicit filename tests. I would argue that any extra keystrokes saved for the code authors is definitely not worth the potential confusion and security concerns that glob() brings with it.
As an end user, I would definitely like to see all glob() calls removed and filenames limited to something sane for the given situation. If I should be opening a bug report for this, let me know.
Cheers
Howdy. I have been working on a custom theme for Vanilla with the goal to get as much as I can into Smarty templates. Being fairly new to Vanilla I am still coming to grips with the logic flow. This means that it is not always obvious what file is actually being called to create a given page due to the fact that the default themes do not use actual templates for the most part (they use inline PHP code or rely on the core code's view). During this process I was experiencing what I thought was odd caching but upon further investigation turns out to be the result of an (IMHO) unusual design decision by the Vanilla team.
Within Vanilla, theme files (and a lot of other things as well looking at the code) are searched for by using PHP's "glob()" function. For theme files, all of the following are identical and will be read in as a template by Vanilla and parsed either via Smarty or in some cases passed directly to the PHP interpreter:
index.tpl
index.this_is_not_a_template
index.jpg
index.gif
index.foobar
The first problem with this is that it is highly counter-intuitive and unpredictable. In my case, my code editor creates backup files with a "~" suffix and these were being read in by Vanilla in some cases which caused a lot of head scratching and confusion until I discovered what was actually happening.
The second problem is a security one. Essentially, using glob() so wantonly allows malicious code to be embedded within safe looking file extensions. For example, a GIF could be created with PHP code embedded within it but after the GIF header so it appears like a proper image and then stored as "themes/themename/views/discussions/index.gif". This will be parsed through the PHP interpreter and unless you are well versed with the Vanilla codebase, it would be far from obvious where the malicious code was being loaded from.
I have had to do enough codebase cleanups for clients to know that this is a real concern. It is extremely common for hosting accounts that have been compromised to have backdoors setup in non-obvious locations and Vanilla's glob() usage has created a new vector for this.
Looking at the code I am still not clear why glob() was chosen other than perhaps to avoid having to put in explicit filename tests. I would argue that any extra keystrokes saved for the code authors is definitely not worth the potential confusion and security concerns that glob() brings with it.
As an end user, I would definitely like to see all glob() calls removed and filenames limited to something sane for the given situation. If I should be opening a bug report for this, let me know.
Cheers
0
Comments
While I translated locale/definition.php, I encountered many strange results.
Few days after, I noticed that Vanilla also read definition.bak file!
I intended that .bak is old backup but Vanilla didn't care.
I think this is a trap for developers like 'Roach Motel'.
I can look into restricting these includes to php files OR you can submit a patch.