mwiede / jsch

fork of the popular jsch library
Other
708 stars 130 forks source link

coverity SAST anaylsis #148

Open bhav1234 opened 2 years ago

bhav1234 commented 2 years ago

There are approx. 150 to 180 vulnerabilities reported by coverity SAST tool from low to high risk. Some high risks are following: 1.Cleartext transmission of sensitive data (SENSITIVE_DATA_LEAK) in session.connect(). 2.Very weak password hashing (WEAK_PASSWORD_HASH),weak_hash_no_salt: Hashing a password using a computationally cheap cryptographic hash function and no salt. If password hashes leak to an attacker, they can use readily-available hash lookup tables to recover large numbers of passwords with little effort. in byte[] hpass = sha512.digest(password); in BCrypt.java

  1. Bad choice of lock object (BAD_LOCK_OBJECT). lock_on_assigned_field: Locking on the object referenced by field known_hosts. This lock acquisition may race with another thread assigning to this field. The contents of known_hosts may change while a thread is inside the critical section, allowing two threads to enter the critical section simultaneously. in line , in JSCH.java synchronized(known_hosts){ ((KnownHosts)known_hosts).setKnownHosts(stream); } 4.Check of thread-shared field evades lock acquisition (LOCK_EVASION). thread1_overwrites_value_in_field: Thread1 sets salt to a new value. Now the two threads have an inconsistent view of salt and updates to fields of salt or fields correlated with salt may be lost. in line salt=new byte[macsha1.getBlockSize()]; in JSCH.java
  2. Bad choice of lock object (BAD_LOCK_OBJECT). lock_on_assigned_field: Locking on the object referenced by field thread. This lock acquisition may race with another thread assigning to this field. The contents of thread may change while a thread is inside the critical section, allowing two threads to enter the critical section simultaneously. synchronized(_thread){ _thread.notifyAll(); } in channelsession.java
  3. Bad choice of lock object (BAD_LOCK_OBJECT). lock_on_assigned_field: Locking on the object referenced by field proxy. This lock acquisition may race with another thread assigning to this field. The contents of proxy may change while a thread is inside the critical section, allowing two threads to enter the critical section simultaneously. Instead of using proxy as a lock, create a final field of type Object which is only used as a lock. synchronized(proxy){ proxy.close(); } in session.java Please provide your suggestions
norrisjeremy commented 2 years ago

Hi @bhav1234,

I believe Coverity SAST is a commercial offering, so I'm not sure how we could review and enact changes to address the 150 to 180 vulnerabilities that you claim it is reporting?

Thanks, Jeremy

norrisjeremy commented 2 years ago

@mwiede It looks like Coverity offers a free service for open source projects here: https://scan.coverity.com/

Do you want to sign JSch up for this so we can see what it reports?

mwiede commented 2 years ago

@norrisjeremy the project is setup at https://scan.coverity.com/projects/mwiede-jsch?tab=overview Unfortunately the results are not public but it is possible to authenticate using GitHub login.

Please click "add me to the project" so you can see the defects.

norrisjeremy commented 2 years ago

@mwiede Ok, thanks! I'm waiting on them to authorize me.