Add a CAN bus TLS example using ISO-TP transport by LinuxJedi · Pull Request #279 · wolfSSL/wolfssl-examples

LinuxJedi · 2021-12-07T10:41:55Z

This provides a simple echo server / client that uses ISO-TP over CAN
bus and wolfSSL for TLS.

dgarske

Great work! Just a few easy cleanups and this will be ready.

dgarske · 2021-12-07T16:43:50Z

+
+#include "common.h"
+
+#define ISOTP_BUFSIZE 65535


Why not use the max TLS record size 16KB (16384)?

It was arbitrary whilst I was getting things running, but a good point. I'll clean up this and the rest.

dgarske · 2021-12-07T16:44:17Z

+    ctx = wolfSSL_CTX_new(method);
+    if (!ctx) {
+        printf("Could not init wolfSSL context\n");
+        return -1;


Will leak method.

dgarske · 2021-12-07T16:44:56Z

+    if (wolfSSL_CTX_load_verify_locations(ctx, "client.pem", NULL)
+        != SSL_SUCCESS) {
+        fprintf(stderr, "ERROR: failed to load cert, please check the file.\n");
+        return -1;


Will leak ctx. Perhaps a goto with cleanup would be a good option for this example? I realize this may have been a copy paste from another example, but the example should not leak memory in error cases.

dgarske · 2021-12-07T16:45:28Z

+    }
+
+    /* Setup ISO-TP to ID 0x7 and set the buffers */
+    isotp_init_link(&g_link, 0x7, g_isotpSendBuf, sizeof(g_isotpSendBuf), g_isotpRecvBuf, sizeof(g_isotpRecvBuf));


Please limit line length to 80 chars.

dgarske · 2021-12-07T16:46:34Z

+        printf("Message sent\n");
+        free(line);
+    }
+    can_close();


I don't see any cleanup. wolfSSL_free(ssl) or wolfSSL_CTX_free(ctx).

dgarske · 2021-12-07T16:52:09Z

+ * buffer and just return what wolfSSL is asking for */
+int recv_ssl(WOLFSSL* ssl, char* buf, int sz, void* ctx)
+{
+    uint8_t data[8];


Can you make the magic 8 data size a macro or enum? I suspect this is the max CAN payload?

dgarske · 2021-12-07T16:52:18Z

+int send_ssl(WOLFSSL *ssl, char *buf, int sz, void *ctx);
+int recv_ssl(WOLFSSL* ssl, char* buf, int sz, void* ctx);
+
+#endif // __CANCOMMON_H__


Please use C style /* */ comment.

dgarske · 2021-12-07T16:52:25Z

+        }
+    }
+
+    can_close();


Add proper cleanup.

dgarske · 2021-12-07T16:53:22Z

+       return 1;
+    }
+    setsockopt(sock, SOL_CAN_RAW, CAN_RAW_FILTER, &rfilter, sizeof(rfilter));
+    struct ifreq ifr;


Variables must all be declared at top of function or in brace section.

This provides a simple echo server / client that uses ISO-TP over CAN bus and wolfSSL for TLS.

LinuxJedi · 2021-12-08T11:53:41Z

New commit does the following:

Fixes everything mentioned
Moves a lot of common code into common.c
Uses a signal handler for clean shutdown

Leak after Ctrl-C in CAN bus client.

dgarske

Great work here. A few review items have not been addressed.

Also with -Wall -Wextra get these warnings:

$ make clean && make
isotp-c/isotp.c: In function ‘isotp_receive_flow_control_frame’:
isotp-c/isotp.c:222:56: warning: unused parameter ‘link’ [-Wunused-parameter]
  222 | static int isotp_receive_flow_control_frame(IsoTpLink *link, IsoTpCanMessage *message, uint8_t len) {
      |                                             ~~~~~~~~~~~^~~~
isotp-c/isotp.c:222:79: warning: unused parameter ‘message’ [-Wunused-parameter]
  222 | static int isotp_receive_flow_control_frame(IsoTpLink *link, IsoTpCanMessage *message, uint8_t len) {
      |                                                              ~~~~~~~~~~~~~~~~~^~~~~~~
common.c: In function ‘send_ssl’:
common.c:162:34: warning: pointer targets in passing argument 2 of ‘isotp_send’ differ in signedness [-Wpointer-sign]
  162 |     int ret = isotp_send(g_link, buf, sz);
      |                                  ^~~
      |                                  |
      |                                  char *
In file included from common.h:40,
                 from common.c:22:
isotp-c/isotp.h:106:47: note: expected ‘const uint8_t *’ {aka ‘const unsigned char *’} but argument is of type ‘char *’
  106 | int isotp_send(IsoTpLink *link, const uint8_t payload[], uint16_t size);
      |                                 ~~~~~~~~~~~~~~^~~~~~~~~
common.c:157:23: warning: unused parameter ‘ssl’ [-Wunused-parameter]
  157 | int send_ssl(WOLFSSL *ssl, char *buf, int sz, void *ctx)
      |              ~~~~~~~~~^~~
common.c: In function ‘recv_ssl’:
common.c:216:22: warning: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Wsign-compare]
  216 |     if (copy_buf_len >= sz) {
      |                      ^~
common.c:192:9: warning: unused variable ‘ret’ [-Wunused-variable]
  192 |     int ret;
      |         ^~~
common.c:187:23: warning: unused parameter ‘ssl’ [-Wunused-parameter]
  187 | int recv_ssl(WOLFSSL* ssl, char* buf, int sz, void* ctx)
      |              ~~~~~~~~~^~~
common.c: In function ‘sig_handle’:
common.c:252:21: warning: unused parameter ‘dummy’ [-Wunused-parameter]
  252 | void sig_handle(int dummy)
      |                 ~~~~^~~~~
client.c: In function ‘main’:
client.c:32:9: warning: unused variable ‘length’ [-Wunused-variable]
   32 |     int length;
      |         ^~~~~~
client.c:31:13: warning: unused variable ‘data’ [-Wunused-variable]
   31 |     uint8_t data[CAN_MSG_LEN];
      |             ^~~~

dgarske · 2021-12-08T16:09:11Z

+    WOLFSSL* ssl = NULL;
+
+    if (type == SERVICE_TYPE_CLIENT) {
+        method = wolfTLSv1_2_client_method();


Why not use TLS v1.3? I think it is better suited for this example.

dgarske · 2021-12-08T16:09:57Z

+                SSL_FILETYPE_PEM);
+    }
+
+    if (ret != SSL_SUCCESS) {


Please use our WOLFSSL_SUCCESS. The shorter is the openssl compat version.

dgarske · 2021-12-08T16:10:55Z

+    }
+
+    while(keep_running) {
+        char reply[64];


Consider adding memset on the reply to make sure it is null terminated. Not all printf's support the %.*s version.

dgarske · 2021-12-08T16:11:26Z

+    struct can_frame frame;
+    struct pollfd p[1];
+
+	p[0].fd = sock;


Can indent here.

dgarske · 2021-12-08T16:11:38Z

+	p[0].events = POLLIN;
+
+    /* Poll for new data */
+	int retval = poll(p, 1, 10);


All variable decorations need to be at top of function or within brace.

Adding -Wdeclaration-after-statement in the next commit to detect these in future.

LinuxJedi · 2021-12-09T10:59:11Z

New commit does the following:

Add warnings to CFLAGS (except for isotp)
Fix items found by warnings
Remove %.*s usage
Fix indentation issue
Use WOLFSSL_* instead of SSL_* constants
Switch to TLS v1.3
Add missing wolfSSL_Cleanup()

* Add warnings to CFLAGS (except for isotp) * Fix items found by warnings * Remove %.*s usage * Fix indentation issue * Use WOLFSSL_* instead of SSL_* constants * Switch to TLS v1.3 * Add missing wolfSSL_Cleanup()

* Init `sock` with -1 and make static * Brace style fix in `isotp_user_debug` * Use wolfCrypt `MEMORY_E` for memory allocation error

Add a CAN bus TLS example using ISO-TP transport

LinuxJedi requested a review from dgarske December 7, 2021 10:42

LinuxJedi assigned dgarske Dec 7, 2021

dgarske requested changes Dec 7, 2021

View reviewed changes

dgarske assigned LinuxJedi and unassigned dgarske Dec 7, 2021

Add a CAN bus TLS example using ISO-TP transport

191b5db

This provides a simple echo server / client that uses ISO-TP over CAN bus and wolfSSL for TLS.

LinuxJedi force-pushed the add-canbus branch from ed745e2 to 191b5db Compare December 8, 2021 11:52

LinuxJedi assigned dgarske Dec 8, 2021

LinuxJedi requested a review from dgarske December 8, 2021 11:53

Fix valgrind detected leak

0242c88

Leak after Ctrl-C in CAN bus client.

dgarske requested changes Dec 8, 2021

View reviewed changes

dgarske removed their assignment Dec 8, 2021

LinuxJedi requested a review from dgarske December 9, 2021 10:58

LinuxJedi force-pushed the add-canbus branch from 484f518 to bc5ff4f Compare December 9, 2021 10:59

Cleanups to CAN bus

809acb3

* Add warnings to CFLAGS (except for isotp) * Fix items found by warnings * Remove %.*s usage * Fix indentation issue * Use WOLFSSL_* instead of SSL_* constants * Switch to TLS v1.3 * Add missing wolfSSL_Cleanup()

LinuxJedi force-pushed the add-canbus branch from bc5ff4f to 809acb3 Compare December 9, 2021 11:33

Minor fixes to CAN bus

dca547c

* Init `sock` with -1 and make static * Brace style fix in `isotp_user_debug` * Use wolfCrypt `MEMORY_E` for memory allocation error

dgarske approved these changes Dec 9, 2021

View reviewed changes

dgarske merged commit 7be7d7f into wolfSSL:master Dec 9, 2021

yota22721 pushed a commit to yota22721/wolfssl-examples that referenced this pull request Jan 25, 2025

Merge pull request wolfSSL#279 from LinuxJedi/add-canbus

fb673d4

Add a CAN bus TLS example using ISO-TP transport

Conversation

LinuxJedi commented Dec 7, 2021

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LinuxJedi commented Dec 8, 2021

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LinuxJedi commented Dec 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants