3 ms·

That was actually the part where they partially mitigated the vulnerability with. So before it was more insecure. The green one is the "fix" :) The constant ti

That was actually the part where they partially mitigated the vulnerability with. So before it was more insecure. The green one is the "fix" :) The constant time comparison is pretty irrelevant here. Check https://github.com/haiwen/seahub/commit/7cdb70368aa7acbf05461e22e83278b00c2036c8#diff-31b2676ed825badbf63e5be98fb54c25R68 https://github.com/haiwen/seahub/commit/7cdb70368aa7acbf0546..., basically a valid password reset token in default Django requires an attacker to know: - Primary Key of the user (User ID) - Hashed version of the user password - Timestamp of the last login (1 second accuracy) - Number of days since 2001-1-1 converted to base 36 - The configured SECRET_KEY Basically except the configured SECRET_KEY and the hashed version of the password everything is known to an attacker. And even if an attacker knows the SECRET_KEY they would not be able to generate a valid token as the old password hash is required. However, Seafile has removed the hashed password out of their version of "tokens.py". This means that if somebody knows the SECRET_KEY they could create valid password reset tokens. That the initial SECRET_KEY was basically generated by `str(random.randint(0,100000))` did make this all much more worse… (https://github.com/haiwen/seahub/blob/b6f8935c0f355cc70145f9c2706cc99512e39ac2/tools/secret_key_generator.py https://github.com/haiwen/seahub/blob/b6f8935c0f355cc70145f9...) Bad thing about this is that this basically makes a lot of old Seafile instances insecure unless they get a new SECRET_KEY. They did somewhat tell users to regenerate it within a forum post and some wiki page (https://seacloud.cc/group/3/wiki/whats-new-in-the-next-version-4-3/ https://seacloud.cc/group/3/wiki/whats-new-in-the-next-versi...) but that's not really a good way to spread awareness…

gingerlime 11y ago
Thanks for taking the time to explain... Sorry to nitpick, but I'm trying to get things clear. your original comment pointing to the commit said > And SeaFile has additionally modified the default Django components making it more insecure So I was under the impression that this commit was making things worse, not better. Thanks for clarifying that this was already fixed in this commit, rather than introduce a vulnerability. It looks like they added the hashed password on the diff to mitigate this issue - unless I am missing something here? Can you explain why they "partially mitigated the vulnerability" here? is there still some gap that they've missed? I'm not underestimating the vulnerability, or the fact that it was there in the first place. Just trying to get a full(er) picture. I've had a read through this, and some other places where insecurity of Seafile is exposed/discussed/fixed. I agree that the overall impression I'm getting is that they're not security experts, and security could have definitely been better designed or taken into account in the first place. I still think they're trying to do their best, and fix problems and improve. I hope for their and their users' sake that they can get some security-dedicated contributors (mostly to prevent bad things in the first place rather than fixing them later on). There's always more that can be done, but I'd like to give them the benefit of the doubt here and have faith that things will be better in the long run. Having a (robustly) secure, open-source dropbox replacement is a good thing. I think that's their aim, even if they are some times misguided. disclaimed: I'm not even a user of Seafile, let alone know much about its codebase. It's the first time I hear about it. I was just curious about the security aspects of their product in light of some criticism here and on github etc.

gingerlime 11y ago

Thanks for taking the time to explain... Sorry to nitpick, but I'm trying to get things clear. your original comment pointing to the commit said > And SeaFile has additionally modified the default Django components making it more insecure So I was under the impression that this commit was making things worse, not better. Thanks for clarifying that this was already fixed in this commit, rather than introduce a vulnerability. It looks like they added the hashed password on the diff to mitigate this issue - unless I am missing something here? Can you explain why they "partially mitigated the vulnerability" here? is there still some gap that they've missed? I'm not underestimating the vulnerability, or the fact that it was there in the first place. Just trying to get a full(er) picture. I've had a read through this, and some other places where insecurity of Seafile is exposed/discussed/fixed. I agree that the overall impression I'm getting is that they're not security experts, and security could have definitely been better designed or taken into account in the first place. I still think they're trying to do their best, and fix problems and improve. I hope for their and their users' sake that they can get some security-dedicated contributors (mostly to prevent bad things in the first place rather than fixing them later on). There's always more that can be done, but I'd like to give them the benefit of the doubt here and have faith that things will be better in the long run. Having a (robustly) secure, open-source dropbox replacement is a good thing. I think that's their aim, even if they are some times misguided. disclaimed: I'm not even a user of Seafile, let alone know much about its codebase. It's the first time I hear about it. I was just curious about the security aspects of their product in light of some criticism here and on github etc.