search

UWB-Biocomputing / WorkBench

Software and data provenance management platform for simulations of dissociated cortical cultures.
https://uwb-biocomputing.github.io/WorkBench/
Apache License 2.0
1 stars 3 forks source link

Remote execution has serious security problems #45

Closed stiber closed 4 years ago

stiber commented 4 years ago

Remote execution requires the user's password for the remote Linux machine. Right now, I believe that this is stored in plain text, so that the simulation scripts can access it to use ssh to execute commands on the remote machine. Moreover, merely when the Workbench is running, this is likely stored as plain text in a variable in memory, which is also problematic.

jConquest commented 4 years ago

Hey @karthikvetrivel, if you get the time can you investigate where these files are being generated and stored in the project

karthikvetrivel commented 4 years ago

Looking through the code, I didn't find that username and password were being stored in any text-file. However, the username is temporarily held in a String and the password as a character array (so yes, they're held as plain text in memory).

Below is the testConnection method from ScriptSpecificationDialog.java, which is where the remote execution happens. carbon

As you can see, all data is temporarily held in variables, but the password one seems to be wiped with 0s at the end of the method. So, it doesn't seem very problematic. I looked into the SecureFileTransfer class too, just to make sure nothing was being stored in a file.

carbon (1)

This is the testConnection method from SecureFileTransfer.java (stf.testConnection), which tests the availability of an SFTP connection with a remote host.JSch, which is used here, is a java implementation of SSH2 and provides support for secure remote login. As it seems, it doesn't seem like any file is being generated or saved, but I may be overlooking something.

Let me know if I missed something!

karthikvetrivel commented 4 years ago

UPDATE: In ScriptManager.java, where I assume most of the ssh scripts a running from, in order to access the password and username, runRemoteScript calls a class called LoginCredentialsDialog, which manages the login process without saving a file.

carbon (2) (a snippet from runRemoteScript() which calls LoginCredentialsDIalog)

I do, however, see that ScriptManager.java imports FileReader, but I don't see that being related to authentication...yet.

davisdb1 commented 4 years ago

I couldn't find a way to avoid using the String parameter in any code that calls setSession, at the time, and it is a problem in Java. Storing sensitive information in a character array, you can simply write over it after the session info is set but a string is immutable and can't be written over. It will remain until it is garbage collected and will persist in memory after the process exits until it is overwritten by another process. I think it prompts every time a connection is made (if not, it should). However, you should try to use the setPassword that takes a byte[] and then zero it out before returning from whatever method set the password (internally it's a byte array): https://epaul.github.io/jsch-documentation/javadoc/com/jcraft/jsch/Session.html#setPassword-byte:A-

On Mon, Jul 27, 2020 at 3:42 PM Karthik notifications@github.com wrote:

UPDATE: This is what I found initially- I'll continue looking into it because there is still some other code with the password could be used.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/UWB-Biocomputing/WorkBench/issues/45#issuecomment-664675173, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIFGQNTUL5VUFP7NAJCIZ3R5X7FTANCNFSM4O2YMXBQ .

jConquest commented 4 years ago

I agree with Del. Strings must be avoided in Java for sensitive information, and using char or byte array is the safest method, making sure to zero out the array immediately after use. I see from this code that it is implemented as so, and does not need updating.

Karthik, you have not missed any file creation, so we can rest assured this is not happening. Let's double check FileReader and the script execution when using remote machines to make doubly sure.

jConquest commented 4 years ago

There is a text file that is created and not deleted which stores the SHA1 key for remote log in. SHA1 is depreciating in its security robustness for well organized attacks. It would be best to at least delete this file after it's use is done. This can be added as an additional option to the script generator to enable deletion of this file if remote server is used

davisdb1 commented 4 years ago

In this case, the SHA1 key only corresponds to the git commit number used. Perhaps there is more to the security issue, but I thought I would point out that removing the SHA1 commit key will also remove the software provenance for the remote build/execution step.

On Tue, Aug 4, 2020 at 9:16 AM Joseph Conquest notifications@github.com wrote:

There is a text file that is created and not deleted which stores the SHA1 key for remote log in. SHA1 is depreciating in its security robustness for well organized attacks. It would be best to at least delete this file after it's use is done. This can be added as an additional option to the script generator to enable deletion of this file if remote server is used

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/UWB-Biocomputing/WorkBench/issues/45#issuecomment-668690731, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABIFGQJEYTDIHGUSBKO53SDR7AX6XANCNFSM4O2YMXBQ .