[PATCH 3/5] libselinux: label_file: fix memory management in store_stem()

[Nicolas Iooss]

If store_stem() fails to expand the memory allocated on data->stem_arr,
some things go wrong:
* the memory referenced by "buf" is leaked,
* data->alloc_stems has been increased without data->stem_arr having
been expanded. So the next time store_stem() is called, the function
will behave as if the buffer holds enough space, and will write data
after the end of data->stem_arr.

The first issue is being spotted by clang's static analyzer, which warns
about leaking variable "stem" in find_stem_from_spec() (this function
calls store_stem()).

This both issues by freeing buf when realloc(data->stem_arr) fails, and
by not increasing data->alloc_stems when this happens.

Signed-off-by: Nicolas Iooss <***@m4x.org>
---
libselinux/src/label_file.h | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/libselinux/src/label_file.h b/libselinux/src/label_file.h
index 3f9ce53b7ffe..1ab139e962f2 100644
--- a/libselinux/src/label_file.h
+++ b/libselinux/src/label_file.h
@@ -278,12 +278,14 @@ static inline int store_stem(struct saved_data *data, char *buf, int stem_len)

if (data->alloc_stems == num) {
struct stem *tmp_arr;
-
- data->alloc_stems = data->alloc_stems * 2 + 16;
+ int alloc_stems = data->alloc_stems * 2 + 16;
tmp_arr = realloc(data->stem_arr,
- sizeof(*tmp_arr) * data->alloc_stems);
- if (!tmp_arr)
+ sizeof(*tmp_arr) * alloc_stems);
+ if (!tmp_arr) {
+ free(buf);
return -1;
+ }
+ data->alloc_stems = alloc_stems;
data->stem_arr = tmp_arr;
}
data->stem_arr[num].len = stem_len;

[Nicolas Iooss]

In getconlist.c's main(), "level" is duplicated from an optional
argument without being ever freed. clang's static analyzer warns about
this memory leak.

Free the allocated memory properly in order to remove a warning reported
by clang's static analyzer.

Signed-off-by: Nicolas Iooss <***@m4x.org>
---
libselinux/utils/getconlist.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
index adec1781658b..abfe2c742bfb 100644
--- a/libselinux/utils/getconlist.c
+++ b/libselinux/utils/getconlist.c
@@ -40,6 +40,7 @@ int main(int argc, char **argv)
if (!is_selinux_enabled()) {
fprintf(stderr,
"getconlist may be used only on a SELinux kernel.\n");
+ free(level);
return 1;
}

@@ -49,6 +50,7 @@ int main(int argc, char **argv)
if (((argc - optind) < 2)) {
if (getcon(&cur_context) < 0) {
fprintf(stderr, "Couldn't get current context.\n");
+ free(level);
return 2;
}
} else
@@ -68,6 +70,7 @@ int main(int argc, char **argv)
}

free(usercon);
+ free(level);

return 0;
}

[Nicolas Iooss]

When load_users() parses an invalid line with an empty level context
(ie. nothing between "level" and "range" keywords), it allocates memory
with malloc(0) and uses it. The behavior of malloc() in this case is
an unspecified behavior: it might return NULL, which would lead to a
segmentation fault.

Fix this issue by reporting the invalid entry instead. While at it,
ensure that the character before "range" is a space, and change the
logic slightly in order to avoid using "--p; ... p++;".

This issue is reported by clang's static analyzer with the following
message:

genusers.c:222:11: warning: Use of zero-allocated memory
*r++ = *s;
^
genusers.c:225:7: warning: Use of zero-allocated memory
*r = 0;
^

Signed-off-by: Nicolas Iooss <***@m4x.org>
---
libsepol/src/genusers.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libsepol/src/genusers.c b/libsepol/src/genusers.c
index 556821066e3e..9bea83fd8304 100644
--- a/libsepol/src/genusers.c
+++ b/libsepol/src/genusers.c
@@ -201,11 +201,11 @@ static int load_users(struct policydb *policydb, const char *path)
if (!(*p))
BADLINE();
q = p;
- while (*p && strncasecmp(p, "range", 5))
+ while (*p && (!isspace(*p) || strncasecmp(p + 1, "range", 5)))
p++;
- if (!(*p))
+ if (!(*p) || p == q)
BADLINE();
- *--p = 0;
+ *p = 0;
p++;

scontext = malloc(p - q);

[Nicolas Iooss]

In getconlist.c, main() does not use usercon. Remove this variable.

Signed-off-by: Nicolas Iooss <***@m4x.org>
---
libselinux/utils/getconlist.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
index abfe2c742bfb..5ac0ca85075c 100644
--- a/libselinux/utils/getconlist.c
+++ b/libselinux/utils/getconlist.c
@@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d

int main(int argc, char **argv)
{
- char **list, *usercon = NULL, *cur_context = NULL;
+ char **list, *cur_context = NULL;
char *user = NULL, *level = NULL;
int ret, i, opt;

@@ -69,7 +69,6 @@ int main(int argc, char **argv)
freeconary(list);
}

- free(usercon);
free(level);

return 0;

[William Roberts]

In general this series looks fine.

However, checkpatch.pl is complaining about DOS line endings in your patches:

For example:
ERROR: DOS line endings
#325: FILE: libselinux/src/label_file.h:281:
+^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$

Post by Nicolas Iooss
In getconlist.c, main() does not use usercon. Remove this variable.
---
libselinux/utils/getconlist.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
index abfe2c742bfb..5ac0ca85075c 100644
--- a/libselinux/utils/getconlist.c
+++ b/libselinux/utils/getconlist.c
@@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d
int main(int argc, char **argv)
{
- char **list, *usercon = NULL, *cur_context = NULL;
+ char **list, *cur_context = NULL;
char *user = NULL, *level = NULL;
int ret, i, opt;
@@ -69,7 +69,6 @@ int main(int argc, char **argv)
freeconary(list);
}
- free(usercon);
free(level);
return 0;
--
2.17.0

[Nicolas Iooss]

On Sat, Apr 14, 2018 at 2:40 AM, William Roberts

Post by William Roberts
In general this series looks fine.
ERROR: DOS line endings
+^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$

This is unexpected. On my computer, "file" gives the following output
on the .patch files (there is no "..., with CRLF line terminators"):

0001-libsepol-do-not-dereference-NULL-if-stack_init-fails.patch:
unified diff output, ASCII text
0002-libsepol-ensure-the-level-context-is-not-empty.patch:
unified diff output, ASCII text
0003-libselinux-label_file-fix-memory-management-in-store.patch:
unified diff output, ASCII text
0004-libselinux-fix-memory-leak-in-getconlist.patch:
unified diff output, ASCII text
0005-libselinux-remove-unused-variable-usercon.patch:
unified diff output, ASCII text

Nevertheless, my messages seem to use CR+LF line endings. I am
wondering whether this is a new feature of "git send-email" or of my
SMTP server mangling text emails... Anyway, "git am *.patch && git
format-patch origin/master" produces files with LF line endings, so
git seems to handle fine this kind of modification.
By the way, I have also pushed these patches on my Github repository,
https://github.com/fishilico/selinux/commits/92717db1cb9dd6c8faf8fae3325f6a9d4c1002ca
.

Thanks!
Nicolas

[Stephen Smalley]

Post by William Roberts
In general this series looks fine.
ERROR: DOS line endings
+^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$

If needed, dos2unix can be used to strip them. However, I think git am takes care of this for you.

Post by William Roberts

Post by Nicolas Iooss
In getconlist.c, main() does not use usercon. Remove this variable.
---
libselinux/utils/getconlist.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
index abfe2c742bfb..5ac0ca85075c 100644
--- a/libselinux/utils/getconlist.c
+++ b/libselinux/utils/getconlist.c
@@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d
int main(int argc, char **argv)
{
- char **list, *usercon = NULL, *cur_context = NULL;
+ char **list, *cur_context = NULL;
char *user = NULL, *level = NULL;
int ret, i, opt;
@@ -69,7 +69,6 @@ int main(int argc, char **argv)
freeconary(list);
}
- free(usercon);
free(level);
return 0;
--
2.17.0

[William Roberts]

Post by Stephen Smalley

Post by William Roberts
In general this series looks fine.
ERROR: DOS line endings
+^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$

If needed, dos2unix can be used to strip them. However, I think git am takes care of this for you.

FYI your patches are staged here:
https://github.com/SELinuxProject/selinux/pull/93

If no one nacks them, ill merge latter this week. Thanks.

Post by Stephen Smalley

Post by William Roberts

Post by Nicolas Iooss
In getconlist.c, main() does not use usercon. Remove this variable.
---
libselinux/utils/getconlist.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
index abfe2c742bfb..5ac0ca85075c 100644
--- a/libselinux/utils/getconlist.c
+++ b/libselinux/utils/getconlist.c
@@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d
int main(int argc, char **argv)
{
- char **list, *usercon = NULL, *cur_context = NULL;
+ char **list, *cur_context = NULL;
char *user = NULL, *level = NULL;
int ret, i, opt;
@@ -69,7 +69,6 @@ int main(int argc, char **argv)
freeconary(list);
}
- free(usercon);
free(level);
return 0;
--
2.17.0

[jwcart2]

Post by William Roberts

Post by Stephen Smalley

Post by William Roberts
In general this series looks fine.
ERROR: DOS line endings
+^I^Iint alloc_stems = data->alloc_stems * 2 + 16;^M$

If needed, dos2unix can be used to strip them. However, I think git am takes care of this for you.

https://github.com/SELinuxProject/selinux/pull/93
If no one nacks them, ill merge latter this week. Thanks.

These patches look good to me.

Thanks,
Jim

Post by William Roberts

Post by Stephen Smalley

Post by William Roberts

Post by Nicolas Iooss
In getconlist.c, main() does not use usercon. Remove this variable.
---
libselinux/utils/getconlist.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/libselinux/utils/getconlist.c b/libselinux/utils/getconlist.c
index abfe2c742bfb..5ac0ca85075c 100644
--- a/libselinux/utils/getconlist.c
+++ b/libselinux/utils/getconlist.c
@@ -19,7 +19,7 @@ static __attribute__ ((__noreturn__)) void usage(const char *name, const char *d
int main(int argc, char **argv)
{
- char **list, *usercon = NULL, *cur_context = NULL;
+ char **list, *cur_context = NULL;
char *user = NULL, *level = NULL;
int ret, i, opt;
@@ -69,7 +69,6 @@ int main(int argc, char **argv)
freeconary(list);
}
- free(usercon);
free(level);
return 0;
--
2.17.0