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

Hide DB password in the administration "system info" page #7716

Closed
marcbria opened this issue Feb 18, 2022 · 13 comments
Closed

Hide DB password in the administration "system info" page #7716

marcbria opened this issue Feb 18, 2022 · 13 comments
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. Try Me This issue might be good for a new contributor. Can you help us?
Milestone

Comments

@marcbria
Copy link
Collaborator

Describe the bug
Every user with rights to see the Administration pages can see the DB pwd.
As far as admins know this info (or can check it in the config.inc.php) may be is a good idea not showing this info in this pages... or at least, transform it to asterisks.

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'Administration'
  2. Click on 'System Information' (url: https://foojournal.org/admin/systemInfo)
  3. Scroll down to 'database section'
  4. See the DB pwd.

What application are you using?
OJS, 3.x

Additional information

imagen

@marcbria
Copy link
Collaborator Author

Not sure if it need to be a bug or a feature request.

@NateWr NateWr added the Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. label Feb 21, 2022
@NateWr NateWr added this to the 3.3.0-10 milestone Feb 21, 2022
@NateWr
Copy link
Contributor

NateWr commented Feb 21, 2022

@asmecher seems like a sensible change, but do you find that the community needs this information in the admin screen to get adequate support?

@asmecher
Copy link
Member

No, I haven't seen problems e.g. in the forum that have led to users tracking down database passwords here. I've always sent them to the configuration file (which is usually painless).

@NateWr NateWr added the Try Me This issue might be good for a new contributor. Can you help us? label Feb 22, 2022
@Tyl13
Copy link
Contributor

Tyl13 commented Mar 9, 2022

A few questions about this issue. In the template file, it is getting the password from the value in settings within the database configData category.

At least, from what I am understanding about smarty templates. Please correct me if I am incorrect, and I was trying to see where it is pulling these variables from exactly. So, I would appreciate if someone could point me to that location.

Is it a better option to not print out the password at all, or redact it with asterisks? Additionally, should it be an option for users to pick either of those two options or none of them at all?

@asmecher
Copy link
Member

asmecher commented Mar 9, 2022

Hi @Tyl13!

The PHP controller class that's responsible for the systemInfo display is lib/pkp/pages/admin/AdminHandler.inc.php, in the systemInfo function. There you'll see that the configuration file's data is assigned to the Smarty template manager:

$templateMgr->assign([
    ...
    'configData' => Config::getData(),
);

$templateMgr->display('admin/systemInfo.tpl');

The systemInfo.tpl file is in lib/pkp/templates/admin/systemInfo.tpl. The configData variable is formatted into HTML in that file.

@NateWr
Copy link
Contributor

NateWr commented Mar 10, 2022

Is it a better option to not print out the password at all, or redact it with asterisks?

Redact it with asterisks, but do not make the string the same length. In other words, if my password is 1234, don't make it ****. Instead, make it a set number of asterisks regardless of the actual value of the password. This prevents someone from using this to identify the length of the password.

Additionally, should it be an option for users to pick either of those two options or none of them at all?

No options are needed. Just redact the password.

asmecher pushed a commit that referenced this issue Mar 11, 2022
* Redacts DB password

* Reduced changes to 4 lines

* Removed extra line
asmecher pushed a commit that referenced this issue Mar 11, 2022
* Redacts DB password

* Reduced changes to 4 lines

* Removed extra line
@asmecher
Copy link
Member

Thanks, @Tyl13!

@ctgraham
Copy link
Collaborator

Reopening. The existing change compares the $value against the string "password", when it needs to compare the $name against the string "password". This also should be an exact equals comparison, to avoid the (unlikely) risk of a numeric name being equalish to a string.

@ctgraham ctgraham reopened this May 13, 2022
asmecher added a commit that referenced this issue May 14, 2022
@asmecher
Copy link
Member

Whoops -- fixed. Thanks, @ctgraham.

asmecher added a commit that referenced this issue May 14, 2022
@marcbria
Copy link
Collaborator Author

marcbria commented Jul 15, 2024

Not sure if I should reopen this or create a new issue.

Taking the problem form a generic perspective, the point is the full page is too much... "verbose", and a lot of sensitive is shown there. As far as journalmanagers and editors could reach this page, it's a security concern.

It's not only the dbpwd, there are a few more fields the sysadmin won't probably like to share with journalmanagers/editors, so I suggest hide (as we did with the DB password) any pwd, key, secret (even usernames?) included in this page.

My preliminary list of fields to be hiden (as sysadmin could always check them in config.ini.php) is:

  • database.password
  • security.salt
  • security.apy_key_secret
  • email.smtp_user
  • email.smtp_password
  • email.smtp_oauth_clientsecret
  • captcha.recaptcha_private_key

I would also suggest to remove the last link with the phpinfo() output we have in the bottom of the page, as some sensitive data could be exposed there and is info the sysadmin could reach whenever he/she likes to.

@ctgraham
Copy link
Collaborator

How are journal managers or editor reaching this page? I think it requires the Administrator (last tested: OJS 3.4).

@marcbria
Copy link
Collaborator Author

My fault Clinton.
As you say, JournalManagers and Editors can't reach this page... only admins.

But do we really need to expose this sensitive data to them?
If we hide the DB pwd don't make sense to hide the rest?

@ctgraham
Copy link
Collaborator

I make a distinction between credentials that I would pass along to a new owner, and those credentials which I would keep private, if I transferred the OJS installation to another party.

From your list, I think the database password, the smtp secrets, and the ReCAPTCHA private key all fall into the category of secrets that belong to the hosting provider, not to the hosted journal.

I would suggest opening a new issue or discussion and linking to this closed issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. Try Me This issue might be good for a new contributor. Can you help us?
Projects
None yet
Development

No branches or pull requests

5 participants