[ABRT PATCH] add environment variable whitelist to debuginfo install wrapper - closes 692

[Petr Kubat]

The install-debuginfo C wrapper which is run by a non-root user
clears the whole environment. However, since the installer
asks for user input by the use of an environment variable,
we have to create a whitelist of variables to keep when doing
the environment purge.

Signed-off-by: Petr Kubat <pkubat-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
.../abrt-action-install-debuginfo-to-abrt-cache.c | 28 ++++++++++++++++++++++
1 file changed, 28 insertions(+)

diff --git a/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c b/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c
index 60ac5c9..53be57a 100644
--- a/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c
+++ b/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c
@@ -70,7 +70,35 @@ int main(int argc, char **argv)
// We forgot to sanitize PYTHONPATH. And who knows what else we forgot
// (especially considering *future* new variables of this kind).
// We switched to clearing entire environment instead:
+
+ // However since we communicate through environment variables
+ // we have to keep a whitelist of variables to keep.
+ static const char *whitelist[] = {
+ "REPORT_CLIENT_SLAVE" // Check if the app is being run as a slave
+ };
+ int wlsize = sizeof(whitelist)/sizeof(char*);
+ char *setlist[wlsize];
+ char *p = NULL;
+ for (int i = 0; i < wlsize; i++) {
+ if ((p = getenv(whitelist[i])) != NULL) {
+ setlist[i] = malloc(strlen(p)*sizeof(char*));
+ setlist[i] = strncpy(setlist[i], p, strlen(p));
+ }
+ else
+ setlist[i] = NULL;
+ }
+ // Now we can clear the environment
clearenv();
+ // And once again set whitelisted variables
+ for (int i = 0; i < wlsize; i++) {
+ if (setlist[i] != NULL) {
+ int slsize = sizeof(setlist)/sizeof(char*);
+ char buffer[slsize+wlsize+1];
+ sprintf(buffer, "%s=%s", whitelist[i], setlist[i]);
+ putenv(buffer);
+ free(setlist[i]);
+ }
+ }
#else
/* Clear dangerous stuff from env */
static const char forbid[] =

[Jakub Filak]

It looks good to me except some coding style issues.

Post by Petr Kubat
+ // However since we communicate through environment variables
+ // we have to keep a whitelist of variables to keep.
+ static const char *whitelist[] = {
+ "REPORT_CLIENT_SLAVE" // Check if the app is being run as a slave
+ };
+ int wlsize = sizeof(whitelist)/sizeof(char*);

I'd use a more accurate type. Maybe size_t or rather const size_t.

Post by Petr Kubat
+ char *setlist[wlsize];

You can statically initialize the entire array to NULLs by assigning
NULL to the first element.

char *setlist[sizeof(whitelist)/sizeof(char*)] = { NULL };

Post by Petr Kubat
+ char *p = NULL;
+ for (int i = 0; i < wlsize; i++) {

for (size_t i = 0; i < wlsize; ++i)
{

Post by Petr Kubat
+ if ((p = getenv(whitelist[i])) != NULL)

{

Post by Petr Kubat
+ setlist[i] = malloc(strlen(p)*sizeof(char*));
+ setlist[i] = strncpy(setlist[i], p, strlen(p));

How about:
setlist[i] = xstrdup(p);

Post by Petr Kubat
+ }
+ else
+ setlist[i] = NULL;

And if the array is initialized to NULLs, you can drop the two above
lines.

Post by Petr Kubat
+ }
+ // Now we can clear the environment
clearenv();
+ // And once again set whitelisted variables
+ for (int i = 0; i < wlsize; i++)

{

Post by Petr Kubat
+ if (setlist[i] != NULL)

{

How about:
xsetenv(whitelist[i], setlist[i]);

Post by Petr Kubat
+ free(setlist[i]);
+ }
+ }
#else
/* Clear dangerous stuff from env */
static const char forbid[] =

[Petr Kubat]

The install-debuginfo C wrapper which is run by a non-root user
clears the whole environment. However, since the installer
asks for user input by the use of an environment variable,
we have to create a whitelist of variables to keep when doing
the environment purge.

Signed-off-by: Petr Kubat <pkubat-H+wXaHxf7aLQT0dZR+***@public.gmane.org>
---
.../abrt-action-install-debuginfo-to-abrt-cache.c | 25 ++++++++++++++++++++++
1 file changed, 25 insertions(+)

diff --git a/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c b/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c
index 60ac5c9..d53f09f 100644
--- a/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c
+++ b/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c
@@ -70,7 +70,32 @@ int main(int argc, char **argv)
// We forgot to sanitize PYTHONPATH. And who knows what else we forgot
// (especially considering *future* new variables of this kind).
// We switched to clearing entire environment instead:
+
+ // However since we communicate through environment variables
+ // we have to keep a whitelist of variables to keep.
+ static const char *whitelist[] = {
+ "REPORT_CLIENT_SLAVE" // Check if the app is being run as a slave
+ };
+ const size_t wlsize = sizeof(whitelist)/sizeof(char*);
+ char *setlist[wlsize];
+ char *p = NULL;
+ for (size_t i = 0; i < wlsize; i++)
+ if ((p = getenv(whitelist[i])) != NULL)
+ setlist[i] = xstrdup(p);
+ else
+ // Variable size arrays cannot be statically initialized
+ setlist[i] = NULL;
+
+ // Now we can clear the environment
clearenv();
+
+ // And once again set whitelisted variables
+ for (size_t i = 0; i < wlsize; i++)
+ if (setlist[i] != NULL)
+ {
+ setenv(whitelist[i], setlist[i]);
+ free(setlist[i]);
+ }
#else
/* Clear dangerous stuff from env */
static const char forbid[] =

[Jakub Filak]

Fixed, pushed, thanks.