diff mbox series

[RFC,1/4] libselinux: simplify policy path logic to avoid uninitialized read

Message ID 20220510182039.28771-1-cgzones@googlemail.com (mailing list archive)
State New
Headers show
Series [RFC,1/4] libselinux: simplify policy path logic to avoid uninitialized read | expand

Commit Message

Christian Göttsche May 10, 2022, 6:20 p.m. UTC
In case the function __policy_init() gets called with a NULL pointer,
the stack variable path remains uninitialized (except at its last
index).  If parsing the binary policy fails in sepol_policydb_read() the
error branch would access those uninitialized memory.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/audit2why.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

Comments

James Carter May 18, 2022, 3:22 p.m. UTC | #1
On Tue, May 10, 2022 at 4:53 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> In case the function __policy_init() gets called with a NULL pointer,
> the stack variable path remains uninitialized (except at its last
> index).  If parsing the binary policy fails in sepol_policydb_read() the
> error branch would access those uninitialized memory.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

For the series with v2 of patch 4:
Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libselinux/src/audit2why.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/libselinux/src/audit2why.c b/libselinux/src/audit2why.c
> index ca38e13c..44a9a341 100644
> --- a/libselinux/src/audit2why.c
> +++ b/libselinux/src/audit2why.c
> @@ -192,25 +192,16 @@ static PyObject *finish(PyObject *self __attribute__((unused)), PyObject *args)
>  static int __policy_init(const char *init_path)
>  {
>         FILE *fp;
> -       char path[PATH_MAX];
> +       const char *curpolicy;
>         char errormsg[PATH_MAX+1024+20];
>         struct sepol_policy_file *pf = NULL;
>         int rc;
>         unsigned int cnt;
>
> -       path[PATH_MAX-1] = '\0';
>         if (init_path) {
> -               strncpy(path, init_path, PATH_MAX-1);
> -               fp = fopen(path, "re");
> -               if (!fp) {
> -                       snprintf(errormsg, sizeof(errormsg),
> -                                "unable to open %s:  %m\n",
> -                                path);
> -                       PyErr_SetString( PyExc_ValueError, errormsg);
> -                       return 1;
> -               }
> +               curpolicy = init_path;
>         } else {
> -               const char *curpolicy = selinux_current_policy_path();
> +               curpolicy = selinux_current_policy_path();
>                 if (!curpolicy) {
>                         /* SELinux disabled, must use -p option. */
>                         snprintf(errormsg, sizeof(errormsg),
> @@ -218,14 +209,15 @@ static int __policy_init(const char *init_path)
>                         PyErr_SetString( PyExc_ValueError, errormsg);
>                         return 1;
>                 }
> -               fp = fopen(curpolicy, "re");
> -               if (!fp) {
> -                       snprintf(errormsg, sizeof(errormsg),
> -                                "unable to open %s:  %m\n",
> -                                curpolicy);
> -                       PyErr_SetString( PyExc_ValueError, errormsg);
> -                       return 1;
> -               }
> +       }
> +
> +       fp = fopen(curpolicy, "re");
> +       if (!fp) {
> +               snprintf(errormsg, sizeof(errormsg),
> +                        "unable to open %s:  %m\n",
> +                        curpolicy);
> +               PyErr_SetString( PyExc_ValueError, errormsg);
> +               return 1;
>         }
>
>         avc = calloc(sizeof(struct avc_t), 1);
> @@ -249,7 +241,7 @@ static int __policy_init(const char *init_path)
>         sepol_policy_file_set_fp(pf, fp);
>         if (sepol_policydb_read(avc->policydb, pf)) {
>                 snprintf(errormsg, sizeof(errormsg),
> -                        "invalid binary policy %s\n", path);
> +                        "invalid binary policy %s\n", curpolicy);
>                 PyErr_SetString( PyExc_ValueError, errormsg);
>                 fclose(fp);
>                 return 1;
> --
> 2.36.1
>
James Carter June 8, 2022, 1:40 p.m. UTC | #2
On Wed, May 18, 2022 at 11:22 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, May 10, 2022 at 4:53 PM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > In case the function __policy_init() gets called with a NULL pointer,
> > the stack variable path remains uninitialized (except at its last
> > index).  If parsing the binary policy fails in sepol_policydb_read() the
> > error branch would access those uninitialized memory.
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> For the series with v2 of patch 4:
> Acked-by: James Carter <jwcart2@gmail.com>
>
Merged with v4 of patch 4.
Thanks,
Jim

> > ---
> >  libselinux/src/audit2why.c | 34 +++++++++++++---------------------
> >  1 file changed, 13 insertions(+), 21 deletions(-)
> >
> > diff --git a/libselinux/src/audit2why.c b/libselinux/src/audit2why.c
> > index ca38e13c..44a9a341 100644
> > --- a/libselinux/src/audit2why.c
> > +++ b/libselinux/src/audit2why.c
> > @@ -192,25 +192,16 @@ static PyObject *finish(PyObject *self __attribute__((unused)), PyObject *args)
> >  static int __policy_init(const char *init_path)
> >  {
> >         FILE *fp;
> > -       char path[PATH_MAX];
> > +       const char *curpolicy;
> >         char errormsg[PATH_MAX+1024+20];
> >         struct sepol_policy_file *pf = NULL;
> >         int rc;
> >         unsigned int cnt;
> >
> > -       path[PATH_MAX-1] = '\0';
> >         if (init_path) {
> > -               strncpy(path, init_path, PATH_MAX-1);
> > -               fp = fopen(path, "re");
> > -               if (!fp) {
> > -                       snprintf(errormsg, sizeof(errormsg),
> > -                                "unable to open %s:  %m\n",
> > -                                path);
> > -                       PyErr_SetString( PyExc_ValueError, errormsg);
> > -                       return 1;
> > -               }
> > +               curpolicy = init_path;
> >         } else {
> > -               const char *curpolicy = selinux_current_policy_path();
> > +               curpolicy = selinux_current_policy_path();
> >                 if (!curpolicy) {
> >                         /* SELinux disabled, must use -p option. */
> >                         snprintf(errormsg, sizeof(errormsg),
> > @@ -218,14 +209,15 @@ static int __policy_init(const char *init_path)
> >                         PyErr_SetString( PyExc_ValueError, errormsg);
> >                         return 1;
> >                 }
> > -               fp = fopen(curpolicy, "re");
> > -               if (!fp) {
> > -                       snprintf(errormsg, sizeof(errormsg),
> > -                                "unable to open %s:  %m\n",
> > -                                curpolicy);
> > -                       PyErr_SetString( PyExc_ValueError, errormsg);
> > -                       return 1;
> > -               }
> > +       }
> > +
> > +       fp = fopen(curpolicy, "re");
> > +       if (!fp) {
> > +               snprintf(errormsg, sizeof(errormsg),
> > +                        "unable to open %s:  %m\n",
> > +                        curpolicy);
> > +               PyErr_SetString( PyExc_ValueError, errormsg);
> > +               return 1;
> >         }
> >
> >         avc = calloc(sizeof(struct avc_t), 1);
> > @@ -249,7 +241,7 @@ static int __policy_init(const char *init_path)
> >         sepol_policy_file_set_fp(pf, fp);
> >         if (sepol_policydb_read(avc->policydb, pf)) {
> >                 snprintf(errormsg, sizeof(errormsg),
> > -                        "invalid binary policy %s\n", path);
> > +                        "invalid binary policy %s\n", curpolicy);
> >                 PyErr_SetString( PyExc_ValueError, errormsg);
> >                 fclose(fp);
> >                 return 1;
> > --
> > 2.36.1
> >
diff mbox series

Patch

diff --git a/libselinux/src/audit2why.c b/libselinux/src/audit2why.c
index ca38e13c..44a9a341 100644
--- a/libselinux/src/audit2why.c
+++ b/libselinux/src/audit2why.c
@@ -192,25 +192,16 @@  static PyObject *finish(PyObject *self __attribute__((unused)), PyObject *args)
 static int __policy_init(const char *init_path)
 {
 	FILE *fp;
-	char path[PATH_MAX];
+	const char *curpolicy;
 	char errormsg[PATH_MAX+1024+20];
 	struct sepol_policy_file *pf = NULL;
 	int rc;
 	unsigned int cnt;
 
-	path[PATH_MAX-1] = '\0';
 	if (init_path) {
-		strncpy(path, init_path, PATH_MAX-1);
-		fp = fopen(path, "re");
-		if (!fp) {
-			snprintf(errormsg, sizeof(errormsg), 
-				 "unable to open %s:  %m\n",
-				 path);
-			PyErr_SetString( PyExc_ValueError, errormsg);
-			return 1;
-		}
+		curpolicy = init_path;
 	} else {
-		const char *curpolicy = selinux_current_policy_path();
+		curpolicy = selinux_current_policy_path();
 		if (!curpolicy) {
 			/* SELinux disabled, must use -p option. */
 			snprintf(errormsg, sizeof(errormsg),
@@ -218,14 +209,15 @@  static int __policy_init(const char *init_path)
 			PyErr_SetString( PyExc_ValueError, errormsg);
 			return 1;
 		}
-		fp = fopen(curpolicy, "re");
-		if (!fp) {
-			snprintf(errormsg, sizeof(errormsg), 
-				 "unable to open %s:  %m\n",
-				 curpolicy);
-			PyErr_SetString( PyExc_ValueError, errormsg);
-			return 1;
-		}
+	}
+
+	fp = fopen(curpolicy, "re");
+	if (!fp) {
+		snprintf(errormsg, sizeof(errormsg),
+			 "unable to open %s:  %m\n",
+			 curpolicy);
+		PyErr_SetString( PyExc_ValueError, errormsg);
+		return 1;
 	}
 
 	avc = calloc(sizeof(struct avc_t), 1);
@@ -249,7 +241,7 @@  static int __policy_init(const char *init_path)
 	sepol_policy_file_set_fp(pf, fp);	
 	if (sepol_policydb_read(avc->policydb, pf)) {
 		snprintf(errormsg, sizeof(errormsg), 
-			 "invalid binary policy %s\n", path);
+			 "invalid binary policy %s\n", curpolicy);
 		PyErr_SetString( PyExc_ValueError, errormsg);
 		fclose(fp);
 		return 1;