LWIP random library and TCP sequence number improvements #4284

mikaleppanen · 2017-05-08T10:52:35Z

Description

Added support for common pal random library to LWIP. This corrects issue: #3891
Integrated LWIP TCP sequence number initialization reference code to mbed LWIP stack.
Corrected LWIP adaptation Coverity and compiler warnings.

Status

READY

Migrations

NO

Related PRs

None

Todos

Tests
Documentation

Deploy notes

None

Steps to test or reproduce

None

0xc0170 · 2017-05-08T12:58:57Z

cc @kjbracey-arm @geky

0xc0170 · 2017-05-08T12:51:23Z

features/FEATURE_LWIP/lwip-interface/lwip-sys/lwip_random.h

+#endif
+
+#if FEATURE_COMMON_PAL
+void lwip_seed_random(void);


please provide documentation for this new API

Added function descriptions.

0xc0170 · 2017-05-08T12:52:13Z

features/FEATURE_LWIP/lwip-interface/lwip-sys/lwip_random.h

+void lwip_seed_random(void);
+void lwip_add_ramdom_seed(uint64_t seed);
+#else
+#define lwip_seed_random()


why are these defined? I would rather provide an empty implementation ?

will it work fro the random_seed ?

If you want them defined as nothing, they need to at least be expressions - make them (void)0.

Made empty functions for non-common pal case.

kjbracey · 2017-05-09T11:12:46Z

features/FEATURE_LWIP/lwip-interface/lwip-sys/lwip_random.c

+    randLIB_seed_random();
+}
+
+void lwip_add_ramdom_seed(uint64_t seed)


kjbracey · 2017-05-09T11:19:27Z

features/FEATURE_LWIP/lwip-interface/EthernetInterface.cpp

@@ -24,12 +24,35 @@ EthernetInterface::EthernetInterface()
 {
 }

+bool validate_str(const char *str, const int len)


This all seems a bit clunky. This whole routine could just be

return str != NULL && strlen(str) < len;

You could also have replaced strncpy with snprintf, which doesn't have the no-zero-termination-if-truncating quirk.

Does lwip already verify that the addresses are syntactically correct?

snprintf does come with a code-size cost, though you can just always force a null terminator:

strncpy(_ip_address, ip_address ? ip_address : "", sizeof(_ip_address)); _ip_address[sizeof(_ip_address)-1] = '\0';

Yes, lwip validates the addresses. Added enforcing of null terminator and removed validate_str functions.

@geky You reckon nobody else is using snprintf?? Seems more likely to me that no-one is using strncpy.

kjbracey · 2017-05-09T11:22:15Z

features/FEATURE_LWIP/lwip-interface/lwip-sys/lwip_random.c

+    randLIB_add_seed(seed);
+}
+
+inline uint32_t lwip_get_random(void)


Only having inline in the C file doesn't achieve anything.

If you really want inlining, the only easily C99/C++ portable form is a "static inline" definition in the header file.

Removed inline.

kjbracey · 2017-05-09T11:30:51Z

features/FEATURE_LWIP/lwip-interface/lwip-sys/lwip_tcp_isn.c

+
+  /* Generate the hash, using MD5. */
+  lwip_md5_starts(&ctx);
+  lwip_md5_update(&ctx, input, sizeof(input));


This seems oddly pointless - trying to a copy of all the data bits into a static 64-byte buffer to pass it in one go, when you could freely pass the existing data pointers in multiple calls to update - the MD5 has its own 64-byte buffer.

I guess this is unmodified reference code though?

Yes, reference code has that so leaving as it is.

Is a bit bloaty though - 64 entire bytes of static RAM usage. Oh well. I guess it always needs 16 for the secret.

kjbracey · 2017-05-09T11:33:28Z

features/FEATURE_LWIP/lwip-interface/lwipopts.h

+#include "lwip_tcp_isn.h"
+#define LWIP_HOOK_TCP_ISN lwip_hook_tcp_isn
+#if MBEDTLS_MD5_C
+#define LWIP_USE_EXTERNAL_MBEDTLS 1


Can't this be set unconditionally? Not that it probably makes a difference.

mbed tls c-functions are flagged with MBEDTLS_MD5_C so they do not exists in build without the flag.

Ah, was being dense. Thought that was a LWIP flag.

kjbracey · 2017-05-09T11:40:50Z

features/FEATURE_LWIP/lwip-interface/lwip-sys/lwip_random.h

+void lwip_seed_random(void);
+void lwip_add_ramdom_seed(uint64_t seed);
+#else
+#define lwip_seed_random()


If you want them defined as nothing, they need to at least be expressions - make them (void)0.

geky · 2017-05-09T14:48:51Z

features/FEATURE_LWIP/lwip-interface/lwip-sys/lwip_random.c

+
+#include "lwip_random.h"
+
+#if FEATURE_COMMON_PAL


Ah I see, this works for me 👍

geky

Looks good to me as long as the other guys' comments are handled

lwip now uses mbed client random library under common pal when available. Ported lwip reference TCP initial sequence number handling to mbed-os lwip stack. Handling is based on RFC 6528.

Coverity ids: 1373147 and 1374442.

mikaleppanen · 2017-05-10T07:29:59Z

Corrected review defects.

0xc0170 · 2017-05-10T13:09:06Z

/morph test-nightly

mbed-bot · 2017-05-10T19:01:56Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 177

Test failed!

geky · 2017-05-10T20:11:49Z

@bridadan, @studavekar, it's the arch pro issue again

A believe a workaround is in progress
#4274 (comment)

0xc0170 · 2017-05-11T09:06:15Z

It should be fixed now, restarting

/morph test-nightly

mbed-bot · 2017-05-11T15:10:28Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 185

All builds and test passed!

JanneKiiskila · 2017-06-05T06:40:29Z

@adbridge @sg- @0xc0170 - please note this is a fatal blocking stability issue for TLS connections, please patch this ASAP to mbed OS 5.4.x.

0xc0170 reviewed May 8, 2017

View reviewed changes

0xc0170 added the needs: review label May 8, 2017

theotherjimmy requested review from geky and kjbracey May 8, 2017 15:09

kjbracey reviewed May 9, 2017

View reviewed changes

geky reviewed May 9, 2017

View reviewed changes

geky approved these changes May 9, 2017

View reviewed changes

Mika Leppänen added 2 commits May 10, 2017 10:20

lwip added support for random library and TCP ISN initialisation

053139b

lwip now uses mbed client random library under common pal when available. Ported lwip reference TCP initial sequence number handling to mbed-os lwip stack. Handling is based on RFC 6528.

lwip corrected coverity and compiler warnings

55753be

Coverity ids: 1373147 and 1374442.

kjbracey approved these changes May 10, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels May 10, 2017

0xc0170 approved these changes May 10, 2017

View reviewed changes

0xc0170 added ready for merge release-version: 5.5.0-rc1 and removed needs: CI ready for merge labels May 12, 2017

0xc0170 merged commit 27a6965 into ARMmbed:master May 15, 2017

mikaleppanen mentioned this pull request May 24, 2017

lwip shouldn't use rand() for port or sequence number randomisation (or at all in general) #3891

Closed

adbridge added the release-version: 5.4.7 label Jun 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LWIP random library and TCP sequence number improvements #4284

LWIP random library and TCP sequence number improvements #4284

mikaleppanen commented May 8, 2017

0xc0170 commented May 8, 2017

0xc0170 May 8, 2017

mikaleppanen May 10, 2017

0xc0170 May 8, 2017

kjbracey May 9, 2017

mikaleppanen May 10, 2017

kjbracey May 9, 2017

mikaleppanen May 10, 2017

kjbracey May 9, 2017

geky May 9, 2017

mikaleppanen May 10, 2017

kjbracey May 10, 2017 •

edited

Loading

kjbracey May 9, 2017

mikaleppanen May 10, 2017

kjbracey May 9, 2017

mikaleppanen May 10, 2017

kjbracey May 10, 2017

kjbracey May 9, 2017

mikaleppanen May 10, 2017

kjbracey May 10, 2017

kjbracey May 9, 2017

geky May 9, 2017

geky left a comment

mikaleppanen commented May 10, 2017

0xc0170 commented May 10, 2017

mbed-bot commented May 10, 2017

geky commented May 10, 2017

0xc0170 commented May 11, 2017

mbed-bot commented May 11, 2017

JanneKiiskila commented Jun 5, 2017


		#include "lwip_random.h"

		#if FEATURE_COMMON_PAL

LWIP random library and TCP sequence number improvements #4284

LWIP random library and TCP sequence number improvements #4284

Conversation

mikaleppanen commented May 8, 2017

Description

Status

Migrations

Related PRs

Todos

Deploy notes

Steps to test or reproduce

0xc0170 commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky left a comment

Choose a reason for hiding this comment

mikaleppanen commented May 10, 2017

0xc0170 commented May 10, 2017

mbed-bot commented May 10, 2017

Output

geky commented May 10, 2017

0xc0170 commented May 11, 2017

mbed-bot commented May 11, 2017

Output

JanneKiiskila commented Jun 5, 2017

kjbracey May 10, 2017 •

edited

Loading