On 2014-09-16 16:52:12, Steve Beattie wrote:
> On Mon, Sep 15, 2014 at 02:55:59PM -0500, Tyler Hicks wrote:
> > The unix_socket_client test program was using an abstract socket, which
> > was set up using the autobind feature, when testing any socket address
> > types.
> >
> > To more accurately test a specific address type, this patch changes the
> > client code to use whatever address type that the server is using. The
> > string ".client" will be added to the end of the server's address.
> >
> > Signed-off-by: Tyler Hicks <tyhi...@canonical.com>
> > ---
> > tests/regression/apparmor/unix_socket_client.c | 29 ++++++++++++++--
> > tests/regression/apparmor/unix_socket_pathname.sh | 40
> > +++++++++++++----------
> > 2 files changed, 49 insertions(+), 20 deletions(-)
> >
> > diff --git a/tests/regression/apparmor/unix_socket_client.c
> > b/tests/regression/apparmor/unix_socket_client.c
> > index bda7db6..30e5ee7 100644
> > --- a/tests/regression/apparmor/unix_socket_client.c
> > +++ b/tests/regression/apparmor/unix_socket_client.c
> > @@ -24,6 +24,9 @@
> >
> > #define MSG_BUF_MAX 1024
> >
> > +#define SUN_PATH_SUFFIX ".client"
> > +#define SUN_PATH_SUFFIX_LEN strlen(SUN_PATH_SUFFIX)
> > +
> > static int connection_based_messaging(int sock)
> > {
> > char msg_buf[MSG_BUF_MAX];
> > @@ -44,14 +47,33 @@ static int connection_based_messaging(int sock)
> > return 0;
> > }
> >
> > -static int connectionless_messaging(int sock)
> > +static int connectionless_messaging(int sock, struct sockaddr_un
> > *peer_addr,
> > + socklen_t peer_addr_len)
> > {
> > struct sockaddr_un addr;
> > + size_t peer_path_len = peer_addr_len - sizeof(addr.sun_family);
> > + size_t path_len = peer_path_len + SUN_PATH_SUFFIX_LEN;
> > char msg_buf[MSG_BUF_MAX];
> > int rc;
> >
> > + if (path_len > sizeof(addr.sun_path)) {
> > + fprintf(stderr, "FAIL CLIENT - path_len too big\n");
> > + return 1;
> > + }
> > +
> > + /**
> > + * Subtract 1 to get rid of nul-terminator in pathname address types.
> > + * We're essentially moving the nul char so path_len stays the same.
> > + */
> > + if (peer_addr->sun_path[0])
> > + peer_path_len--;
> > +
> > addr.sun_family = AF_UNIX;
> > - rc = bind(sock, (struct sockaddr *)&addr, sizeof(sa_family_t));
> > + memcpy(addr.sun_path, peer_addr->sun_path, peer_path_len);
> > + strcpy(addr.sun_path + peer_path_len, SUN_PATH_SUFFIX);
> > +
> > + rc = bind(sock, (struct sockaddr *)&addr,
> > + path_len + sizeof(addr.sun_family));
> > if (rc < 0) {
> > perror("FAIL CLIENT - bind");
> > return 1;
> > @@ -174,7 +196,8 @@ int main(int argc, char *argv[])
> >
> > rc = (type == SOCK_STREAM || type == SOCK_SEQPACKET) ?
> > connection_based_messaging(sock) :
> > - connectionless_messaging(sock);
> > + connectionless_messaging(sock, &peer_addr,
> > + sun_path_len + sizeof(peer_addr.sun_family));
>
> Is there a reason to add sizeof(peer_addr.sun_family) to sun_path_len
> to use as the peer_addr_len argument to connectionless_messaging(), when
> the only use of peer_addr_len is to immediately subtract it back off?
> Or would it just make sense to have connectionless_messaging() take the
> just the sun_path and path len as arguments?
I did it that way because the next patch in the series moves the call to
connect() into connection_based_messaging(), which means that we must
pass in (sun_path_len + sizeof(peer_addr.sun_family)) to
connection_based_messaging().
Instead of having connection_based_messaging() take (sun_path_len +
sizeof(peer_addr.sun_family)) and connectionless_messaging() take
sun_path_len, I decided to make them both take (sun_path_len +
sizeof(peer_addr.sun_family)).
>
> > if (rc)
> > exit(1);
> >
> > diff --git a/tests/regression/apparmor/unix_socket_pathname.sh
> > b/tests/regression/apparmor/unix_socket_pathname.sh
> > index 17ed855..d089d09 100755
> > --- a/tests/regression/apparmor/unix_socket_pathname.sh
> > +++ b/tests/regression/apparmor/unix_socket_pathname.sh
> > @@ -32,7 +32,8 @@ requires_features policy/versions/v6
> > settest unix_socket
> >
> > client=$bin/unix_socket_client
> > -sockpath=${tmpdir}/unix_socket.sock
> > +sockpath=${tmpdir}/aa_sock
> > +client_sockpath=${tmpdir}/aa_sock.client
> > message=4a0c83d87aaa7afa2baab5df3ee4df630f0046d5bfb7a3080c550b721f401b3b\
> > 8a738e1435a3b77aa6482a70fb51c44f20007221b85541b0184de66344d46a4c
> >
> > @@ -57,11 +58,15 @@ okclient=rw
> > badclient1=r
> > badclient2=w
> >
> > -removesocket()
> > +removesockets()
> > {
> > - if [ -S "$1" ]; then
> > - rm -f "$1"
> > - fi
> > + local sock
> > +
> > + for sock in "$@"; do
> > + if [ -S "$sock" ]; then
> > + rm -f "$sock"
> > + fi
> > + done
> > }
> >
> > testsocktype()
> > @@ -70,30 +75,30 @@ testsocktype()
> > local testdesc="AF_UNIX pathname socket ($socktype)"
> > local args="$sockpath $socktype $message $client"
> >
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> >
> > # PASS - unconfined
> >
> > runchecktest "$testdesc; unconfined" pass $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> >
> > # PASS - server w/ access to the file
> >
> > genprofile $sockpath:$okserver $af_unix $client:Ux
> > runchecktest "$testdesc; confined server w/ access ($okserver)" pass
> > $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> >
> > # FAIL - server w/o access to the file
> >
> > genprofile $af_unix $client:Ux
> > runchecktest "$testdesc; confined server w/o access" fail $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> >
> > # FAIL - server w/ bad access to the file
> >
> > genprofile $sockpath:$badserver1 $af_unix $client:Ux
> > runchecktest "$testdesc; confined server w/ bad access ($badserver1)"
> > fail $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> >
> > # $badserver2 is set to non-null at the top of the test script if the
> > # kernel advertises ABI v7 or newer
> > @@ -102,7 +107,8 @@ testsocktype()
> >
> > genprofile $sockpath:$badserver2 $af_unix $client:Ux
> > runchecktest "$testdesc; confined server w/ bad access
> > ($badserver2)" fail $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> > +
> > fi
> >
> > if [ -n "$af_unix" ] ; then
> > @@ -113,38 +119,38 @@ testsocktype()
> > removesockets $sockpath
>
> Ah, this is where removesockets leaked in to patch 04. Presumably
> $client_sockpath needs to be added there as well?
I moved some of these patches around between v1 and v2. I thought I had
caught all of the removesocket -> removesockets changes but obviously
not. Thanks! :)
Tyler
>
> > fi
> >
> > - server="$sockpath:$okserver $af_unix $client:px"
> > + server="$sockpath:$okserver $client_sockpath:$okserver $af_unix
> > $client:px"
> >
> > # PASS - client w/ access to the file
> >
> > genprofile $server -- image=$client $sockpath:$okclient $af_unix
> > runchecktest "$testdesc; confined client w/ access ($okclient)" pass
> > $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> >
> > # FAIL - client w/o access to the file
> >
> > genprofile $server -- image=$client $af_unix
> > runchecktest "$testdesc; confined client w/o access" fail $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> >
> > # FAIL - client w/ bad access to the file
> >
> > genprofile $server -- image=$client $sockpath:$badclient1 $af_unix
> > runchecktest "$testdesc; confined client w/ bad access ($badclient1)"
> > fail $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> >
> > # FAIL - client w/ bad access to the file
> >
> > genprofile $server -- image=$client $sockpath:$badclient2
> > runchecktest "$testdesc; confined client w/ bad access ($badclient2)"
> > fail $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> >
> > if [ -n "$af_unix" ] ; then
> > # FAIL - client w/o af_unix access
> >
> > genprofile $server -- image=$client $sockpath:$okclient
> > runchecktest "$testdesc; confined client w/o af_unix" fail $args
> > - removesocket $sockpath
> > + removesockets $sockpath $client_sockpath
> > fi
> >
> > removeprofile
>
> --
> Steve Beattie
> <sbeat...@ubuntu.com>
> http://NxNW.org/~steve/
> --
> AppArmor mailing list
> AppArmor@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
