In April 2021, while researching how to develop an end-2-end encryption plugin for OnlyOffice web using the Seald E2EE SDK, we reviewed the source code of the plugin developed for OnlyOffice Desktop and found some weaknesses in the way the E2EE was implemented.

Disclaimer: This review was by no means a full code review nor a formal security audit. Some weaknesses may have not been found during this review, and the recommendations may not be perfect.

Summary: We found multiple critical flaws in the E2EE implementation of the latest version at the time of the audit (in April 2021), which was 6.3.0:

  • the PRNG was unsafe,
  • the password derivation was unsalted,
  • the symmetric encryption was unauthenticated and re-used IVs,
  • and there was a potential MITM if there's an active attack on the server.

Disclosure policy: the disclosure policy of the Google Project Zero (which is to us a reference in the field) is to disclose publicly after a maximum of 120 days after the initial report was made. In this case, the initial report was made 433 days ago, and therefore we decided to publish this report publicly.

History

  • 2021/04/23: We contacted multiple members of the development team at OnlyOffice to have further details regarding the Private Rooms feature, which implements end-2-end encryption with a plugin.
  • 2021/04/26: a marketing manager at OnlyOffice sent me the GitHub link to the source code of the E2EE plugin.
  • 2021/04/27:  we contacted this person back via email to warn him we wanted to send this report. This person forwarded this request to the developer in charge of the development of the E2EE plugin. This developer asked the internal security team to give us instructions on how to send the report.
  • 2021/04/30: we sent the report via a secure channel established with the security team of OnlyOffice with the contact information provided here.
  • 2021/05/03: we received confirmation that the report had been received and fixes were prioritized
  • 2021/06/23: we received confirmation that in v6.3.1 the RNG was changed to a more secure RNG, and that AES-CBC was changed to AES-GCM. We haven't reviewed their modifications, and they assured us that the other issues would be addressed in a further release of the DocumentServer, which we haven't checked.
  • 2022/07/07: Since the disclosure deadline of the Google Project Zero is of a maximum of 120 days, and it has been more than a year since we made the initial report, it seems reasonable to publish this report publicly today.

Findings

The findings are sorted by type of operation rather than criticity.

They all relate to v6.3.0 which was the latest when writing this report (in April 2021).

Symmetric encryption

Pseudo-random number generator

The code OnlyOffice used to, in fine, generate a password is the following:

window.AscCrypto.CryptoWorker.createPasswordNew = function() {
  function s4() {
    return Math.floor((1 + Math.random()) * 0x10000).toString(16).substring(1);
  }
  return s4() + s4() + '-' + s4() + '-' + s4() + '-' + s4() + '-' + s4() + s4() + s4();
}

This uses as a source of random the function Math.random() which is not cryptographically secure.

There have been exploits in the past of weaknesses with this function as exposed here, the version of V8 used is more resilient to this kind of exploits, but this is still not cryptographically secure as explained on the V8 blog: https://v8.dev/blog/math-random as it uses xorshift128+.

This issue is CRITICAL as it could lead an attacker to guessing what encryption keys are used.

We had recommended to either:

  • using webcrypto APIs if Chromium Embedded Framework allows to do so, or;
  • using node-forge which has a JS fallback for generating a safer random, or;
  • using Cpp bindings to OpenSSL secure PRNG just like it is done for the rest of the primitives.

Password derivation

When initializing the AES "session", OnlyOffice called CryptoAES_Init with one argument password from worker.js, this is defined here:

else if (name == "CryptoAES_Init")
{
    std::string sPassword = arguments[0]->GetStringValue().ToString();
    std::string sSalt = "";
    if (arguments.size() > 1)
        sSalt = arguments[0]->GetStringValue().ToString();
    if (NULL != m_pAES_KeyIv)
        NSOpenSSL::openssl_free(m_pAES_KeyIv);
    m_pAES_KeyIv = NSOpenSSL::PBKDF2_desktop(sPassword, sSalt);
    return true;
}

This function has two issues:

  • the first one is that the sSalt is never given (as is it always called with one argument only);
  • the second one is that if it were called with a second argument, it wouldn't be used: sSalt is initialized with arguments[0] instead of arguments[1], so PBKDF2_desktop would be called with sPassword as both the sPassword and the sSalt.

PBKDF2_desktop is defined here:

unsigned char* PBKDF2_desktop(const std::string& pass, const std::string& salt)
{
    unsigned char* key_iv = NULL;
    if (salt.empty())
    {
        unsigned int pass_salt_len = 0;
        unsigned char* pass_salt = NSOpenSSL::GetHash((unsigned char*)pass.c_str(), (unsigned int)pass.length(), OPENSSL_HASH_ALG_SHA512, pass_salt_len);
        key_iv = PBKDF2(pass.c_str(), (int)pass.length(), pass_salt, pass_salt_len, OPENSSL_HASH_ALG_SHA256, 32 + 16);
        openssl_free(pass_salt);
    }
    else
    {
        key_iv = PBKDF2(pass.c_str(), (int)pass.length(), (const unsigned char*)salt.c_str(), (unsigned int)salt.length(), OPENSSL_HASH_ALG_SHA256, 32 + 16);
    }
    return key_iv;
}

Because the salt is empty in this codepath, OnlyOffice derives the salt out of the password itself using a SHA512.

This leads to having always the same key for a given password, which is in itself a bad practice.

This issue is MINOR, as this would only be problematic if it were an actual "password" which could be re-used elsewhere.

We had recommended generating a random salt and storing it in the document info (where the encrypted symmetric keys are stored), and retrieving it using the readPassword which already parses the docInfo.

Encryption / decryption

The encryption was done via worker.js that calls CryptoAES_Encrypt, which calls Cpp code which calls AES_Encrypt_desktop defined elsewhere:

bool AES_Encrypt_desktop(const unsigned char* key_iv, const std::string& input, std::string& output)
{
    unsigned char* data_crypt = NULL;
    unsigned int data_crypt_len = 0;
    bool bRes = AES_Encrypt(OPENSSL_AES_256_CBC, key_iv, key_iv + 32, (unsigned char*)input.c_str(), (unsigned int)input.length(), data_crypt, data_crypt_len);
    if (!bRes)
        return false;
    output = Serialize(data_crypt, data_crypt_len, OPENSSL_SERIALIZE_TYPE_BASE64);
    openssl_free(data_crypt);
    return true;
}

This has two major issues :

1/ The IV is reused across encryption operations:

The IV comes from the derivation of the password (with the PBKDF2 described in the above paragraph), which always gives the same IV for a given password.

This issue is CRITICAL as the CBC mode is used, which leaks some information when re-using the IV with the same key on similar clearTexts.

We had recommended generating a random 16 bytes IV for each encryption operation, and prepending the cipherText with the IV, instead of deriving the IV from the password.

2/ No authentication of the encryption:

AES-CBC is not an authenticated symmetric encryption scheme (like ChaCha20 or AES-GCM are) which means an attacker could inject in the cipherText malicious blocks of encrypted data, which would be decrypted upon decryption without any warning. This has led (along with other vulnerabilities) to efail.de attacks, which manage to exfiltrate encrypted data by injecting CBC gadgets that are in turn decrypted into a payload that exploits XSS vulnerabilities in the client that renders the decrypted data. This seems far-fetched, but there have been exploits in the wild of this.

This issue is CRITICAL, as an attacker could alter an encrypted document in such a way that when a user decrypts it, the attacker would be able to inject a payload at the beginning of the document, which could be the basis of a multi-staged exploit.
We had recommended doing either:

  • switch to another algorithm such as ChaCha20 or AES-GCM, or;
  • implement a MAC (such as HMAC-SHA256) on the {IV + cipherText}, calculate this MAC with another key, append the result after the cipherText, and check the MAC upon decryption. If the MAC verification fails, it means the cipherText has been corrupted, possibly by an attacker. To generate the other key, we would recommend deriving a 64 bytes instead of a 32 bytes key (assuming the IV is no longer derived from the password), using the first 32 bytes as the AES key, and the last 32 bytes as the MAC key.

The underlying function that actually executes the AES encryption AES_Encrypt is odd:

    bool AES_Encrypt(int type, const unsigned char* key, const unsigned char* iv, const unsigned char* data, const unsigned int& size, unsigned char*& data_crypt, unsigned int& data_crypt_len)
    {
        EVP_CIPHER_CTX* ctx = EVP_CIPHER_CTX_new();
        EVP_CIPHER_CTX_init(ctx);
        EVP_EncryptInit_ex(ctx, _get_cipher_aes(type), NULL, key, iv);
        EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_IVLEN, 16, NULL);
        int out_len1 = (int)size + AES_BLOCK_SIZE;
        int out_len2 = 0;
        data_crypt = openssl_alloc(out_len1);
        EVP_EncryptUpdate(ctx, data_crypt, &out_len1, data, (int)size);
        EVP_EncryptFinal_ex(ctx, data_crypt + out_len1, &out_len2);
        data_crypt_len = out_len1 + out_len2;
        EVP_CIPHER_CTX_free(ctx);
        EVP_cleanup();
        return true;
    }

It instantiates a Cipher context with AES-CBC and then uses a function to set the IV length with a constant specific to another mode of encryption EVP_CTRL_GCM_SET_IVLEN (specific to AES-GCM), we really don't know what is the effect of this function call, and that's never a good thing when doing cryptography.

This issue is needs more investigation, as we are unable to determine the potential side effects of such a function call.

Asymmetric encryption

Key generation

OnlyOffice uses RSA_generate_multi_prime_key with a primes argument at 2 which is the default behavior of RSA_generate_key_ex.

This is not an issue, just an oddity.

Encryption / Decryption

If the USE_DEPRECATED code path is used, it uses RSA_NO_PADDING which would be a CRITICAL issue, but it has apparently already been spotted.

The other code path that uses RSA_PKCS1_OAEP_PADDING is ok (even though Victor Shoup proposed OAEP+ that is even more "optimal" in 2001, it has never been implemented because the attacks against OAEP are only theoretical at this point).

Private key protection

The private key seems to be protected using the same symmetric encryption as above, which has the same flaws (which are CRITICAL), using a password m_sPassword which comes from tmpInfo (we found this here).

On this topic, we are not familiar with the mechanism used to pass the password to this context (using tmpInfo), we are unsure this is correctly handled.

Public key retrieval

The public keys come from a 'getsharingkeys' command on the cloudCryptoCommandMainFrame which in turn seem to trigger getEncryptionAccess here, which in turn seems to request to the server the keys of the privacyRoom:

  return request({
    method: "get",
    url: `privacyroom/access/${fileId}`,
    data: fileId,
  });

There is no mechanism that I've seen that prevents the OnlyOffice server from introducing a new participant to the room. This is a literal backdoor : the server could become malicious to modify the keys in this array with a new public key, for which it has the private key, nearly silently (it could decrypt / reencrypt for the other participants on-the-fly to keep the same number of participants).

This issue is MAJOR as this can lead to a literal backdoor if the servers are actively hacked.

The strategy around this issue would be to sign the list of the participants (metadata & public keys) in such a way that the server cannot lie, or at least that it cannot lie once the participants have verified a client-side generated hash of the participants list via another channel at one point in the life-cycle of the privacyRoom (this is called the Trust On First Use paradigm, or TOFU). If signatures are introduced, separate key pairs will be needed, as one cannot sign and encrypt with the same key pair.

Conclusion

This report is based on a partial code review only, we may have misunderstood some of the codebase, the findings need to be confirmed by the development team of OnlyOffice. The recommendations we made may have themselves flaws we are not aware of, their robustness needs to be confirmed as well before implementing them.