Fix stack buffer overflow in zxcvbn.

[hannob]

The array PossChars is filled with a 48 byte string plus a trailing zero
byte. Therefore it needs to be 49 bytes long.

This fixes a buffer overflow in the zxcvbn code. The code comes from the zxvcbn-c project, where I have also submitted a pull request:
tsyrogit/zxcvbn-c#11

Apart from the bug itself I find it worrying that a security tool contains such a bug. Such bug classes can be trivially found with address sanitizer, a compiler feature in gcc/clang (just do something like "CFLAGS="-fsanitize=address -g" CXXFLAGS="-fsanitize=address -g" cmake .; make" and try to run keepassxc). Given that this bug is present means apparently that nobody ever tested the keepassxc code with address sanitizer. I'd consider that very basic quality assurance that especially security-related projects should do by default.

[phoerious]

@Could you rebase that patch against release/2.1.3, please?

And you are right, we should enable address sanitation by default. We don't use any raw char arrays in our own code, but considering that some third-party code does, it is probably a good idea.

The bug is only critical when the DB is already unlocked and the user is editing a password, though.

[hannob]

Hope I did this the right way (my git skills are... limited).

Also to clarify, I also don't think this is a severe issue. But it's a symptom. ASAN-testing is a really easy way to spot some nasty C bugs.

(And also: It's generally not a good idea to enable address sanitizer by default - it's a debugging / testing-tool and shouldn't be used in production)

[phoerious]

(And also: It's generally not a good idea to enable address sanitizer by default - it's a debugging / testing-tool and shouldn't be used in production)

Of course not. This would only be enabled for debug builds. However, the problem we are having with ASAN is that without ASAN_OPTIONS=detect_odr_violation=0 set, KeePassXC won't run at all. The Q_DECL_EXPORT macro in our AutoType library generates false positives.

[phoerious]

Closing in favor of #365.

[droidmonkey]

@hannob thanks for the tips. Are there any other flags or processes you think would be smart to run or have the option to run at compile time?

We have plans to work with Flaw Finder as a static code analysis tool.

[hannob]

From the asan doc it looks to me that detect_odr_violation is off by default:
https://github.com/google/sanitizers/wiki/AddressSanitizerOneDefinitionRuleViolation

I also was able to run keepassxc once I fixed the overflow.

There are other sanitizers in gcc/clang, but none of them is as effective and straightforward to use as asan:

• msan finds uninitialized memory leaks, but requires all dependency libraries to be compiled with msan as well (only clang, not ported to gcc)
• ubsan finds undefined behavior, also interesting, but finds a lot of noise (==they are bugs and should be fixed, but likely with no real impact, as most compilers will do what you expect)
• tsan finds concurrency issues, I don't have much experience with it.

Start with asan, add ubsan next, then think about the rest.

[phoerious]

I'm compiling with Clang 3.9.1 and I can't run KeePassXC without explicitly disabling odr violation detection.