Remove -- usage from session ID creation

[OpreaFlorin]

Description

When a COOKIE ID containing the character sequence '--' is generated, mod_security thinks it's a SQL injection and returns a 404 error. This sequence should be avoided, or the '-' character could be replaced with '_'. However, the sequence '--' in the session ID creates a lot of problems.

SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|!REQUEST_COOKIES:/_pk_ref/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "@rx (?:/\*!?|\*/|[';]--|--[\s\r\n\v\f]|--[^-]*?-|[^&-]#.*?[\s\r\n\v\f]|;?\\x00)" \

SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/|!REQUEST_COOKIES:/_pk_ref/|REQUEST_COOKIES_NAMES|ARGS_NAMES|ARGS|XML:/* "(/\*!?|\*/|[';]--|--[\s\r\n\v\f]|(?:--[^-]*?-)|([^\-&])#.*?[\s\r\n\v\f]|;?\\x00)" "phase:2,rev:'2',ver:'OWASP_CRS/2.2.9',maturity:'8',accuracy:'8',\ id:'981231',t:none,t:urlDecodeUni,block,\ msg:'SQL Comment Sequence Detected.'\ ,severity:'2',capture,logdata:'Matched Data: %{TX.0} found within %{MATCHED_VAR_NAME}: %{MATCHED_VAR}',tag:'OWASP_CRS/WEB_ATTACK/SQL_INJECTION',tag:'WASCTC/WASC-19',tag:'OWASP_TOP_10/A1',tag:'OWASP_AppSensor/CIE1',tag:'PCI/6.5.2',setvar:tx.anomaly_score=+%{tx.critical_anomaly_score},setvar:tx.sql_injection_score=+1,setvar:'tx.msg=%{rule.msg}',setvar:tx.%{rule.id}-OWASP_CRS/WEB_ATTACK/SQL_INJECTION-%{matched_var_name}=%{tx.0}"

[TimWolla]

It probably makes sense to deprecate both session.sid_length and session.sid_bits_per_character and just always return a 32 character hexadecimal string, thus avoiding this issue and removing useless INI knobs.

32 hex characters are 128 bits of entropy. This is plenty for a session ID: TLS connections are commonly encrypted with AES-128 which uses a 128 bit key and is considered secure.

/cc @Girgias

[Girgias]

It probably makes sense to deprecate both session.sid_length and session.sid_bits_per_character and just always return a 32 character hexadecimal string, thus avoiding this issue and removing useless INI knobs.

32 hex characters are 128 bits of entropy. This is plenty for a session ID: TLS connections are commonly encrypted with AES-128 which uses a 128 bit key and is considered secure.

/cc @Girgias

Deprecating these INI settings is probably sensible, maybe add them to the 8.4 deprecation list?

[TimWolla]

Deprecating these INI settings is probably sensible, maybe add them to the 8.4 deprecation list?

Added as a stub with a reference back to this issue: https://wiki.php.net/rfc/deprecations_php_8_4#sessionsid_length_and_sessionsid_bits_per_character

[OpreaFlorin]

I think the user should be able to use more complex session IDs than that. The problem could be solved much faster by changing the '-' character to '_' in the list of used characters when session.sid_bits_per_character is 6.

session.sid_bits_per_character allows you to specify the number of bits in encoded session ID character. The possible values are '4' (0-9, a-f), '5' (0-9, a-v), and '6' (0-9, a-z, A-Z, "-", ","). The default is 4. The more bits results in stronger session ID. 5 is recommended value for most environments.

So, to resolve this issue, a solution is just to replace this line:
static const char hexconvtab[] = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ,-";
with
static const char hexconvtab[] = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ,_";

[KapitanOczywisty]

32 hex characters are 128 bits of entropy. This is plenty for a session ID: TLS connections are commonly encrypted with AES-128 which uses a 128 bit key and is considered secure.

For AES you need to have the exact key for specific message/connection, with session ID there are many IDs which could provide access to some data.

The problem could be solved much faster by changing the '-' character to '_' in the list of used characters when session.sid_bits_per_character is 6.

In my opinion mod_security OWASP ruleset is flawed and should be fixed, but I'm in favor of this change 👍

[TimWolla]

I think the user should be able to use more complex session IDs than that.

Why?

The problem could be solved much faster by changing the '-' character to '_' in the list of used characters when session.sid_bits_per_character is 6.

That would possibly a breaking change for anyone using a custom session save handler that does not anticipate a session ID containing an underscore.

For AES you need to have the exact key for specific message/connection, with session ID there are many IDs which could provide access to some data.

Mathematically, guessing a random AES key is effectively equivalent to guessing a random session ID, because the number of valid session IDs at a given point is time is completely dwarfed by the size of the ID space.

Anyway, as per the birthday problem, guessing a random 128 bit number with a probability of 10^-11 requires ~2^46 guesses. At 100_000 guesses per second that takes you 23 years to even reach this negligible probability.

[Girgias]

I think the user should be able to use more complex session IDs than that. The problem could be solved much faster by changing the '-' character to '_' in the list of used characters when session.sid_bits_per_character is 6.

This is a BC break and cannot be done.

[marcboon]

I think the user should be able to use more complex session IDs than that. The problem could be solved much faster by changing the '-' character to '_' in the list of used characters when session.sid_bits_per_character is 6.

Instead of replacing '-' with '_' I suggest replacing '--' in the session id with a randomly selected combination of two characters from hexconvtab not being '--'. This won't break existing code.

[KapitanOczywisty]

A few notes:

1. From my testing, to cause error, a double dash -- have to be followed by another dash - or other characters and dash. Meaning: PHPSESSID=abc--def is fine but PHPSESSID=abc--d-ef causes error, of course second dash can be a part of another cookie.
2. There are more recent rules than 2.2.9 and you should test them, however I don't think this was fixed anyway.
3. You should report false-positive in this repository: https://github.com/coreruleset/coreruleset