Re: [PATCH] (02/14/08 Linus git) Smack unlabeled outgoing ambient packets - v3

[Paul Moore]

On Friday 15 February 2008 12:38:49 am Casey Schaufler wrote:
> From: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>
> Smack uses CIPSO labeling, but allows for unlabeled packets
> by specifying an "ambient" label that is applied to incoming
> unlabeled packets. Because the other end of the connection
> may dislike IP options, and ssh is one know application that
> behaves thus, it is prudent to respond in kind. This patch
> changes the network labeling behavior such that an outgoing
> packet that would be given a CIPSO label that matches the
> ambient label is left unlabeled. An "unlbl" domain is added
> and the netlabel defaulting mechanism invoked rather than
> assuming that everything is CIPSO. Locking has been added
> around changes to the ambient label as the mechanisms used
> to do so are more involved.
>
> Cleaned up some issues noted in review.
> Make smk_cipso_doi() static.
> Create a hook for the new security_secctx_to_secid()
> using existing underlying code.
> Fill in audit data for netlbl domain calls.
> Collapse unnecessary multiple assignments.
>
> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>

Hi Casey,

Thanks for the update, it's much improved.  I'd ack it except for one 
last thing which popped up in this revision ... (and don't worry, it's 
kinda my fault - not yours) ...

> @@ -1282,15 +1281,21 @@ static int smack_netlabel(struct sock *s
>  {
>  	struct socket_smack *ssp;
>  	struct netlbl_lsm_secattr secattr;
> -	int rc = 0;
> +	int rc;
>
>  	ssp = sk->sk_security;
>  	netlbl_secattr_init(&secattr);
>  	smack_to_secattr(ssp->smk_out, &secattr);
> -	if (secattr.flags != NETLBL_SECATTR_NONE)
> -		rc = netlbl_sock_setattr(sk, &secattr);
> -
> +	rc = netlbl_sock_setattr(sk, &secattr);
>  	netlbl_secattr_destroy(&secattr);
> +
> +	/*
> +	 * A return of -ENOENT from netlbl_sock_setattr
> +	 * indicates that the "domain" was not found, but that's
> +	 * not an issue because of the defaulting behavior.
> +	 */
> +	if (rc == -ENOENT)
> +		rc = 0;
>  	return rc;
>  }

... you shouldn't fix-up the return value from netlbl_sock_setattr().  
It only returns an error when there really is an error, if there are no 
matching domain mappings and the default catches the "domain" then the 
function will return 0 (assuming no other failures).

The fact that you ran into this problem isn't your fault, it's mine, but 
thankfully for both of us Pavel Emelyanov found this bug and fixed 
it[1].  It hasn't hit Linus' tree yet but it's in the net-2.6 tree.  If 
you can't wait for it to hit Linus' tree you can always apply the fix 
by hand, it's pretty minor.

Sorry about that.

[1]http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=4c3a0a254e5d706d3fe01bf42261534858d05586

-- 
paul moore
linux security @ hp
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/