archlinux / archinstall

Arch Linux installer - guided, templates etc.
GNU General Public License v3.0
5.82k stars 508 forks source link

insecure password storage #1111

Open jamincollins opened 2 years ago

jamincollins commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Can we not simply delete the file after the installation?

ryleu commented 1 year ago

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

cyphunk commented 1 year ago

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 commented 1 year ago

My suggestion:

Torxed commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Torxed commented 1 year ago

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 commented 11 months ago

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 commented 9 months ago

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 commented 5 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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: https://github.com/archlinux/archinstall/issues/1111#issuecomment-1413476189

drHyperion451 commented 3 months ago

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 commented 3 months ago

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

drHyperion451 commented 3 months ago

/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 commented 2 months ago

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