These strings may come from untrusted sources (e.g. file xattrs) so they need to be properly escaped.
Reproducer: # setenforce 0 # touch /tmp/test # setfattr -n security.selinux -v 'kuřecí řízek' /tmp/test # runcon system_u:system_r:sshd_t:s0 cat /tmp/test (look at the generated AVCs)
Actual result: type=AVC [...] trawcon=kuřecí řízek
Expected result: type=AVC [...] trawcon=6B75C5996563C3AD20C599C3AD7A656B
Fixes: fede148324c3 ("selinux: log invalid contexts in AVCs") Cc: stable@vger.kernel.org # v5.1+ Signed-off-by: Ondrej Mosnacek omosnace@redhat.com --- security/selinux/avc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 8346a4f7c5d7..a99be508f93d 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -739,14 +739,20 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext, &scontext_len); if (!rc && scontext) { - audit_log_format(ab, " srawcon=%s", scontext); + if (scontext_len && scontext[scontext_len - 1] == '\0') + scontext_len--; + audit_log_format(ab, " srawcon="); + audit_log_n_untrustedstring(ab, scontext, scontext_len); kfree(scontext); }
rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext, &scontext_len); if (!rc && scontext) { - audit_log_format(ab, " trawcon=%s", scontext); + if (scontext_len && scontext[scontext_len - 1] == '\0') + scontext_len--; + audit_log_format(ab, " trawcon="); + audit_log_n_untrustedstring(ab, scontext, scontext_len); kfree(scontext); } }
On 2019-06-11 10:07, Ondrej Mosnacek wrote:
These strings may come from untrusted sources (e.g. file xattrs) so they need to be properly escaped.
Reproducer: # setenforce 0 # touch /tmp/test # setfattr -n security.selinux -v 'kuřecí řízek' /tmp/test # runcon system_u:system_r:sshd_t:s0 cat /tmp/test (look at the generated AVCs)
Actual result: type=AVC [...] trawcon=kuřecí řízek
Expected result: type=AVC [...] trawcon=6B75C5996563C3AD20C599C3AD7A656B
Fixes: fede148324c3 ("selinux: log invalid contexts in AVCs") Cc: stable@vger.kernel.org # v5.1+ Signed-off-by: Ondrej Mosnacek omosnace@redhat.com
Acked-by: Richard Guy Briggs rgb@redhat.com
security/selinux/avc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 8346a4f7c5d7..a99be508f93d 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -739,14 +739,20 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext, &scontext_len); if (!rc && scontext) {
audit_log_format(ab, " srawcon=%s", scontext);
if (scontext_len && scontext[scontext_len - 1] == '\0')
scontext_len--;
audit_log_format(ab, " srawcon=");
kfree(scontext); }audit_log_n_untrustedstring(ab, scontext, scontext_len);
rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext, &scontext_len); if (!rc && scontext) {
audit_log_format(ab, " trawcon=%s", scontext);
if (scontext_len && scontext[scontext_len - 1] == '\0')
scontext_len--;
audit_log_format(ab, " trawcon=");
kfree(scontext); }audit_log_n_untrustedstring(ab, scontext, scontext_len);
}
2.20.1
-- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
-- Richard Guy Briggs rgb@redhat.com Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Tue, Jun 11, 2019 at 4:07 AM Ondrej Mosnacek omosnace@redhat.com wrote:
These strings may come from untrusted sources (e.g. file xattrs) so they need to be properly escaped.
Reproducer: # setenforce 0 # touch /tmp/test # setfattr -n security.selinux -v 'kuřecí řízek' /tmp/test # runcon system_u:system_r:sshd_t:s0 cat /tmp/test (look at the generated AVCs)
Actual result: type=AVC [...] trawcon=kuřecí řízek
Expected result: type=AVC [...] trawcon=6B75C5996563C3AD20C599C3AD7A656B
Fixes: fede148324c3 ("selinux: log invalid contexts in AVCs") Cc: stable@vger.kernel.org # v5.1+ Signed-off-by: Ondrej Mosnacek omosnace@redhat.com
security/selinux/avc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
Thanks, the patch looks fine to me, but it is borderline -stable material in my opinion. I'll add it to the stable-5.2 branch, but in the future I would prefer if you left the stable marking off patches and sent a reply discussing *why* this should go to stable so we can discuss it. I realize Greg likes to pull a lot of stuff into stable, but I try to be a bit more conservative about what gets marked. Even the simplest fix can still break things :)
I'm going to start building a test kernel now with this fix, but I might hold off on sending this up to Linus for a couple of days to see if I can catch Gen Zhang's patches in the same PR.
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 8346a4f7c5d7..a99be508f93d 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -739,14 +739,20 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext, &scontext_len); if (!rc && scontext) {
audit_log_format(ab, " srawcon=%s", scontext);
if (scontext_len && scontext[scontext_len - 1] == '\0')
scontext_len--;
audit_log_format(ab, " srawcon=");
audit_log_n_untrustedstring(ab, scontext, scontext_len); kfree(scontext); } rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext, &scontext_len); if (!rc && scontext) {
audit_log_format(ab, " trawcon=%s", scontext);
if (scontext_len && scontext[scontext_len - 1] == '\0')
scontext_len--;
audit_log_format(ab, " trawcon=");
audit_log_n_untrustedstring(ab, scontext, scontext_len); kfree(scontext); }
}
2.20.1
On Wed, Jun 12, 2019 at 12:56 AM Paul Moore paul@paul-moore.com wrote:
On Tue, Jun 11, 2019 at 4:07 AM Ondrej Mosnacek omosnace@redhat.com wrote:
These strings may come from untrusted sources (e.g. file xattrs) so they need to be properly escaped.
Reproducer: # setenforce 0 # touch /tmp/test # setfattr -n security.selinux -v 'kuřecí řízek' /tmp/test # runcon system_u:system_r:sshd_t:s0 cat /tmp/test (look at the generated AVCs)
Actual result: type=AVC [...] trawcon=kuřecí řízek
Expected result: type=AVC [...] trawcon=6B75C5996563C3AD20C599C3AD7A656B
Fixes: fede148324c3 ("selinux: log invalid contexts in AVCs") Cc: stable@vger.kernel.org # v5.1+ Signed-off-by: Ondrej Mosnacek omosnace@redhat.com
security/selinux/avc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
Thanks, the patch looks fine to me, but it is borderline -stable material in my opinion. I'll add it to the stable-5.2 branch, but in the future I would prefer if you left the stable marking off patches and sent a reply discussing *why* this should go to stable so we can discuss it. I realize Greg likes to pull a lot of stuff into stable, but I try to be a bit more conservative about what gets marked. Even the simplest fix can still break things :)
OK, I was a bit unsure whether to mark it as stable or not and eventually inclined to do so... I'll try be more careful about it in the future.
I'm going to start building a test kernel now with this fix, but I might hold off on sending this up to Linus for a couple of days to see if I can catch Gen Zhang's patches in the same PR.
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 8346a4f7c5d7..a99be508f93d 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -739,14 +739,20 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext, &scontext_len); if (!rc && scontext) {
audit_log_format(ab, " srawcon=%s", scontext);
if (scontext_len && scontext[scontext_len - 1] == '\0')
scontext_len--;
audit_log_format(ab, " srawcon=");
audit_log_n_untrustedstring(ab, scontext, scontext_len); kfree(scontext); } rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext, &scontext_len); if (!rc && scontext) {
audit_log_format(ab, " trawcon=%s", scontext);
if (scontext_len && scontext[scontext_len - 1] == '\0')
scontext_len--;
audit_log_format(ab, " trawcon=");
audit_log_n_untrustedstring(ab, scontext, scontext_len); kfree(scontext); }
}
2.20.1
-- paul moore www.paul-moore.com
On Wed, Jun 12, 2019 at 3:37 AM Ondrej Mosnacek omosnace@redhat.com wrote:
On Wed, Jun 12, 2019 at 12:56 AM Paul Moore paul@paul-moore.com wrote:
On Tue, Jun 11, 2019 at 4:07 AM Ondrej Mosnacek omosnace@redhat.com wrote:
These strings may come from untrusted sources (e.g. file xattrs) so they need to be properly escaped.
Reproducer: # setenforce 0 # touch /tmp/test # setfattr -n security.selinux -v 'kuřecí řízek' /tmp/test # runcon system_u:system_r:sshd_t:s0 cat /tmp/test (look at the generated AVCs)
Actual result: type=AVC [...] trawcon=kuřecí řízek
Expected result: type=AVC [...] trawcon=6B75C5996563C3AD20C599C3AD7A656B
Fixes: fede148324c3 ("selinux: log invalid contexts in AVCs") Cc: stable@vger.kernel.org # v5.1+ Signed-off-by: Ondrej Mosnacek omosnace@redhat.com
security/selinux/avc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
Thanks, the patch looks fine to me, but it is borderline -stable material in my opinion. I'll add it to the stable-5.2 branch, but in the future I would prefer if you left the stable marking off patches and sent a reply discussing *why* this should go to stable so we can discuss it. I realize Greg likes to pull a lot of stuff into stable, but I try to be a bit more conservative about what gets marked. Even the simplest fix can still break things :)
OK, I was a bit unsure whether to mark it as stable or not and eventually inclined to do so... I'll try be more careful about it in the future.
If it makes you feel better, it's not that big of a deal, I just felt it was worth mentioning since we've been doing a bit of a "best practices for submitting SELinux kernel patches" on the mailing list lately and I felt this was worth mentioning. The basic idea is that I think marking something for stable shouldn't be taken lightly and it is worth a discussion, even if it is short.
linux-stable-mirror@lists.linaro.org