Skip to content

Commit 6d26372

Browse files
kjbraceykjbracey
committed
Fix secure operation; some name and logical tidies
Secure sending code was passing the remote address as the source address, bizarrely. This records the local address last used as the destination for incoming packets and uses that. Other fix-ups: * dest_addr (ie remote address) removed from internal_socket_t; this isn't safe, as one socket talks to multiple remote peers. Use address in secure_session_t instead. * Some renaming: listen_socket->socket, send/receive callbacks, remote_address+remote_port -> remote_host
1 parent 115aa3e commit 6d26372

File tree

1 file changed

+51
-55
lines changed

1 file changed

+51
-55
lines changed

‎source/coap_connection_handler.c

Lines changed: 51 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,12 @@ typedef struct internal_socket_s {
2727
uint32_t timeout_min;
2828
uint32_t timeout_max;
2929

30-
uint16_t listen_port;
31-
int8_t listen_socket;
30+
uint16_t listen_port; // 0 for ephemeral-port sockets
3231

33-
ns_address_t dest_addr;
3432
int16_t data_len;
3533
uint8_t *data;
3634

35+
int8_t socket;
3736
bool real_socket;
3837
uint8_t usage_counter;
3938
bool is_secure;
@@ -64,8 +63,9 @@ typedef struct secure_session {
6463
coap_security_t *sec_handler; //owned
6564
internal_socket_t *parent; //not owned
6665

67-
uint8_t remote_address[16];
68-
uint16_t remote_port;
66+
ns_address_t remote_host;
67+
uint8_t local_address[16];
68+
// local port is fixed by socket
6969

7070
secure_timer_t timer;
7171

@@ -75,8 +75,8 @@ typedef struct secure_session {
7575
} secure_session_t;
7676

7777
static NS_LIST_DEFINE(secure_session_list, secure_session_t, link);
78-
static int send_to_socket(int8_t socket_id, void *handle, const void *buf, size_t len);
79-
static int receive_from_socket(int8_t socket_id, unsigned char *buf, size_t len);
78+
static int secure_session_sendto(int8_t socket_id, void *handle, const void *buf, size_t len);
79+
static int secure_session_recvfrom(int8_t socket_id, unsigned char *buf, size_t len);
8080
static void start_timer(int8_t timer_id, uint32_t int_ms, uint32_t fin_ms);
8181
static int timer_status(int8_t timer_id);
8282

@@ -95,7 +95,7 @@ static secure_session_t *secure_session_find_by_timer_id(int8_t timer_id)
9595
static void secure_session_delete(secure_session_t *this)
9696
{
9797
if (this) {
98-
transactions_delete_all(this->parent->dest_addr.address, this->parent->dest_addr.identifier);
98+
transactions_delete_all(this->remote_host.address, this->remote_host.identifier);
9999
ns_list_remove(&secure_session_list, this);
100100
if( this->sec_handler ){
101101
coap_security_destroy(this->sec_handler);
@@ -150,11 +150,12 @@ static secure_session_t *secure_session_create(internal_socket_t *parent, const
150150
timer_id++;
151151
}
152152
this->timer.id = timer_id;
153-
memcpy(this->remote_address, address_ptr, 16);
154-
this->remote_port = port;
153+
this->remote_host.type = ADDRESS_IPV6;
154+
memcpy(this->remote_host.address, address_ptr, 16);
155+
this->remote_host.identifier = port;
155156

156-
this->sec_handler = coap_security_create(parent->listen_socket, this->timer.id, this, ECJPAKE,
157-
&send_to_socket, &receive_from_socket, &start_timer, &timer_status);
157+
this->sec_handler = coap_security_create(parent->socket, this->timer.id, this, ECJPAKE,
158+
&secure_session_sendto, &secure_session_recvfrom, &start_timer, &timer_status);
158159
if( !this->sec_handler ){
159160
ns_dyn_mem_free(this);
160161
return NULL;
@@ -184,10 +185,9 @@ static secure_session_t *secure_session_find(internal_socket_t *parent, const ui
184185
secure_session_t *this = NULL;
185186
ns_list_foreach(secure_session_t, cur_ptr, &secure_session_list) {
186187
if( cur_ptr->sec_handler ){
187-
if (cur_ptr->parent == parent && cur_ptr->remote_port == port &&
188-
memcmp(cur_ptr->remote_address, address_ptr, 16) == 0) {
188+
if (cur_ptr->parent == parent && cur_ptr->remote_host.identifier == port &&
189+
memcmp(cur_ptr->remote_host.address, address_ptr, 16) == 0) {
189190
this = cur_ptr;
190-
// hack_save_remote_address(address_ptr, port);
191191
break;
192192
}
193193
}
@@ -217,36 +217,36 @@ static internal_socket_t *int_socket_create(uint16_t listen_port, bool use_ephem
217217
this->listen_port = listen_port;
218218
this->real_socket = real_socket;
219219
this->bypass_link_sec = bypassSec;
220-
this->listen_socket = -1;
220+
this->socket = -1;
221221
if( real_socket ){
222222
if( use_ephemeral_port ){ //socket_api creates ephemeral port if the one provided is 0
223223
listen_port = 0;
224224
}
225225
if( !is_secure ){
226-
this->listen_socket = socket_open(SOCKET_UDP, listen_port, recv_sckt_msg);
226+
this->socket = socket_open(SOCKET_UDP, listen_port, recv_sckt_msg);
227227
}else{
228228
#ifdef COAP_SECURITY_AVAILABLE
229-
this->listen_socket = socket_open(SOCKET_UDP, listen_port, secure_recv_sckt_msg);
229+
this->socket = socket_open(SOCKET_UDP, listen_port, secure_recv_sckt_msg);
230230
#else
231231
tr_err("Secure CoAP unavailable - SSL library not configured, possibly due to lack of entropy source");
232232
#endif
233233
}
234234
// Socket create failed
235-
if(this->listen_socket < 0){
235+
if(this->socket < 0){
236236
ns_dyn_mem_free(this);
237237
return NULL;
238238
}
239239

240-
socket_setsockopt(this->listen_socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &(const int8_t) {bypassSec ? 0 : 1}, sizeof(int8_t));
240+
socket_setsockopt(this->socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &(const int8_t) {bypassSec ? 0 : 1}, sizeof(int8_t));
241241

242242
// XXX API for this? May want to get clever to do recommended first query = 1 hop, retries = whole PAN
243-
socket_setsockopt(this->listen_socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_MULTICAST_HOPS, &(const int16_t) {16}, sizeof(int16_t));
243+
socket_setsockopt(this->socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_MULTICAST_HOPS, &(const int16_t) {16}, sizeof(int16_t));
244244

245245
// Set socket option to receive packet info
246-
socket_setsockopt(this->listen_socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_RECVPKTINFO, &(const bool) {1}, sizeof(bool));
246+
socket_setsockopt(this->socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_RECVPKTINFO, &(const bool) {1}, sizeof(bool));
247247

248248
}else{
249-
this->listen_socket = -1;
249+
this->socket = -1;
250250
}
251251

252252
ns_list_add_to_start(&socket_list, this);
@@ -259,7 +259,7 @@ static void int_socket_delete(internal_socket_t *this)
259259
this->usage_counter--;
260260
if(this->usage_counter == 0){
261261
clear_secure_sessions(this);
262-
socket_free(this->listen_socket);
262+
socket_free(this->socket);
263263
ns_list_remove(&socket_list, this);
264264
if( this->data ){
265265
ns_dyn_mem_free(this->data);
@@ -277,7 +277,7 @@ static internal_socket_t *int_socket_find_by_socket_id(int8_t id)
277277
{
278278
internal_socket_t *this = NULL;
279279
ns_list_foreach(internal_socket_t, cur_ptr, &socket_list) {
280-
if( cur_ptr->listen_socket == id ) {
280+
if( cur_ptr->socket == id ) {
281281
this = cur_ptr;
282282
break;
283283
}
@@ -339,7 +339,7 @@ static int8_t send_to_real_socket(int8_t socket_id, const ns_address_t *address,
339339
return socket_sendmsg(socket_id, &msghdr, 0);
340340
}
341341

342-
static int send_to_socket(int8_t socket_id, void *handle, const void *buf, size_t len)
342+
static int secure_session_sendto(int8_t socket_id, void *handle, const void *buf, size_t len)
343343
{
344344
secure_session_t *session = handle;
345345
internal_socket_t *sock = int_socket_find_by_socket_id(socket_id);
@@ -348,7 +348,7 @@ static int send_to_socket(int8_t socket_id, void *handle, const void *buf, size_
348348
}
349349
if(!sock->real_socket){
350350
// Send to virtual socket cb
351-
int ret = sock->parent->_send_cb(sock->listen_socket, session->remote_address, session->remote_port, buf, len);
351+
int ret = sock->parent->_send_cb(sock->socket, session->remote_host.address, session->remote_host.identifier, buf, len);
352352
if( ret < 0 )
353353
return ret;
354354
return len;
@@ -359,19 +359,19 @@ static int send_to_socket(int8_t socket_id, void *handle, const void *buf, size_
359359
if (sock->bypass_link_sec) {
360360
securityLinkLayer = 0;
361361
}
362-
socket_setsockopt(sock->listen_socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_ADDR_PREFERENCES, &opt_name, sizeof(int));
363-
socket_setsockopt(sock->listen_socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &securityLinkLayer, sizeof(int8_t));
362+
socket_setsockopt(sock->socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_ADDR_PREFERENCES, &opt_name, sizeof(int));
363+
socket_setsockopt(sock->socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &securityLinkLayer, sizeof(int8_t));
364364
//For some reason socket_sendto returns 0 in success, while other socket impls return number of bytes sent!!!
365365
//TODO: check if address_ptr is valid and use that instead if it is
366366

367-
int8_t ret = send_to_real_socket(sock->listen_socket, &sock->dest_addr, session->remote_address, buf, len);
367+
int8_t ret = send_to_real_socket(sock->socket, &session->remote_host, session->local_address, buf, len);
368368
if (ret < 0) {
369369
return ret;
370370
}
371371
return len;
372372
}
373373

374-
static int receive_from_socket(int8_t socket_id, unsigned char *buf, size_t len)
374+
static int secure_session_recvfrom(int8_t socket_id, unsigned char *buf, size_t len)
375375
{
376376
(void)len;
377377
internal_socket_t *sock = int_socket_find_by_socket_id(socket_id);
@@ -537,21 +537,23 @@ static void secure_recv_sckt_msg(void *cb_res)
537537

538538
// Create session
539539
if (!session) {
540-
memcpy( sock->dest_addr.address, src_address.address, 16 );
541-
sock->dest_addr.identifier = src_address.identifier;
542-
sock->dest_addr.type = src_address.type;
543540
session = secure_session_create(sock, src_address.address, src_address.identifier);
544541
}
545542
if (!session) {
546543
tr_err("secure_recv_sckt_msg session creation failed - OOM");
547544
return;
548545
}
546+
// Record the destination. We are not strict on local address - all
547+
// session_find calls match only on remote address and port. But we
548+
// record the last-used destination address to use it as the source of
549+
// outgoing packets.
550+
memcpy(session->local_address, dst_address, 16);
549551
session->last_contact_time = coap_service_get_internal_timer_ticks();
550552
// Start handshake
551553
if (!coap_security_handler_is_started(session->sec_handler) ){
552554
uint8_t *pw = ns_dyn_mem_alloc(64);
553555
uint8_t pw_len;
554-
if( sock->parent->_get_password_cb && 0 == sock->parent->_get_password_cb(sock->listen_socket, src_address.address, src_address.identifier, pw, &pw_len)){
556+
if( sock->parent->_get_password_cb && 0 == sock->parent->_get_password_cb(sock->socket, src_address.address, src_address.identifier, pw, &pw_len)){
555557
//TODO: get_password_cb should support certs and PSK also
556558
coap_security_keys_t keys;
557559
keys._priv = pw;
@@ -570,7 +572,7 @@ static void secure_recv_sckt_msg(void *cb_res)
570572
session->timer.timer = NULL;
571573
session->session_state = SECURE_SESSION_OK;
572574
if( sock->parent->_security_done_cb ){
573-
sock->parent->_security_done_cb(sock->listen_socket, src_address.address,
575+
sock->parent->_security_done_cb(sock->socket, src_address.address,
574576
src_address.identifier,
575577
(void *)coap_security_handler_keyblock(session->sec_handler));
576578
}
@@ -592,7 +594,7 @@ static void secure_recv_sckt_msg(void *cb_res)
592594
ns_dyn_mem_free(data);
593595
} else {
594596
if (sock->parent->_recv_cb) {
595-
sock->parent->_recv_cb(sock->listen_socket, src_address.address, src_address.identifier, dst_address, data, len);
597+
sock->parent->_recv_cb(sock->socket, src_address.address, src_address.identifier, dst_address, data, len);
596598
}
597599
ns_dyn_mem_free(data);
598600
}
@@ -610,7 +612,7 @@ static void recv_sckt_msg(void *cb_res)
610612

611613
if (sock && read_data(sckt_data, sock, &src_address, dst_address) == 0) {
612614
if (sock->parent && sock->parent->_recv_cb) {
613-
sock->parent->_recv_cb(sock->listen_socket, src_address.address, src_address.identifier, dst_address, sock->data, sock->data_len);
615+
sock->parent->_recv_cb(sock->socket, src_address.address, src_address.identifier, dst_address, sock->data, sock->data_len);
614616
}
615617
ns_dyn_mem_free(sock->data);
616618
sock->data = NULL;
@@ -656,7 +658,7 @@ int coap_connection_handler_virtual_recv(coap_conn_handler_t *handler, uint8_t a
656658
if (!coap_security_handler_is_started(session->sec_handler)) {
657659
uint8_t *pw = ns_dyn_mem_alloc(64);
658660
uint8_t pw_len;
659-
if (sock->parent->_get_password_cb && 0 == sock->parent->_get_password_cb(sock->listen_socket, address, port, pw, &pw_len)) {
661+
if (sock->parent->_get_password_cb && 0 == sock->parent->_get_password_cb(sock->socket, address, port, pw, &pw_len)) {
660662
//TODO: get_password_cb should support certs and PSK also
661663
coap_security_keys_t keys;
662664
keys._priv = pw;
@@ -675,7 +677,7 @@ int coap_connection_handler_virtual_recv(coap_conn_handler_t *handler, uint8_t a
675677
if(ret == 0){
676678
session->session_state = SECURE_SESSION_OK;
677679
if( handler->_security_done_cb ){
678-
handler->_security_done_cb(sock->listen_socket,
680+
handler->_security_done_cb(sock->socket,
679681
address, port,
680682
(void *)coap_security_handler_keyblock(session->sec_handler));
681683
}
@@ -700,7 +702,7 @@ int coap_connection_handler_virtual_recv(coap_conn_handler_t *handler, uint8_t a
700702
return 0;
701703
} else {
702704
if (sock->parent->_recv_cb) {
703-
sock->parent->_recv_cb(sock->listen_socket, address, port, ns_in6addr_any, data, len);
705+
sock->parent->_recv_cb(sock->socket, address, port, ns_in6addr_any, data, len);
704706
}
705707
ns_dyn_mem_free(data);
706708
data = NULL;
@@ -711,7 +713,7 @@ int coap_connection_handler_virtual_recv(coap_conn_handler_t *handler, uint8_t a
711713
} else {
712714
/* unsecure*/
713715
if (sock->parent->_recv_cb) {
714-
sock->parent->_recv_cb(sock->listen_socket, address, port, ns_in6addr_any, sock->data, sock->data_len);
716+
sock->parent->_recv_cb(sock->socket, address, port, ns_in6addr_any, sock->data, sock->data_len);
715717
}
716718
if (sock->data) {
717719
ns_dyn_mem_free(sock->data);
@@ -757,7 +759,7 @@ void connection_handler_close_secure_connection( coap_conn_handler_t *handler, u
757759
{
758760
if (handler) {
759761
if (handler->socket && handler->socket->is_secure) {
760-
secure_session_t *session = secure_session_find( handler->socket, destination_addr_ptr, port);
762+
secure_session_t *session = secure_session_find( handler->socket, destination_addr_ptr, port );
761763
if (session) {
762764
coap_security_send_close_alert( session->sec_handler );
763765
session->session_state = SECURE_SESSION_CLOSED;
@@ -806,26 +808,20 @@ int coap_connection_handler_send_data(coap_conn_handler_t *handler, const ns_add
806808
}
807809
if (handler->socket->is_secure) {
808810
handler->socket->bypass_link_sec = bypass_link_sec;
809-
memcpy(handler->socket->dest_addr.address, dest_addr->address, 16);
810-
handler->socket->dest_addr.identifier = dest_addr->identifier;
811-
handler->socket->dest_addr.type = dest_addr->type;
812811
secure_session_t *session = secure_session_find(handler->socket, dest_addr->address, dest_addr->identifier);
813812
if (!session) {
814813
session = secure_session_create(handler->socket, dest_addr->address, dest_addr->identifier);
815814
if (!session) {
816815
return -1;
817816
}
818817
session->last_contact_time = coap_service_get_internal_timer_ticks();
819-
memcpy( handler->socket->dest_addr.address, dest_addr->address, 16 );
820-
handler->socket->dest_addr.identifier = dest_addr->identifier;
821-
handler->socket->dest_addr.type = dest_addr->type;
822818
uint8_t *pw = ns_dyn_mem_alloc(64);
823819
if (!pw) {
824820
//todo: free secure session?
825821
return -1;
826822
}
827823
uint8_t pw_len;
828-
if (handler->_get_password_cb && 0 == handler->_get_password_cb(handler->socket->listen_socket, (uint8_t*)dest_addr->address, dest_addr->identifier, pw, &pw_len)) {
824+
if (handler->_get_password_cb && 0 == handler->_get_password_cb(handler->socket->socket, (uint8_t*)dest_addr->address, dest_addr->identifier, pw, &pw_len)) {
829825
//TODO: get_password_cb should support certs and PSK also
830826
coap_security_keys_t keys;
831827
keys._priv = pw;
@@ -847,18 +843,18 @@ int coap_connection_handler_send_data(coap_conn_handler_t *handler, const ns_add
847843
return -1;
848844
}else{
849845
if (!handler->socket->real_socket && handler->_send_cb) {
850-
return handler->_send_cb((int8_t)handler->socket->listen_socket, dest_addr->address, dest_addr->identifier, data_ptr, data_len);
846+
return handler->_send_cb((int8_t)handler->socket->socket, dest_addr->address, dest_addr->identifier, data_ptr, data_len);
851847
}
852848
int opt_name = SOCKET_IPV6_PREFER_SRC_6LOWPAN_SHORT;
853849
int8_t securityLinkLayer = 1;
854850
if (bypass_link_sec) {
855851
securityLinkLayer = 0;
856852
}
857853

858-
socket_setsockopt(handler->socket->listen_socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_ADDR_PREFERENCES, &opt_name, sizeof(int));
859-
socket_setsockopt(handler->socket->listen_socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &securityLinkLayer, sizeof(int8_t));
854+
socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_ADDR_PREFERENCES, &opt_name, sizeof(int));
855+
socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &securityLinkLayer, sizeof(int8_t));
860856

861-
return send_to_real_socket(handler->socket->listen_socket, dest_addr, src_address, data_ptr, data_len);
857+
return send_to_real_socket(handler->socket->socket, dest_addr, src_address, data_ptr, data_len);
862858
}
863859
}
864860

@@ -868,7 +864,7 @@ bool coap_connection_handler_socket_belongs_to(coap_conn_handler_t *handler, int
868864
return false;
869865
}
870866

871-
if( handler->socket->listen_socket == socket_id){
867+
if( handler->socket->socket == socket_id){
872868
return true;
873869
}
874870
return false;

0 commit comments

Comments
 (0)