Back on February 2nd, 2014 Taylor Hornby kindly performed a security audit of ZeroBin "for free as a service to the free/opensource community".
Some of the issues brought up were already fixed in 2014 by Sébastien Sauvage. Before releasing a stable version of the PrivateBin fork, we wanted to make sure to address all remaining issues as best as we could. Here is an overview over each point and the measures taken on them.
2.1. Salt and HMAC Key Generated with mt_rand()
Exploitability: Medium
Security Impact: LowIn serversalt.php, generateRandomSalt() generates a salt using the mt_rand() pseudo-random number generator. Because mt_rand() is not a cryptographic random number generator, this function will return easily-guessed salts with a much higher probability of collision.
The salts generated by this function are relied on for security. For example, it is used as an HMAC key when checking delete tokens in processPasteDelete(). It should be replaced with mcrypt_create_iv() or openssl_random_pseudo_bytes().
Four days after the release of the audit, Seb fixed this by using the mcrypt extension instead, if present. mt_rand was still in use as a fallback until and including version 0.22 of the ZeroBin fork.
For the upcoming release we decided to remove the fallback to mt_rand() completly and switch to PHP 7's random_bytes() instead, providing a couple of fallbacks for older PHP versions by using the random_compat library. With this change, the following sources of cryptographically safe random numbers are used in the following priority:
- PHP 7's
random_bytes() - libsodium
/dev/urandom- mcrypt
- com_dotnet
More information about the order and reasons for this can be found in the official documentation.
We consider this point resolved.
2.2 Fixed Server Salt
Exploitability: Low
Security Impact: LowThe security of the VizHash hashes of IP addresses (used to generate comment avatars) and delete tokens both rely on a fixed "salt" which is generated once per deployment, saved in data/salt.php.
We switched to creating a salt per paste, which is used to create the delete tokens.
Using a paste specific salt for the VizHashes in comments with nicknames would make it impossible to recognize the same IP accross pastes and therefore make the use of VizHashes moot.
We consider this point solved for the delete tokens. Regarding the VizHashes, server admins now have the option to disable icons in the configuration file to avoid this risk and users can simply choose to comment anonymously without nickname.
All in all we consider this point partly resolved. We provided reasonable configuration options for admins wanting to enhance the security of their PrivateBin instance.
2.3. Traffic Limiter Race Conditions
To rate limit requests, ZeroBin keeps a history of hit times in data/traffic_limiter.php, which is generated and re-generated in traffic_limiter_canPass(). Because requests can be made asynchronously, the file may become corrupted.
This might allow remote code execution, if the attacker is clever enough to corrupt the file in just the right way, since the traffic_limiter.php file is require()'d.
Although the patch for this was already sent in at September 28th, 2013, the pull request was only merged a day after the release of the security audit.
We consider this point resolved.
2.4. VizHash IP Address Online Guessing
Exploitability: Very Low
Security Impact: LowThe VizHash system is used to give users who comment an avatar derived from their IP address. If an attacker can create connections to the ZeroBin server from arbitrary source IPs (e.g. if they are in the same LAN as the ZeroBin server, or man-in-the-middling its traffic), they can perform an online guessing attack on the VizHash they are interested in.
As explained above this is an inherant problem of using IP based icons. Apart from the option to disable icons in the configuration file to avoid this risk for server admin and the option for users to simply not comment with a nickname, we also switched to using a SHA512 HMAC instead of the odd combination of MD5, SHA1 and a reversal of these that was used in VizHash originally.
We consider to have reduced the risk of this point and given options to both server admins and users to avoid it completly. We could disable icons by default, to solve it permanently.
2.5. Relies on .htaccess files, which may not be enabled.
Exploitability: N/A
Security Impact: Very LowZeroBin relies on .htaccess files being enabled to prevent access to the 'data' directory. This directory contains the ciphertexts, salt, and rate limiter array. If .htaccess is disabled, of if ZeroBin is installed on a non-Apache web server, then an attacker may be able to access these files.
Back in 2015 we implemented an option that allows server admins to install most sensitive parts of the application outside of the webservers document root. This is explained in the installation documentation. The .htaccess files are still generated to protect lazy server admins that use an Apache webserver.
We consider this point resolved.
2.6. The robots.txt does not work in a subdomain.
Exploitability: N/A
Security Impact: LowZeroBin uses a robots.txt file to prevent search engines from indexing the posts. If you install ZeroBin into a subdirectory, this does not work.
We added a section to the installation documentation to remind server admins to change the file if PrivateBin runs in a subdirectory.
We consider this point resolved.
2.7 HMAC Not Compared in Constant Time
Exploitability: Medium
Security Impact: LowZeroBin uses an HMAC to generate and check delete tokens. The HMAC is compared against the correct one with PHP's "!=" operator. A timing attack can exploit this error to compute the HMAC of arbitrary data with the server's salt.
Seb implemented the suggested slow_equals() function on February 6th, 2014.
We consider this point resolved.
2.8. Arbitrary File Unlink
Exploitability: Medium
Security Impact: HighAn attacker can delete arbitrary files on the server by exploiting a vulnerability in processPasteDelete().
This was addressed in the same cherry picked and merged commit in August 15th, 2015 that mainly addressed 2.7 above. Not only is the paste ID now checked for general format validity (16 hexadecimal characters), but also if that paste actually exists.
We consider this point resolved.
2.9. HMAC Uses SHA1 instead of SHA256
Exploitability: Very Low
Security Impact: LowZeroBin uses a SHA1 HMAC to derive the delete token. SHA1 should be replaced with a better hash function, like SHA256.
This was addressed in a commit on July 6th, 2016. As this is a breaking change (it means that old deletion links will no longer work) it is enabled by default, but for admins upgrading from older installs there is an option zerobincompatibility to return to the old behaviour.
We consider this point resolved.
2.10. No Cross-Site Request Forgery Protection
Exploitability: Medium
Security Impact: MediumZeroBin does not attempt to prevent Cross-Site Request Forgery (CSRF) attacks. A malicious website can make a user's browser delete ZeroBin posts (if the delete token is known), create posts, and post comments to existing posts.
Due to the nature of PrivateBin it is inherently possible to create pastes and comments anonymously. Hence we can't just introduce a user account system or API tokens. As we plan to allow alternative clients to use existing PrivateBin instances adding a CORS or CSP header to disallow external sources to access the API would not work either.
We consider this an ongoing design challenge and still keep an open issue on this point.
3.1. Always Assume Malice
When a string is used in a way that might affect security, it should always be assumed to be malicious, even if it is just a constant string.
All the mentioned examples were escaped and/or reinforced with checks. Unit testing was used to ensure that these filtering functions aren't weakened in some later regression.
This danger also applies for any newly created template. We addressed this with a reminder on escaping in the template creation documentation.
We consider this point resolved.
4.1. Secure Code Delivery
ZeroBin relies on the JavaScript code being delivered securely. A malicious server or man-in-the-middle can modify the code to leak the key. This is already well known and is being addressed in GH17.
The method using a JS snipped to validate hashes of the JS code was never implemented. We did instead implement subresource integrity (SRI) hashes on August 16th, 2016. Combined with content security policy (CSP) headers, this limits the attack surface to man-in-the-middle attacks on the index.html file and rogue server admins.
Server admins can reduce the potential for man-in-the-middle attacks by setting up HTTPS following high standards. We documented this in the main README.md of the project and also added some recommendations on how to set up HTTPS more securely.
For users we provide recommendations how they can find the best servers.
But as long as the encryption logic is delivered by the server, the risk of manipulation is always a possibility. This is not fixable and therefore users have to trust the webserver running PrivateBin. We try to make PrivateBin users aware of this by e.g. stating it in our Readme.
We consider this issue unsolvable within the scope of the project.
4.2. urls2links XSS
In zerobin.js, urls2links() converts a URL text into a clickable link. The replacement is done purely by regular expression, and no escaping is done. This may be a source of XSS vulnerabilities.
Only URLs starting with the schemas http, https, ftp and magnet are converted. Also no characters outside A-Z, a-z, 0-9 and the following ones are allowed: ? = & . \ / - ; # @ ~ % + -
We tried to find better tested implementations, but they ended up being less restrictive on the allowed schemas. So far we could not find a way to inject any executable code, but that is no proof that there isn't.
We consider this a risky piece of code.
4.3. Low Entropy in Browsers Without CSPRNGs
If the web browser does not have window.crypto.getRandomValues(), the key is derived from mouse movements and stuff. That may not be good enough. Perhaps in these cases the server could help the client by supplying some of its own entropy from mcrypt_create_iv() or openssl_random_pseudo_bytes().
We ignored this issue so far, as it affects mostly older platforms.
4.4. Plaintext is Compressed Before Encryption
Posts are compressed before they are encrypted. This makes the ciphertext length depend on the plaintext content. This may create vulnerabilities in specific use cases where an attacker can choose variations of the same post to be encrypted. This would be similar to the CRIME and BREACH attacks, except happening at the application layer.
We don't think this is applicable in our case, as even when generating many pastes with variations of the same content, they would also all get encrypted with different keys. But an issue was created to implement an option that would allow server admins to disable compression.
We ignored this issue so far.
4.5. Is SJCL used correctly?
This audit did not check if zerobin.js uses the SJCL cryptography library correctly.
The implementation follows the examples of the SJCL librarys homepage and its API documentation. We tried to harden its usage further by explicitly passing encryption options on mode, key size and authentication tag size instead of using the defaults.
We consider this point resolved.
4.6. Possible XSS in tpl/page.html
JSON encoded data is inserted into the HTML page unescaped in tpl/page.html. This is because $STATUS is set to the return value of processPasteFetch(), which returns JSON encoded data based on the user's input.
In the current templates, this and the other variables are encoded/escaped before insertion. As mentioned in point 3.1 this needs to be rechecked for every newly created template.
We consider this point resolved.
Summary
We think that what we are left with are two issues that pose larger design challenges or we consider unsolvable in the scope of the project, two issues we ignored as they do not seem to apply to our case or only affect older platforms and for one issue we haven't yet found a better solution short of removing the feature.
All in all we are confident that most points of this audit could be resolved and are ready for a stable release and future audits.