Security Issues

[ghost]

Hello!

On a security expert's blog (Felix von Leitner), there is the following "bug of the day" post on security issues in Seafile:

http://blog.fefe.de/?ts=adb7eb89

This seems to affect seafile/common/seafile-crypt.c

Have you realized this already? Wouldn't it be better to use stuff like bcrypt, PBKDF2 or scrypt, as he suggests?

[qgi]

For those not speaking German, here's a short summary of the post mentioned above:

• Each password should be salted with an individual salt to require attackers using a dictionary attack to hash every entry in the dictionary for each individual user/salt.
• If salts are used this way, first the salt should be hashed, then the password. This requires attackers to hash every password with every salt instead of hashing the password once and then just applying the salts
• Hashing is relativly cheap these days, so several passes are adviced
• Use bcrypt, PBKDF2 or scrypt (scrypt is designed to make cracking with ASICs as hard as possible)

[killing]

Thank you for pointing out. After reading more on the web, it's necessary to do 2 things to strengthen Seafile's password security:

1. Use different salt for each user
2. Use PBKDF2 for hashing

2014-04-12 21:16 GMT+08:00 qgi notifications@github.com:

For those not speaking German, here's a short summary of the post mentioned above:

• Each password should be salted with an individual salt to require attackers using a dictionary attack to hash every entry in the dictionary for each individual user/salt.
• If salts are used this way, first the salt should be hashed, then the password. This requires attackers to hash every password with every salt instead of hashing the password once and then just applying the salts
• Hashing is relativly cheap these days, so several passes are adviced
• Use bcrypt, PBKDF2 or scrypt (scrypt is designed to make cracking with ASICs as hard as possible)

— Reply to this email directly or view it on GitHubhttps://github.com/haiwen/seafile/issues/587#issuecomment-40280108 .

[afics]

https://github.com/haiwen/ccnet/issues/35

[peterheinrich]

Yeah, I did post it to Felix in the first place but opened a new issue on the ccnet site here on github before (reference by the previous comment). Everything else would have been pretty irresponsible.

I realised this bug after reading the source code in order to batch-create 20 users as I couldn't find any information on the password hashes elsewhere.

However, I encourage you all to deeply look into the whole code as some "bad smells" (security wise) are already present: 1) Roll you own crypto 2) Presumably low level of experience in that field (at least regarding salted hashes) 3) Presumably problematic development process with no peer review happening. (Such a bug would have been caught by ANY reasonably experienced programmer having a second look) 4) Relatively large, security relevant codebase, challenging to audit: Several sub systems, different languages, multiple ports and protocols etc. etc.

Remember, this project is very sensitive. It reaches out to directly protect and manage user's private data. It nearly doesn't get more sensitive than that!

So yes, maybe the rest of the code is just fine as is but have a closer look, please (at least I will do so). The last thing the world needs right now is another system not protecting passwords (or data) … therefore let us all help and read the code!

Many people, like me, have a strong demand in a secure alternative to the well known cloud storage providers no one can trust. Let's keep it like this, as the project is on a good way in many other aspects.

Best, Pete

[peterheinrich]

Could someone please have a second look into handling of SQL parameters in ccnet? Looks like SQL-injection is also possible… Hopefully I'm wrong this time!

Same file as the salted-PW bug: net/server/user-mgr.c

snprintf (sql, sizeof(sql), "SELECT passwd FROM EmailUser WHERE email='%s'", email);

[killing]

@peterheinrich SQL injection is not possible since all incomming user names have been filtered by Seahub. With regard to other security problems:

• We don't roll our own crypto. We use Openssl library. Yes we're not security experts. We just learn more in the development process. So some early code may be using Openssl functions in a non-standard way. So please tell us when you find other bugs.
• There are two parts of the code base in which "we roll our own crypto": the transfer protocl and encrypted library. You can learn more about them on https://seacloud.cc/group/3/wiki/faq-for-security-features/ I should admit that the transfer protocol is not as secure as TLS and we're going to implement a http(s) based protocol.

[peterheinrich]

Seriously? Do I now have to write an exploit to convince you? Look, ccnet opens a local port on the machine (8000) where the RPC-Interface listens. Every local user can connect to it. Why should't it then be exploitable?

It is so bad practice to rely on the caller of your lib to provide security measures. If it might not be a problem right now (haven't checked yet) but an accident is likely to happen soon. Immagine some one adding a new feature here and using your lib ... Therefore either escape really careful (AT THE TIME CONSTRUCTING THE SQL) or USE PREPARED STATEMENT !!!

Regarding your last post: There never is or was a good reason to 'roll your own'-Crypto. And the two parts where you admit to do are arguably the most sensitive ones.

When you say from yourself that you are no security experts then I am really worried by now!

Look on sea file.com: with advanced features on file syncing, privacy protection and teamwork! Trusted by thousands of teams, companies and organizations worldwide.

ARE YOU SURE THAT YOU ARE UP TO SUCH A PROJECT?

[killing]

I just admit that designing our own transfer protocol is not secure enough. But it's necessary to design the security mechanism for encrypted libraries. There is no standard like TLS we can rely on directly. And all the encryption function we use are from OpenSSL. We don't reinvent our own crypto. Which is what I think you misunderstand.

Yes you're right for more security we should escape or check all incoming strings.

We're surely up to the goal we set! I think what we should discuss here is how to make Seafile better. Claiming that we're not capable of doing this can't help anyone, right? And I don't buy your opinion that only security expert can write secure program. As long as we fix existing bugs and follow standard ways that should not be a problem.

[peterheinrich]

No, by roll your own crypto I meant it exactly as you are applying it: Use some hash function here, do some cypher there so on and so on. It is not about reimplementing SHA256 on your own, it is about bolting together something not understanding the underlying concepts in depth. The result is exactly what the initial bug has shown!

Regarding the SQL injection: It's not about 'more security' (if such a concept exist) it is about 'not having security at all' under certain circumstances. "If your server itself is compromised, the attacker can do a lot more things than SQL injection. Right?" This statement is so utterly ignorant! Of course it matters! Why should a local user be able to steal all files by dropping an sql-injection? It's like the question, why not every user is root on the system by default!?

I didn't claim that you are not capable, I asked you. The world has just seen (heartbleed) how hard it is to write secure software. Of course non-experts can write secure software but the less experienced on is in that field, the more likely it becomes to make mistakes …

As I said, there is a huge interest in a secure and reliable alternative to cloud providers. So please behave responsible!

[killing]

About the encrypted library mechanism, we wrote about how we do that and the source code is open. If you have read the design doc, you should find that we're not so ignorant about cryptography. And if we have any design flaw in it, we're open for any advice and will fix it soon.

I want to thank you for your time and advice. We'll make Seafile more secure.

[tomglindmeier]

Thats an interesting discussion that will hopefully make Seafile more secure.

I want to use Seafile on a public root server hosted at a provider. To make my data as secure as possible I will wait for https sync and use client side encryption only. Is the client side encryption part of Seafile considered secure yet?

[killing]

Library encryption mechanism also use a static salt for password hashing. This is also not secure enough and makes dictionary attack easier. Unlike user password, library encryption uses PBKDF2 for hash, it's more secure. But this is harder to attack since the attacker must be able to access the data locally on the server. So it'll be fixed in 3.0.x release. Anyway, you have to wait.

[killing]

The password hash bug and potential SQL injection bug have been fixed. See haiwen/ccnet#35