based upon Kevinsbranch encrypted key in config.json using cryptojs && start performance fix #5465
Conversation
oops forget some small issue. removed the userpasswordprompt.js from code (which I never use in the actual PR)
added information for the user for the 20 chars minimum ``` if (password === null || password === undefined) return 'data.error.nopwentered_or_abort'; if (password.length < 20) return 'data.error.pwshort'; if (password.trim().length < 20) return 'data.error.pwwhitespace'; if (password.length > 128) return 'data.error.pwlong'; return 'data.success'; ```
added information for the user more than 20 chars `` if (password === null || password === undefined) return 'data.error.nopwentered_or_abort'; if (password.length < 20) return 'data.error.pwshort'; if (password.trim().length < 20) return 'data.error.pwwhitespace'; if (password.length > 128) return 'data.error.pwlong'; ```
ksaadDE
changed the title
|
Thanks for opening this PR. We value your time and understand why this is important to you. With features like this, however, we'd appreciate if you could start by requesting them on community forums ( https://community.signalusers.org/c/feature-requests/desktop-feature-requests/20 ) before investing any time into it. Unfortunately we cannot merge your Pull Request at this time, but feel free to keep it open for yours and ours future reference. Some immediate feedback that I could give is that it might not be a good idea to combine refactor changes with the logic changes as it makes it harder for a reviewer to look through the code. Additionally, the multiple database initialization that you mentioned is just awaiting for the same promise so it doesn't happen thrice. Furthermore awaiting this initialization is necessary for the startup logic and migrations to proceed correctly. |
|
First of all Hi! @indutny-signal
We actually have multiple feature requests in the Signal community and on Github!
In my testing it didn't play a big role, since I reordered them and let it do the important stuff in the right order (nothing more or less). The code is now much cleaner. I only moved the consts and lets which were (before) outside of functions to the top in a cleaner order. If you want to see a working example, without pairing between devices and no setup as standlone (no time for it): We all would like to see the feature implemented asap, since the key unencrypted on disk is currently a HIGH risk! Two things wo might bypass any mentioned protection:
Also, I agree it is a bit difficult to differentiate between the big changes, the prompt and AES stuff, but also the rest. But it's possible, I inserted comments at the right places. |
|
Thanks for your hard work. Unfortunately, this pull request has many unrelated things, and we won't be merging it.
I'm going to close this issue, but please email evanhahn@signal.org if you want to help out in a more directed way! |
Well you don't have plans to fix the insecurity in Signal-Desktop? Well, I'm not sure if the Community really likes that.
start.sh wasn't changed, it was added. I used it for dev purposes (don't want to type in everytime the same commands), like stated in the comments inside the file.
Where? The 18 Files did not include compiled JS. Please link them individually.
I've used the Kevinsbranch, because your Captcha Verification doesn't work in the actual Signal-Dev Version. You didn't know that? I bet you knew it before, there's even a issue for it! Btw: Keytar is not suited for that use-case, it would conflict with the currrent implementation of your encrypted SQLiteDB. You're closing a issue the Community requested a lot of times. Without any real reasons, just to underline that. |
All the |
The funny thing is, I didn't work with TypeScript. I guess it was generated when doing the testing process or building process. Like you put compiled into quotes, in my understanding compiling is not the process of generating files with "interesting" code. So I searched or better to say looked out for binary code. I wouldn't call it either minifying or something like that. After seeing the mentioned stuff inside the System Tray Service js and with your hint , I agree to that point. Usually Devs excluding these files in .gitignore. Why it wasn't done here? So that I don't have to take a look into a part where I didn't change anything Still doesn't explain that people confusing Linux and Windows (especially basic skills like .sh files) xD TL;DR: But the rest is still unclear. ^^
Btw2: Don't run in the trap making the mistake to confuse JS and TS. ;) |
The threat model is this: if you have malware on your computer, it has free reign. It could, for example, take screenshots every 5 seconds and steal your data that way. So: encrypting the data on disk, while possible for us to do, would be redundant. That's not to say we'll never address this, but we don't see it as an obvious insecurity.
That's great—please keep this file around for future development on Signal Desktop, but I don't think we'll need it in the source repository.
You're totally right—my mistake. I've edited my original comment.
It's there: Line 29 in 9ada9f6 It's possible that you added these with |
Think about some oldschool drive-by download or webexploit. Lets say the Malware is not able to do a PrivEsc, but it can exfiltrate Due to the complete exfiltration the attacker has nothing to do than just reading the decrypted stuff inside the DB. It's easy, it's fast and powerful. Uh that guy writes about bringing his kids to some school? Well we'll take his kids out of there, "earning" some money. Uh nice passwords and creditcard information...? uh he's ill? good to know! My mitigation allows the user to set a password which encrypts the DB key inside the FDE Keyloggers Screenshots
What ever you want, but I would prefer to have it directly in there. So if I would test under Linux I just run that script an it's done ;)
I went through the history I've never used the Do you still want to prevent a protection against that threat the next 20 years? I really appreciate your offers, but I don't think I will contribute in the future. Why should I waste more time on that? Serious threats getting downplayed, while less important stuff seems to be a priority. Users had the wish to fully encrypt the DB, for years now. Why not making a optional version of Signal, where it's done, like in my code? There's no point to say "no" to that, or do I need to know something? Greetz |
|
Thanks a lot @ksaadDE for providing this pull request! I am also heavily disappointed by the attitude exhibited here and elsewhere by Signal representatives. P.S. Here is the ultimate bug fix: use some other, sincerely secure messenger. |
|
Signal stores the key unencrypted along with the database. is there going to be any changes to this? |
You can read the messages above by the Signal Team. They don't want to change anything to it, because it is in their opinion not important (official opinion of them). People might wonder if that's true. I went silent about this issue, not only because I am a bit speechless, more because I got very interesting E-Mails. Regards |
Problem: as mentioned in issue #1850
dbKey is in desktop-app of signal unencrypted on disk inside the config.json
A attacker with local privileges or system privileges could exfiltrate the dbKey (unencrypted inside config.json), the SQLIteDB and decrypt the content (either remote or locally).
Also, the code was blurry which resulted in higher start time (three times SQLinit sounds not so good)
-> reduced it to one time SQLinit in app.on('ready',...)
Tests:
https://streamable.com/ab5yvj
Tested manually by starting the app, sending && recieving messages and so on in dev env (not tested yet in any builds)
based upon Kevinsbranch since reCaptcha needed for dev
What the PR should do:
Speed up the start time of the Signal-Desktop app;
Should automatically ask the user the first time when started the app which password to choose.
AND should ask on every start what the password is
OR when the password is not set yet, asking for one and encrypting the old DB key.
The key property in config.json is now encrypted with AES && only user knows password.
A new property inside the config.js called "usesPassword" (true|false) is used for differentiating between an encrypted Key and not an encrypted key.
And added start.sh with nvm, making testing easier
And sorted the code salad inside the main.js (see comment at top of main.js)
Reasons not to store a hash inside config.json:
Hash would provide authentication, but the hash can be reverted under circumstances, therefore no hashing at all.
Also, the security is ensured through the encrypted dbKey. Since the issue was only the unencrypted storing on disk.
Dependencies:
Last words:
Thanks! :-)