| Message ID | 20220510182039.28771-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 31e3537624ad |
| Headers | show |
| Series | [RFC,1/4] libselinux: simplify policy path logic to avoid uninitialized read | expand |
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 >
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 --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;
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(-)