Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added HTTP status code check feature > for the installation #18294 #18295

Closed
wants to merge 14 commits into from
Closed

Added HTTP status code check feature > for the installation #18294 #18295

wants to merge 14 commits into from

Conversation

HRIT-Florian
Copy link
Contributor

@HRIT-Florian HRIT-Florian commented Oct 9, 2017

The security file check at the installation could be skipped, if the HTTP status code is not 401 (HTTP authentication required) which means the installation ist http access protected.

I think some web companys like me did work on http access protected development sites. The installation anyway take some steps, so, let's make the life easier and let's skip this if it is not needed.

For this, I have opened this pull request, where the http chek is now implemented.

Summary of Changes

Added HTTP status code check,
for skipping security file if website is http 401 authentication required, which means http access protected.
70173fb

Outsoureced functionality for the checks to two new methods
called checkHostSecurity() and checkSecurityFile(). Did also renamed some names called remoteDBFile to securityFile, because i think it is more understandable. Because we are at the InstallationModelDatabase so think its clear which file and it's more associated to the method names. And some improvements in the check order logic. Hope the community like it ;)

The new methodes are called during initialise()

// Save host checks
if ($this->checkHostSecurity($options) === false)
{
    if ($this->checkSecurityFile() === false)
    {
            return false;
    }
}

Testing Instructions

  • Create a new installation site which is behind an directory protection like http access authentication.
  • Go through the installation steps and you shouldn't see the security file check notification.
  • And check again, but this time without the http directory protection and the security check should work, the notification should be shown as known ;)

Documentation Changes Required

https://docs.joomla.org/J3.x:Secured_procedure_for_installing_Joomla_with_a_remote_database

Changes below in bold italic

English New Documenation Part:

Starting with Joomla! 3.7.4 the Joomla! Security Strike Team (JSST) implemented additional security checks in the install application in order to protect your web hosting accounts from being overtaken by a remote attacker. In case your database is not on the same server as your website and your website is not through a HTTP authentication protected, we require an extra check that makes sure you are the owner of the website.

...

German New Documenation Part:

Beginnend mit Joomla! 3.7.4 hat das Joomla! Security Strike Team (JSST) weitere Sicherheitsprüfungen in die Installations-Anwendung eingeführt, um Ihre Webhosting-Konten vor der Übernahme von einem entfernten Angreifer zu schützen. Im Falle, dass die genutzte Datenbank sich nicht auf dem gleichen Server befindet und die Website nicht durch eine HTTP-Authentifizierung geschützt ist, wird eine separate Prüfung verlangt, um zu versichern, dass Du der Inhaber der Website bist.

...

for skipping security file if website is http 401 authentication required, which means http access protected.
@brianteeman
Copy link
Contributor

To me it looks like you haven't understood the purpose of the security check and the vulnerability it is protecting against.

@HRIT-Florian
Copy link
Contributor Author

Hi @brianteeman as i know, the security check is there to protect the installation from outside, right?

@brianteeman
Copy link
Contributor

No it's much more than that

@HRIT-Florian
Copy link
Contributor Author

Of course @brianteeman , and there are also other checks. But this PR did just effect the dbhost check. And it would be very exotic if we check also the dbhost if the site ist http protected. So, why shouldn't we simplify it on protected installations?

@HRIT-Florian
Copy link
Contributor Author

HRIT-Florian commented Oct 9, 2017

Did now also added the changes for the documentation in my PR comment. Documentation in English and German if this help. If wished, we could write it to the official documentation site: https://docs.joomla.org/J3.x:Secured_procedure_for_installing_Joomla_with_a_remote_database

@brianteeman
Copy link
Contributor

@SniperSister one for JSST to review

@joomla-cms-bot joomla-cms-bot changed the title Added HTTP status code check feature > for the insatallation #18294 Added HTTP status code check feature > for the installation #18294 Oct 11, 2017
@ghost
Copy link

ghost commented Oct 11, 2017

corrected Typo in Title.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18295.

@zero-24
Copy link
Contributor

zero-24 commented Oct 11, 2017

@Didldu-Florian To me the idea looks ok but can we move that checks (all three) to a separate private method? That would keep the code simpler to understand as this checks should not all placed in the initialise method so we keep that small and simple to read. Thanks.

@HRIT-Florian
Copy link
Contributor Author

HRIT-Florian commented Oct 11, 2017

@zero-24 I'm glad you're okay with this. Did outsoureced the functionality for the checks to two new methods called checkHostSecurity() and checkSecurityFile(). Did also renamed some names called remoteDBFile to securityFile, because i think it is more understandable. Because we are at the InstallationModelDatabase so think its clear which file and it's more associated to the method names. And some improvements in the check order logic. Hope the community like it ;)

The new methodes are called during initialise()

// Save host checks
if ($this->checkHostSecurity($options) == false)
{
        $this->checkSecurityFile();
}

Hope it is more clear

@zero-24
Copy link
Contributor

zero-24 commented Oct 12, 2017

This does not work as expected. The check can be skipped. The method initialise needs to return false in case of an error / not matching file ;)

@HRIT-Florian
Copy link
Contributor Author

HRIT-Florian commented Oct 12, 2017

@zero-24 sorry, can't repoduce your concerns. Did tested again and it works. By the, way, the method initialise returns JDatabaseDriver or false on failure. Same as it worked before my PR.
Can you describe your concern in more detail?

Please try hardset $statusCode = 401; or $statusCode = 200; at line 216 and be sure db_host is not localhost.

HTTP 200 >The securityFileCheck will be required.
HTTP 401 >The securityFileCheck will be skipped.

@zero-24
Copy link
Contributor

zero-24 commented Oct 12, 2017

@Didldu-Florian yes that part is working (401 aka .htaccess)

but when the check should be done you can just hit "install" and it starts doing things. (even that it stop working on another issue)

Basically that check in the initialise method should make sure that the validation fails and the user can't go any step forward. For that reason initialise needs to return false when the checks did not match on a host where we require to check it ;)

@HRIT-Florian
Copy link
Contributor Author

HRIT-Florian commented Oct 12, 2017

@zero-24 Oh dear! You are right, that was lost during outsourcing methods. My fault.
Did fixed it with c0a90dd Now the logic is same as before on return.

@zero-24
Copy link
Contributor

zero-24 commented Oct 12, 2017

@Didldu-Florian it it just allowed to return false in case there is a error ;)

Else you can't move forward as the method return void just before we try to setup the database ;)

// Save host checks
if ($this->checkHostSecurity($options) === false)
{
        if ($this->checkSecurityFile() === false)
        {
                return false;
        }
}

This should do the trick 😄

@HRIT-Florian
Copy link
Contributor Author

HRIT-Florian commented Oct 12, 2017

@zero-24 Thanks, done ;)

*
* @return boolean true if all checks have been passed
*
* @since 3.8.1
Copy link
Contributor

@Quy Quy Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace @since 3.8.1 to @since __DEPLOY_VERSION__ so it will be set automatically. Also below.

@zero-24
Copy link
Contributor

zero-24 commented Oct 12, 2017

I have just added the missing= for the typesafe check. Looks good from here than. (the issue @Quy mention should be fixed too)

@HRIT-Florian
Copy link
Contributor Author

HRIT-Florian commented Oct 12, 2017

@zero-24, @Quy __DEPLOY_VERSION__ added ;)

@zero-24
Copy link
Contributor

zero-24 commented Oct 13, 2017

👍 thanks @Didldu-Florian

@Quy
Copy link
Contributor

Quy commented Oct 27, 2019

Replaced by PR #26840


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18295.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants