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

segfault with assigned user id on OpenShift #2046

Merged
merged 1 commit into from Nov 23, 2022
Merged

segfault with assigned user id on OpenShift #2046

merged 1 commit into from Nov 23, 2022

Conversation

arminabf
Copy link

When running httpd on OpenShift, by default the server instance will run with an assigned user ID not appearing in the UNIX password file.

There are several places in the code of mod_security where the user name is tried to retrieved by use of apr_uid_name_get(). As the function can not find a corresponding user name it returns with an error (and argument pointer being NULL). Due to missing error handling constant segmentation faults are faced on OpenShift.

As a fallback on OpenShift (and probably other container platforms), the user id should be used instead of the user name.

@marcstern
Copy link
Contributor

marcstern commented Apr 27, 2021

Better to have a centralized function for this:

char* get_username(apr_pool_t* mp) {
 char* username;
 apr_uid_t uid;
 apr_gid_t gid;
 int rc = apr_uid_current(&uid, &gid, mp);
 if (rc != APR_SUCCESS) return "apache";
 rc = apr_uid_name_get(&username, uid, mp);
 if (rc != APR_SUCCESS) return "apache";
 return username;
}

@FedericoHeichou
Copy link

Why isn't this merged? Sometimes segmentation fault won't happens and it could lead to other major vulnerabilities.
In apache2/re_variables.c:2597 the rc is checked, why it isn't in these other 2 files?

@martinhsv
Copy link
Contributor

There are two slightly differing proposals for this issue if apr_uid_name_get() fails:

  • use the numeric value from apr_uid_current
  • use a fixed string such as "apache" (in the related issue)

There are some things I like about the first proposal, but I'm wondering if having an all-digit default could be confusing or obscure for some users.

@marcstern
Copy link
Contributor

Using the numeric value from apr_uid_current is OK (and maybe even better).
Both solutions are better than the current bogus behaviour anyway.

@martinhsv martinhsv merged commit 8f04f44 into owasp-modsecurity:v2/master Nov 23, 2022
@martinhsv
Copy link
Contributor

Thanks all, for the contributions on this matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
v2.9.7
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

6 participants