realm / realm-core

Core database component for the Realm Mobile Database SDKs
https://realm.io
Apache License 2.0
1.02k stars 164 forks source link

Security vulnerability in AESCryptor #2899

Closed nacho4d closed 7 years ago

nacho4d commented 7 years ago

I used this Application Security Analyzer to check security vulnerabilites in an app for a client.

The report says there is a potential vulnerability (medium level) that could facilitates replay attacks and cryptanalytic attacks at the moment of encrypting keys.

The report suggests the iv (initial vector) parameter in the CCCryptorCreate function should not be 0. It should be randomized to avoid such attacks. See encrypted_file_mapping.cpp

Goal:

Have zero security vulnerabilities in Realm. Can we have iv randomized ?

Expected results:

iv is randomized resulting in no security issues

Actual results:

iv is hardcoded as 0.

Steps to reproduce:

Just use encryption for a realm in an app Upload the app to the analyzer (link above - I don't mean to advertise any product). Wait for results, it takes up to some hours

Code sample that highlights the issue

See encrypted_file_mapping.cpp Lines 148 & 149

I will try to make a fix for this asap but I am afraid I am not sure realm-cocoa is using my realm-core binary (for testing purposes). How can I check this?

(FWIW, I also checked other documentation and Jonathan Zdziarski suggests the same in his book "Hacking and Securing iOS Applications". If needed. download example code and see ch10/stateful_crypt.m)

finnschiermer commented 7 years ago

Thx.

@tgoyne @rrrlasse

bdash commented 7 years ago

We don't set an IV when creating the cryptor, we instead set one explicitly before each encryption / decryption operation: https://github.com/realm/realm-core/blob/22475fef9321201abb5cfee4f5d68d28359dcd8a/src/realm/util/encrypted_file_mapping.cpp#L321-L322

This looks like a false-positive from the analysis tool to me.

nacho4d commented 7 years ago

@bdash Thank you for clarifying this.

As you might already know I am far from being an expert in security but I am trying to understand ...

I see that CCCryptorReset uses a different iv everytime. This iv comes originally from iv_table& iv = get_iv_table(fd, pos); encrypted_file_mapping.cpp#L289 and iv is incremented every time a write happens. So it is random, not hardcoded :)

Is this correct?

rrrlasse commented 7 years ago

@nacho4d That's correct. It's also not a requirement that the initialization vectors are random, they should just all be distinct - which they are.

rrrlasse commented 7 years ago

@nacho4d We've contacted Bluemix about the false positive.

In the meanwhile, do you know if the security analyzer is only analyzing the source code, or if it's also observing API calls at runtime?

Because if it's only analyzing source code, it might be sufficient to trick it by passing it a null pointer through a named variable that contains null, instead of the literal 0.

nacho4d commented 7 years ago

I think it statically analyses calls from the binary. I never provided the source original code to the analyser.

rrrlasse commented 7 years ago

superseded by https://github.com/realm/realm-core/pull/2915

youn-byungchul commented 5 years ago

hi As i read this issue, i got A question i know when realmDB encrypt Data, they bring IV(initialization Vector) from get_iv_tabe function in AESCryptor::write. However when i looked into that function(get_iv_table), it just give uninitialized IV.. is it ture?? what it is true, i guess realm have big vulnerability about padding Oracle Attack.

ironage commented 5 years ago

Hi @youn-byungchul, thanks for your concern. Your question is about C++ construction of new elements in a vector. In this case, new elements are dynamically added by std::vector::resize() which means that they are default initialized. This means that they do not contain uninitialized memory, but always begin at zero. Therefore there is no padding Oracle Attack as you suggested, because the iv's cannot be manipulated through uninitialized memory.