* [PATCH 0/2] Fix AppArmor issues found through static analysis @ 2018-07-06 5:24 ` Tyler Hicks 0 siblings, 0 replies; 14+ messages in thread From: Tyler Hicks @ 2018-07-06 5:24 UTC (permalink / raw) To: John Johansen Cc: James Morris, Serge Hallyn, Seth Arnold, linux-security-module, linux-kernel This set fixes three issues, discovered by CoverityScan, that I don't believe are serious in practice but are worth fixing to make the AppArmor code more robust and prevent them from becoming more serious issues in the future. The first patch fixes possible out of bounds access issues when mapping a permissions mask to a string. The second fixes an uninitialized struct that could have some of its contents leaked to userspace during a permissions query. I've tested the patches with the AppArmor regression test suite. The test results are the same with and without these patches applied. There are identical failures, in both situations, from the aa_policy_cache test but that's likely due to some unrelated, recent changes in the AppArmor userspace code. Tyler ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 0/2] Fix AppArmor issues found through static analysis @ 2018-07-06 5:24 ` Tyler Hicks 0 siblings, 0 replies; 14+ messages in thread From: Tyler Hicks @ 2018-07-06 5:24 UTC (permalink / raw) To: linux-security-module This set fixes three issues, discovered by CoverityScan, that I don't believe are serious in practice but are worth fixing to make the AppArmor code more robust and prevent them from becoming more serious issues in the future. The first patch fixes possible out of bounds access issues when mapping a permissions mask to a string. The second fixes an uninitialized struct that could have some of its contents leaked to userspace during a permissions query. I've tested the patches with the AppArmor regression test suite. The test results are the same with and without these patches applied. There are identical failures, in both situations, from the aa_policy_cache test but that's likely due to some unrelated, recent changes in the AppArmor userspace code. Tyler -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] apparmor: Check buffer bounds when mapping permissions mask 2018-07-06 5:24 ` Tyler Hicks @ 2018-07-06 5:25 ` Tyler Hicks -1 siblings, 0 replies; 14+ messages in thread From: Tyler Hicks @ 2018-07-06 5:25 UTC (permalink / raw) To: John Johansen Cc: James Morris, Serge Hallyn, Seth Arnold, linux-security-module, linux-kernel Don't read past the end of the buffer containing permissions characters or write past the end of the destination string. Detected by CoverityScan CID#1415361, 1415376 ("Out-of-bounds access") Fixes: e53cfe6c7caa ("apparmor: rework perm mapping to a slightly broader set") Signed-off-by: Tyler Hicks <tyhicks@canonical.com> --- security/apparmor/file.c | 3 ++- security/apparmor/include/perms.h | 3 ++- security/apparmor/lib.c | 17 +++++++++++++---- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index 224b2fef93ca..4285943f7260 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -47,7 +47,8 @@ static void audit_file_mask(struct audit_buffer *ab, u32 mask) { char str[10]; - aa_perm_mask_to_str(str, aa_file_perm_chrs, map_mask_to_chr_mask(mask)); + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, + map_mask_to_chr_mask(mask)); audit_log_string(ab, str); } diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h index 38aa6247d00f..b94ec114d1a4 100644 --- a/security/apparmor/include/perms.h +++ b/security/apparmor/include/perms.h @@ -137,7 +137,8 @@ extern struct aa_perms allperms; xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2))) -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask); +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, + u32 mask); void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names, u32 mask); void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index a7b3f681b80e..7ab368c3789b 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -198,15 +198,24 @@ const char *aa_file_perm_names[] = { /** * aa_perm_mask_to_str - convert a perm mask to its short string * @str: character buffer to store string in (at least 10 characters) + * @str_size: size of the @str buffer + * @chrs: NUL-terminated character buffer of permission characters * @mask: permission mask to convert */ -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask) +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, u32 mask) { unsigned int i, perm = 1; + size_t num_chrs = strlen(chrs); + + for (i = 0; i < num_chrs; perm <<= 1, i++) { + if (mask & perm) { + /* Ensure that one byte is left for NUL-termination */ + if (WARN_ON_ONCE(str_size <= 1)) + continue; - for (i = 0; i < 32; perm <<= 1, i++) { - if (mask & perm) *str++ = chrs[i]; + str_size--; + } } *str = '\0'; } @@ -236,7 +245,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, audit_log_format(ab, "\""); if ((mask & chrsmask) && chrs) { - aa_perm_mask_to_str(str, chrs, mask & chrsmask); + aa_perm_mask_to_str(str, sizeof(str), chrs, mask & chrsmask); mask &= ~chrsmask; audit_log_format(ab, "%s", str); if (mask & namesmask) -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] apparmor: Check buffer bounds when mapping permissions mask @ 2018-07-06 5:25 ` Tyler Hicks 0 siblings, 0 replies; 14+ messages in thread From: Tyler Hicks @ 2018-07-06 5:25 UTC (permalink / raw) To: linux-security-module Don't read past the end of the buffer containing permissions characters or write past the end of the destination string. Detected by CoverityScan CID#1415361, 1415376 ("Out-of-bounds access") Fixes: e53cfe6c7caa ("apparmor: rework perm mapping to a slightly broader set") Signed-off-by: Tyler Hicks <tyhicks@canonical.com> --- security/apparmor/file.c | 3 ++- security/apparmor/include/perms.h | 3 ++- security/apparmor/lib.c | 17 +++++++++++++---- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index 224b2fef93ca..4285943f7260 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -47,7 +47,8 @@ static void audit_file_mask(struct audit_buffer *ab, u32 mask) { char str[10]; - aa_perm_mask_to_str(str, aa_file_perm_chrs, map_mask_to_chr_mask(mask)); + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, + map_mask_to_chr_mask(mask)); audit_log_string(ab, str); } diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h index 38aa6247d00f..b94ec114d1a4 100644 --- a/security/apparmor/include/perms.h +++ b/security/apparmor/include/perms.h @@ -137,7 +137,8 @@ extern struct aa_perms allperms; xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2))) -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask); +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, + u32 mask); void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names, u32 mask); void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index a7b3f681b80e..7ab368c3789b 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -198,15 +198,24 @@ const char *aa_file_perm_names[] = { /** * aa_perm_mask_to_str - convert a perm mask to its short string * @str: character buffer to store string in (at least 10 characters) + * @str_size: size of the @str buffer + * @chrs: NUL-terminated character buffer of permission characters * @mask: permission mask to convert */ -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask) +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, u32 mask) { unsigned int i, perm = 1; + size_t num_chrs = strlen(chrs); + + for (i = 0; i < num_chrs; perm <<= 1, i++) { + if (mask & perm) { + /* Ensure that one byte is left for NUL-termination */ + if (WARN_ON_ONCE(str_size <= 1)) + continue; - for (i = 0; i < 32; perm <<= 1, i++) { - if (mask & perm) *str++ = chrs[i]; + str_size--; + } } *str = '\0'; } @@ -236,7 +245,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, audit_log_format(ab, "\""); if ((mask & chrsmask) && chrs) { - aa_perm_mask_to_str(str, chrs, mask & chrsmask); + aa_perm_mask_to_str(str, sizeof(str), chrs, mask & chrsmask); mask &= ~chrsmask; audit_log_format(ab, "%s", str); if (mask & namesmask) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info@ http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] apparmor: Check buffer bounds when mapping permissions mask 2018-07-06 5:25 ` Tyler Hicks @ 2018-07-10 15:00 ` Serge E. Hallyn -1 siblings, 0 replies; 14+ messages in thread From: Serge E. Hallyn @ 2018-07-10 15:00 UTC (permalink / raw) To: Tyler Hicks Cc: John Johansen, James Morris, Serge Hallyn, Seth Arnold, linux-security-module, linux-kernel Quoting Tyler Hicks (tyhicks@canonical.com): > Don't read past the end of the buffer containing permissions > characters or write past the end of the destination string. > > Detected by CoverityScan CID#1415361, 1415376 ("Out-of-bounds access") > > Fixes: e53cfe6c7caa ("apparmor: rework perm mapping to a slightly broader set") > Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Acked-by: Serge Hallyn <serge@hallyn.com> > --- > security/apparmor/file.c | 3 ++- > security/apparmor/include/perms.h | 3 ++- > security/apparmor/lib.c | 17 +++++++++++++---- > 3 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/security/apparmor/file.c b/security/apparmor/file.c > index 224b2fef93ca..4285943f7260 100644 > --- a/security/apparmor/file.c > +++ b/security/apparmor/file.c > @@ -47,7 +47,8 @@ static void audit_file_mask(struct audit_buffer *ab, u32 mask) > { > char str[10]; > > - aa_perm_mask_to_str(str, aa_file_perm_chrs, map_mask_to_chr_mask(mask)); > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > + map_mask_to_chr_mask(mask)); > audit_log_string(ab, str); > } > > diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h > index 38aa6247d00f..b94ec114d1a4 100644 > --- a/security/apparmor/include/perms.h > +++ b/security/apparmor/include/perms.h > @@ -137,7 +137,8 @@ extern struct aa_perms allperms; > xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2))) > > > -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask); > +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, > + u32 mask); > void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names, > u32 mask); > void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, > diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c > index a7b3f681b80e..7ab368c3789b 100644 > --- a/security/apparmor/lib.c > +++ b/security/apparmor/lib.c > @@ -198,15 +198,24 @@ const char *aa_file_perm_names[] = { > /** > * aa_perm_mask_to_str - convert a perm mask to its short string > * @str: character buffer to store string in (at least 10 characters) > + * @str_size: size of the @str buffer > + * @chrs: NUL-terminated character buffer of permission characters > * @mask: permission mask to convert > */ > -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask) > +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, u32 mask) > { > unsigned int i, perm = 1; > + size_t num_chrs = strlen(chrs); > + > + for (i = 0; i < num_chrs; perm <<= 1, i++) { > + if (mask & perm) { > + /* Ensure that one byte is left for NUL-termination */ > + if (WARN_ON_ONCE(str_size <= 1)) > + continue; > > - for (i = 0; i < 32; perm <<= 1, i++) { > - if (mask & perm) > *str++ = chrs[i]; > + str_size--; > + } > } > *str = '\0'; > } > @@ -236,7 +245,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, > > audit_log_format(ab, "\""); > if ((mask & chrsmask) && chrs) { > - aa_perm_mask_to_str(str, chrs, mask & chrsmask); > + aa_perm_mask_to_str(str, sizeof(str), chrs, mask & chrsmask); > mask &= ~chrsmask; > audit_log_format(ab, "%s", str); > if (mask & namesmask) > -- > 2.7.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] apparmor: Check buffer bounds when mapping permissions mask @ 2018-07-10 15:00 ` Serge E. Hallyn 0 siblings, 0 replies; 14+ messages in thread From: Serge E. Hallyn @ 2018-07-10 15:00 UTC (permalink / raw) To: linux-security-module Quoting Tyler Hicks (tyhicks at canonical.com): > Don't read past the end of the buffer containing permissions > characters or write past the end of the destination string. > > Detected by CoverityScan CID#1415361, 1415376 ("Out-of-bounds access") > > Fixes: e53cfe6c7caa ("apparmor: rework perm mapping to a slightly broader set") > Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Acked-by: Serge Hallyn <serge@hallyn.com> > --- > security/apparmor/file.c | 3 ++- > security/apparmor/include/perms.h | 3 ++- > security/apparmor/lib.c | 17 +++++++++++++---- > 3 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/security/apparmor/file.c b/security/apparmor/file.c > index 224b2fef93ca..4285943f7260 100644 > --- a/security/apparmor/file.c > +++ b/security/apparmor/file.c > @@ -47,7 +47,8 @@ static void audit_file_mask(struct audit_buffer *ab, u32 mask) > { > char str[10]; > > - aa_perm_mask_to_str(str, aa_file_perm_chrs, map_mask_to_chr_mask(mask)); > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > + map_mask_to_chr_mask(mask)); > audit_log_string(ab, str); > } > > diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h > index 38aa6247d00f..b94ec114d1a4 100644 > --- a/security/apparmor/include/perms.h > +++ b/security/apparmor/include/perms.h > @@ -137,7 +137,8 @@ extern struct aa_perms allperms; > xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2))) > > > -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask); > +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, > + u32 mask); > void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names, > u32 mask); > void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, > diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c > index a7b3f681b80e..7ab368c3789b 100644 > --- a/security/apparmor/lib.c > +++ b/security/apparmor/lib.c > @@ -198,15 +198,24 @@ const char *aa_file_perm_names[] = { > /** > * aa_perm_mask_to_str - convert a perm mask to its short string > * @str: character buffer to store string in (at least 10 characters) > + * @str_size: size of the @str buffer > + * @chrs: NUL-terminated character buffer of permission characters > * @mask: permission mask to convert > */ > -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask) > +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, u32 mask) > { > unsigned int i, perm = 1; > + size_t num_chrs = strlen(chrs); > + > + for (i = 0; i < num_chrs; perm <<= 1, i++) { > + if (mask & perm) { > + /* Ensure that one byte is left for NUL-termination */ > + if (WARN_ON_ONCE(str_size <= 1)) > + continue; > > - for (i = 0; i < 32; perm <<= 1, i++) { > - if (mask & perm) > *str++ = chrs[i]; > + str_size--; > + } > } > *str = '\0'; > } > @@ -236,7 +245,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, > > audit_log_format(ab, "\""); > if ((mask & chrsmask) && chrs) { > - aa_perm_mask_to_str(str, chrs, mask & chrsmask); > + aa_perm_mask_to_str(str, sizeof(str), chrs, mask & chrsmask); > mask &= ~chrsmask; > audit_log_format(ab, "%s", str); > if (mask & namesmask) > -- > 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] apparmor: Check buffer bounds when mapping permissions mask 2018-07-06 5:25 ` Tyler Hicks @ 2018-07-19 23:28 ` John Johansen -1 siblings, 0 replies; 14+ messages in thread From: John Johansen @ 2018-07-19 23:28 UTC (permalink / raw) To: Tyler Hicks Cc: James Morris, Serge Hallyn, Seth Arnold, linux-security-module, linux-kernel On 07/05/2018 10:25 PM, Tyler Hicks wrote: > Don't read past the end of the buffer containing permissions > characters or write past the end of the destination string. > > Detected by CoverityScan CID#1415361, 1415376 ("Out-of-bounds access") > > Fixes: e53cfe6c7caa ("apparmor: rework perm mapping to a slightly broader set") > Signed-off-by: Tyler Hicks <tyhicks@canonical.com> I've pulled this into apparmor-next with a minor modification noted below > security/apparmor/file.c | 3 ++- > security/apparmor/include/perms.h | 3 ++- > security/apparmor/lib.c | 17 +++++++++++++---- > 3 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/security/apparmor/file.c b/security/apparmor/file.c > index 224b2fef93ca..4285943f7260 100644 > --- a/security/apparmor/file.c > +++ b/security/apparmor/file.c > @@ -47,7 +47,8 @@ static void audit_file_mask(struct audit_buffer *ab, u32 mask) > { > char str[10]; > > - aa_perm_mask_to_str(str, aa_file_perm_chrs, map_mask_to_chr_mask(mask)); > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > + map_mask_to_chr_mask(mask)); > audit_log_string(ab, str); > } > > diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h > index 38aa6247d00f..b94ec114d1a4 100644 > --- a/security/apparmor/include/perms.h > +++ b/security/apparmor/include/perms.h > @@ -137,7 +137,8 @@ extern struct aa_perms allperms; > xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2))) > > > -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask); > +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, > + u32 mask); > void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names, > u32 mask); > void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, > diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c > index a7b3f681b80e..7ab368c3789b 100644 > --- a/security/apparmor/lib.c > +++ b/security/apparmor/lib.c > @@ -198,15 +198,24 @@ const char *aa_file_perm_names[] = { > /** > * aa_perm_mask_to_str - convert a perm mask to its short string > * @str: character buffer to store string in (at least 10 characters) > + * @str_size: size of the @str buffer > + * @chrs: NUL-terminated character buffer of permission characters > * @mask: permission mask to convert > */ > -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask) > +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, u32 mask) > { > unsigned int i, perm = 1; > + size_t num_chrs = strlen(chrs); > + > + for (i = 0; i < num_chrs; perm <<= 1, i++> + if (mask & perm) { > + /* Ensure that one byte is left for NUL-termination */ > + if (WARN_ON_ONCE(str_size <= 1)) > + continue; might as well break here > > - for (i = 0; i < 32; perm <<= 1, i++) { > - if (mask & perm) > *str++ = chrs[i]; > + str_size--; > + } > } > *str = '\0'; > } > @@ -236,7 +245,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, > > audit_log_format(ab, "\""); > if ((mask & chrsmask) && chrs) { > - aa_perm_mask_to_str(str, chrs, mask & chrsmask); > + aa_perm_mask_to_str(str, sizeof(str), chrs, mask & chrsmask); > mask &= ~chrsmask; > audit_log_format(ab, "%s", str); > if (mask & namesmask) > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] apparmor: Check buffer bounds when mapping permissions mask @ 2018-07-19 23:28 ` John Johansen 0 siblings, 0 replies; 14+ messages in thread From: John Johansen @ 2018-07-19 23:28 UTC (permalink / raw) To: linux-security-module On 07/05/2018 10:25 PM, Tyler Hicks wrote: > Don't read past the end of the buffer containing permissions > characters or write past the end of the destination string. > > Detected by CoverityScan CID#1415361, 1415376 ("Out-of-bounds access") > > Fixes: e53cfe6c7caa ("apparmor: rework perm mapping to a slightly broader set") > Signed-off-by: Tyler Hicks <tyhicks@canonical.com> I've pulled this into apparmor-next with a minor modification noted below > security/apparmor/file.c | 3 ++- > security/apparmor/include/perms.h | 3 ++- > security/apparmor/lib.c | 17 +++++++++++++---- > 3 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/security/apparmor/file.c b/security/apparmor/file.c > index 224b2fef93ca..4285943f7260 100644 > --- a/security/apparmor/file.c > +++ b/security/apparmor/file.c > @@ -47,7 +47,8 @@ static void audit_file_mask(struct audit_buffer *ab, u32 mask) > { > char str[10]; > > - aa_perm_mask_to_str(str, aa_file_perm_chrs, map_mask_to_chr_mask(mask)); > + aa_perm_mask_to_str(str, sizeof(str), aa_file_perm_chrs, > + map_mask_to_chr_mask(mask)); > audit_log_string(ab, str); > } > > diff --git a/security/apparmor/include/perms.h b/security/apparmor/include/perms.h > index 38aa6247d00f..b94ec114d1a4 100644 > --- a/security/apparmor/include/perms.h > +++ b/security/apparmor/include/perms.h > @@ -137,7 +137,8 @@ extern struct aa_perms allperms; > xcheck(fn_for_each((L1), (P), (FN1)), fn_for_each((L2), (P), (FN2))) > > > -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask); > +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, > + u32 mask); > void aa_audit_perm_names(struct audit_buffer *ab, const char * const *names, > u32 mask); > void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, > diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c > index a7b3f681b80e..7ab368c3789b 100644 > --- a/security/apparmor/lib.c > +++ b/security/apparmor/lib.c > @@ -198,15 +198,24 @@ const char *aa_file_perm_names[] = { > /** > * aa_perm_mask_to_str - convert a perm mask to its short string > * @str: character buffer to store string in (at least 10 characters) > + * @str_size: size of the @str buffer > + * @chrs: NUL-terminated character buffer of permission characters > * @mask: permission mask to convert > */ > -void aa_perm_mask_to_str(char *str, const char *chrs, u32 mask) > +void aa_perm_mask_to_str(char *str, size_t str_size, const char *chrs, u32 mask) > { > unsigned int i, perm = 1; > + size_t num_chrs = strlen(chrs); > + > + for (i = 0; i < num_chrs; perm <<= 1, i++> + if (mask & perm) { > + /* Ensure that one byte is left for NUL-termination */ > + if (WARN_ON_ONCE(str_size <= 1)) > + continue; might as well break here > > - for (i = 0; i < 32; perm <<= 1, i++) { > - if (mask & perm) > *str++ = chrs[i]; > + str_size--; > + } > } > *str = '\0'; > } > @@ -236,7 +245,7 @@ void aa_audit_perm_mask(struct audit_buffer *ab, u32 mask, const char *chrs, > > audit_log_format(ab, "\""); > if ((mask & chrsmask) && chrs) { > - aa_perm_mask_to_str(str, chrs, mask & chrsmask); > + aa_perm_mask_to_str(str, sizeof(str), chrs, mask & chrsmask); > mask &= ~chrsmask; > audit_log_format(ab, "%s", str); > if (mask & namesmask) > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] apparmor: Fully initialize aa_perms struct when answering userspace query 2018-07-06 5:24 ` Tyler Hicks @ 2018-07-06 5:25 ` Tyler Hicks -1 siblings, 0 replies; 14+ messages in thread From: Tyler Hicks @ 2018-07-06 5:25 UTC (permalink / raw) To: John Johansen Cc: James Morris, Serge Hallyn, Seth Arnold, linux-security-module, linux-kernel Fully initialize the aa_perms struct in profile_query_cb() to avoid the potential of using an uninitialized struct member's value in a response to a query from userspace. Detected by CoverityScan CID#1415126 ("Uninitialized scalar variable") Fixes: 4f3b3f2d79a4 ("apparmor: add profile permission query ability") Signed-off-by: Tyler Hicks <tyhicks@canonical.com> --- security/apparmor/apparmorfs.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 949dd8a48164..e09fe4d7307c 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -603,7 +603,7 @@ static const struct file_operations aa_fs_ns_revision_fops = { static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, const char *match_str, size_t match_len) { - struct aa_perms tmp; + struct aa_perms tmp = { }; struct aa_dfa *dfa; unsigned int state = 0; @@ -613,7 +613,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, dfa = profile->file.dfa; state = aa_dfa_match_len(dfa, profile->file.start, match_str + 1, match_len - 1); - tmp = nullperms; if (state) { struct path_cond cond = { }; @@ -627,8 +626,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, match_str, match_len); if (state) aa_compute_perms(dfa, state, &tmp); - else - tmp = nullperms; } aa_apply_modes_to_perms(profile, &tmp); aa_perms_accum_raw(perms, &tmp); -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] apparmor: Fully initialize aa_perms struct when answering userspace query @ 2018-07-06 5:25 ` Tyler Hicks 0 siblings, 0 replies; 14+ messages in thread From: Tyler Hicks @ 2018-07-06 5:25 UTC (permalink / raw) To: linux-security-module Fully initialize the aa_perms struct in profile_query_cb() to avoid the potential of using an uninitialized struct member's value in a response to a query from userspace. Detected by CoverityScan CID#1415126 ("Uninitialized scalar variable") Fixes: 4f3b3f2d79a4 ("apparmor: add profile permission query ability") Signed-off-by: Tyler Hicks <tyhicks@canonical.com> --- security/apparmor/apparmorfs.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 949dd8a48164..e09fe4d7307c 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -603,7 +603,7 @@ static const struct file_operations aa_fs_ns_revision_fops = { static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, const char *match_str, size_t match_len) { - struct aa_perms tmp; + struct aa_perms tmp = { }; struct aa_dfa *dfa; unsigned int state = 0; @@ -613,7 +613,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, dfa = profile->file.dfa; state = aa_dfa_match_len(dfa, profile->file.start, match_str + 1, match_len - 1); - tmp = nullperms; if (state) { struct path_cond cond = { }; @@ -627,8 +626,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, match_str, match_len); if (state) aa_compute_perms(dfa, state, &tmp); - else - tmp = nullperms; } aa_apply_modes_to_perms(profile, &tmp); aa_perms_accum_raw(perms, &tmp); -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] apparmor: Fully initialize aa_perms struct when answering userspace query 2018-07-06 5:25 ` Tyler Hicks @ 2018-07-10 15:03 ` Serge E. Hallyn -1 siblings, 0 replies; 14+ messages in thread From: Serge E. Hallyn @ 2018-07-10 15:03 UTC (permalink / raw) To: Tyler Hicks Cc: John Johansen, James Morris, Serge Hallyn, Seth Arnold, linux-security-module, linux-kernel Quoting Tyler Hicks (tyhicks@canonical.com): > Fully initialize the aa_perms struct in profile_query_cb() to avoid the > potential of using an uninitialized struct member's value in a response > to a query from userspace. > > Detected by CoverityScan CID#1415126 ("Uninitialized scalar variable") > > Fixes: 4f3b3f2d79a4 ("apparmor: add profile permission query ability") > Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Acked-by: Serge Hallyn <serge@hallyn.com> > --- > security/apparmor/apparmorfs.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index 949dd8a48164..e09fe4d7307c 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -603,7 +603,7 @@ static const struct file_operations aa_fs_ns_revision_fops = { > static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > const char *match_str, size_t match_len) > { > - struct aa_perms tmp; > + struct aa_perms tmp = { }; > struct aa_dfa *dfa; > unsigned int state = 0; > > @@ -613,7 +613,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > dfa = profile->file.dfa; > state = aa_dfa_match_len(dfa, profile->file.start, > match_str + 1, match_len - 1); > - tmp = nullperms; > if (state) { > struct path_cond cond = { }; > > @@ -627,8 +626,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > match_str, match_len); > if (state) > aa_compute_perms(dfa, state, &tmp); > - else > - tmp = nullperms; > } > aa_apply_modes_to_perms(profile, &tmp); > aa_perms_accum_raw(perms, &tmp); > -- > 2.7.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] apparmor: Fully initialize aa_perms struct when answering userspace query @ 2018-07-10 15:03 ` Serge E. Hallyn 0 siblings, 0 replies; 14+ messages in thread From: Serge E. Hallyn @ 2018-07-10 15:03 UTC (permalink / raw) To: linux-security-module Quoting Tyler Hicks (tyhicks at canonical.com): > Fully initialize the aa_perms struct in profile_query_cb() to avoid the > potential of using an uninitialized struct member's value in a response > to a query from userspace. > > Detected by CoverityScan CID#1415126 ("Uninitialized scalar variable") > > Fixes: 4f3b3f2d79a4 ("apparmor: add profile permission query ability") > Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Acked-by: Serge Hallyn <serge@hallyn.com> > --- > security/apparmor/apparmorfs.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index 949dd8a48164..e09fe4d7307c 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -603,7 +603,7 @@ static const struct file_operations aa_fs_ns_revision_fops = { > static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > const char *match_str, size_t match_len) > { > - struct aa_perms tmp; > + struct aa_perms tmp = { }; > struct aa_dfa *dfa; > unsigned int state = 0; > > @@ -613,7 +613,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > dfa = profile->file.dfa; > state = aa_dfa_match_len(dfa, profile->file.start, > match_str + 1, match_len - 1); > - tmp = nullperms; > if (state) { > struct path_cond cond = { }; > > @@ -627,8 +626,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > match_str, match_len); > if (state) > aa_compute_perms(dfa, state, &tmp); > - else > - tmp = nullperms; > } > aa_apply_modes_to_perms(profile, &tmp); > aa_perms_accum_raw(perms, &tmp); > -- > 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] apparmor: Fully initialize aa_perms struct when answering userspace query 2018-07-06 5:25 ` Tyler Hicks @ 2018-07-19 23:28 ` John Johansen -1 siblings, 0 replies; 14+ messages in thread From: John Johansen @ 2018-07-19 23:28 UTC (permalink / raw) To: Tyler Hicks Cc: James Morris, Serge Hallyn, Seth Arnold, linux-security-module, linux-kernel On 07/05/2018 10:25 PM, Tyler Hicks wrote: > Fully initialize the aa_perms struct in profile_query_cb() to avoid the > potential of using an uninitialized struct member's value in a response > to a query from userspace. > > Detected by CoverityScan CID#1415126 ("Uninitialized scalar variable") > > Fixes: 4f3b3f2d79a4 ("apparmor: add profile permission query ability"> Signed-off-by: Tyler Hicks <tyhicks@canonical.com> I've pulled this into apparmor-next > --- > security/apparmor/apparmorfs.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index 949dd8a48164..e09fe4d7307c 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -603,7 +603,7 @@ static const struct file_operations aa_fs_ns_revision_fops = { > static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > const char *match_str, size_t match_len) > { > - struct aa_perms tmp; > + struct aa_perms tmp = { }; > struct aa_dfa *dfa; > unsigned int state = 0; > > @@ -613,7 +613,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > dfa = profile->file.dfa; > state = aa_dfa_match_len(dfa, profile->file.start, > match_str + 1, match_len - 1); > - tmp = nullperms; > if (state) { > struct path_cond cond = { }; > > @@ -627,8 +626,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > match_str, match_len); > if (state) > aa_compute_perms(dfa, state, &tmp); > - else > - tmp = nullperms; > } > aa_apply_modes_to_perms(profile, &tmp); > aa_perms_accum_raw(perms, &tmp); > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] apparmor: Fully initialize aa_perms struct when answering userspace query @ 2018-07-19 23:28 ` John Johansen 0 siblings, 0 replies; 14+ messages in thread From: John Johansen @ 2018-07-19 23:28 UTC (permalink / raw) To: linux-security-module On 07/05/2018 10:25 PM, Tyler Hicks wrote: > Fully initialize the aa_perms struct in profile_query_cb() to avoid the > potential of using an uninitialized struct member's value in a response > to a query from userspace. > > Detected by CoverityScan CID#1415126 ("Uninitialized scalar variable") > > Fixes: 4f3b3f2d79a4 ("apparmor: add profile permission query ability"> Signed-off-by: Tyler Hicks <tyhicks@canonical.com> I've pulled this into apparmor-next > --- > security/apparmor/apparmorfs.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c > index 949dd8a48164..e09fe4d7307c 100644 > --- a/security/apparmor/apparmorfs.c > +++ b/security/apparmor/apparmorfs.c > @@ -603,7 +603,7 @@ static const struct file_operations aa_fs_ns_revision_fops = { > static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > const char *match_str, size_t match_len) > { > - struct aa_perms tmp; > + struct aa_perms tmp = { }; > struct aa_dfa *dfa; > unsigned int state = 0; > > @@ -613,7 +613,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > dfa = profile->file.dfa; > state = aa_dfa_match_len(dfa, profile->file.start, > match_str + 1, match_len - 1); > - tmp = nullperms; > if (state) { > struct path_cond cond = { }; > > @@ -627,8 +626,6 @@ static void profile_query_cb(struct aa_profile *profile, struct aa_perms *perms, > match_str, match_len); > if (state) > aa_compute_perms(dfa, state, &tmp); > - else > - tmp = nullperms; > } > aa_apply_modes_to_perms(profile, &tmp); > aa_perms_accum_raw(perms, &tmp); > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-07-19 23:29 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-06 5:24 [PATCH 0/2] Fix AppArmor issues found through static analysis Tyler Hicks 2018-07-06 5:24 ` Tyler Hicks 2018-07-06 5:25 ` [PATCH 1/2] apparmor: Check buffer bounds when mapping permissions mask Tyler Hicks 2018-07-06 5:25 ` Tyler Hicks 2018-07-10 15:00 ` Serge E. Hallyn 2018-07-10 15:00 ` Serge E. Hallyn 2018-07-19 23:28 ` John Johansen 2018-07-19 23:28 ` John Johansen 2018-07-06 5:25 ` [PATCH 2/2] apparmor: Fully initialize aa_perms struct when answering userspace query Tyler Hicks 2018-07-06 5:25 ` Tyler Hicks 2018-07-10 15:03 ` Serge E. Hallyn 2018-07-10 15:03 ` Serge E. Hallyn 2018-07-19 23:28 ` John Johansen 2018-07-19 23:28 ` John Johansen
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.