Skip to content

Fix: in the WebSocket Client add-on, fix the base64 encoding #396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Yves57 wants to merge 1 commit into from
+8 −2
Closed

Fix: in the WebSocket Client add-on, fix the base64 encoding #396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion addons/websocket/nx_websocket_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,9 @@ NX_PACKET *packet_ptr;
client_ptr -> nx_websocket_client_guid[i] = (UCHAR)(NX_RAND());
}

/* Set the last extra byte of the buffer to be zero, since the function _nx_utility_base64_encode will use this extra-byte for calculation */
client_ptr -> nx_websocket_client_guid[NX_WEBSOCKET_CLIENT_GUID_SIZE] = 0;

Comment on lines +515 to +517

@fdesbiens fdesbiens Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the comment on :1110. The GUID is 16 bytes, which is a multiple of neither 3 nor 4 but still does not cause an extra read: the encode loop ends with i == 16 after step 1 takes its guarded else branch, so guid[16] is never read.

I ran every length from 1 to 40 through the harness against an independent encoder, each on an exact-size heap buffer under ASan. Zero over-reads, zero output mismatches, and zero cases where the output changed when the byte at name[name_size] was flipped from 0x00 to 0xFF. Size 16 is included in that sweep.

So this store is dead and can be removed along with the + 1 in the header.

/* Encode the GUID as key. */
_nx_utility_base64_encode(client_ptr -> nx_websocket_client_guid, NX_WEBSOCKET_CLIENT_GUID_SIZE,
client_ptr -> nx_websocket_client_key, NX_WEBSOCKET_CLIENT_KEY_SIZE,
Expand Down Expand Up @@ -963,7 +966,7 @@ UCHAR *field_name;
UINT field_name_length;
UCHAR *field_value;
UINT field_value_length;
UCHAR digest[NX_WEBSOCKET_ACCEPT_DIGEST_SIZE];
UCHAR digest[NX_WEBSOCKET_ACCEPT_DIGEST_SIZE + 1];

@fdesbiens fdesbiens Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistent with the store at :1111, so it should be reverted together with it. NX_WEBSOCKET_ACCEPT_DIGEST_SIZE is exactly the SHA-1 output length and _nx_sha1_digest_calculate() writes exactly that many bytes, so 20 is the correct size.

To be clear, the PR is internally consistent — it enlarges the array and writes to the new slot, so there is no out-of-bounds access here. The point is only that neither is needed.

UCHAR key[NX_WEBSOCKET_ACCEPT_KEY_SIZE + 1];
UINT key_size = 0;
UCHAR upgrade_flag = NX_FALSE;
Expand Down Expand Up @@ -1104,6 +1107,9 @@ UCHAR accept_cnt = 0;
_nx_sha1_update(&(client_ptr -> nx_websocket_client_sha1), (UCHAR*)NX_WEBSOCKET_ACCEPT_PREDEFINED_GUID, NX_WEBSOCKET_ACCEPT_PREDEFINED_GUID_SIZE);
_nx_sha1_digest_calculate(&(client_ptr -> nx_websocket_client_sha1), digest);

/* Set the last extra byte of the digest to be zero, since the function _nx_utility_base64_encode will use this byte for calculation */
digest[NX_WEBSOCKET_ACCEPT_DIGEST_SIZE] = 0;

Comment on lines +1110 to +1112

@fdesbiens fdesbiens Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This store has no effect on the result. _nx_utility_base64_encode() never reads name[name_size].

In the encode loop, name[i + 1] is only read inside step 1 (nx_utility.c:397) and step 2 (:414), and each is guarded by if ((i + 1) < input_name_size) with a zero-padded fallback in the else. Steps 0 (:390) and 3 (:431) read only name[i].

For the 20-byte digest here, name_size is adjusted to 27 output characters with 1 pad, and the loop terminates with i == 20 after step 2 takes its else branch — so index 20 is never dereferenced.

I verified this rather than relying on the reading. The function was extracted verbatim into a harness and given a 20-byte exact-size heap allocation under AddressSanitizer, using the digest from your PR description:

status       = 0
bytes_copied = 28
nx output    = esgBQXYr0So6/Oj99PJCQ5SHuWk=
go expected  = esgBQXYr0So6/Oj99PJCQ5SHuWk=
match        = YES

No ASan report, and the output already matches your Go reference without this store. Encoding the same input with name[20] set to 0xFF instead of 0 gives byte-identical output, so the byte genuinely does not participate.

Please drop this store and the + 1 on the digest declaration.

/* Encode the hash and compare it with the field value from the server. */
_nx_utility_base64_encode(digest, NX_WEBSOCKET_ACCEPT_DIGEST_SIZE, key, (NX_WEBSOCKET_ACCEPT_KEY_SIZE + 1), &key_size);
if ((field_value_length != NX_WEBSOCKET_ACCEPT_KEY_SIZE) || (memcmp((void *)field_value, (void *)key, NX_WEBSOCKET_ACCEPT_KEY_SIZE))) /* Use case of memcpy is verified. */
Expand Down
2 changes: 1 addition & 1 deletion addons/websocket/nx_websocket_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ typedef struct NX_WEBSOCKET_CLIENT_STRUCT
NX_PACKET *nx_websocket_client_processing_packet;

/* Globally Unique Identifier. */
UCHAR nx_websocket_client_guid[NX_WEBSOCKET_CLIENT_GUID_SIZE];
UCHAR nx_websocket_client_guid[NX_WEBSOCKET_CLIENT_GUID_SIZE + 1];

@fdesbiens fdesbiens Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the extra byte is never read, this enlarges NX_WEBSOCKET_CLIENT_STRUCT for nothing. In practice it will cost four bytes per client instance rather than one, because of the alignment of the pointer field that follows at :179.

NX_WEBSOCKET_CLIENT_STRUCT is a public type that applications allocate, so its size is part of the ABI — worth avoiding a change to it unless it buys something. Given ThreadX's footprint goals I would rather not pay this.


/* Protocol Name and length */
UCHAR *nx_websocket_client_subprotocol;
Expand Down