Develop #1888

Closed
wants to merge 4 commits into
from

Projects

None yet

6 participants

TheRook commented Oct 14, 2012

Improvements of Security, Efficiency, Usability, and Compatibility for CI's Session Handler.

Vulnerablities patched with this merge:
CWE - 649 : Reliance on Obfuscation or Encryption of Security-Relevant Inputs without Integrity Checking
 -  AES-CBC  wiouth an message authentication code.   This design allows for  an attacker construction various cryptography oracle  including  a Decryption Oracle and a Padding Oracle
 Cipher text is now  protected with a SHA1 HMAC

CWE-329: Not Using a Random IV with CBC Mode
 - Block Cipher implamentation used a NULL iv
  ecrypt() generates a random iv for each message

CWE-327: Use of a Broken or Risky Cryptographic Algorithm
 - Use of MD5 for integraty checking allowing Hash Length Extension Attacks.  
 Replaced with a proper sha1 hmac. 

 CWE-291: Trusting Self-reported IP Address
 - The ip_address() method can a return user supplied value.  
  Only one variable can be trusted $_SERVER['REMOTE_ADDR']



List of Changes by file:
config.php - was changed for efficiency and security.  Good security options are selected,  and bandwidth consuption has be

AES.php - used as a backup if we cannot find an AES library.

Crypto.php - Renamed from "Encrypt.php",  this now contains useful cryptographic tools such as a solid block cipher,  PRNG, and HMAC.

Rijndael.php - used by AES.php

Session_cookie.php - Modified for the new crypto changes.  Produces a more compact, and more random session id.  Helpful er
TheRook added some commits Oct 13, 2012
@TheRook TheRook Fixed IP address spoofing vulnerablity in Session_cookie.php
$_SERVER['SERVER_NAME'] is the only variable that cannot be completely controlled by an attacker.
0dcaee3
@TheRook TheRook Improvements of Security, Efficiency, Usability, and Compatibility fo…
…r CI's Session Handler.

Vulnerablities patched with this merge:
CWE - 649 : Reliance on Obfuscation or Encryption of Security-Relevant Inputs without Integrity Checking
 -  AES-CBC  wiouth an message authentication code.   This design allows for  an attacker constructino various oracle types,  including a Decryption Oracle as well as Padding Oracle.

CWE-329: Not Using a Random IV with CBC Mode
 - Block Cipher implamentation used a NULL iv

CWE-327: Use of a Broken or Risky Cryptographic Algorithm
 - Use of MD5 for integraty checking allowing Hash Length Extension Attacks.

 CWE-291: Trusting Self-reported IP Address
 - The ip_address() method can a return user supplied value.  There is only one variable that can be trusted,  and that is $_SERVER['REMOTE_ADDR']

List of Changes by file:
config.php - was changed for efficiency and security.  Good security options are selected,  and bandwidth consuption has been reduced.

AES.php - used as a backup if we cannot find an AES library.

Crypto.php - Renamed from "Encrypt.php",  this now contains useful cryptographic tools such as a solid block cipher,  PRNG,  and HMAC.

Rijndael.php - used by AES.php

Session_cookie.php - Modified for the new crypto changes.  Produces a more compact, and more random session id.  Helpful error message that generates a good encryption key for the user.
27d1e61
Contributor
narfbg commented Oct 14, 2012

Sorry dude, but this ain't going in. Not in it's current form.

You can't just "rename" a library - there are docs written about it (which should be updated), stuff depending on it. Even unit tests fail miserably and you haven't even added a changelog entry.

Also, all code in CodeIgniter must be published using the same license, you can't just throw in a library. Ideally, whatever code you submit should be written (or owned) by you.

On the technical part - no PHP4 code and use mcrypt or another library already available when possible.

And please don't change the default settings. They are optimized for convenience, not minimum bandwidth and maximum security.

On what you've reported as CWE-291 - spoofing is only possible from users originating from what is configured as a trusted IP address (list), so while I agree that REMOTE_ADDR is really the only value that can be trusted - it's still arguable on what we should actually use. CodeIgniter tries to allow more control to the developers using them than forcing common practices on them. The proxy_ips setting is a feature that would be rendered almost worthless with your suggested changed.
There indeed was a bug allowing anybody to spoof their IP address, but as I already said on the previous thread - that's already fixed.

I'd suggest submitting these changes as separate pull requests after you consider the above suggestions - that way we don't have to wait on everything to be perfect in order to include separate improvements.

@derekjones, @wesbaker @alexbilbie I guess you guys would want to take a look at this one as well.

On a personal note - don't get me wrong, I admire your efforts and I'll always consider and respect any security-related suggestions. There's just a different way of how we do it in here. How you've handled this (including #1702) is basically like saying "Hey, you guys are wrong and I'm right. Here - use this code that I've got. Bye". This might work for an individual, but it just doesn't work in a community environment.

TheRook added some commits Oct 14, 2012
@TheRook TheRook I forgot the most important part! Enabling security flags on the cookie!
  Use the cookie_secure flag to prevent OWASP A9 violations when HTTPS is aviable
  Enable the httponly flag to prevent an attacker from obtaining the cookie with XSS.
804905f
@TheRook TheRook change default flags in config
changed iv system in crypto.php
8119ea7
TheRook commented Oct 14, 2012

I have a job as a penetration tester and i work full time, I don' have time to fix all of your problems. You should be thankful for what you have been given.

I suggest you fix these serious vulnerabilities before the exploit code is released.

Contributor
narfbg commented Oct 15, 2012

... and that's exactly why you should've left the description on issue #1702 instead of changing it to a "Waiting for patch" non-sense and that patch now turns out to be unusable.

We appreciate your help of course, but as I already said - this just isn't how it works.

Contributor
pkriete commented Oct 15, 2012

Michael,

We do appreciate the work you've done here. Large pull requests are simply hard to review and merge. Especially if the individual exploits have to be teased apart for selective inclusion. On a community project where everyone is contributing their spare time, that is the kind of contribution that takes a long time to actually make it in.

Regardless, discussion feedback is certainly appreciated.

As Andrey already mentioned, remote_addr is great until you have a proxy in the chain. Obviously recording the proxy's IP address every time isn't going to do much for security.

The hmac issue(s) have come up repeatedly so I agree it is prudent we switched over to sha_hmac. It will invalidate all active sessions on upgrade, but while this is annoying I think it is an acceptable tradeoff.

The encryption changes on the other hand would completely invalidate all encrypted data. That is not going to fly with our users. The encryption library is used outside the session context by enough people that breaking compatibility is not really an option. Incidentally, that is also the reason for why the cryptographically weak xor fallback is still in there.

We can encourage mcrypt in big, bold, red letters in the documentation, but completely vanquishing what is currently there is not going to work for us. The solution may be to provide an alternative library under a new name at some point. Similar to how we slowly moved from validation to form_validation. This could then have its own set of requirements, but would certainly warrant more planning and discussion.

Thanks,
Pascal

TheRook commented Oct 15, 2012

While on a penetration test our assessment team uncovered numerous
vulnerabilities in the framework used by our client, which so happened to
be CodeIgnitor.

This specific block cipher implementation allows an attacker to construct a
Decryption oracle which can be used to carry out a CBC-R attack. Using
CBC-R an attacker is able to encrypt arbitrary data, and when you are
storing your user_id in the session handler then the attacker can become an
administrator without knowing the encryption key used. This is much worse
than the ASP.net session exploit that utilized a padding oracle. If
anyone is relying upon this vulnerable block cipher implantation then they
must update their code ASAP.

I changed the code to use REMOTE_ADDR because all other values are attacker
controlled, and there for a CWE-291 violation. A configurable
vulnerability is still a vulnerability. It is not proper to check the ip
address of a client because their real ip address will change if they are
behind a corporate load balancer.

As it stands our client is no longer vulnerable to the list of CWE numbers
provided. My boss has green lit the release of exploit code. Do you have
a time frame on when the cryptographic violations will be addressed? Do
you know of a skilled cryptographer such as my self that is willing to help
you with your project? If you are willing to answer these questions then I
am willing to work with you and delay the release of the exploit code.

On Sun, Oct 14, 2012 at 11:28 PM, Pascal Kriete notifications@github.comwrote:

Michael,

We do appreciate the work you've done here. Large pull requests are simply
hard to review and merge. Especially if the individual exploits have to be
teased apart for selective inclusion. On a community project where everyone
is contributing their spare time, that is the kind of contribution that
takes a long time to actually make it in.

Regardless, discussion feedback is certainly appreciated.

As Andrey already mentioned, remote_addr is great until you have a proxy
in the chain. Obviously recording the proxy's IP address every time isn't
going to do much for security.

The hmac issue(s) have come up repeatedly so I agree it is prudent we
switched over to sha_hmac. It will invalidate all active sessions on
upgrade, but while this is annoying I think it is an acceptable tradeoff.

The encryption changes on the other hand would completely invalidate all
encrypted data. That is not going to fly with our users. The encryption
library is used outside the session context by enough people that breaking
compatibility is not really an option. Incidentally, that is also the
reason for why the cryptographically weak xor fallback is still in there.

We can encourage mcrypt in big, bold, red letters in the documentation,
but completely vanquishing what is currently there is not going to work for
us. The solution may be to provide an alternative library under a new name
at some point. Similar to how we slowly moved from validation to
form_validation. This could then have its own set of requirements, but
would certainly warrant more planning and discussion.

Thanks,
Pascal


Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/pull/1888#issuecomment-9435384.

Contributor
GDmac commented Oct 16, 2012

When using database session there is no reason whatsoever to store any data on the client other than the session_id. Currently it seems ip_address, user_agent, and last_activity are stored clientside, ok, but WHY even compare session validity based on values return by the client at all?

  1. get input->cookie->session_id
  2. get database session data
  3. compare anything you want to compare with session data from database (eg. if row->ip !== input->ip_address)
TheRook commented Oct 16, 2012

@GDmac I didn't look at that feature. Nice catch! It should just be
cryptographic nonce.

On Mon, Oct 15, 2012 at 9:16 PM, GDmac notifications@github.com wrote:

When using database session there is no reason to store whatsoever data on
the client side other than the session_id. Currently it seems ip_address,
user_agent, and last_activity are even stored clientside, WHY at all store
anything at the client?

  1. get input->cookie->session_id

  2. get database session data

  3. compare anything you want to compare with session data from
    database (eg. if row->ip !== input->ip_address)


    Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/pull/1888#issuecomment-9472612.

Contributor
narfbg commented Oct 16, 2012

@GDmac That's issue #1344.

runcom commented Nov 20, 2012

You should update this ASAP..I, as a CI developer and user, can't stand to these serious vulnerability in my code...security updates should be done always before everything else

TheRook commented Nov 20, 2012

and cryptographic oracles are a very serious... I don't think the CI team
understands what these are and so they are not addressing them.

Contributor
narfbg commented Nov 20, 2012

To be honest, I indeed don't understand CWE-329. If you're willing to explain that one in a language understandable by a non-cryptographer, then I'd be happy to work on a suitable fix. The description posted for it, as well as the one for CWE-649 are (ironically) cryptic to me personally. They are written by a cryptographer, for a cryptographer and the fact is - security issues have always been taken seriously, but we are no cryptographers.

CWE-291 and CWE-327 have already been addressed.
Fix for the first one is even linked to this thread, right above your two comments. As far as I understand it, that referenced PR should also fix CWE-649, although I might be wrong on that one.

TheRook commented Nov 20, 2012

CWE-649 and CWE-329 have not been fixed in the current code base but where
addressed with my patch. CWE-291 and CWE-327 have been addressed although
it is a bit concerning that you even allow for an insecure configuration
you should at least warn users of this. The CWE-649 allows for
cryptographic oracles, both padding and decryption oracles are possible
with the current session handler. In short this allows the attacker to
decrypt messages and encrypt messages respectively despite the use of AES
(this is NOT a vulnerability in AES, just how you are using CBC mode). On
a side note the XOR encode function is very dangerous, and has no security
benefit what so ever and a PoC has already been provided for this. If the
mcrypt library isn't available, you need to use a PHP implementation, AES
is in the public domain so licensing should not be a concern.

Here is some documentation about CWE-329:
http://stackoverflow.com/questions/3008139/why-is-using-a-non-random-iv-with-cbc-mode-a-vulnerability
https://www.owasp.org/index.php/Not_using_a_random_initialization_vector_with_cipher_block_chaining_mode

On Tue, Nov 20, 2012 at 11:50 AM, Andrey Andreev
notifications@github.comwrote:

To be honest, I indeed don't understand CWE-329. If you're willing to
explain that one in a language understandable by a non-cryptographer, then
I'd be happy to work on a suitable fix. The description posted for it, as
well as the one for CWE-649 are (ironically) cryptic to me personally. They
are written by a cryptographer, for a cryptographer and the fact is -
security issues have always been taken seriously, but we are no
cryptographers.

CWE-291 and CWE-327 have already been addressed.
Fix for the first one is even linked to this thread, right above your two
comments. As far as I understand it, that referenced PR should also fix
CWE-649, although I might be wrong on that one.


Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/pull/1888#issuecomment-10567432.

Contributor
narfbg commented Nov 22, 2012

I don't want to further the discussion on your patch. As I and @pkriete already explained - it might satisfy your and/or your client's needs, but is not practically applicable because it would break a lot of existing applications that depend on the old code.

On one thing I do agree - we should try and provide a pure PHP implementation of what mcrypt provides, for the parts of it that we use.
On the rest I guess I'll have to do some research on my own, cause I'm really not getting any of your explanations (thanks for trying though).

TheRook commented Nov 23, 2012

... depend on extremely vulnerable code.

On Thu, Nov 22, 2012 at 4:56 AM, Andrey Andreev notifications@github.comwrote:

I don't want to further the discussion on your patch. As I and @pkrietehttps://github.com/pkrietealready explained - it might satisfy your and/or your client's needs, but
is not practically applicable because it would break a lot of existing
applications that depend on the old code.

On one thing I do agree - we should try and provide a pure PHP
implementation of what mcrypt provides, for the parts of it that we use.
On the rest I guess I'll have to do some research on my own, cause I'm
really not getting any of your explanations (thanks for trying though).


Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/pull/1888#issuecomment-10632196.

Contributor
narfbg commented Nov 23, 2012

I understand your arguments, but the Encryption class is not only used by the Session library. Invalidating session cookies wouldn't be a problem as everybody could log in again, but we don't know if that wouldn't invalidate other, more valuable data. It could even render a whole application to be useless. We need to have some kind of a fallback.

Need to also check for X-Forwarded-Proto since that will actually be set for Amazon Web Services, for example.

@padraic padraic commented on the diff Feb 9, 2013
application/config/config.php
| 'cookie_httponly' = Cookie will only be accessible via HTTP(S) (no javascript)
|
*/
-$config['cookie_prefix'] = '';
-$config['cookie_domain'] = '';
-$config['cookie_path'] = '/';
-$config['cookie_secure'] = FALSE;
-$config['cookie_httponly'] = FALSE;
+$config['cookie_prefix'] = '';
+$config['cookie_domain'] = '';
+$config['cookie_path'] = '/';
+//If we have HTTPS then this must be enabled.
+if(isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == "on")
padraic
padraic Feb 9, 2013

Need to also check for X-Forwarded-Proto since that will actually be set for Amazon Web Services, for example.

Contributor
narfbg commented Feb 3, 2014

Not (yet) related to sessions, but an alternative to the current CI_Encrypt class: 818aedb

TheRook commented Feb 3, 2014

@Andrey Andreev

Nice, that looks like this patch fixes the embracing home-brew crypto
vulnerability. One question, at any point is a hash function refereed to as
"encryption"? That would also be an embracing misnomer, because apples !=
orangutans, and cartographic hash != encryption.

On Mon, Feb 3, 2014 at 2:59 AM, Andrey Andreev notifications@github.comwrote:

Not (yet) related to sessions, but an alternative to the current
CI_Encrypt class: 818aedbhttps://github.com/EllisLab/CodeIgniter/commit/818aedb6ca46ee8498624d49403e5ff1404bc692

Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/pull/1888#issuecomment-33937147
.

Contributor
narfbg commented Feb 3, 2014

I'm not sure what you mean ... did you see that somewhere or ... ?

$ grep 'hash' system/libraries/Encryption.php 
$
Contributor
narfbg commented Feb 10, 2014

The new Encryption library was just merged and solves the issues listed here.
It is integrated into the Session lib as well, although that is to be replaced soon as well.

Thanks again for reporting this.

@narfbg narfbg closed this Feb 10, 2014
TheRook commented Feb 10, 2014

Freakin' Awesome. This is encouraging to security researchers to see
improvement.

I'm really happy to see this fixed. This patch should get a CVE number.
If you guys don't mind I'll contact Mitre and get that ball rolling.

On Sun, Feb 9, 2014 at 10:12 PM, Andrey Andreev notifications@github.comwrote:

Closed #1888 #1888.

Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/pull/1888
.

Contributor
narfbg commented Feb 10, 2014

I'm not sure of what importance that is but I personally don't mind, whatever it means. :)

Still, the framework is owned by EllisLab (I'm not their employee) and it is a work in progress. We should be able to release as soon as I finish the new Session lib - just to ensure that it's all done properly in one sweep. When that happens, I don't think there would be a reason not to publish/register it wherever you wish.

Glad to hear you like it after so much criticism (not that it wasn't appropriate). :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment