This is a reply to Radeks blog The currently discovered bug in VirtueMart is ...
All quotations in this article are from the Radek Suski's blog The currently discovered bug in VirtueMart is ...
Thank you Radek for this sensible blog post. From my point of view, there are a lot of misconceptions, that means your assumptions or what you have heard is not right. I can understand that, because I wrote our news from my memory while I was on holidays.
The first misconception of the whole issue is this:
Despite the attempts of VM developers to pin this vulnerability to Joomla!
I wrote in our news and it should be also fixed in the JUser to prevent misuse of it
. It exactly says that we used it wrong. So we admit our error here. So anyone who blames us for blaming Joomla, did not read exactly.
I am a bit astonished and disappointed about particular medias reaction and also about VirtueMart efforts to declare that this is Joomla!'s issue.
We are also very disappointed. I was informed by the Sucuri team 2 days before my holiday about the security issue. As mentioned everywhere, we did the fix in record time. I also tried to get in informal contact with some Joomla LT members to talk about the issue and how to proceed. Actually my question is not if it is the fault of A or B. My intention was just to make it secure for everyone as best as possible. So if we use a car analogy, Joomla works imho like a car without central locking. Yes, we forget to lock all our doors sometimes, but actually it is so easy to add a central locking. Please read my news again and you will see that I do not blame Joomla. I just explained why we did not directly proceed with the Full Disclosure. So, please, anyone who is thinking that I blamed Joomla, take a step back and take a deep breath and understand that we did not.
The other thing is that Max mistakenly declares that a similar issue was fixed in Joomla! 2.5.16.
I meant it completely different. The problem was similar on meta level. We had a security issue a year ago, which was also due to the use of a Joomla function. Our filter was added to Joomla to prevent that others might do the same mistake again, and this was in Joomla 2.5.16 (a year ago btw). So anyone updating to the new Joomla core had the fix directly through Joomla. We had a similar discussion if it makes sense to teach every developer or just to add the protection within the framework. A framework should help developers to write secure software.
First at all, the JUser class is not a model. I mistakenly called it also a model in few of my comments to simplify the understanding of the issue. The class JUser is however a database table interface.
I tried to explain that in Twitter. I think we agree on that it is not a PHP interface, else the class would have the interface keyword. On the other hand, what is a model?
The model directly manages the application's data, logic and rules.
Wikipedia - Model–view–controller
So actually a database table interface is a model. It is one of the most simplistic models you can have. It is like saying a chair is not a furniture. This is also the reason you called it yourself a model, because it IS used as model. In Joomla we have two different kinds of models. There are the low level API table interfaces, the JTable and the higher models, which for example combine different tables. When I started with Joomla, I did not find any abstract user model within the library, except JUser. The other models belong to components of Joomla. So if it is just a low level database interface, why is it not a JTable then?
Furthermore if you dig in deeper how models are used, then you will also read that the business logic is more and more moved into the model. The idea is exactly to get models which can be easier reused and which are easy to handle. Rights management is part of the business logic (The german wiki is different than the english one Model–view–controller German version).
The biggest misunderstanding is actually the rumor that we do not filter the array.
Of course we filter any data, which is in our form. But I exactly learned from JTable that it is not needed to white list the data, because a JTable binds only the data which is meant to be bind. This is quite handy, you can have a post data of different tables in one form and easily store it within a chain.
To underline, the variable isRoot is a cached switch for if this user has root access rights. It should be never possible to override it. If JUser would be concepted as real low level table interface, it would be a JTable and isRoot would be called _isRoot and there wouldnt be any problem.
It is an known PHP habit to use underscores for internal variables of a class. Joomla is considering this in the general getProperties function:
public function getProperties($public = true)
{
$vars = get_object_vars($this);
if ($public)
{
foreach ($vars as $key => $value)
{
if ('_' == substr($key, 0, 1))
{
unset($vars[$key]);
}
}
}
return $vars;
}
This function is called in the JTable bind function like that:
foreach ($this->getProperties() as $k => $v)
{
// Only process fields not in the ignore array.
if (!in_array($k, $ignore))
{
if (isset($src[$k]))
{
$this->$k = $src[$k];
}
}
}
So JTable only binds variables which are without underscore and not on the ignore list.
But JUser just calls in bind
public function setProperties($properties)
{
if (is_array($properties) || is_object($properties))
{
foreach ((array) $properties as $k => $v)
{
// Use the set function which might be overridden.
$this->set($k, $v);
}
return true;
}
return false;
}
So JUser works different than JTable. I don't see a reason todo so. If you work with a framework, you assume some consistency, else it makes it hard to use it. For me it looks like an open door which must be closed manually by any extension developer.
In VirtueMart we follow the philosophy that we try our best to protect our software against accidentally opened security leaks. For our customers it is important to have a secure software and they are not interested in finger pointing. So if we can prevent a vulnerability by the core, we do that. We know we are all humans and errors happen, so it makes sense to reduce the possibility todo errors. Our goal is to provide an API, which is secure and you can only open doors if you do not use the API. We just assumed for Joomla a similar handling.
Also our last security leak was due the use of a Joomla function http://virtuemart.net/news/latest-news/446-important-security-release-vm-team-at-joomladay-germany.
Finally, I am not going to waste my time and check the current version of VM with the bug fix,
Well, not having the time, but spreading bad words about it. Not really fair, sorry.
but the fix published in the news on the VM website doesn't fix the vulnerability. It just handles a single case - creating a Super User.
The fix on the page is a quickndirty for the people who heavily modified their user model. Actually the model even works with very old versions and so most people will just exchange the class.
If this is the whole fix then VM is actually still vulnerable. Not to mention that the part about the fix in the article on the VM site has been supplemented at least once, but there is no information about the fact that it has been updated. That way, someone who applied the first (partial) fix is not even aware that the fix has been changed in the meantime.
Yes, I admit, that was really not good. I should have provided directly the real fix, which we actually used.
First an example how we filter the name (as example)
if(empty ($data['name'])){
$name = $user->get('name');
if(!empty($name)){
$data['name'] = $name;
}
} else {
$data['name'] = JRequest::getString('name', '', 'post', 'name');
}
$data['name'] = str_replace(array('\'','"',',','%','*','/','\\','?','^','`','{','}','|','~'),array(''),$data['name']);
We do this for any used variable of course adjusted to the kind (email has for example another filter). So of course we filter all the variables. Do you really think no one would have noticed that?
Our fix now works like this. After we filtered the input data, we just use:
if(!$user->authorise('core.admin','com_virtuemart')){
$whiteDataToBind = array();
$whiteDataToBind['name'] = $data['name'];
$whiteDataToBind['username'] = $data['username'];
$whiteDataToBind['email'] = $data['email'];
if(isset($data['password'])) $whiteDataToBind['password'] = $data['password'];
if(isset($data['password2'])) $whiteDataToBind['password2'] = $data['password2'];
} else {
$whiteDataToBind = $data;
}
// Bind Joomla userdata
if (!$user->bind($whiteDataToBind)) {
.....
If there is an error, please let us know. I can also write a suggestion how I would handle the JUser object. You wrote Joomla!'s code is a living organism. Things are changing.
and therefore we anyway going to replace the JUser in our code against a vmUser Object.