User tests: Successful: Unsuccessful:
If the user supplies either an unknown user name or a wrong password or a wrong secret key the com_users login form is shown (probably again) and it contains not only the user name typed in but also the password and (if activated) the secret key. The latter two should never be sent back to the user's browser for security reasons as is the case with the registration form when you enter e.g. a user name already in use or an email address already used with any other registration.
Test instructions: locate the login form at any page in the frontend and enter any invalid login information. After sending this data to the server you'll be shown the com_users login form containing the login information you just entered (the password field contains only dots but when you look into the HTML code you'll see the password you entered in clear text).
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
Well - yes and no. It might be a real password, e.g. in case you mistyped the user name - or maybe even more dangerous: both the user name and the password might be correct, but the secret key is not. And this login form might be shown using HTTP even if you enabled sending the login data using HTTPS ("usesecure" setting in your mod_login instance).
if you are sending to a https then the resultant error page is also on https - I still fail to see how this is an exploitable security issue. You are sat infront of your pc and you provide the credentials and they are displayed back to you... no one else, and not pulled from the db or anything like that - Garbage in Garbage Out.
MANY web applications return your failed password if you fail to login, so that you can change the typo you made.
However I agree removing it from the error page is a good step and I recommend this is merged.
For future reference:
Report security issues to the Joomla! Security Strike Team (JSST) at security@joomla.org or use the JSST contact form. Please do not use the public tracker for security issues.
https://github.com/joomla/joomla-cms/blob/staging/CONTRIBUTING.md
I guess Travis should be relaunched here.
@infograf768
Why do you think so? Is there something wrong with my change?
The first Travis check failed in the PHP:7 check, but this was as far as I've seen not a problem with my change but it was a problem with Travis itself: something about pecl being used but not supported. I saw this and I found that it was fixed some time later within Travis (at least I think the fix I've seen was due to that pecl problem).
In order to rerun Travis I made another change by adding a comment to my patch which was nevertheless good to do. After committing this second change Travis was relaunched automatically, and this time it succeeded for all PHP versions.
Yes, it was a Travis issue but I think it is now solved.
I have tested this item successfully on 06ffe93
One thing i notice after this patch is the behaviour is slightly different in frontend login and backend login.
frontend login: on fail the user remains in the form, but not the password
backend login: on fail, neither the user nor the password remains in the form
Since my patch only addresses the com_users login form I assume that the different behaviour with the backend login already existed before my patch. As far as I've seen in the source code the backend login form is shown again (without any data filled in) if the supplied user name or password is wrong.
I suggest to add
$data['username'] = '';
to your PR. This will clear the Username field on failure.
@jo-sf. Yes, i know that was not to do with your PR. As you noticed i tested this PR successfully.
The difference in behaviour between frontend and backend login was already there before your PR. With your PR they are more likely, just the username part is different.
I just said that because IMHO all login form should have equal behaviors in that reggard.
I have tested this item successfully on 06ffe93
This PR has received new commits.
CC: @andrepereiradasilva, @waader
As @infograf768 and @andrepereiradasilva suggested I changed the frontend behaviour in case of a failure: now the user name will be cleared too.
Works! Thanks jo-sf.
I have tested this item successfully on 77f0d81
I have tested this item successfully on 77f0d81
Status | Pending | ⇒ | Ready to Commit |
Labels |
Added:
?
|
Milestone |
Added: |
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-01-06 11:55:38 |
Closed_By | ⇒ | roland-d |
Good catch - Tested and the patch works well.
This is NOT a security issue however, as the data sent back is not the REAL password, but the data you sent to the server anyway.