Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hack to show that there is an issue with the session collection's logic #1274

Closed
wants to merge 1 commit into from

Conversation

csutherl
Copy link

This PR is a hack to show that there is an issue somewhere with the collection storage/retrieval logic for session collections (I also think this affects user and resource collections, but haven't verified this yet) which is causing #1273. After applying this change the test in #1273 always adds up to the total number of requests. I arrived at this conclusion because I noticed some inconsistencies in the logging, like:

Added collection "default_SESSION" to the list as "SESSION".
....
collection_retrieve_ex: collection_retrieve_ex: Retrieving collection (name "default_SESSION", filename "/var/lib/mod_security/default_SESSION")

and that the SESSION collection never receives delta updates like the global one does:

collection_store: Delta applied for global.counter 1->2 (1): 1 + (1) = 2 [2,1]

because orig_col in re_actions.c always returns NULL for the session collection:

458     orig_col = (const apr_table_t *)apr_table_get(msr->collections_original, var_name->value);

The only difference that I see in the way that the global and session collections are handled is in the real_col_name and col_name passed into init_collection.

Obviously I don't want this to be accepted, but I wanted to get a conversation started with someone that understands mod_security better than I do and post the results of my testing thus far. I was told that a PR is the best way to do that :)

@csutherl
Copy link
Author

@bostrt

@csutherl
Copy link
Author

Oh yeah, after this change is applied you have to update the configuration to use 'default_SESSION' instead of 'SESSION' (which is why I said this is a hack) because it tries to lookup the exact name you give it there.

SecAction phase:5,id:119,nolog,pass,setvar:default_SESSION.my_counter=+1

…need a change, but the session counts are 100% accurate.
@bostrt bostrt mentioned this pull request Feb 11, 2017
@zimmerle zimmerle self-requested a review May 11, 2017 13:27
@zimmerle zimmerle self-assigned this May 11, 2017
zimmerle pushed a commit that referenced this pull request May 11, 2017
@zimmerle
Copy link
Contributor

Just to add more details...

Not sure if you have noticed but the deafult_ that you are referring to, is in fact the name of the WEBAPPID. The idea behind the WebAppID is to create a namespace for the collection. There are further information about the SecWebAppId here:

As I've commented before, regardless of the computation of the delta, it is expected to have a conter on the top of a collection with values that are not the exactly same amount of requests. That may differ upon to the limitations of the server vs the amount of requests.

@zimmerle zimmerle closed this May 16, 2017
@zimmerle zimmerle reopened this May 16, 2017
zimmerle pushed a commit that referenced this pull request May 19, 2017
As reported on #1274 we had a problem while merging the collections.
Turns out that the collection name was wrong while passing the
information to setvar.
zimmerle pushed a commit that referenced this pull request May 19, 2017
As reported on #1274 we had a problem while merging the collections.
Turns out that the collection name was wrong while passing the
information to setvar.
@zimmerle
Copy link
Contributor

As described at #1273, this issue has being fixed therefore we no longer need this hack to expose the problem. Thank you for the report @csutherl :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants