openssh bugs - Mar 2015 - [Bug 2361] New: seccomp filter (not only) for aarch64

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

            Bug ID: 2361
           Summary: seccomp filter (not only) for aarch64
           Product: Portable OpenSSH
           Version: 6.7p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

Created attachment 2561
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2561&action=edit
aarh64 patch

We started using seccomp filter in openssh and there appeared to some
problems with secondary architectures:
https://bugzilla.redhat.com/show_bug.cgi?id=1195065

Seccomp filter is available on aarch64 architecture, but openssh code
was not ready for it so I am providing here patch to make it working.

Changes and explanations:
 * First of all we need to whitelist this architecture in configure.ac
  * (also fixing some indentation inconsistency around arm)
 * Then we need to adjust filter settings for syscalls denial
  * (if syscall doesn't exist openssh will not build)
  * open is not on aarch64, openat exists also on primary architectures
  * stat is never used, x86_64 is using fstat, ix86 and arm is using
fstat64 and stat64 => whitelisting, aarch64 is using fstat and
newfstatat
  * poll, select are not available on aarch64
  * pselect6 is used instead of select
 (see attached patch)

This patch was tested and is currently used in Fedora.
We plan to add support for other architectures sooner or later. Further
discussion welcome.



I'm also appending table with syscall names and numbers I collected
during my testing and which are mentioned in filter and differ across
architectures:

          open       stat                       select()
x86_64   open(2)    fstat(5)                   select(23)
 ix86    open(5)    stat64(195)  fstat64(197)  _newselect(142)
 arm     open(5)    stat64(195)  fstat64(197)  _newselect(142)
aarch64  openat(56) fstat(80) newfstatat(79)   pselect6(72)

Feel free to commend or add more syscalls you are interested in.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Dmitry V. Levin <ldv at altlinux.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ldv at altlinux.org

--- Comment #1 from Dmitry V. Levin <ldv at altlinux.org> ---
Wouldn't it be better if

+#if defined(__NR_stat64) && defined(__NR_fstat64)
+    SC_DENY(stat64, EACCES), /* ix86, arm */
+    SC_DENY(fstat64, EACCES),
+#endif

was written as

+#ifdef __NR_stat64
+    SC_DENY(stat64, EACCES), /* ix86, arm */
+#endif
+#ifdef __NR_fstat64
+    SC_DENY(fstat64, EACCES), /* ix86, arm */
+#endif

?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #2 from Jakub Jelen <jjelen at redhat.com> ---
Yes, it would be better, but
 1) These two calls are on same arch together so I wanted to point this
out.
 2) There is enough of ifdefs from my point of view in this file.

But I don't mind if you will apply it the other way.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org,
                   |                            |dtucker at zip.com.au
   Attachment #2561|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2561
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2561
aarh64 patch

This looks ok to me. Darren, any comments?

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2360

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #4 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2561
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2561
aarh64 patch
>+#ifdef __NR_select /* not on AArch64 */
> 	SC_ALLOW(select),
> #endif
>+#ifdef __NR_pselect6 /* AArch64 */
>+	SC_ALLOW(pselect6),
>+#endif
>+#endif
This nesting looks wrong and it's getting messy.

Could we put the __NR_$thing test inside the SC_ALLOW/SC_DENY macros?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
(In reply to Darren Tucker from comment #4)
> Comment on attachment 2561 [details]
> aarh64 patch
> 
> >+#ifdef __NR_select /* not on AArch64 */
> > 	SC_ALLOW(select),
> > #endif
> >+#ifdef __NR_pselect6 /* AArch64 */
> >+	SC_ALLOW(pselect6),
> >+#endif
> >+#endif
> 
> This nesting looks wrong and it's getting messy.
I can reindent, which makes it a bit clearer

#ifdef __NR__newselect
        SC_ALLOW(_newselect),
#else
# ifdef __NR_select /* not on AArch64 */
        SC_ALLOW(select),
# endif
# ifdef __NR_pselect6 /* AArch64 */
        SC_ALLOW(pselect6),
# endif
#endif

Though maybe it is just better to allow each syscall based on its own
presence:

#ifdef __NR__newselect
        SC_ALLOW(_newselect),
#endif
#ifdef __NR_select /* not on AArch64 */
        SC_ALLOW(select),
#endif
#ifdef __NR_pselect6 /* AArch64 */
        SC_ALLOW(pselect6),
#endif
> Could we put the __NR_$thing test inside the SC_ALLOW/SC_DENY macros?
How would this work? You can't have #if/#ifdef inside a #define

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #6 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Damien Miller from comment #5) 
> I can reindent, which makes it a bit clearer
No, I think it'll still be wrong:

$ lynx -source
'https://bugzilla.mindrot.org/attachment.cgi?id=2561&action=diff&context=patch&collapsed=&headers=1&format=raw'
| egrep '^\+' | awk '/ifdef|endif/{print $1}' | sort | uniq -c
      7 +#endif
      6 +#ifdef

(there are no "-" lines)
> How would this work? You can't have #if/#ifdef inside a #define
I dunno.  It might not be possible (but if it is, it'd be a lot cleared
and somewhat more future-proof).

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #7 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Darren Tucker from comment #6)
> I dunno.  It might not be possible.
I don't think it is possible to do this, at least via any mechanism I'd
be willing to support :-)


(In reply to Damien Miller from comment #5)
> Though maybe it is just better to allow each syscall based on its
> own presence:
I think that's a better idea.  Although it's a bit ugly, IMO having a
consistent pattern is easier to spot errors in and less ugly than
having some inside ifdefs and some not.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #8 from Jakub Jelen <jjelen at redhat.com> ---
Thanks for coming back to this.

I was trying to create it in some way it will be less ugly, but I
didn't find any better solution than having every SC_ALLOW covered with
ifdef. I agree with dividing that select ifdefs according to presence.
It doesn't hurt anything and would be much more clear.

This construction discussed from comment #4 is coming from patch [1] by
Marcin, which I had to rewrite little bit, but I left this one, because
it was working.

To make it less uggly, it would be possible to use more high-level
library libseccomp [2], which solves such problems under the hood, but
I'm not sure about support on different platforms.

[1] https://bugzilla.redhat.com/attachment.cgi?id=994437&action=diff
[2] https://github.com/seccomp/libseccomp

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #9 from Darren Tucker <dtucker at zip.com.au> ---
(In reply to Jakub Jelen from comment #8)
> To make it less ugly, it would be possible to use more high-level
> library libseccomp [2]
Given the use case is relatively simple I think we'd rather take a few
extra lines of mechanical code than pull in another library dependency.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2561|0                           |1
        is obsolete|                            |
   Attachment #2561|ok?(dtucker at zip.com.au)     |
              Flags|                            |
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
   Attachment #2601|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #10 from Damien Miller <djm at mindrot.org> ---
Created attachment 2601
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2601&action=edit
flatten and sort syscall ACL

This flattens out the syscall tests in the ACL to make them independent
and sorts them to make them a bit more maintainable.

I agree with Darren wrt libseccomp - our requirements are very simple
and we don't want to have to manage another library dependency.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2601|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

--- Comment #11 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2601
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2601
flatten and sort syscall ACL

diff looks OK but I'd like confirmation it actually works on the target
hardware.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #12 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2601
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2601
flatten and sort syscall ACL

diff looks OK but I'd like confirmation it actually works on the target
hardware.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #13 from Damien Miller <djm at mindrot.org> ---
applied - this will be in openssh-6.9

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #14 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 2648
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2648&action=edit
proposed patch - stat and shutdown on ix68

Some further catch ups:
 1) stat syscall is not as legacy as expected -- gsssapi library issues
such syscall even on x86_64 (based on [1]). Proposing to add back also
stat to make sure everything works.

 2) Socket shutdown is handled by socketcall on i386 linux so we are
getting "socket closed" errors instead of correct closing connection.
Audit messages:

Jun 16 09:27:51 host audit[11004]: SECCOMP auid=4294967295 uid=74
gid=74 ses=4294967295 subj=system_u:system_r:sshd_net_t:s0-s0:c0.c1023
pid=11004 comm="sshd" exe="/usr/sbin/sshd" sig=31
arch=40000003
syscall=102 compat=0 ip=0xb77d5be8 code=0x0
Jun 16 09:27:51 host sshd[11003]: error: mm_request_receive: socket
closed
# ausyscall 102
socketcall

We don't want to allow all the syscalls [2] from socketcall. Best would
be to allow only SYS_SHUTDOWN as first argument but in the current code
of seccomp filter, there is no possibility to filter through function
arguments.
Adding so would require additional complexity, but it would be great to
have it "right way", even if it doesn't matter much during
connection
close.
See proposed patch with hand-baked seccomp filter for first argument
check. I don't see the changes from previous comments in portable
repository so the patch is not directly applicable (stat part).

Tested on Fedora 22 with openssh-6.8 and after applying this patch, I
no longer see SECCOMP messages.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1228323
[2]
http://jkukunas.blogspot.cz/2010/05/x86-linux-networking-system-calls.html

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
Created attachment 2649
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2649&action=edit
use a macro for socketcall test

IMO it would be better to make the test into another macro; it's more
readable and might be handy if we need to deal with similar calls in
the future.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #16 from Jakub Jelen <jjelen at redhat.com> ---
Comment on attachment 2649
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2649
use a macro for socketcall test

Thank you for fast reply.
Yes. It would be better for readability. Additionally we can
parametrize the argument number to make it more generic:
>#define SC_ALLOW_ARG(_nr, _argn, _arg)
[...]
>	BPF_STMT(BPF_LD+BPF_W+BPF_ABS, \
>	    offsetof(struct seccomp_data, args[_argn])), \
Additionally I don't know what happened to the stat()-thing -- if the
previous patch was applied upstream, there is stat missing, also the
fstat needs to be conditional if it is not on some other architectures.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2601|0                           |1
        is obsolete|                            |
   Attachment #2648|0                           |1
        is obsolete|                            |
   Attachment #2649|0                           |1
        is obsolete|                            |
   Attachment #2650|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #17 from Damien Miller <djm at mindrot.org> ---
Created attachment 2650
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2650&action=edit
tweaked diff

We already have stat in the fail-with-EACCESS list. We didn't have
fstat though - is fstat not a syscall on some architectures?

I made the syscall argument number a macro argument as you suggested.
Could you try this diff? I don't have an i386 system with seccomp-bpf
handy...

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #18 from Jakub Jelen <jjelen at redhat.com> ---
Damien, your diff works for me.

Can you make sure where ended the aarch64 diff which you marked as
applied? I don't see it in portable git repo.

Currently I don't have around arm architectures so I can't make sure,
but I believe stat is not on aarch64 based on this attachment [1].
Syscall fstat will is probably missing on arm architecture, but I can't
test now.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1195065#c2

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2650|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

--- Comment #19 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2650
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2650
tweaked diff

Looks plausible although I know nothing of BPF.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2650|0                           |1
        is obsolete|                            |
   Attachment #2651|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #20 from Damien Miller <djm at mindrot.org> ---
Created attachment 2651
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2651&action=edit
with aarch64 bits, re-sorted syscall list

This adds the aarch64 syscalls and configure stanza.

It also re-sorts the syscall list, and make it just include every
requested syscall if their syscall numbers are present, rather than
depending on architecture or libc. This means we might allow one or two
additional syscalls than are strictly necessary (e.g. madvise on
!musllibc) but it's more readable and easier to maintain.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2651|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #21 from Dmitry V. Levin <ldv at altlinux.org> ---
(In reply to Damien Miller from comment #20)
> Created attachment 2651 [details]
> with aarch64 bits, re-sorted syscall list
Shouldn't the "jump false" argument of the first BPF_JUMP in
SC_ALLOW_ARG's definition be changed from 3 to 4?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #22 from Damien Miller <djm at mindrot.org> ---
(In reply to Dmitry V. Levin from comment #21)
> Shouldn't the "jump false" argument of the first BPF_JUMP in
> SC_ALLOW_ARG's definition be changed from 3 to 4?
It could, at present it jumps to reloading the syscall number into the
accumulator. Jumping past it could save an instruction.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #23 from Damien Miller <djm at mindrot.org> ---
Created attachment 2652
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2652&action=edit
jump past accumulator reload

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2652|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2652|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

--- Comment #24 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2652
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2652
jump past accumulator reload

LGTM based on my approximately zero knowledge of BPF.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

--- Comment #25 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 2655
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2655&action=edit
missing pselect6

Just wanted to thank you that everything is applied in git and I was
just getting to test it, but now I found that pselect6 is missing from
the filter even though it was in the first patch. Can you add also this
one?

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #26 from Damien Miller <djm at mindrot.org> ---
Applied - thanks

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2361

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #27 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after 7.3p1 release

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.