* [PATCH v2] libsepol: Use a dynamic buffer in sepol_av_to_string()
@ 2023-12-12 15:11 James Carter
2024-02-26 16:25 ` Christian Göttsche
2024-03-04 19:15 ` James Carter
0 siblings, 2 replies; 4+ messages in thread
From: James Carter @ 2023-12-12 15:11 UTC (permalink / raw)
To: selinux; +Cc: cgzones, James Carter
In the internal function sepol_av_to_string(), use a dynamically
allocated buffer for the permission names of an access vector instead
of a fixed static buffer to support very long permission names.
Update the internal users of sepol_av_to_string() to free the buffer.
The exported function sepol_perm_to_string() is just a wrapper to
the internal function. To avoid changing the behavior of this function,
use a static buffer and copy the resulting string from the internal
function. If the string is too long for the buffer or there was an
error in creating the string, return a string indicating the error.
All of the changes to the internal function and users was the work
of Christian Göttsche <cgzones@googlemail.com>.
Reported-by: oss-fuzz (issue 64832, 64933)
Suggested-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
v2: In sepol_av_perm_to_string():
- Use "sizeof(avbuf)" instead of 1024
- Added "free(avstr)"
In sepol_av_to_string():
- Added "*p = '\0';" to ensure buffer is initialized
checkpolicy/test/dismod.c | 11 +++---
checkpolicy/test/dispol.c | 7 ++--
libsepol/include/sepol/policydb/util.h | 2 +-
libsepol/src/assertion.c | 16 ++++++---
libsepol/src/hierarchy.c | 7 ++--
libsepol/src/kernel_to_cil.c | 7 ++++
libsepol/src/kernel_to_conf.c | 15 +++++---
libsepol/src/module_to_cil.c | 7 ++++
libsepol/src/services.c | 26 ++++++++++++--
libsepol/src/util.c | 50 +++++++++++++++-----------
10 files changed, 103 insertions(+), 45 deletions(-)
diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
index ac2d61d2..bd45c95e 100644
--- a/checkpolicy/test/dismod.c
+++ b/checkpolicy/test/dismod.c
@@ -118,12 +118,11 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
static void render_access_mask(uint32_t mask, uint32_t class, policydb_t * p,
FILE * fp)
{
- char *perm;
+ char *perm = sepol_av_to_string(p, class, mask);
fprintf(fp, "{");
- perm = sepol_av_to_string(p, class, mask);
- if (perm)
- fprintf(fp, "%s ", perm);
+ fprintf(fp, "%s ", perm ?: "<format-failure>");
fprintf(fp, "}");
+ free(perm);
}
static void render_access_bitmap(ebitmap_t * map, uint32_t class,
@@ -135,8 +134,8 @@ static void render_access_bitmap(ebitmap_t * map, uint32_t class,
for (i = ebitmap_startbit(map); i < ebitmap_length(map); i++) {
if (ebitmap_get_bit(map, i)) {
perm = sepol_av_to_string(p, class, UINT32_C(1) << i);
- if (perm)
- fprintf(fp, "%s", perm);
+ fprintf(fp, "%s", perm ?: "<format-failure>");
+ free(perm);
}
}
fprintf(fp, " }");
diff --git a/checkpolicy/test/dispol.c b/checkpolicy/test/dispol.c
index 944ef7ec..2662048e 100644
--- a/checkpolicy/test/dispol.c
+++ b/checkpolicy/test/dispol.c
@@ -93,12 +93,11 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
static int render_access_mask(uint32_t mask, avtab_key_t * key, policydb_t * p,
FILE * fp)
{
- char *perm;
+ char *perm = sepol_av_to_string(p, key->target_class, mask);
fprintf(fp, "{");
- perm = sepol_av_to_string(p, key->target_class, mask);
- if (perm)
- fprintf(fp, "%s ", perm);
+ fprintf(fp, "%s ", perm ?: "<format-failure>");
fprintf(fp, "}");
+ free(perm);
return 0;
}
diff --git a/libsepol/include/sepol/policydb/util.h b/libsepol/include/sepol/policydb/util.h
index db8da213..70c531d3 100644
--- a/libsepol/include/sepol/policydb/util.h
+++ b/libsepol/include/sepol/policydb/util.h
@@ -31,7 +31,7 @@ extern "C" {
extern int add_i_to_a(uint32_t i, uint32_t * cnt, uint32_t ** a);
-extern char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
+extern char *sepol_av_to_string(const policydb_t *policydbp, sepol_security_class_t tclass,
sepol_access_vector_t av);
char *sepol_extended_perms_to_string(avtab_extended_perms_t *xperms);
diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
index 6de7d031..3076babe 100644
--- a/libsepol/src/assertion.c
+++ b/libsepol/src/assertion.c
@@ -48,26 +48,30 @@ static void report_failure(sepol_handle_t *handle, policydb_t *p, const avrule_t
unsigned int stype, unsigned int ttype,
const class_perm_node_t *curperm, uint32_t perms)
{
+ char *permstr = sepol_av_to_string(p, curperm->tclass, perms);
+
if (avrule->source_filename) {
ERR(handle, "neverallow on line %lu of %s (or line %lu of %s) violated by allow %s %s:%s {%s };",
avrule->source_line, avrule->source_filename, avrule->line, policy_name(p),
p->p_type_val_to_name[stype],
p->p_type_val_to_name[ttype],
p->p_class_val_to_name[curperm->tclass - 1],
- sepol_av_to_string(p, curperm->tclass, perms));
+ permstr ?: "<format-failure>");
} else if (avrule->line) {
ERR(handle, "neverallow on line %lu violated by allow %s %s:%s {%s };",
avrule->line, p->p_type_val_to_name[stype],
p->p_type_val_to_name[ttype],
p->p_class_val_to_name[curperm->tclass - 1],
- sepol_av_to_string(p, curperm->tclass, perms));
+ permstr ?: "<format-failure>");
} else {
ERR(handle, "neverallow violated by allow %s %s:%s {%s };",
p->p_type_val_to_name[stype],
p->p_type_val_to_name[ttype],
p->p_class_val_to_name[curperm->tclass - 1],
- sepol_av_to_string(p, curperm->tclass, perms));
+ permstr ?: "<format-failure>");
}
+
+ free(permstr);
}
static int match_any_class_permissions(class_perm_node_t *cp, uint32_t class, uint32_t data)
@@ -200,13 +204,17 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
/* failure on the regular permissions */
if (!found_xperm) {
+ char *permstr = sepol_av_to_string(p, curperm->tclass, perms);
+
ERR(handle, "neverallowxperm on line %lu of %s (or line %lu of %s) violated by\n"
"allow %s %s:%s {%s };",
avrule->source_line, avrule->source_filename, avrule->line, policy_name(p),
p->p_type_val_to_name[stype],
p->p_type_val_to_name[ttype],
p->p_class_val_to_name[curperm->tclass - 1],
- sepol_av_to_string(p, curperm->tclass, perms));
+ permstr ?: "<format-failure>");
+
+ free(permstr);
errors++;
}
diff --git a/libsepol/src/hierarchy.c b/libsepol/src/hierarchy.c
index 350443a8..06e05310 100644
--- a/libsepol/src/hierarchy.c
+++ b/libsepol/src/hierarchy.c
@@ -443,12 +443,15 @@ static void bounds_report(sepol_handle_t *handle, policydb_t *p, uint32_t child,
p->p_type_val_to_name[child - 1],
p->p_type_val_to_name[parent - 1]);
for (; cur; cur = cur->next) {
+ char *permstr = sepol_av_to_string(p, cur->key.target_class, cur->datum.data);
+
ERR(handle, " %s %s : %s { %s }",
p->p_type_val_to_name[cur->key.source_type - 1],
p->p_type_val_to_name[cur->key.target_type - 1],
p->p_class_val_to_name[cur->key.target_class - 1],
- sepol_av_to_string(p, cur->key.target_class,
- cur->datum.data));
+ permstr ?: "<format-failure>");
+
+ free(permstr);
}
}
diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index bcb58eee..634826d5 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -297,6 +297,11 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
}
perms = sepol_av_to_string(pdb, class->s.value, curr->permissions);
+ if (!perms) {
+ ERR(NULL, "Failed to generate permission string");
+ rc = -1;
+ goto exit;
+ }
if (is_mls) {
key_word = "mlsconstrain";
@@ -307,6 +312,7 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
}
rc = strs_create_and_add(strs, "(%s (%s (%s)) %s)", key_word, classkey, perms+1, expr);
+ free(perms);
free(expr);
if (rc != 0) {
goto exit;
@@ -1772,6 +1778,7 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
}
rule = create_str("(%s %s %s (%s (%s)))",
flavor, src, tgt, class, perms+1);
+ free(perms);
} else if (key->specified & AVTAB_XPERMS) {
perms = xperms_to_str(datum->xperms);
if (perms == NULL) {
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index 83f46e0f..de1d9e09 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -292,6 +292,11 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
}
perms = sepol_av_to_string(pdb, class->s.value, curr->permissions);
+ if (!perms) {
+ ERR(NULL, "Failed to generate permission string");
+ rc = -1;
+ goto exit;
+ }
if (strchr(perms, ' ')) {
perm_prefix = "{ ";
perm_suffix = " }";
@@ -311,6 +316,7 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
flavor, classkey,
perm_prefix, perms+1, perm_suffix,
expr);
+ free(perms);
free(expr);
if (rc != 0) {
goto exit;
@@ -1682,7 +1688,7 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
{
uint32_t data = datum->data;
type_datum_t *type;
- const char *flavor, *src, *tgt, *class, *perms, *new;
+ const char *flavor, *src, *tgt, *class, *new;
char *rule = NULL, *permstring;
switch (0xFFF & key->specified) {
@@ -1730,13 +1736,14 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
class = pdb->p_class_val_to_name[key->target_class - 1];
if (key->specified & AVTAB_AV) {
- perms = sepol_av_to_string(pdb, key->target_class, data);
- if (perms == NULL) {
+ permstring = sepol_av_to_string(pdb, key->target_class, data);
+ if (permstring == NULL) {
ERR(NULL, "Failed to generate permission string");
goto exit;
}
rule = create_str("%s %s %s:%s { %s };",
- flavor, src, tgt, class, perms+1);
+ flavor, src, tgt, class, permstring+1);
+ free(permstring);
} else if (key->specified & AVTAB_XPERMS) {
permstring = sepol_extended_perms_to_string(datum->xperms);
if (permstring == NULL) {
diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index ee22dbbd..2ec66292 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -597,6 +597,7 @@ static int avrule_to_cil(int indent, struct policydb *pdb, uint32_t type, const
rule, src, tgt,
pdb->p_class_val_to_name[classperm->tclass - 1],
perms + 1);
+ free(perms);
} else {
cil_println(indent, "(%s %s %s %s %s)",
rule, src, tgt,
@@ -1967,7 +1968,13 @@ static int constraints_to_cil(int indent, struct policydb *pdb, char *classkey,
if (is_constraint) {
perms = sepol_av_to_string(pdb, class->s.value, node->permissions);
+ if (perms == NULL) {
+ ERR(NULL, "Failed to generate permission string");
+ rc = -1;
+ goto exit;
+ }
cil_println(indent, "(%sconstrain (%s (%s)) %s)", mls, classkey, perms + 1, expr);
+ free(perms);
} else {
cil_println(indent, "(%svalidatetrans %s %s)", mls, classkey, expr);
}
diff --git a/libsepol/src/services.c b/libsepol/src/services.c
index 0eeee7ec..36e2368f 100644
--- a/libsepol/src/services.c
+++ b/libsepol/src/services.c
@@ -347,9 +347,11 @@ static char *get_class_info(sepol_security_class_t tclass,
p += len;
buf_used += len;
if (state_num < 2) {
+ char *permstr = sepol_av_to_string(policydb, tclass, constraint->permissions);
+
len = snprintf(p, class_buf_len - buf_used, "{%s } (",
- sepol_av_to_string(policydb, tclass,
- constraint->permissions));
+ permstr ?: "<format-failure>");
+ free(permstr);
} else {
len = snprintf(p, class_buf_len - buf_used, "(");
}
@@ -1237,7 +1239,25 @@ out:
const char *sepol_av_perm_to_string(sepol_security_class_t tclass,
sepol_access_vector_t av)
{
- return sepol_av_to_string(policydb, tclass, av);
+ static char avbuf[1024];
+ char *avstr = sepol_av_to_string(policydb, tclass, av);
+ size_t len;
+
+ memset(avbuf, 0, sizeof(avbuf));
+
+ if (avstr) {
+ len = strlen(avstr);
+ if (len < sizeof(avbuf)) {
+ strcpy(avbuf, avstr);
+ } else {
+ sprintf(avbuf, "<access-vector overflowed buffer>");
+ }
+ free(avstr);
+ } else {
+ sprintf(avbuf, "<format-failure>");
+ }
+
+ return avbuf;
}
/*
diff --git a/libsepol/src/util.c b/libsepol/src/util.c
index 2f877920..dcbdccf1 100644
--- a/libsepol/src/util.c
+++ b/libsepol/src/util.c
@@ -32,7 +32,7 @@
struct val_to_name {
unsigned int val;
- char *name;
+ const char *name;
};
/* Add an unsigned integer to a dynamically reallocated array. *cnt
@@ -82,20 +82,27 @@ static int perm_name(hashtab_key_t key, hashtab_datum_t datum, void *data)
return 0;
}
-char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
+char *sepol_av_to_string(const policydb_t *policydbp, sepol_security_class_t tclass,
sepol_access_vector_t av)
{
struct val_to_name v;
- static char avbuf[1024];
- class_datum_t *cladatum;
- char *perm = NULL, *p;
- unsigned int i;
+ const class_datum_t *cladatum = policydbp->class_val_to_struct[tclass - 1];
+ uint32_t i;
int rc;
- int avlen = 0, len;
+ char *buffer = NULL, *p;
+ int len;
+ size_t remaining, size = 64;
+
+retry:
+ if (__builtin_mul_overflow(size, 2, &size))
+ goto err;
+ p = realloc(buffer, size);
+ if (!p)
+ goto err;
+ *p = '\0'; /* Just in case there are no permissions */
+ buffer = p;
+ remaining = size;
- memset(avbuf, 0, sizeof avbuf);
- cladatum = policydbp->class_val_to_struct[tclass - 1];
- p = avbuf;
for (i = 0; i < cladatum->permissions.nprim; i++) {
if (av & (UINT32_C(1) << i)) {
v.val = i + 1;
@@ -106,22 +113,23 @@ char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
permissions.table, perm_name,
&v);
}
- if (rc)
- perm = v.name;
- if (perm) {
- len =
- snprintf(p, sizeof(avbuf) - avlen, " %s",
- perm);
- if (len < 0
- || (size_t) len >= (sizeof(avbuf) - avlen))
- return NULL;
+ if (rc == 1) {
+ len = snprintf(p, remaining, " %s", v.name);
+ if (len < 0)
+ goto err;
+ if ((size_t) len >= remaining)
+ goto retry;
p += len;
- avlen += len;
+ remaining -= len;
}
}
}
- return avbuf;
+ return buffer;
+
+err:
+ free(buffer);
+ return NULL;
}
#define next_bit_in_range(i, p) (((i) + 1 < sizeof(p)*8) && xperm_test(((i) + 1), p))
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] libsepol: Use a dynamic buffer in sepol_av_to_string()
2023-12-12 15:11 [PATCH v2] libsepol: Use a dynamic buffer in sepol_av_to_string() James Carter
@ 2024-02-26 16:25 ` Christian Göttsche
2024-02-28 13:47 ` James Carter
2024-03-04 19:15 ` James Carter
1 sibling, 1 reply; 4+ messages in thread
From: Christian Göttsche @ 2024-02-26 16:25 UTC (permalink / raw)
To: James Carter; +Cc: selinux
On Tue, 12 Dec 2023 at 16:11, James Carter <jwcart2@gmail.com> wrote:
>
> In the internal function sepol_av_to_string(), use a dynamically
> allocated buffer for the permission names of an access vector instead
> of a fixed static buffer to support very long permission names.
>
> Update the internal users of sepol_av_to_string() to free the buffer.
>
> The exported function sepol_perm_to_string() is just a wrapper to
> the internal function. To avoid changing the behavior of this function,
> use a static buffer and copy the resulting string from the internal
> function. If the string is too long for the buffer or there was an
> error in creating the string, return a string indicating the error.
>
> All of the changes to the internal function and users was the work
> of Christian Göttsche <cgzones@googlemail.com>.
>
> Reported-by: oss-fuzz (issue 64832, 64933)
> Suggested-by: Christian Göttsche <cgzones@googlemail.com>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
> v2: In sepol_av_perm_to_string():
> - Use "sizeof(avbuf)" instead of 1024
> - Added "free(avstr)"
> In sepol_av_to_string():
> - Added "*p = '\0';" to ensure buffer is initialized
Kindly Ping.
Any progress on this one?
> checkpolicy/test/dismod.c | 11 +++---
> checkpolicy/test/dispol.c | 7 ++--
> libsepol/include/sepol/policydb/util.h | 2 +-
> libsepol/src/assertion.c | 16 ++++++---
> libsepol/src/hierarchy.c | 7 ++--
> libsepol/src/kernel_to_cil.c | 7 ++++
> libsepol/src/kernel_to_conf.c | 15 +++++---
> libsepol/src/module_to_cil.c | 7 ++++
> libsepol/src/services.c | 26 ++++++++++++--
> libsepol/src/util.c | 50 +++++++++++++++-----------
> 10 files changed, 103 insertions(+), 45 deletions(-)
>
> diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
> index ac2d61d2..bd45c95e 100644
> --- a/checkpolicy/test/dismod.c
> +++ b/checkpolicy/test/dismod.c
> @@ -118,12 +118,11 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
> static void render_access_mask(uint32_t mask, uint32_t class, policydb_t * p,
> FILE * fp)
> {
> - char *perm;
> + char *perm = sepol_av_to_string(p, class, mask);
> fprintf(fp, "{");
> - perm = sepol_av_to_string(p, class, mask);
> - if (perm)
> - fprintf(fp, "%s ", perm);
> + fprintf(fp, "%s ", perm ?: "<format-failure>");
> fprintf(fp, "}");
> + free(perm);
> }
>
> static void render_access_bitmap(ebitmap_t * map, uint32_t class,
> @@ -135,8 +134,8 @@ static void render_access_bitmap(ebitmap_t * map, uint32_t class,
> for (i = ebitmap_startbit(map); i < ebitmap_length(map); i++) {
> if (ebitmap_get_bit(map, i)) {
> perm = sepol_av_to_string(p, class, UINT32_C(1) << i);
> - if (perm)
> - fprintf(fp, "%s", perm);
> + fprintf(fp, "%s", perm ?: "<format-failure>");
> + free(perm);
> }
> }
> fprintf(fp, " }");
> diff --git a/checkpolicy/test/dispol.c b/checkpolicy/test/dispol.c
> index 944ef7ec..2662048e 100644
> --- a/checkpolicy/test/dispol.c
> +++ b/checkpolicy/test/dispol.c
> @@ -93,12 +93,11 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
> static int render_access_mask(uint32_t mask, avtab_key_t * key, policydb_t * p,
> FILE * fp)
> {
> - char *perm;
> + char *perm = sepol_av_to_string(p, key->target_class, mask);
> fprintf(fp, "{");
> - perm = sepol_av_to_string(p, key->target_class, mask);
> - if (perm)
> - fprintf(fp, "%s ", perm);
> + fprintf(fp, "%s ", perm ?: "<format-failure>");
> fprintf(fp, "}");
> + free(perm);
> return 0;
> }
>
> diff --git a/libsepol/include/sepol/policydb/util.h b/libsepol/include/sepol/policydb/util.h
> index db8da213..70c531d3 100644
> --- a/libsepol/include/sepol/policydb/util.h
> +++ b/libsepol/include/sepol/policydb/util.h
> @@ -31,7 +31,7 @@ extern "C" {
>
> extern int add_i_to_a(uint32_t i, uint32_t * cnt, uint32_t ** a);
>
> -extern char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
> +extern char *sepol_av_to_string(const policydb_t *policydbp, sepol_security_class_t tclass,
> sepol_access_vector_t av);
>
> char *sepol_extended_perms_to_string(avtab_extended_perms_t *xperms);
> diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
> index 6de7d031..3076babe 100644
> --- a/libsepol/src/assertion.c
> +++ b/libsepol/src/assertion.c
> @@ -48,26 +48,30 @@ static void report_failure(sepol_handle_t *handle, policydb_t *p, const avrule_t
> unsigned int stype, unsigned int ttype,
> const class_perm_node_t *curperm, uint32_t perms)
> {
> + char *permstr = sepol_av_to_string(p, curperm->tclass, perms);
> +
> if (avrule->source_filename) {
> ERR(handle, "neverallow on line %lu of %s (or line %lu of %s) violated by allow %s %s:%s {%s };",
> avrule->source_line, avrule->source_filename, avrule->line, policy_name(p),
> p->p_type_val_to_name[stype],
> p->p_type_val_to_name[ttype],
> p->p_class_val_to_name[curperm->tclass - 1],
> - sepol_av_to_string(p, curperm->tclass, perms));
> + permstr ?: "<format-failure>");
> } else if (avrule->line) {
> ERR(handle, "neverallow on line %lu violated by allow %s %s:%s {%s };",
> avrule->line, p->p_type_val_to_name[stype],
> p->p_type_val_to_name[ttype],
> p->p_class_val_to_name[curperm->tclass - 1],
> - sepol_av_to_string(p, curperm->tclass, perms));
> + permstr ?: "<format-failure>");
> } else {
> ERR(handle, "neverallow violated by allow %s %s:%s {%s };",
> p->p_type_val_to_name[stype],
> p->p_type_val_to_name[ttype],
> p->p_class_val_to_name[curperm->tclass - 1],
> - sepol_av_to_string(p, curperm->tclass, perms));
> + permstr ?: "<format-failure>");
> }
> +
> + free(permstr);
> }
>
> static int match_any_class_permissions(class_perm_node_t *cp, uint32_t class, uint32_t data)
> @@ -200,13 +204,17 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
>
> /* failure on the regular permissions */
> if (!found_xperm) {
> + char *permstr = sepol_av_to_string(p, curperm->tclass, perms);
> +
> ERR(handle, "neverallowxperm on line %lu of %s (or line %lu of %s) violated by\n"
> "allow %s %s:%s {%s };",
> avrule->source_line, avrule->source_filename, avrule->line, policy_name(p),
> p->p_type_val_to_name[stype],
> p->p_type_val_to_name[ttype],
> p->p_class_val_to_name[curperm->tclass - 1],
> - sepol_av_to_string(p, curperm->tclass, perms));
> + permstr ?: "<format-failure>");
> +
> + free(permstr);
> errors++;
>
> }
> diff --git a/libsepol/src/hierarchy.c b/libsepol/src/hierarchy.c
> index 350443a8..06e05310 100644
> --- a/libsepol/src/hierarchy.c
> +++ b/libsepol/src/hierarchy.c
> @@ -443,12 +443,15 @@ static void bounds_report(sepol_handle_t *handle, policydb_t *p, uint32_t child,
> p->p_type_val_to_name[child - 1],
> p->p_type_val_to_name[parent - 1]);
> for (; cur; cur = cur->next) {
> + char *permstr = sepol_av_to_string(p, cur->key.target_class, cur->datum.data);
> +
> ERR(handle, " %s %s : %s { %s }",
> p->p_type_val_to_name[cur->key.source_type - 1],
> p->p_type_val_to_name[cur->key.target_type - 1],
> p->p_class_val_to_name[cur->key.target_class - 1],
> - sepol_av_to_string(p, cur->key.target_class,
> - cur->datum.data));
> + permstr ?: "<format-failure>");
> +
> + free(permstr);
> }
> }
>
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index bcb58eee..634826d5 100644
> --- a/libsepol/src/kernel_to_cil.c
> +++ b/libsepol/src/kernel_to_cil.c
> @@ -297,6 +297,11 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> }
>
> perms = sepol_av_to_string(pdb, class->s.value, curr->permissions);
> + if (!perms) {
> + ERR(NULL, "Failed to generate permission string");
> + rc = -1;
> + goto exit;
> + }
>
> if (is_mls) {
> key_word = "mlsconstrain";
> @@ -307,6 +312,7 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> }
>
> rc = strs_create_and_add(strs, "(%s (%s (%s)) %s)", key_word, classkey, perms+1, expr);
> + free(perms);
> free(expr);
> if (rc != 0) {
> goto exit;
> @@ -1772,6 +1778,7 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
> }
> rule = create_str("(%s %s %s (%s (%s)))",
> flavor, src, tgt, class, perms+1);
> + free(perms);
> } else if (key->specified & AVTAB_XPERMS) {
> perms = xperms_to_str(datum->xperms);
> if (perms == NULL) {
> diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
> index 83f46e0f..de1d9e09 100644
> --- a/libsepol/src/kernel_to_conf.c
> +++ b/libsepol/src/kernel_to_conf.c
> @@ -292,6 +292,11 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> }
>
> perms = sepol_av_to_string(pdb, class->s.value, curr->permissions);
> + if (!perms) {
> + ERR(NULL, "Failed to generate permission string");
> + rc = -1;
> + goto exit;
> + }
> if (strchr(perms, ' ')) {
> perm_prefix = "{ ";
> perm_suffix = " }";
> @@ -311,6 +316,7 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> flavor, classkey,
> perm_prefix, perms+1, perm_suffix,
> expr);
> + free(perms);
> free(expr);
> if (rc != 0) {
> goto exit;
> @@ -1682,7 +1688,7 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
> {
> uint32_t data = datum->data;
> type_datum_t *type;
> - const char *flavor, *src, *tgt, *class, *perms, *new;
> + const char *flavor, *src, *tgt, *class, *new;
> char *rule = NULL, *permstring;
>
> switch (0xFFF & key->specified) {
> @@ -1730,13 +1736,14 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
> class = pdb->p_class_val_to_name[key->target_class - 1];
>
> if (key->specified & AVTAB_AV) {
> - perms = sepol_av_to_string(pdb, key->target_class, data);
> - if (perms == NULL) {
> + permstring = sepol_av_to_string(pdb, key->target_class, data);
> + if (permstring == NULL) {
> ERR(NULL, "Failed to generate permission string");
> goto exit;
> }
> rule = create_str("%s %s %s:%s { %s };",
> - flavor, src, tgt, class, perms+1);
> + flavor, src, tgt, class, permstring+1);
> + free(permstring);
> } else if (key->specified & AVTAB_XPERMS) {
> permstring = sepol_extended_perms_to_string(datum->xperms);
> if (permstring == NULL) {
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index ee22dbbd..2ec66292 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -597,6 +597,7 @@ static int avrule_to_cil(int indent, struct policydb *pdb, uint32_t type, const
> rule, src, tgt,
> pdb->p_class_val_to_name[classperm->tclass - 1],
> perms + 1);
> + free(perms);
> } else {
> cil_println(indent, "(%s %s %s %s %s)",
> rule, src, tgt,
> @@ -1967,7 +1968,13 @@ static int constraints_to_cil(int indent, struct policydb *pdb, char *classkey,
>
> if (is_constraint) {
> perms = sepol_av_to_string(pdb, class->s.value, node->permissions);
> + if (perms == NULL) {
> + ERR(NULL, "Failed to generate permission string");
> + rc = -1;
> + goto exit;
> + }
> cil_println(indent, "(%sconstrain (%s (%s)) %s)", mls, classkey, perms + 1, expr);
> + free(perms);
> } else {
> cil_println(indent, "(%svalidatetrans %s %s)", mls, classkey, expr);
> }
> diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> index 0eeee7ec..36e2368f 100644
> --- a/libsepol/src/services.c
> +++ b/libsepol/src/services.c
> @@ -347,9 +347,11 @@ static char *get_class_info(sepol_security_class_t tclass,
> p += len;
> buf_used += len;
> if (state_num < 2) {
> + char *permstr = sepol_av_to_string(policydb, tclass, constraint->permissions);
> +
> len = snprintf(p, class_buf_len - buf_used, "{%s } (",
> - sepol_av_to_string(policydb, tclass,
> - constraint->permissions));
> + permstr ?: "<format-failure>");
> + free(permstr);
> } else {
> len = snprintf(p, class_buf_len - buf_used, "(");
> }
> @@ -1237,7 +1239,25 @@ out:
> const char *sepol_av_perm_to_string(sepol_security_class_t tclass,
> sepol_access_vector_t av)
> {
> - return sepol_av_to_string(policydb, tclass, av);
> + static char avbuf[1024];
> + char *avstr = sepol_av_to_string(policydb, tclass, av);
> + size_t len;
> +
> + memset(avbuf, 0, sizeof(avbuf));
> +
> + if (avstr) {
> + len = strlen(avstr);
> + if (len < sizeof(avbuf)) {
> + strcpy(avbuf, avstr);
> + } else {
> + sprintf(avbuf, "<access-vector overflowed buffer>");
> + }
> + free(avstr);
> + } else {
> + sprintf(avbuf, "<format-failure>");
> + }
> +
> + return avbuf;
> }
>
> /*
> diff --git a/libsepol/src/util.c b/libsepol/src/util.c
> index 2f877920..dcbdccf1 100644
> --- a/libsepol/src/util.c
> +++ b/libsepol/src/util.c
> @@ -32,7 +32,7 @@
>
> struct val_to_name {
> unsigned int val;
> - char *name;
> + const char *name;
> };
>
> /* Add an unsigned integer to a dynamically reallocated array. *cnt
> @@ -82,20 +82,27 @@ static int perm_name(hashtab_key_t key, hashtab_datum_t datum, void *data)
> return 0;
> }
>
> -char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
> +char *sepol_av_to_string(const policydb_t *policydbp, sepol_security_class_t tclass,
> sepol_access_vector_t av)
> {
> struct val_to_name v;
> - static char avbuf[1024];
> - class_datum_t *cladatum;
> - char *perm = NULL, *p;
> - unsigned int i;
> + const class_datum_t *cladatum = policydbp->class_val_to_struct[tclass - 1];
> + uint32_t i;
> int rc;
> - int avlen = 0, len;
> + char *buffer = NULL, *p;
> + int len;
> + size_t remaining, size = 64;
> +
> +retry:
> + if (__builtin_mul_overflow(size, 2, &size))
> + goto err;
> + p = realloc(buffer, size);
> + if (!p)
> + goto err;
> + *p = '\0'; /* Just in case there are no permissions */
> + buffer = p;
> + remaining = size;
>
> - memset(avbuf, 0, sizeof avbuf);
> - cladatum = policydbp->class_val_to_struct[tclass - 1];
> - p = avbuf;
> for (i = 0; i < cladatum->permissions.nprim; i++) {
> if (av & (UINT32_C(1) << i)) {
> v.val = i + 1;
> @@ -106,22 +113,23 @@ char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
> permissions.table, perm_name,
> &v);
> }
> - if (rc)
> - perm = v.name;
> - if (perm) {
> - len =
> - snprintf(p, sizeof(avbuf) - avlen, " %s",
> - perm);
> - if (len < 0
> - || (size_t) len >= (sizeof(avbuf) - avlen))
> - return NULL;
> + if (rc == 1) {
> + len = snprintf(p, remaining, " %s", v.name);
> + if (len < 0)
> + goto err;
> + if ((size_t) len >= remaining)
> + goto retry;
> p += len;
> - avlen += len;
> + remaining -= len;
> }
> }
> }
>
> - return avbuf;
> + return buffer;
> +
> +err:
> + free(buffer);
> + return NULL;
> }
>
> #define next_bit_in_range(i, p) (((i) + 1 < sizeof(p)*8) && xperm_test(((i) + 1), p))
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] libsepol: Use a dynamic buffer in sepol_av_to_string()
2024-02-26 16:25 ` Christian Göttsche
@ 2024-02-28 13:47 ` James Carter
0 siblings, 0 replies; 4+ messages in thread
From: James Carter @ 2024-02-28 13:47 UTC (permalink / raw)
To: Christian Göttsche; +Cc: selinux
On Mon, Feb 26, 2024 at 11:25 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> On Tue, 12 Dec 2023 at 16:11, James Carter <jwcart2@gmail.com> wrote:
> >
> > In the internal function sepol_av_to_string(), use a dynamically
> > allocated buffer for the permission names of an access vector instead
> > of a fixed static buffer to support very long permission names.
> >
> > Update the internal users of sepol_av_to_string() to free the buffer.
> >
> > The exported function sepol_perm_to_string() is just a wrapper to
> > the internal function. To avoid changing the behavior of this function,
> > use a static buffer and copy the resulting string from the internal
> > function. If the string is too long for the buffer or there was an
> > error in creating the string, return a string indicating the error.
> >
> > All of the changes to the internal function and users was the work
> > of Christian Göttsche <cgzones@googlemail.com>.
> >
> > Reported-by: oss-fuzz (issue 64832, 64933)
> > Suggested-by: Christian Göttsche <cgzones@googlemail.com>
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> > v2: In sepol_av_perm_to_string():
> > - Use "sizeof(avbuf)" instead of 1024
> > - Added "free(avstr)"
> > In sepol_av_to_string():
> > - Added "*p = '\0';" to ensure buffer is initialized
>
> Kindly Ping.
> Any progress on this one?
>
I plan on merging this soon.
Thanks,
Jim
> > checkpolicy/test/dismod.c | 11 +++---
> > checkpolicy/test/dispol.c | 7 ++--
> > libsepol/include/sepol/policydb/util.h | 2 +-
> > libsepol/src/assertion.c | 16 ++++++---
> > libsepol/src/hierarchy.c | 7 ++--
> > libsepol/src/kernel_to_cil.c | 7 ++++
> > libsepol/src/kernel_to_conf.c | 15 +++++---
> > libsepol/src/module_to_cil.c | 7 ++++
> > libsepol/src/services.c | 26 ++++++++++++--
> > libsepol/src/util.c | 50 +++++++++++++++-----------
> > 10 files changed, 103 insertions(+), 45 deletions(-)
> >
> > diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
> > index ac2d61d2..bd45c95e 100644
> > --- a/checkpolicy/test/dismod.c
> > +++ b/checkpolicy/test/dismod.c
> > @@ -118,12 +118,11 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
> > static void render_access_mask(uint32_t mask, uint32_t class, policydb_t * p,
> > FILE * fp)
> > {
> > - char *perm;
> > + char *perm = sepol_av_to_string(p, class, mask);
> > fprintf(fp, "{");
> > - perm = sepol_av_to_string(p, class, mask);
> > - if (perm)
> > - fprintf(fp, "%s ", perm);
> > + fprintf(fp, "%s ", perm ?: "<format-failure>");
> > fprintf(fp, "}");
> > + free(perm);
> > }
> >
> > static void render_access_bitmap(ebitmap_t * map, uint32_t class,
> > @@ -135,8 +134,8 @@ static void render_access_bitmap(ebitmap_t * map, uint32_t class,
> > for (i = ebitmap_startbit(map); i < ebitmap_length(map); i++) {
> > if (ebitmap_get_bit(map, i)) {
> > perm = sepol_av_to_string(p, class, UINT32_C(1) << i);
> > - if (perm)
> > - fprintf(fp, "%s", perm);
> > + fprintf(fp, "%s", perm ?: "<format-failure>");
> > + free(perm);
> > }
> > }
> > fprintf(fp, " }");
> > diff --git a/checkpolicy/test/dispol.c b/checkpolicy/test/dispol.c
> > index 944ef7ec..2662048e 100644
> > --- a/checkpolicy/test/dispol.c
> > +++ b/checkpolicy/test/dispol.c
> > @@ -93,12 +93,11 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
> > static int render_access_mask(uint32_t mask, avtab_key_t * key, policydb_t * p,
> > FILE * fp)
> > {
> > - char *perm;
> > + char *perm = sepol_av_to_string(p, key->target_class, mask);
> > fprintf(fp, "{");
> > - perm = sepol_av_to_string(p, key->target_class, mask);
> > - if (perm)
> > - fprintf(fp, "%s ", perm);
> > + fprintf(fp, "%s ", perm ?: "<format-failure>");
> > fprintf(fp, "}");
> > + free(perm);
> > return 0;
> > }
> >
> > diff --git a/libsepol/include/sepol/policydb/util.h b/libsepol/include/sepol/policydb/util.h
> > index db8da213..70c531d3 100644
> > --- a/libsepol/include/sepol/policydb/util.h
> > +++ b/libsepol/include/sepol/policydb/util.h
> > @@ -31,7 +31,7 @@ extern "C" {
> >
> > extern int add_i_to_a(uint32_t i, uint32_t * cnt, uint32_t ** a);
> >
> > -extern char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
> > +extern char *sepol_av_to_string(const policydb_t *policydbp, sepol_security_class_t tclass,
> > sepol_access_vector_t av);
> >
> > char *sepol_extended_perms_to_string(avtab_extended_perms_t *xperms);
> > diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
> > index 6de7d031..3076babe 100644
> > --- a/libsepol/src/assertion.c
> > +++ b/libsepol/src/assertion.c
> > @@ -48,26 +48,30 @@ static void report_failure(sepol_handle_t *handle, policydb_t *p, const avrule_t
> > unsigned int stype, unsigned int ttype,
> > const class_perm_node_t *curperm, uint32_t perms)
> > {
> > + char *permstr = sepol_av_to_string(p, curperm->tclass, perms);
> > +
> > if (avrule->source_filename) {
> > ERR(handle, "neverallow on line %lu of %s (or line %lu of %s) violated by allow %s %s:%s {%s };",
> > avrule->source_line, avrule->source_filename, avrule->line, policy_name(p),
> > p->p_type_val_to_name[stype],
> > p->p_type_val_to_name[ttype],
> > p->p_class_val_to_name[curperm->tclass - 1],
> > - sepol_av_to_string(p, curperm->tclass, perms));
> > + permstr ?: "<format-failure>");
> > } else if (avrule->line) {
> > ERR(handle, "neverallow on line %lu violated by allow %s %s:%s {%s };",
> > avrule->line, p->p_type_val_to_name[stype],
> > p->p_type_val_to_name[ttype],
> > p->p_class_val_to_name[curperm->tclass - 1],
> > - sepol_av_to_string(p, curperm->tclass, perms));
> > + permstr ?: "<format-failure>");
> > } else {
> > ERR(handle, "neverallow violated by allow %s %s:%s {%s };",
> > p->p_type_val_to_name[stype],
> > p->p_type_val_to_name[ttype],
> > p->p_class_val_to_name[curperm->tclass - 1],
> > - sepol_av_to_string(p, curperm->tclass, perms));
> > + permstr ?: "<format-failure>");
> > }
> > +
> > + free(permstr);
> > }
> >
> > static int match_any_class_permissions(class_perm_node_t *cp, uint32_t class, uint32_t data)
> > @@ -200,13 +204,17 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
> >
> > /* failure on the regular permissions */
> > if (!found_xperm) {
> > + char *permstr = sepol_av_to_string(p, curperm->tclass, perms);
> > +
> > ERR(handle, "neverallowxperm on line %lu of %s (or line %lu of %s) violated by\n"
> > "allow %s %s:%s {%s };",
> > avrule->source_line, avrule->source_filename, avrule->line, policy_name(p),
> > p->p_type_val_to_name[stype],
> > p->p_type_val_to_name[ttype],
> > p->p_class_val_to_name[curperm->tclass - 1],
> > - sepol_av_to_string(p, curperm->tclass, perms));
> > + permstr ?: "<format-failure>");
> > +
> > + free(permstr);
> > errors++;
> >
> > }
> > diff --git a/libsepol/src/hierarchy.c b/libsepol/src/hierarchy.c
> > index 350443a8..06e05310 100644
> > --- a/libsepol/src/hierarchy.c
> > +++ b/libsepol/src/hierarchy.c
> > @@ -443,12 +443,15 @@ static void bounds_report(sepol_handle_t *handle, policydb_t *p, uint32_t child,
> > p->p_type_val_to_name[child - 1],
> > p->p_type_val_to_name[parent - 1]);
> > for (; cur; cur = cur->next) {
> > + char *permstr = sepol_av_to_string(p, cur->key.target_class, cur->datum.data);
> > +
> > ERR(handle, " %s %s : %s { %s }",
> > p->p_type_val_to_name[cur->key.source_type - 1],
> > p->p_type_val_to_name[cur->key.target_type - 1],
> > p->p_class_val_to_name[cur->key.target_class - 1],
> > - sepol_av_to_string(p, cur->key.target_class,
> > - cur->datum.data));
> > + permstr ?: "<format-failure>");
> > +
> > + free(permstr);
> > }
> > }
> >
> > diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> > index bcb58eee..634826d5 100644
> > --- a/libsepol/src/kernel_to_cil.c
> > +++ b/libsepol/src/kernel_to_cil.c
> > @@ -297,6 +297,11 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> > }
> >
> > perms = sepol_av_to_string(pdb, class->s.value, curr->permissions);
> > + if (!perms) {
> > + ERR(NULL, "Failed to generate permission string");
> > + rc = -1;
> > + goto exit;
> > + }
> >
> > if (is_mls) {
> > key_word = "mlsconstrain";
> > @@ -307,6 +312,7 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> > }
> >
> > rc = strs_create_and_add(strs, "(%s (%s (%s)) %s)", key_word, classkey, perms+1, expr);
> > + free(perms);
> > free(expr);
> > if (rc != 0) {
> > goto exit;
> > @@ -1772,6 +1778,7 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
> > }
> > rule = create_str("(%s %s %s (%s (%s)))",
> > flavor, src, tgt, class, perms+1);
> > + free(perms);
> > } else if (key->specified & AVTAB_XPERMS) {
> > perms = xperms_to_str(datum->xperms);
> > if (perms == NULL) {
> > diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
> > index 83f46e0f..de1d9e09 100644
> > --- a/libsepol/src/kernel_to_conf.c
> > +++ b/libsepol/src/kernel_to_conf.c
> > @@ -292,6 +292,11 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> > }
> >
> > perms = sepol_av_to_string(pdb, class->s.value, curr->permissions);
> > + if (!perms) {
> > + ERR(NULL, "Failed to generate permission string");
> > + rc = -1;
> > + goto exit;
> > + }
> > if (strchr(perms, ' ')) {
> > perm_prefix = "{ ";
> > perm_suffix = " }";
> > @@ -311,6 +316,7 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> > flavor, classkey,
> > perm_prefix, perms+1, perm_suffix,
> > expr);
> > + free(perms);
> > free(expr);
> > if (rc != 0) {
> > goto exit;
> > @@ -1682,7 +1688,7 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
> > {
> > uint32_t data = datum->data;
> > type_datum_t *type;
> > - const char *flavor, *src, *tgt, *class, *perms, *new;
> > + const char *flavor, *src, *tgt, *class, *new;
> > char *rule = NULL, *permstring;
> >
> > switch (0xFFF & key->specified) {
> > @@ -1730,13 +1736,14 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
> > class = pdb->p_class_val_to_name[key->target_class - 1];
> >
> > if (key->specified & AVTAB_AV) {
> > - perms = sepol_av_to_string(pdb, key->target_class, data);
> > - if (perms == NULL) {
> > + permstring = sepol_av_to_string(pdb, key->target_class, data);
> > + if (permstring == NULL) {
> > ERR(NULL, "Failed to generate permission string");
> > goto exit;
> > }
> > rule = create_str("%s %s %s:%s { %s };",
> > - flavor, src, tgt, class, perms+1);
> > + flavor, src, tgt, class, permstring+1);
> > + free(permstring);
> > } else if (key->specified & AVTAB_XPERMS) {
> > permstring = sepol_extended_perms_to_string(datum->xperms);
> > if (permstring == NULL) {
> > diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> > index ee22dbbd..2ec66292 100644
> > --- a/libsepol/src/module_to_cil.c
> > +++ b/libsepol/src/module_to_cil.c
> > @@ -597,6 +597,7 @@ static int avrule_to_cil(int indent, struct policydb *pdb, uint32_t type, const
> > rule, src, tgt,
> > pdb->p_class_val_to_name[classperm->tclass - 1],
> > perms + 1);
> > + free(perms);
> > } else {
> > cil_println(indent, "(%s %s %s %s %s)",
> > rule, src, tgt,
> > @@ -1967,7 +1968,13 @@ static int constraints_to_cil(int indent, struct policydb *pdb, char *classkey,
> >
> > if (is_constraint) {
> > perms = sepol_av_to_string(pdb, class->s.value, node->permissions);
> > + if (perms == NULL) {
> > + ERR(NULL, "Failed to generate permission string");
> > + rc = -1;
> > + goto exit;
> > + }
> > cil_println(indent, "(%sconstrain (%s (%s)) %s)", mls, classkey, perms + 1, expr);
> > + free(perms);
> > } else {
> > cil_println(indent, "(%svalidatetrans %s %s)", mls, classkey, expr);
> > }
> > diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> > index 0eeee7ec..36e2368f 100644
> > --- a/libsepol/src/services.c
> > +++ b/libsepol/src/services.c
> > @@ -347,9 +347,11 @@ static char *get_class_info(sepol_security_class_t tclass,
> > p += len;
> > buf_used += len;
> > if (state_num < 2) {
> > + char *permstr = sepol_av_to_string(policydb, tclass, constraint->permissions);
> > +
> > len = snprintf(p, class_buf_len - buf_used, "{%s } (",
> > - sepol_av_to_string(policydb, tclass,
> > - constraint->permissions));
> > + permstr ?: "<format-failure>");
> > + free(permstr);
> > } else {
> > len = snprintf(p, class_buf_len - buf_used, "(");
> > }
> > @@ -1237,7 +1239,25 @@ out:
> > const char *sepol_av_perm_to_string(sepol_security_class_t tclass,
> > sepol_access_vector_t av)
> > {
> > - return sepol_av_to_string(policydb, tclass, av);
> > + static char avbuf[1024];
> > + char *avstr = sepol_av_to_string(policydb, tclass, av);
> > + size_t len;
> > +
> > + memset(avbuf, 0, sizeof(avbuf));
> > +
> > + if (avstr) {
> > + len = strlen(avstr);
> > + if (len < sizeof(avbuf)) {
> > + strcpy(avbuf, avstr);
> > + } else {
> > + sprintf(avbuf, "<access-vector overflowed buffer>");
> > + }
> > + free(avstr);
> > + } else {
> > + sprintf(avbuf, "<format-failure>");
> > + }
> > +
> > + return avbuf;
> > }
> >
> > /*
> > diff --git a/libsepol/src/util.c b/libsepol/src/util.c
> > index 2f877920..dcbdccf1 100644
> > --- a/libsepol/src/util.c
> > +++ b/libsepol/src/util.c
> > @@ -32,7 +32,7 @@
> >
> > struct val_to_name {
> > unsigned int val;
> > - char *name;
> > + const char *name;
> > };
> >
> > /* Add an unsigned integer to a dynamically reallocated array. *cnt
> > @@ -82,20 +82,27 @@ static int perm_name(hashtab_key_t key, hashtab_datum_t datum, void *data)
> > return 0;
> > }
> >
> > -char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
> > +char *sepol_av_to_string(const policydb_t *policydbp, sepol_security_class_t tclass,
> > sepol_access_vector_t av)
> > {
> > struct val_to_name v;
> > - static char avbuf[1024];
> > - class_datum_t *cladatum;
> > - char *perm = NULL, *p;
> > - unsigned int i;
> > + const class_datum_t *cladatum = policydbp->class_val_to_struct[tclass - 1];
> > + uint32_t i;
> > int rc;
> > - int avlen = 0, len;
> > + char *buffer = NULL, *p;
> > + int len;
> > + size_t remaining, size = 64;
> > +
> > +retry:
> > + if (__builtin_mul_overflow(size, 2, &size))
> > + goto err;
> > + p = realloc(buffer, size);
> > + if (!p)
> > + goto err;
> > + *p = '\0'; /* Just in case there are no permissions */
> > + buffer = p;
> > + remaining = size;
> >
> > - memset(avbuf, 0, sizeof avbuf);
> > - cladatum = policydbp->class_val_to_struct[tclass - 1];
> > - p = avbuf;
> > for (i = 0; i < cladatum->permissions.nprim; i++) {
> > if (av & (UINT32_C(1) << i)) {
> > v.val = i + 1;
> > @@ -106,22 +113,23 @@ char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
> > permissions.table, perm_name,
> > &v);
> > }
> > - if (rc)
> > - perm = v.name;
> > - if (perm) {
> > - len =
> > - snprintf(p, sizeof(avbuf) - avlen, " %s",
> > - perm);
> > - if (len < 0
> > - || (size_t) len >= (sizeof(avbuf) - avlen))
> > - return NULL;
> > + if (rc == 1) {
> > + len = snprintf(p, remaining, " %s", v.name);
> > + if (len < 0)
> > + goto err;
> > + if ((size_t) len >= remaining)
> > + goto retry;
> > p += len;
> > - avlen += len;
> > + remaining -= len;
> > }
> > }
> > }
> >
> > - return avbuf;
> > + return buffer;
> > +
> > +err:
> > + free(buffer);
> > + return NULL;
> > }
> >
> > #define next_bit_in_range(i, p) (((i) + 1 < sizeof(p)*8) && xperm_test(((i) + 1), p))
> > --
> > 2.43.0
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] libsepol: Use a dynamic buffer in sepol_av_to_string()
2023-12-12 15:11 [PATCH v2] libsepol: Use a dynamic buffer in sepol_av_to_string() James Carter
2024-02-26 16:25 ` Christian Göttsche
@ 2024-03-04 19:15 ` James Carter
1 sibling, 0 replies; 4+ messages in thread
From: James Carter @ 2024-03-04 19:15 UTC (permalink / raw)
To: selinux; +Cc: cgzones
On Tue, Dec 12, 2023 at 10:11 AM James Carter <jwcart2@gmail.com> wrote:
>
> In the internal function sepol_av_to_string(), use a dynamically
> allocated buffer for the permission names of an access vector instead
> of a fixed static buffer to support very long permission names.
>
> Update the internal users of sepol_av_to_string() to free the buffer.
>
> The exported function sepol_perm_to_string() is just a wrapper to
> the internal function. To avoid changing the behavior of this function,
> use a static buffer and copy the resulting string from the internal
> function. If the string is too long for the buffer or there was an
> error in creating the string, return a string indicating the error.
>
> All of the changes to the internal function and users was the work
> of Christian Göttsche <cgzones@googlemail.com>.
>
> Reported-by: oss-fuzz (issue 64832, 64933)
> Suggested-by: Christian Göttsche <cgzones@googlemail.com>
> Signed-off-by: James Carter <jwcart2@gmail.com>
This patch has been merged.
Jim
> ---
> v2: In sepol_av_perm_to_string():
> - Use "sizeof(avbuf)" instead of 1024
> - Added "free(avstr)"
> In sepol_av_to_string():
> - Added "*p = '\0';" to ensure buffer is initialized
>
> checkpolicy/test/dismod.c | 11 +++---
> checkpolicy/test/dispol.c | 7 ++--
> libsepol/include/sepol/policydb/util.h | 2 +-
> libsepol/src/assertion.c | 16 ++++++---
> libsepol/src/hierarchy.c | 7 ++--
> libsepol/src/kernel_to_cil.c | 7 ++++
> libsepol/src/kernel_to_conf.c | 15 +++++---
> libsepol/src/module_to_cil.c | 7 ++++
> libsepol/src/services.c | 26 ++++++++++++--
> libsepol/src/util.c | 50 +++++++++++++++-----------
> 10 files changed, 103 insertions(+), 45 deletions(-)
>
> diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
> index ac2d61d2..bd45c95e 100644
> --- a/checkpolicy/test/dismod.c
> +++ b/checkpolicy/test/dismod.c
> @@ -118,12 +118,11 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
> static void render_access_mask(uint32_t mask, uint32_t class, policydb_t * p,
> FILE * fp)
> {
> - char *perm;
> + char *perm = sepol_av_to_string(p, class, mask);
> fprintf(fp, "{");
> - perm = sepol_av_to_string(p, class, mask);
> - if (perm)
> - fprintf(fp, "%s ", perm);
> + fprintf(fp, "%s ", perm ?: "<format-failure>");
> fprintf(fp, "}");
> + free(perm);
> }
>
> static void render_access_bitmap(ebitmap_t * map, uint32_t class,
> @@ -135,8 +134,8 @@ static void render_access_bitmap(ebitmap_t * map, uint32_t class,
> for (i = ebitmap_startbit(map); i < ebitmap_length(map); i++) {
> if (ebitmap_get_bit(map, i)) {
> perm = sepol_av_to_string(p, class, UINT32_C(1) << i);
> - if (perm)
> - fprintf(fp, "%s", perm);
> + fprintf(fp, "%s", perm ?: "<format-failure>");
> + free(perm);
> }
> }
> fprintf(fp, " }");
> diff --git a/checkpolicy/test/dispol.c b/checkpolicy/test/dispol.c
> index 944ef7ec..2662048e 100644
> --- a/checkpolicy/test/dispol.c
> +++ b/checkpolicy/test/dispol.c
> @@ -93,12 +93,11 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
> static int render_access_mask(uint32_t mask, avtab_key_t * key, policydb_t * p,
> FILE * fp)
> {
> - char *perm;
> + char *perm = sepol_av_to_string(p, key->target_class, mask);
> fprintf(fp, "{");
> - perm = sepol_av_to_string(p, key->target_class, mask);
> - if (perm)
> - fprintf(fp, "%s ", perm);
> + fprintf(fp, "%s ", perm ?: "<format-failure>");
> fprintf(fp, "}");
> + free(perm);
> return 0;
> }
>
> diff --git a/libsepol/include/sepol/policydb/util.h b/libsepol/include/sepol/policydb/util.h
> index db8da213..70c531d3 100644
> --- a/libsepol/include/sepol/policydb/util.h
> +++ b/libsepol/include/sepol/policydb/util.h
> @@ -31,7 +31,7 @@ extern "C" {
>
> extern int add_i_to_a(uint32_t i, uint32_t * cnt, uint32_t ** a);
>
> -extern char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
> +extern char *sepol_av_to_string(const policydb_t *policydbp, sepol_security_class_t tclass,
> sepol_access_vector_t av);
>
> char *sepol_extended_perms_to_string(avtab_extended_perms_t *xperms);
> diff --git a/libsepol/src/assertion.c b/libsepol/src/assertion.c
> index 6de7d031..3076babe 100644
> --- a/libsepol/src/assertion.c
> +++ b/libsepol/src/assertion.c
> @@ -48,26 +48,30 @@ static void report_failure(sepol_handle_t *handle, policydb_t *p, const avrule_t
> unsigned int stype, unsigned int ttype,
> const class_perm_node_t *curperm, uint32_t perms)
> {
> + char *permstr = sepol_av_to_string(p, curperm->tclass, perms);
> +
> if (avrule->source_filename) {
> ERR(handle, "neverallow on line %lu of %s (or line %lu of %s) violated by allow %s %s:%s {%s };",
> avrule->source_line, avrule->source_filename, avrule->line, policy_name(p),
> p->p_type_val_to_name[stype],
> p->p_type_val_to_name[ttype],
> p->p_class_val_to_name[curperm->tclass - 1],
> - sepol_av_to_string(p, curperm->tclass, perms));
> + permstr ?: "<format-failure>");
> } else if (avrule->line) {
> ERR(handle, "neverallow on line %lu violated by allow %s %s:%s {%s };",
> avrule->line, p->p_type_val_to_name[stype],
> p->p_type_val_to_name[ttype],
> p->p_class_val_to_name[curperm->tclass - 1],
> - sepol_av_to_string(p, curperm->tclass, perms));
> + permstr ?: "<format-failure>");
> } else {
> ERR(handle, "neverallow violated by allow %s %s:%s {%s };",
> p->p_type_val_to_name[stype],
> p->p_type_val_to_name[ttype],
> p->p_class_val_to_name[curperm->tclass - 1],
> - sepol_av_to_string(p, curperm->tclass, perms));
> + permstr ?: "<format-failure>");
> }
> +
> + free(permstr);
> }
>
> static int match_any_class_permissions(class_perm_node_t *cp, uint32_t class, uint32_t data)
> @@ -200,13 +204,17 @@ static int report_assertion_extended_permissions(sepol_handle_t *handle,
>
> /* failure on the regular permissions */
> if (!found_xperm) {
> + char *permstr = sepol_av_to_string(p, curperm->tclass, perms);
> +
> ERR(handle, "neverallowxperm on line %lu of %s (or line %lu of %s) violated by\n"
> "allow %s %s:%s {%s };",
> avrule->source_line, avrule->source_filename, avrule->line, policy_name(p),
> p->p_type_val_to_name[stype],
> p->p_type_val_to_name[ttype],
> p->p_class_val_to_name[curperm->tclass - 1],
> - sepol_av_to_string(p, curperm->tclass, perms));
> + permstr ?: "<format-failure>");
> +
> + free(permstr);
> errors++;
>
> }
> diff --git a/libsepol/src/hierarchy.c b/libsepol/src/hierarchy.c
> index 350443a8..06e05310 100644
> --- a/libsepol/src/hierarchy.c
> +++ b/libsepol/src/hierarchy.c
> @@ -443,12 +443,15 @@ static void bounds_report(sepol_handle_t *handle, policydb_t *p, uint32_t child,
> p->p_type_val_to_name[child - 1],
> p->p_type_val_to_name[parent - 1]);
> for (; cur; cur = cur->next) {
> + char *permstr = sepol_av_to_string(p, cur->key.target_class, cur->datum.data);
> +
> ERR(handle, " %s %s : %s { %s }",
> p->p_type_val_to_name[cur->key.source_type - 1],
> p->p_type_val_to_name[cur->key.target_type - 1],
> p->p_class_val_to_name[cur->key.target_class - 1],
> - sepol_av_to_string(p, cur->key.target_class,
> - cur->datum.data));
> + permstr ?: "<format-failure>");
> +
> + free(permstr);
> }
> }
>
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index bcb58eee..634826d5 100644
> --- a/libsepol/src/kernel_to_cil.c
> +++ b/libsepol/src/kernel_to_cil.c
> @@ -297,6 +297,11 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> }
>
> perms = sepol_av_to_string(pdb, class->s.value, curr->permissions);
> + if (!perms) {
> + ERR(NULL, "Failed to generate permission string");
> + rc = -1;
> + goto exit;
> + }
>
> if (is_mls) {
> key_word = "mlsconstrain";
> @@ -307,6 +312,7 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> }
>
> rc = strs_create_and_add(strs, "(%s (%s (%s)) %s)", key_word, classkey, perms+1, expr);
> + free(perms);
> free(expr);
> if (rc != 0) {
> goto exit;
> @@ -1772,6 +1778,7 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
> }
> rule = create_str("(%s %s %s (%s (%s)))",
> flavor, src, tgt, class, perms+1);
> + free(perms);
> } else if (key->specified & AVTAB_XPERMS) {
> perms = xperms_to_str(datum->xperms);
> if (perms == NULL) {
> diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
> index 83f46e0f..de1d9e09 100644
> --- a/libsepol/src/kernel_to_conf.c
> +++ b/libsepol/src/kernel_to_conf.c
> @@ -292,6 +292,11 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> }
>
> perms = sepol_av_to_string(pdb, class->s.value, curr->permissions);
> + if (!perms) {
> + ERR(NULL, "Failed to generate permission string");
> + rc = -1;
> + goto exit;
> + }
> if (strchr(perms, ' ')) {
> perm_prefix = "{ ";
> perm_suffix = " }";
> @@ -311,6 +316,7 @@ static int class_constraint_rules_to_strs(struct policydb *pdb, char *classkey,
> flavor, classkey,
> perm_prefix, perms+1, perm_suffix,
> expr);
> + free(perms);
> free(expr);
> if (rc != 0) {
> goto exit;
> @@ -1682,7 +1688,7 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
> {
> uint32_t data = datum->data;
> type_datum_t *type;
> - const char *flavor, *src, *tgt, *class, *perms, *new;
> + const char *flavor, *src, *tgt, *class, *new;
> char *rule = NULL, *permstring;
>
> switch (0xFFF & key->specified) {
> @@ -1730,13 +1736,14 @@ static char *avtab_node_to_str(struct policydb *pdb, avtab_key_t *key, avtab_dat
> class = pdb->p_class_val_to_name[key->target_class - 1];
>
> if (key->specified & AVTAB_AV) {
> - perms = sepol_av_to_string(pdb, key->target_class, data);
> - if (perms == NULL) {
> + permstring = sepol_av_to_string(pdb, key->target_class, data);
> + if (permstring == NULL) {
> ERR(NULL, "Failed to generate permission string");
> goto exit;
> }
> rule = create_str("%s %s %s:%s { %s };",
> - flavor, src, tgt, class, perms+1);
> + flavor, src, tgt, class, permstring+1);
> + free(permstring);
> } else if (key->specified & AVTAB_XPERMS) {
> permstring = sepol_extended_perms_to_string(datum->xperms);
> if (permstring == NULL) {
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index ee22dbbd..2ec66292 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -597,6 +597,7 @@ static int avrule_to_cil(int indent, struct policydb *pdb, uint32_t type, const
> rule, src, tgt,
> pdb->p_class_val_to_name[classperm->tclass - 1],
> perms + 1);
> + free(perms);
> } else {
> cil_println(indent, "(%s %s %s %s %s)",
> rule, src, tgt,
> @@ -1967,7 +1968,13 @@ static int constraints_to_cil(int indent, struct policydb *pdb, char *classkey,
>
> if (is_constraint) {
> perms = sepol_av_to_string(pdb, class->s.value, node->permissions);
> + if (perms == NULL) {
> + ERR(NULL, "Failed to generate permission string");
> + rc = -1;
> + goto exit;
> + }
> cil_println(indent, "(%sconstrain (%s (%s)) %s)", mls, classkey, perms + 1, expr);
> + free(perms);
> } else {
> cil_println(indent, "(%svalidatetrans %s %s)", mls, classkey, expr);
> }
> diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> index 0eeee7ec..36e2368f 100644
> --- a/libsepol/src/services.c
> +++ b/libsepol/src/services.c
> @@ -347,9 +347,11 @@ static char *get_class_info(sepol_security_class_t tclass,
> p += len;
> buf_used += len;
> if (state_num < 2) {
> + char *permstr = sepol_av_to_string(policydb, tclass, constraint->permissions);
> +
> len = snprintf(p, class_buf_len - buf_used, "{%s } (",
> - sepol_av_to_string(policydb, tclass,
> - constraint->permissions));
> + permstr ?: "<format-failure>");
> + free(permstr);
> } else {
> len = snprintf(p, class_buf_len - buf_used, "(");
> }
> @@ -1237,7 +1239,25 @@ out:
> const char *sepol_av_perm_to_string(sepol_security_class_t tclass,
> sepol_access_vector_t av)
> {
> - return sepol_av_to_string(policydb, tclass, av);
> + static char avbuf[1024];
> + char *avstr = sepol_av_to_string(policydb, tclass, av);
> + size_t len;
> +
> + memset(avbuf, 0, sizeof(avbuf));
> +
> + if (avstr) {
> + len = strlen(avstr);
> + if (len < sizeof(avbuf)) {
> + strcpy(avbuf, avstr);
> + } else {
> + sprintf(avbuf, "<access-vector overflowed buffer>");
> + }
> + free(avstr);
> + } else {
> + sprintf(avbuf, "<format-failure>");
> + }
> +
> + return avbuf;
> }
>
> /*
> diff --git a/libsepol/src/util.c b/libsepol/src/util.c
> index 2f877920..dcbdccf1 100644
> --- a/libsepol/src/util.c
> +++ b/libsepol/src/util.c
> @@ -32,7 +32,7 @@
>
> struct val_to_name {
> unsigned int val;
> - char *name;
> + const char *name;
> };
>
> /* Add an unsigned integer to a dynamically reallocated array. *cnt
> @@ -82,20 +82,27 @@ static int perm_name(hashtab_key_t key, hashtab_datum_t datum, void *data)
> return 0;
> }
>
> -char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
> +char *sepol_av_to_string(const policydb_t *policydbp, sepol_security_class_t tclass,
> sepol_access_vector_t av)
> {
> struct val_to_name v;
> - static char avbuf[1024];
> - class_datum_t *cladatum;
> - char *perm = NULL, *p;
> - unsigned int i;
> + const class_datum_t *cladatum = policydbp->class_val_to_struct[tclass - 1];
> + uint32_t i;
> int rc;
> - int avlen = 0, len;
> + char *buffer = NULL, *p;
> + int len;
> + size_t remaining, size = 64;
> +
> +retry:
> + if (__builtin_mul_overflow(size, 2, &size))
> + goto err;
> + p = realloc(buffer, size);
> + if (!p)
> + goto err;
> + *p = '\0'; /* Just in case there are no permissions */
> + buffer = p;
> + remaining = size;
>
> - memset(avbuf, 0, sizeof avbuf);
> - cladatum = policydbp->class_val_to_struct[tclass - 1];
> - p = avbuf;
> for (i = 0; i < cladatum->permissions.nprim; i++) {
> if (av & (UINT32_C(1) << i)) {
> v.val = i + 1;
> @@ -106,22 +113,23 @@ char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
> permissions.table, perm_name,
> &v);
> }
> - if (rc)
> - perm = v.name;
> - if (perm) {
> - len =
> - snprintf(p, sizeof(avbuf) - avlen, " %s",
> - perm);
> - if (len < 0
> - || (size_t) len >= (sizeof(avbuf) - avlen))
> - return NULL;
> + if (rc == 1) {
> + len = snprintf(p, remaining, " %s", v.name);
> + if (len < 0)
> + goto err;
> + if ((size_t) len >= remaining)
> + goto retry;
> p += len;
> - avlen += len;
> + remaining -= len;
> }
> }
> }
>
> - return avbuf;
> + return buffer;
> +
> +err:
> + free(buffer);
> + return NULL;
> }
>
> #define next_bit_in_range(i, p) (((i) + 1 < sizeof(p)*8) && xperm_test(((i) + 1), p))
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-03-04 19:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-12 15:11 [PATCH v2] libsepol: Use a dynamic buffer in sepol_av_to_string() James Carter
2024-02-26 16:25 ` Christian Göttsche
2024-02-28 13:47 ` James Carter
2024-03-04 19:15 ` James Carter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.