Edit: initially this issue was opened to discuss a historic security issue that allowed login using a get request that it appeared had not been fixed. I was wrong, at the time it was fixed, and I apologise for my mistake.
During discussion below, it was however discovered, that this is a new issue regarding a polluted input state, a more serious issue that effects the whole of Joomla and not just login feature! and not as originally thought, just a failure to fix the previously reported issue.
Issue described here #37180 (comment)
Labels |
Added:
No Code Attached Yet
|
You miss the point.
Name me 10 web sites where you can login with a GET request? Your bank? PayPal? Amazon? Apple? You can't.
Because it's stupid, wrong, and goes against every single security practice.
Also all query data is logged into the logs....
it was a question
Then let me answer the question - yes, you require the correct username, password and correct and valid token in order to achieve authentication and receive a new authenticated session cookie
But, I could run a million GET requests a second to guess the credentials...
You will be interested in my next Issue ignored by the JSST, where I DONT need your password, and I can still login as you... if a single 0 is changed to a 1 in the db (Ie a single plugin enabled) .... they keep telling me its going to be fixed but we are coming up on 4 years now...
How could it be fixed? I would donate for that because J!4 is going to be my strongest power horse.
Probably one line of code somewhere near
needs to change... but hey...Perfect, may you start a Pull request?
I understand your frustratiom - sometimes it's not easy to work in a team.
Perfect, may you start a Pull request?
I reported it to the official Joomla Security Strike Team to resolve on 9th June 2020 - they have had years to resolve it in a private pull request. It falls squarely in their remit.
Additional lines of code for security are always good imho.
I'm unable to replicate the issue on a current 4.x installation, I get a "The security token did not match. The request was aborted to prevent any security breach. Please try again." message.
A PR fixing the issue was #31236 where you @PhilETaylor confirmed that the issue has been fixed. Do I miss anything here?
I have just confirmed this again, on a Joomla 4.1.0 Stable version on a totally different computer from yesterday.
There is still an issue to be resolved it seems.
Im sorry that the head of the Joomla Security Strike Team is more interested in spreading lies and malicious slander in German Language Chats thinking that I will not understand what is being said, instead of actually investigating and fixing security issues.
as stated in #31236 (comment) - "hint, there is another security issue"
In that post I also revealed other things that the JSST should be taking into account - and was again ignored.
Did you take a wider look, or specific to the report? What about the other 13 uses of checkToken('request') - have you ensured those are also not introducing a security issue? (hint, there is another security issue
Should a GET have the ability to set the default "home" property for a list of items by GET request, when it should only be able to be set by POST (@SInCE 1.6)
Surely the ones using 'request' should be more specifically a "get" or a "post" (which is the default when no method set)? a method that can be called by GET and POST is probably badly coded I guess.
$this->checkToken('request'); is also used in the "Method to confirm the password request." - is that ok in context?
I reproduced that and also think that login via GET /administrator/?option=com_login&task=login&username=adminuser&passwd=password&a148c641d9ce8e27db0f840b07782522=1
should not be possible.
You get the needed token by accessing /administrator
And then you can fire your get request(s).
Why or for what is that needed?
I also followed the Reference Links above where it is clearly described as bad practice.
There is still an issue to be resolved it seems.
Interesting. I was unable to reproduce the issue on multiple installations, but can indeed confirm that it still occurs on one machine. I'm already on it, investigating the actual root cause.
So: thanks for bringing that up again!
investigating the actual root cause.
Pollution of the input state where GET variables are being incorrectly stored as POST variables as well, deep within Joomla Framework components as well as the over use/incorrect use of the getInputForRequestMethod
within com_login when only ever POST request methods should be taken into account for handling credentials.
For example, when on url http://127.0.0.1/administrator/index.php?option=com_login&task=login&username=admin&passwd=adminadminadmin&156648435e27144fd76326f019d9ab33=1
the Input object looks like this.
This image was produced by adding dd( Factory::getApplication()->input);
after $input = Factory::getApplication()->input->getInputForRequestMethod();
in administrator/components/com_login/src/Model/LoginModel.php
A PR fixing the issue was #31236 where you @PhilETaylor confirmed that the issue has been fixed. Do I miss anything here?
Yes what you are missing is that that DID FIX THE ISSUE reported AT THE TIME, this is why I stated AT THE TIME it was fixed - but this now seems to be a NEW ISSUE with the input state being corrupt.
This can be proved with the above image, the change you made previously was to check the token in the POST instead of a token in the "request" (meaning either GET or POST). The token is STILL BEING CHECKED in the POST of the Input, but because something is corrupting the state of the Input object now, that token, sent by the url as a GET, is being stored in the post.
This opens up a whole new can of worms...
Basically right now, (at least in com_users) in Joomla 4.1, it seems, that you can bypass POST token checks, by passing the token in the url (GET) requests. I can think of other serious security issues this opens up. Basically all CSRF checking is probably broken too (when expecting admin action to be a POST, yet a GET now works!) . Doh.
I wonder what other knock on effects a polluted Input state could cause - I'll think on that in the shower and over coffee - but I think today could become a very bad day for Joomla 4....
This opens up a whole new can of worms...
I think today could become a very bad day for Joomla 4....
Confirmed.
Lets create a new user with a GET request... (Site configured to allow User Registrations, which is disabled out of the box)
http://127.0.0.1/index.php/component/users/?task=registration.register&Itemid=101&66214f147fbd3c99b5494bb73e0a5e51=1&jform[name]=TESTUSER&jform[username]=PASSWORD&jform[password1]=12345678901234567890&jform[password2]=12345678901234567890&jform[email1]=nonsuch@example.com
Tested with PHP 8.0.16 Joomal 4.1.0 Stable. Nginx MySQL 5.7 Dockerized.
Title |
|
This was initially broken 6 years ago (Sep 17, 2016 ) by @mbabker (who I have spoken to today, so no shame dropping here) in commit in the joomla-framework/input repo 2.0-dev branch
joomla-framework/input@9135fe4
specifically the change:
More specifically the change to be:
$this->data = empty($source) ? $_REQUEST : $source;
at the same time as changing the constructor to be
public function __construct(array $source = [], array $options = [])
These changes are correct. At this point the code works perfectly.
HOWEVER - then 4 years ago (May 26, 2018) on merging master into 2.0-dev he overwrote the constructor in joomla-framework/input@1ed80e8 so it was reverted to
public function __construct($source = null, array $options = [])
thus introducing a bug as the constructor and the empty check were not doing the same thing.
Jun 9, 2020, 2 years ago @wilsonge moved Joomla 4 to using the Joomla Framework ~2.0@beta d8109cb - this first introduced the bug to Joomla 4 codebase.
Now, on Aug 16, 2021 @wilsonge merged the Joomla Framework 2.0 Stable has cementing this bug into Joomla 4.
The fix therefore, is to revisit the constructors of all the classes (Five of them) in the Joomla-framework/input package that were wrongly changed in joomla-framework/input@1ed80e8 and revert that change, ensuring that any further call to the $source
(which is an array, the contents of $_POST for example) are treated as a null|array
and not treated with empty()
which will return TRUE for empty([])
(which is the root bug here, if the POST is empty when you try to call Factory::$application->input->post->get('somethin')
then it will use the $_REQUEST
global, which has the GET
params in it!!!
Case Solved.
Also this line in Joomla needs to ensure that it only ever uses the POST data and never GET data
Better still, stop accessing request data in a MODEL and start injecting it from the controller... like, modern dependancy injection style haha
This should also be a complete release blocker!
Monkey see Monkey Do. joomla-framework/input#36
Full transparency: The underlying issue was originally discovered by Nic who reported the issue in Nov 2021, see
#35541 (comment)
I failed to properly recognize the effects of that issue at the time of the report and added it to the "needs closer check"-list.
So the official Joomla Security Strike Team have been aware of a security issue since November 2021 - did not correctly triage it - did not fully comprehend the knock on effects - and did nothing about it but put it on a list… got it…
nic literally spelled it out
even though the end result was merely annoying instead of a major security issue. Next time we might not be that lucky..
so once again I’m having to literally spell out and debug security issues that the JSST have completely ignored and completely misunderstood the gravity and nature of.
I can’t tag @nikosdion here as he has blocked me on GitHub
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-03-03 18:35:37 |
Closed_By | ⇒ | PhilETaylor |
but you need to know the username, the password and the token ?