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

insecure password storage #1111

Open
jamincollins opened this issue May 1, 2022 · 25 comments
Open

insecure password storage #1111

jamincollins opened this issue May 1, 2022 · 25 comments
Labels
enhancement Security Issue Security related issues
Milestone

Comments

@jamincollins
Copy link

It would appear that all user passwords are stored in plaintext in file /var/log/archinstall/user_credentials.json. Which is accessible to any user on the system (mode=644).

# ls -l /var/log/archinstall/user_credentials.json 
-rw-r--r-- 1 root root 94 May  1 18:48 /var/log/archinstall/user_credentials.json

At minimum suggest changing the access permissions on this file to mode=600, user=root, group=root.

Better yet, store the password hash. I see that there is another issue (#1029) for allowing passwords to provided in a hashed manner. And in this issue there is some concern over supporting multiple potentially disparate hashing requirements. Given that the entry is already stored as a JSON object (python dict), one solution would be to extend the object to indicate which subsystem the entry is for, such as:

{
    "!superusers": {
        "wilbur": {
            "!password-shadow": "$6$xyz$ZmArb33D/r5Aftt/vkeJD.eOocWEgln8gW2Jn/71EUup1LDTJv0P8nL76leql7HDvsy2ObrAT6k.jQnol2pfV0"
        }
    }
}

or

{
    "!superusers": {
        "wilbur": {
            "!password": {
                "shadow": "$6$xyz$ZmArb33D/r5Aftt/vkeJD.eOocWEgln8gW2Jn/71EUup1LDTJv0P8nL76leql7HDvsy2ObrAT6k.jQnol2pfV0"
            }
        }
    }
}

Personally, I prefer the second form. IMO, it is easily extensible for any additional passwords that need to be convey.

@Torxed
Copy link
Member

Torxed commented May 1, 2022

I do not disagree that we could improve on this. And i'm not saying hashes change frequently. But storing a hash would at some point become equally insecure when it's deprecated. And if someone gets a hold of the hash it's a small bump in the road for a local attack.

I think the chmod is a solid approach and increases security significantly for me to consider this done for now.

I'll submit a PR with it together with the /tmp password and issue surrounding passwords being stored in disk_layout.json

@Torxed Torxed added Security Issue Security related issues enhancement labels May 1, 2022
@Torxed Torxed added this to the v2.4.3 milestone May 1, 2022
@Torxed
Copy link
Member

Torxed commented Aug 28, 2022

I think I've figured out a clean way to approach this.
As storing hashes presents the issue of not being able to rely on toolings like passwd for setting the password.
Obviously not a huge obstacle but we'll need to transition to setting it with opening /etc/shadow manually.

Something in the lines of:

salt = base64.b64encode(os.urandom(12))
password = base64.b64encode(hashlib.pbkdf2_hmac('sha512', b'password', salt, 200000, 64))

with open(f"{target}/etc/shadow", ...) as shadow:
    shadow.write(f"{user}:$6${salt.decode('UTF-8')}${password.decode('UTF-8')}:{the_rest}\n") # In case it's not obvious, pseudo code

200000 appears to be a reasonable iteration count as explained by NIST-SP-800-132 and stackexchange pbkdf2 iterations question, as referenced by hashlib.pbkdf2_hmac

A heads up if we go this route is:

Deprecated since version 3.10: Slow Python implementation of pbkdf2_hmac is deprecated. In the future the function will only be available when Python is compiled with OpenSSL.

The alternative is to use passwd and extract the set password hash, salt and algorithm used.
But that would mean we run the risk of storing clear text passwords in the conf and potentially save it to disk before replacing it.
So I'd want to go the route of generating a /etc/shadow compliant alg+salt+hash combo at the "set password for user X" prompt in the menu.

@Torxed Torxed modified the milestones: v2.5.1, v2.5.2 Aug 30, 2022
@Torxed
Copy link
Member

Torxed commented Aug 30, 2022

Moving this to a later milestone as this could potentially cause issues. And I'd like to get v2.5.1 out to patch some older issues that's been lingering.

@klausenbusk
Copy link
Member

Obviously not a huge obstacle but we'll need to transition to setting it with opening /etc/shadow manually.

FWIW chpasswd has a -e, --encrypted supplied passwords are encrypted option. Opening the file manually is likely easier, but just for the record :)

@Torxed
Copy link
Member

Torxed commented Aug 30, 2022

That would be easier to implement. I couldn't find such a flag for passwd but happy to see that chpasswd prevails ^^ Thanks @klausenbusk!

@Enoumy
Copy link

Enoumy commented Nov 26, 2022

Hi! I have perhaps a dumb question.

How severe of a vulnerability is this? Is there a recommended way of working around this issue in the meantime? Is the current workaround for users to use chpasswd after installation so that the passwords stored on plaintext disk are no longer the actual passwords?

Thanks!

@NervousNullPtr
Copy link

NervousNullPtr commented Dec 28, 2022

How severe of a vulnerability is this?

This seems to be a pretty big security vulnerability if you do not have any LUKS encryption enabled and work/live somewhere, where others might have access to your computer. Additionally, malicious scripts may be able to read your password from this file.

Is there a recommended way of working around this issue in the meantime?

The way to go about this as you described seems to be a good workaround, don't take my word for it, though. I'll be happily corrected if there's something wrong.

Securely deleting this log file also seems like a simple workaround in the mean time.

I won't use archinstall until this is fixed, my stomach just tells me no (Especially since it's such a basic security principle; Do not store passwords in plain text).

Sad, because it looks like a cool script!

Oh, @Torxed: If this has been fixed, make sure to close this issue to let us know :p Just reminding you because I frequently forget to close issues.

As a possible fix: Don't log the passwords at all.

@relthyg
Copy link

relthyg commented Jan 14, 2023

Can we not simply delete the file after the installation?

@ryleu
Copy link

ryleu commented Jan 17, 2023

it should never be written to the drive in the first place because of drive recovery techniques

@cyphunk
Copy link

cyphunk commented Feb 2, 2023

Can we not simply delete the file after the installation?

Not advised. Keep in mind on SSD's and modern storage systems that data might not be actually deleted even when you rm -f. Due to wear leveling often a delete simply markes a page or cell as "used" but the contents of that data can still be recovered using lower level recovery mechanisms. Helpful read here. If your ssd block life is, for example, 3000 writes before block marked as end-of-life, then the contents of the last write before being marked will be retained physically forever.

Assume if you store anything in clear text that it can be recovered through forensic analysis. This is why on modern systems one should make sure /tmp is either encrypted, or stored in memory with swapfs write to disk forbidden or with swapfs encrypted. And if this all sounds confusing and convoluted, it absolutely is which is why most users that care about security but don't have time to dig into linux details should just stick with using a macbook (cough, secureboot, cough)

@MrElendig
Copy link

MrElendig commented Mar 20, 2023

My suggestion:

  • don't log credentials by default
  • 600 the log
  • optionally log credentials with debug logging enabled, or with a separate flag.

@Torxed
Copy link
Member

Torxed commented Mar 21, 2023

Just to shed some light, we don't log passwords in the log that is written to disk.
We do write user credentials to ramdisk while the ISO is alive so that the user can copy them if they choose to.

We could also omit writing the credentials all together since we now have a "save configuration" menu alternative which we didn't have when this ticket was created.

I'll patch out the credential writing by default, and let the users save the creds and deal with permissions and what not if they choose to do so.

@rainyskye
Copy link

Just to shed some light, we don't log passwords in the log that is written to disk. We do write user credentials to ramdisk while the ISO is alive so that the user can copy them if they choose to.

This mitigates the issue right? Since even if it's plaintext, it's in the ramdisk, so unless you're targeted immediately after/shortly after the install it should be "fine"?

@kelna
Copy link

kelna commented May 7, 2023

So this user_credentials.json isn't generated anymore by default with the current archiso, right?

@Torxed
Copy link
Member

Torxed commented May 7, 2023

Just to shed some light, we don't log passwords in the log that is written to disk. We do write user credentials to ramdisk while the ISO is alive so that the user can copy them if they choose to.

This mitigates the issue right? Since even if it's plaintext, it's in the ramdisk, so unless you're targeted immediately after/shortly after the install it should be "fine"?

Should be fine yes.
I'd like to verify a few things before closing this ticket tho :)

@abulvenz
Copy link

abulvenz commented Jul 15, 2023

I found that with the installation image from the end of June, I can still find my disk encryption_password as plain text in the file /var/log/archinstall/install.log also after rebooting into the freshly installed system. The user or root passwords are not part of the log anymore.

Would You consider this a different issue?

@Torxed
Copy link
Member

Torxed commented Sep 16, 2023

I found that with the installation image from the end of June, I can still find my disk encryption_password as plain text in the file /var/log/archinstall/install.log also after rebooting into the freshly installed system. The user or root passwords are not part of the log anymore.

Would You consider this a different issue?

I've been bashing my head for a while now with this one.
I can not reproduce it in any capacity: https://youtu.be/PjG__PGKP4Y

And the credentials on the ISO is no longer world readable:
screenshot

And no configuration files is automatically copied to the installed system:
screenshot

@verbumfeit
Copy link

What is the status on this? Could the secrets in the (manually saved) user_credentials.json be saved/loaded as hashes? I'm currently setting up an unattended archinstall and it would be great if archinstall could load the user_credentials.json via http.

@drHyperion451
Copy link

What is the real reason why this is kept? Isn't just better to just rm -rf after all the install or make a chmod at least? Am I missing something?

@Enoumy
Copy link

Enoumy commented Mar 19, 2024

Should archinstall's readme have the following warning?:

archinstall stores all user and (secondary) disk encryption passwords in plain text.

I feel like this issue should be broadcasted more loudly to Arch newcomers.

The Arch Wiki has this banner on archinstall; I feel like this project's readme should have it too until the issue is resolved.

@cyphunk
Copy link

cyphunk commented Mar 19, 2024

What is the real reason why this is kept? Isn't just better to just rm -rf after all the install or make a chmod at least? Am I missing something?

this exact question was answered above in thread: #1111 (comment)

@drHyperion451
Copy link

What is the real reason why this is kept? Isn't just better to just rm -rf after all the install or make a chmod at least? Am I missing something?

this exact question was answered above in thread: #1111 (comment)

Refill the file with /dev/random then rm -rf ?

@jamincollins
Copy link
Author

Call me crazy, but why write it there in the first place? Why not /dev/shm/?

@drHyperion451
Copy link

drHyperion451 commented Mar 19, 2024

/dev/shm/

That's actually not a bad option. It would completely remove without recovery since it saves into RAM. Also according to the archwiki it's already been used in /run /var/run and /tmp directories without an entry on fstab.

What if we change lib/configuration.py file to something like:

def save(self, dest_path: Optional[Path] = None):
	dest_path = dest_path or self._default_save_path

	if self._is_valid_path(dest_path):
		self.save_user_config(dest_path)
		self.save_user_creds(Path('/var/run/archinstall'))

This is wrong, but it's a starting point

@cyphunk
Copy link

cyphunk commented Apr 11, 2024

What is the real reason why this is kept? Isn't just better to just rm -rf after all the install or make a chmod at least? Am I missing something?

this exact question was answered above in thread: #1111 (comment)

Refill the file with /dev/random then rm -rf ?

The exact reason this too will not work was answered above

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

No branches or pull requests