The currently discovered bug in VirtueMart is ...

... exactly this, a bug in VirtueMart. Despite the attempts of VM developers to pin this vulnerability to Joomla!, I can assure you, this issue is solely a VirtueMart bug. Nothing more.

I am a bit astonished and disappointed about particular medias reaction and also about VirtueMart efforts to declare that this is Joomla!'s issue. It's not.

So let's try to clarify the news on the VM website.

The JUser model bind function just loops through the properties of the class and sets data with the same name to the object.

Max Milbers Security release of vm2.6.10 and vm2.9.9b

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. But even if it would be actually a model, the described behaviour still would be correct. In the most common implementation of the Model-View-Controller pattern, the model is usually used to load, and store data. It shouldn't validate user input. This is something that usually has to be done in the controller. Not in the model, not in the view.

Or to be more specific, as Nicholas describes it: "The Controller filters input data and passes it to the Model's state. The Model then reads it's state data, performs sanity checks (validation - not the user input validation), constructs an array of parameters and passes it to JUser. This is how proper MVC works."

But as I wrote before, the JUser class is not a model. It is a simple class to map the database table. I can think of many possibilities (creating a user from CLI for example) that could be broken if the JUser class would validate the user input passed to it. Validating user input in the JUser class, depending on the currently logged in user, would break the functionality of this class.

So if developers use the joomla model without form, they have to filter the data themself, else it is possible to override internal variables of the object.

Max Milbers Security release of vm2.6.10 and vm2.9.9b

Yes, exactly. The developers have to validate the user input. Always.

The binding for normal JTables does not override internal variables as long you follow the habit/convention to name them with a trailing underscore _.

Max Milbers Security release of vm2.6.10 and vm2.9.9b

I don't understand how this matters, as these variables we are talking about here (isRoot,groups) do not have an underscore at the beginning of their names, so even if the JUser class would filter the data, it wouldn't change anything in the behaviour. It still wouldn't be filtered. 

Additionally it is very unclean to use MVC and to have a model, which needs GUI elements to do correct filtering. There exists enough tasks to use a model without any GUI.

Max Milbers Security release of vm2.6.10 and vm2.9.9b

Here is again the 'not-model'. But beside that, the whole statement is false. I assume that Max is talking here about the JForm and its XML definitions files. So why it is wrong? Because those XML files are not, as he call it, a GUI. Yes, these files are being used to define a form which will be then used to draw the HTML form. But these files are also used to validate the user input. This is exactly how is suppose to work. Once again, the XML definition is not a GUI
Also (thanks to Nicholas for the comment): "You do not need the XML form files to perform validation. If you do have them and do tell it to validate data, JForm/JModelForm will validate data. But you are damned if you rely on XML forms for your entire validation."

For a developer just using the joomla API it is like a trap. A model should be secure by itself, without the need of a "View" or "Controller" to be safe. SCNR, but joomla 2.5.16 fixed a security leak in some the JFormFields. So other solutions based on that were also very unsecure for years.

Max Milbers Security release of vm2.6.10 and vm2.9.9b

No it's not a trap. Not by a long shot. I sincerely never saw any other developer passing the entire user input, unfiltered to the JUser (nor any other table interface) class. Most of us simply know how to use it. And frankly, I personally prefer always to filter user input even if I would know that some other Joomla! API class is going to filter it too. Joomla!'s code is a living organism. Things are changing. You can't simply rely on other code 100%. It's better to double-check than not checking it at all.

The other thing is that Max mistakenly declares that a similar issue was fixed in Joomla! 2.5.16. It was actually Joomla! 1.6.x/1.7.x/2.5.0-2.5.2 and the bug was that, like VM, the data were not validated and you could pass the list of authorised groups into the POST data. Since this vulnerability was fixed so many years ago it stands to reason that VirtueMart developers, like any other Joomla! developer, should have been aware of it and check his own code against similar issues. That is what a responsible developer does.

The suggested fix in the joomla user model is very easy. Just unset the sensitive data, if a user is not admin. This should be done in the bind function and in the store function. The advantage lays on the hand. A lot other extensions for joomla become more secure. It is very unlikely that only VM has this problem. People can do a small joomla update and still use their modified extensions.

Max Milbers Security release of vm2.6.10 and vm2.9.9b

The proposed solution is simply wrong. As I wrote before, if the class JUser would validate the data it wouldn't be for example possible to create a user from the Command Line Interface. I can also think about many other implementation where it is necessary to have this data despite the currently logged-in user. Also for example using web services.
This is clear to me, that such a situation (creating a Super User that way) is probably rather rare, but the proposed solution shows simply a lack of understanding for the MVC pattern and, not to mention, the well documented Joomla! API.

I don't even go to comment the part about the disclosure. Such things happen. It is a courtesy and good habit to publish full information about the vulnerability after it has been fixed. But it's not a good way to blame others for your own mistakes.

Finally, I am not going to waste my time and check the current version of VM with the bug fix, 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. 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.


Update [Wed, Sep 17, 2014]

Update: here are some responses to the currently published response at VirtueMart website. I am not going too deep into it because mostly these are subjective statements and I guess every developer has own opinion on this.

I am glad that Max wrote that he is not blaming Joomla! for this bug nor it was his intention. However when someone writes: "and it should be also fixed in the JUser to prevent misuse of it" then basically everyone is going to assume that this was the intention. I wasn't the only one who assumed it.

I never wrote that JUser is a PHP interface but a db table interface. I can see that it may be a bit confusing.

Although indeed some business logic is usually included in the model it's still shouldn't be validating the user input. It would fundamentally change the strategy of using the MVC pattern.

I still do not agree that a db table interface is a model but as I wrote before, it's my opinion. And it's subjective.

If the data passed to the JUser class is, as stated at originally at sucuri.net, basically the $_POST array then the user input is not validated whatsoever.

Despite the opinion if the attribute should be called "isRoot" or "_isRoot" which is, again, a subjective opinion, as a matter of fact it is called "isRoot" and you can see it in the Joomla! API. As such it doesn't matter how it should be called but how it is called. Period.

I am glad also that Max agree with me that it wasn't a good idea to publish a partial bug fix however this is not what I wrote. My problem is that this bug fix has been changed in this article and there is no information about the change. A small update note would be very helpful. As a matter of fact I am getting signals from people being annoyed that they have to monitor the site to check if this fix has been changed again.


Comments powered by Disqus
Located in: Joomla! | Dev
Powered by SobiPro

Joomla! Cycling World Challenge 2015

Community members only. For more information, please have a look at http://jcycle.org/

Distance Participants Goal Prize Start End
38639 56 Most km What? Nothing! Apr 1, 2015 1:00 AM Oct 31, 2015 2:00 AM
# Name Distance
1 Christian Hent 7263
2 Gerald Martin 5427
3 Randy Carey 3248
4 Pisan Chueachatchai 2665
5 Radek Suski 2661
6 Piotr Szarmach 2333
7 Sebastian Boncol 1424
8 Slawek Sikora 1403
9 Stefanie Thielmann 1291
10 Danuta Zarzeczna 1221