-
Notifications
You must be signed in to change notification settings - Fork 589
insecure password storage #1111
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
Comments
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 |
I think I've figured out a clean way to approach this. 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:
The alternative is to use |
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. |
FWIW |
That would be easier to implement. I couldn't find such a flag for |
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 Thanks! |
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.
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 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. |
Can we not simply delete the file after the installation? |
it should never be written to the drive in the first place because of drive recovery techniques |
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) |
My suggestion:
|
Just to shed some light, we don't log passwords in the log that is written to disk. 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. |
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"? |
So this user_credentials.json isn't generated anymore by default with the current archiso, right? |
Should be fine yes. |
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 Would You consider this a different issue? |
I've been bashing my head for a while now with this one. And the credentials on the ISO is no longer world readable: And no configuration files is automatically copied to the installed system: |
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. |
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? |
Should archinstall's readme have the following warning?:
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. |
this exact question was answered above in thread: #1111 (comment) |
Refill the file with |
Call me crazy, but why write it there in the first place? Why not |
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 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 |
The exact reason this too will not work was answered above |
Hey I just started to play around with the installer and plan to move to arch soon. I am quite confused with this issue still being open and the warning being still present in the arch wiki: Is this still a potential security issue? Since I want to use arch on my daily driver laptop with disk encryption I am a little bit concerned. |
The short answer is, not really. The matter of storing hashes is a really good idea — so I've left the issue (slash feature request) open until I've had time to implement it (or someone chimes in with a PR hehe). |
Should the scary archwiki banner be removed? If this isn't an issue anymore. |
When using archinstall to install Arch Linux from an existing host running Arch Linux the password will be written in plaintext to timestamp ['/usr/bin/arch-chroot', '/mnt/archinstall', 'sh', '-c', 'echo username:password | chpasswd'] |
Was this issue ever fixed? From comments in here it looks like this was patched ages ago and passwords are no longer stored in that json. |
A hashed password is safe and for a high enough password entropy will never be an issue to have it recovered @Torxed , i didn't quite understand the scepticism in your first posts about hashing it and leave it be even with chmod 744 . It really doesn't matter who's hands it falls into, not even in a world of post-quantum will it be an issue: https://cr.yp.to/hash/collisioncost-20090517.pdf I can make a PR using Argon2id @Torxed |
Did anyone consider just using a secure python crypto lib to encrypt the whole Instead of storing |
Why does the password need to be saved anywhere in the first place? |
As long as you store one user on an unencryted drive its easy to provide a short password during an automated install, but providing multiple users with different passwords along with a 32+char encryption key is pretty inconvenient... I admit that encrypting the EDIT: import base64
import os
from cryptography.fernet import Fernet
from cryptography.hazmat.primitives import hashes
from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC
password = b"password" # potential master password provided by user
salt = os.urandom(16)
kdf = PBKDF2HMAC(
algorithm=hashes.SHA256(),
length=32,
salt=salt,
iterations=1_000_000,
)
# derive a secure key from the given master password
key = base64.urlsafe_b64encode(kdf.derive(password))
f = Fernet(key)
# encrypt
token = f.encrypt(b"<user_credentials.json content>")
# token
# b'...'
f.decrypt(token)
# b'<user_credentials.json content>' Source: https://cryptography.io/en/latest/fernet/#using-passwords-with-fernet The only thing that's missing is that the salt has to be prepended / appended to the encrypted message. |
With a thorough review of the comments here, it appears the expectation from users is that the |
Use an approach like Ansible Vault? |
#3276 has partly addressed the issue, usee account passwords will be stored in hashed form in the configuration file. So looking into options of encrypting the entire file as suggested by #1111 (comment) |
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).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:
or
Personally, I prefer the second form. IMO, it is easily extensible for any additional passwords that need to be convey.
The text was updated successfully, but these errors were encountered: