Skip to content

Commit

Permalink
rxrpc: Fix missing security check on incoming calls
Browse files Browse the repository at this point in the history 
Fix rxrpc_new_incoming_call() to check that we have a suitable service key
available for the combination of service ID and security class of a new
incoming call - and to reject calls for which we don't.

This causes an assertion like the following to appear:

	rxrpc: Assertion failed - 6(0x6) == 12(0xc) is false
	kernel BUG at net/rxrpc/call_object.c:456!

Where call->state is RXRPC_CALL_SERVER_SECURING (6) rather than
RXRPC_CALL_COMPLETE (12).

Fixes: 248f219 ("rxrpc: Rewrite the data and ack handling code")
Reported-by: Marc Dionne <[email protected]>
Signed-off-by: David Howells <[email protected]>
  • Loading branch information
@dhowells
dhowells committed Dec 20, 2019
1 parent 13b7955 commit 063c60d
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 60 deletions.
10 changes: 7 additions & 3 deletions net/rxrpc/ar-internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ struct rxrpc_skb_priv {
struct rxrpc_security {
const char *name; /* name of this service */
u8 security_index; /* security type provided */
u32 no_key_abort; /* Abort code indicating no key */

/* Initialise a security service */
int (*init)(void);
Expand Down Expand Up @@ -977,8 +978,9 @@ static inline void rxrpc_reduce_conn_timer(struct rxrpc_connection *conn,
struct rxrpc_connection *rxrpc_find_service_conn_rcu(struct rxrpc_peer *,
struct sk_buff *);
struct rxrpc_connection *rxrpc_prealloc_service_connection(struct rxrpc_net *, gfp_t);
void rxrpc_new_incoming_connection(struct rxrpc_sock *,
struct rxrpc_connection *, struct sk_buff *);
void rxrpc_new_incoming_connection(struct rxrpc_sock *, struct rxrpc_connection *,
const struct rxrpc_security *, struct key *,
struct sk_buff *);
void rxrpc_unpublish_service_conn(struct rxrpc_connection *);

/*
Expand Down Expand Up @@ -1103,7 +1105,9 @@ extern const struct rxrpc_security rxkad;
int __init rxrpc_init_security(void);
void rxrpc_exit_security(void);
int rxrpc_init_client_conn_security(struct rxrpc_connection *);
int rxrpc_init_server_conn_security(struct rxrpc_connection *);
bool rxrpc_look_up_server_security(struct rxrpc_local *, struct rxrpc_sock *,
const struct rxrpc_security **, struct key **,
struct sk_buff *);

/*
* sendmsg.c
Expand Down
14 changes: 11 additions & 3 deletions net/rxrpc/call_accept.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,8 @@ static struct rxrpc_call *rxrpc_alloc_incoming_call(struct rxrpc_sock *rx,
struct rxrpc_local *local,
struct rxrpc_peer *peer,
struct rxrpc_connection *conn,
const struct rxrpc_security *sec,
struct key *key,
struct sk_buff *skb)
{
struct rxrpc_backlog *b = rx->backlog;
Expand Down Expand Up @@ -310,7 +312,7 @@ static struct rxrpc_call *rxrpc_alloc_incoming_call(struct rxrpc_sock *rx,
conn->params.local = rxrpc_get_local(local);
conn->params.peer = peer;
rxrpc_see_connection(conn);
rxrpc_new_incoming_connection(rx, conn, skb);
rxrpc_new_incoming_connection(rx, conn, sec, key, skb);
} else {
rxrpc_get_connection(conn);
}
Expand Down Expand Up @@ -349,9 +351,11 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
struct sk_buff *skb)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
const struct rxrpc_security *sec = NULL;
struct rxrpc_connection *conn;
struct rxrpc_peer *peer = NULL;
struct rxrpc_call *call;
struct rxrpc_call *call = NULL;
struct key *key = NULL;

_enter("");

Expand All @@ -372,7 +376,11 @@ struct rxrpc_call *rxrpc_new_incoming_call(struct rxrpc_local *local,
*/
conn = rxrpc_find_connection_rcu(local, skb, &peer);

call = rxrpc_alloc_incoming_call(rx, local, peer, conn, skb);
if (!conn && !rxrpc_look_up_server_security(local, rx, &sec, &key, skb))
goto no_call;

call = rxrpc_alloc_incoming_call(rx, local, peer, conn, sec, key, skb);
key_put(key);
if (!call) {
skb->mark = RXRPC_SKB_MARK_REJECT_BUSY;
goto no_call;
Expand Down
16 changes: 1 addition & 15 deletions net/rxrpc/conn_event.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,21 +376,7 @@ static void rxrpc_secure_connection(struct rxrpc_connection *conn)
_enter("{%d}", conn->debug_id);

ASSERT(conn->security_ix != 0);

if (!conn->params.key) {
_debug("set up security");
ret = rxrpc_init_server_conn_security(conn);
switch (ret) {
case 0:
break;
case -ENOENT:
abort_code = RX_CALL_DEAD;
goto abort;
default:
abort_code = RXKADNOAUTH;
goto abort;
}
}
ASSERT(conn->server_key);

if (conn->security->issue_challenge(conn) < 0) {
abort_code = RX_CALL_DEAD;
Expand Down
4 changes: 4 additions & 0 deletions net/rxrpc/conn_service.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ struct rxrpc_connection *rxrpc_prealloc_service_connection(struct rxrpc_net *rxn
*/
void rxrpc_new_incoming_connection(struct rxrpc_sock *rx,
struct rxrpc_connection *conn,
const struct rxrpc_security *sec,
struct key *key,
struct sk_buff *skb)
{
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
Expand All @@ -160,6 +162,8 @@ void rxrpc_new_incoming_connection(struct rxrpc_sock *rx,
conn->service_id = sp->hdr.serviceId;
conn->security_ix = sp->hdr.securityIndex;
conn->out_clientflag = 0;
conn->security = sec;
conn->server_key = key_get(key);
if (conn->security_ix)
conn->state = RXRPC_CONN_SERVICE_UNSECURED;
else
Expand Down
5 changes: 3 additions & 2 deletions net/rxrpc/rxkad.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -648,9 +648,9 @@ static int rxkad_issue_challenge(struct rxrpc_connection *conn)
u32 serial;
int ret;

_enter("{%d,%x}", conn->debug_id, key_serial(conn->params.key));
_enter("{%d,%x}", conn->debug_id, key_serial(conn->server_key));

ret = key_validate(conn->params.key);
ret = key_validate(conn->server_key);
if (ret < 0)
return ret;

Expand Down Expand Up @@ -1293,6 +1293,7 @@ static void rxkad_exit(void)
const struct rxrpc_security rxkad = {
.name = "rxkad",
.security_index = RXRPC_SECURITY_RXKAD,
.no_key_abort = RXKADUNKNOWNKEY,
.init = rxkad_init,
.exit = rxkad_exit,
.init_connection_security = rxkad_init_connection_security,
Expand Down
70 changes: 33 additions & 37 deletions net/rxrpc/security.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,62 +101,58 @@ int rxrpc_init_client_conn_security(struct rxrpc_connection *conn)
}

/*
* initialise the security on a server connection
* Find the security key for a server connection.
*/
int rxrpc_init_server_conn_security(struct rxrpc_connection *conn)
bool rxrpc_look_up_server_security(struct rxrpc_local *local, struct rxrpc_sock *rx,
const struct rxrpc_security **_sec,
struct key **_key,
struct sk_buff *skb)
{
const struct rxrpc_security *sec;
struct rxrpc_local *local = conn->params.local;
struct rxrpc_sock *rx;
struct key *key;
key_ref_t kref;
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
key_ref_t kref = NULL;
char kdesc[5 + 1 + 3 + 1];

_enter("");

sprintf(kdesc, "%u:%u", conn->service_id, conn->security_ix);
sprintf(kdesc, "%u:%u", sp->hdr.serviceId, sp->hdr.securityIndex);

sec = rxrpc_security_lookup(conn->security_ix);
sec = rxrpc_security_lookup(sp->hdr.securityIndex);
if (!sec) {
_leave(" = -ENOKEY [lookup]");
return -ENOKEY;
trace_rxrpc_abort(0, "SVS",
sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
RX_INVALID_OPERATION, EKEYREJECTED);
skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
skb->priority = RX_INVALID_OPERATION;
return false;
}

/* find the service */
read_lock(&local->services_lock);
rx = rcu_dereference_protected(local->service,
lockdep_is_held(&local->services_lock));
if (rx && (rx->srx.srx_service == conn->service_id ||
rx->second_service == conn->service_id))
goto found_service;
if (sp->hdr.securityIndex == RXRPC_SECURITY_NONE)
goto out;

/* the service appears to have died */
read_unlock(&local->services_lock);
_leave(" = -ENOENT");
return -ENOENT;

found_service:
if (!rx->securities) {
read_unlock(&local->services_lock);
_leave(" = -ENOKEY");
return -ENOKEY;
trace_rxrpc_abort(0, "SVR",
sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
RX_INVALID_OPERATION, EKEYREJECTED);
skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
skb->priority = RX_INVALID_OPERATION;
return false;
}

/* look through the service's keyring */
kref = keyring_search(make_key_ref(rx->securities, 1UL),
&key_type_rxrpc_s, kdesc, true);
if (IS_ERR(kref)) {
read_unlock(&local->services_lock);
_leave(" = %ld [search]", PTR_ERR(kref));
return PTR_ERR(kref);
trace_rxrpc_abort(0, "SVK",
sp->hdr.cid, sp->hdr.callNumber, sp->hdr.seq,
sec->no_key_abort, EKEYREJECTED);
skb->mark = RXRPC_SKB_MARK_REJECT_ABORT;
skb->priority = sec->no_key_abort;
return false;
}

key = key_ref_to_ptr(kref);
read_unlock(&local->services_lock);

conn->server_key = key;
conn->security = sec;

_leave(" = 0");
return 0;
out:
*_sec = sec;
*_key = key_ref_to_ptr(kref);
return true;
}

0 comments on commit 063c60d

Please sign in to comment.