fix: remove usage of insecure tmpnam

[fzipi]

what

• remove usage of tmpnam
• apr has primitives for file handling and unique name creation
• encapsulate mutex creation in one function acquire_global_lock so we DRY
• create the temporary file template (GLOBAL_LOCK_TEMPLATE) as a definition outside a function
• move preprocessor directives into the new function
• add prototype to modsecurity.h file to enable other functions to create global locks (if needed)

why

• tmpnam is considered unsafe

drawbacks

• static memory usage goes up a bit: L_tmpnam is 20 bytes, and APR_MAX_PATH should be ~ 4096.

references

• https://sonarcloud.io/project/security_hotspots?id=owasp-modsecurity_ModSecurity&pullRequest=3148&sinceLeakPeriod=true

[marcstern]

Sounds good overall, just a few remarks:

1. In case of error, we should log the reason and the path
2. We should create a "get_lock" function englobing apr_file_mktemp(), apr_file_name_get() & apr_global_mutex_create() instead of duplicating this code
3. temp_path_template should probably be initialized as a static value outside modsecurity_init() in case we want re-use that "get_lock" function at another place
   @fzipi, can you modify the code accordingly?

[fzipi]

Sure, makes sense. Will push the changes.

[airween]

LGTM

[marcstern]

acquire_global_lock() prototype should be added in modsecurity.h in case we want ot reuse it elsewhere (or it should be marked as static for now).

The #ifdef __SET_MUTEX_PERMS should also move to acquire_global_lock().

When you're at it:

#ifdef GLOBAL_COLLECTION_LOCK
    msce->geo_lock = NULL;
#endif

This block can be removed, it's redundant with previous line

Thanks for you responsiveness

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[fzipi]

• consolidated all the preprocessor flags in the function.
• Also, I think the

#ifdef GLOBAL_COLLECTION_LOCK
    msce->geo_lock = NULL;
#endif

was a typo, and should be msce->dbm_lock instead (see previous line 188).