treewide: cellular: add support for debug sectags #21727

maxd-nordic · 2025-04-11T14:19:23Z

nRF91 modems support special sectags to trace TLS ephemeral keys,
but these checks mark those as invalid.
This patch removes checks and changes some of them to check for -1,
which is not one of the debug sectags.

NordicBuilder · 2025-04-11T14:27:43Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 3

Inputs:

Sources:

sdk-nrf: PR head: 6697b984aeaff775a267bca29e6b4ddf904b86e5

more details

sdk-nrf:

PR head: 6697b984aeaff775a267bca29e6b4ddf904b86e5
merge base: 5ca42abade7579f7102726fefbda80cb991a8c16
target head (main): 2bf50d927cf3a66b489a1573729c15abede60ee4
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (8)

applications
│  ├── serial_lte_modem
│  │  ├── src
│  │  │  │ slm_at_udp_proxy.c
samples
│  ├── cellular
│  │  ├── http_update
│  │  │  ├── modem_delta_update
│  │  │  │  ├── src
│  │  │  │  │  │ main.c
│  │  │  ├── modem_full_update
│  │  │  │  ├── src
│  │  │  │  │  │ main.c
│  │  ├── modem_shell
│  │  │  ├── src
│  │  │  │  ├── sock
│  │  │  │  │  ├── sock.c
│  │  │  │  │  │ sock_shell.c
subsys
│  ├── net
│  │  ├── lib
│  │  │  ├── fota_download
│  │  │  │  ├── src
│  │  │  │  │  │ fota_download.c
│  │  │  ├── nrf_cloud
│  │  │  │  ├── src
│  │  │  │  │  ├── nrf_cloud_fota.c
│  │  │  │  │  │ nrf_cloud_fota_poll.c

Outputs:

Toolchain

Version: 7cbc0036f4
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:7cbc0036f4_8bf7ca4353

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 349
✅ Integration tests
- ✅ test-fw-nrfconnect-nrf-iot_cloud
- ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- ✅ test-fw-nrfconnect-nrf-iot_samples
- ✅ test-fw-nrfconnect-nrf-iot_thingy91
- ✅ test-fw-nrfconnect-nrf-iot_mosh
- ✅ test-fw-nrfconnect-nrf-iot_positioning
- ⚠️ test-fw-nrfconnect-fw-update

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-proprietary_esb
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-wifi
- test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

pascal-nordic · 2025-04-11T14:39:12Z

samples/cellular/modem_shell/src/sock/sock.c

Knowing that the normal sectag range is from 0 to 2147483647 and for enabling debugging is 0x80000000 - 0x8000000F, this condition will always be valid.

modem_shell works fine with debug sec_tags without this change, but this change breaks the functionality with all sec_tags. Also, using -1 with an unsigned integer does not look nice.

BTW, the last sec_tag reserved for debugging is 2147483667, i.e. 0x80000013.

right, I got the condition the wrong way around. is if (sec_tag == -1) { okay?
The current check does nothing, with the change, you check if the parameter was set.

That works, but I would rather not use -1 with an unsigned integer. The variable can not be made signed (32-bit) integer either, because the debug sec_tag values are larger than it can represent.

Because 0 is also a valid value, I think it would be best to use 0xFFFFFFFF to indicate that no sec_tag value was given. Maybe add a define for that as well.

@tokangas I added a new macro for invalid sec tags with value 0xFFFFFFFF to libmodem, hopefully this will help addressing the current limitations.

@MirkoCovizzi Thanks, I think that's a good idea.

@MirkoCovizzi Did you confirm with the modem team that 0xFFFFFFFF is not used internally for other things? I see that 0xFFFFFFFE is in use for DEV_ID_PUB_KEY.

github-actions · 2025-04-11T14:53:13Z

You can find the documentation preview for this PR here.

MarkusLassila

SLM

tokangas · 2025-04-14T06:01:39Z

samples/cellular/modem_shell/src/sock/sock.c

modem_shell works fine with debug sec_tags without this change, but this change breaks the functionality with all sec_tags. Also, using -1 with an unsigned integer does not look nice.

BTW, the last sec_tag reserved for debugging is 2147483667, i.e. 0x80000013.

nRF91 modems support special sectags to trace TLS ephemeral keys, but these checks mark those as invalid. This patch removes checks and changes some of them to check for -1, which is not one of the debug sectags. Signed-off-by: Maximilian Deubel <[email protected]>

gregersrygg · 2025-04-14T08:06:28Z

There are several places where the sec_tag/sec_tag_list variable are defined as int. Should perhaps it be changed to uint_32t?

pascal-nordic · 2025-04-14T08:16:32Z

samples/cellular/modem_shell/src/sock/sock.c

 		}

-		if (sec_tag < 0) {
+		if (sec_tag == -1) {


I would also recommend to validate if the sec_tag is out of range

if (sec_tag > 0x8000000F) { mosh_error("Invalid security tag"); }

If the check is added, the correct value is 2147483667 (0x80000013).

Transport Layer Security (TLS) traffic can be decrypted with Nordic tools if the TLS session is created using certificates stored to <sec_tag>s 2147483648– 2147483667. These <sec_tag>s shall be used only for test and development purposes.

https://docs.nordicsemi.com/bundle/ref_at_commands_nrf91x1/page/REF/at_commands/security/cmng_set.html

You are right, the maximum value is up to 0x80000013

Range check in modem_shell is not absolutely needed because we do allow testing for some invalid inputs to test other parts of the SW. In this case, I'll leave it to you to decide if it would still be better to have it here.

For modem_shell I'll vote for not having this check, because from modem fw any sec_tag is valid.

SeppoTakalo

I would propose that we don't fix the code like this.

Seems like on may locations we have used -1 to mark a sec_tag that has not been set.
However, as now found out by, this have introduced bugs.

On some run-time checks the sec_tag < 0 offcourse always fails as sec_tag_t is unsigned, as it should be.

If we want to continue like this, we MUST be using only sec_tag_t to store sec tags. Not int and uint32_t mix and max.

So if we refactor, I would accept that we refactor every API to use proper sec_tag_t.
Then runtime checks, if we use (-1) it should ONLY be (sec_tag_t)(-1) and the check can ONLY be if (sec_tag = INVALID_SEC_TAG) .

Only then we can expect that -1 conversion is always the same.

SeppoTakalo · 2025-04-14T08:15:11Z

samples/cellular/http_update/modem_delta_update/src/main.c

 	const char *file;
 	int sec_tag = SEC_TAG;
-	uint8_t sec_tag_count = sec_tag < 0 ? 0 : 1;
+	uint8_t sec_tag_count = 1;


This introduces a bug.
Now it sets as sec_tag count to one, even if sec_tag is not configured.

This file needs refactoring. Usage of HTTPS is controlled by CONFIG_USE_HTTPS but in the code, it uses runtime check if sec_tag is (-1).

Do we need to count the amount of sec_tags or should we just need to know that there is a sec_tag? Seems to me that is more than enough to know the presence of a sec_tag

SeppoTakalo · 2025-04-14T08:16:36Z

applications/serial_lte_modem/src/slm_at_udp_proxy.c

 			if (param_count > 4) {
 				if (at_parser_num_get(parser, 4, &proxy.sec_tag)
-				|| proxy.sec_tag == INVALID_SEC_TAG || proxy.sec_tag < 0) {
+				|| proxy.sec_tag == INVALID_SEC_TAG) {


Seem to be an existing bug in this file.

proxy.sec_tag is unsigned, and therefore cannot be < 0.

SeppoTakalo · 2025-04-14T08:17:01Z

samples/cellular/http_update/modem_full_update/src/main.c

 	const char *file;
 	int sec_tag = SEC_TAG;
-	uint8_t sec_tag_count = sec_tag < 0 ? 0 : 1;
+	uint8_t sec_tag_count = 1;


This introduces a bug.
Now it sets as sec_tag count to one, even if sec_tag is not configured.

This file needs refactoring. Usage of HTTPS is controlled by CONFIG_USE_HTTPS but in the code, it uses runtime check if sec_tag is (-1).

What about checking CONFIG_USE_HTTPS here instead of setting SEC_TAG to -1 (in top of the file)?

SeppoTakalo · 2025-04-14T08:18:14Z

samples/cellular/modem_shell/src/sock/sock.c

 		}

-		if (sec_tag < 0) {
+		if (sec_tag == -1) {


This is a bug.
sec_tag is unsigned and therefore this check has always bee false.

Yes, seems that also this has been broken. 0 can not be used because that's a valid value, so I guess UINT32_MAX should be used to indicate a missing value.

SeppoTakalo · 2025-04-14T08:19:17Z

samples/cellular/modem_shell/src/sock/sock_shell.c

 	int arg_pdn_cid = 0;
 	bool arg_secure = false;
-	uint32_t arg_sec_tag = 0;
+	uint32_t arg_sec_tag = -1;


uint32_t cannot be -1 so this is silently converting the integer to unsigned. Seems extremely weird and prone to bugs.

SeppoTakalo · 2025-04-14T08:19:48Z

subsys/net/lib/fota_download/src/fota_download.c

 {
 	int sec_tag_list[1] = { sec_tag };
-	uint8_t sec_tag_count = sec_tag < 0 ? 0 : 1;
+	uint8_t sec_tag_count = 1;


New bug.
sec_tag_count cannot always be one.

I currently use uint8_t sec_tag_count = sec_tag == -1 ? 0 : 1; in my development branch, but I think the best solution is to use sec_tag_t and introduce UNUSED_SEC_TAG or INVALID_SEC_TAG.

SeppoTakalo · 2025-04-14T08:19:53Z

subsys/net/lib/fota_download/src/fota_download.c

 {
 	int sec_tag_list[1] = { sec_tag };
-	uint8_t sec_tag_count = sec_tag < 0 ? 0 : 1;
+	uint8_t sec_tag_count = 1;


maxd-nordic · 2025-04-25T14:42:10Z

Thanks for the input, folks! How should we proceed? Changing nrf_socket.h where nrf_sec_tag_t is defined and adding an invalid value of 0xFFFFFFFF?

maxd-nordic · 2025-04-25T14:49:16Z

ref: nrfconnect/sdk-nrfxlib#1734

MirkoCovizzi

Please have a look at https://github.com/nrfconnect/sdk-nrfxlib/blob/b7f26d1b1fe2205a8e207aa34c07ff9dcf86a5f5/nrf_modem/include/nrf_socket.h#L568

maxd-nordic · 2025-04-28T08:15:22Z

Please have a look at https://github.com/nrfconnect/sdk-nrfxlib/blob/b7f26d1b1fe2205a8e207aa34c07ff9dcf86a5f5/nrf_modem/include/nrf_socket.h#L568

@MirkoCovizzi Yes, I saw that. What point did you want to make?

maxd-nordic · 2025-04-29T08:03:55Z

Waiting for the next libmodem release which will include a define for an invalid sectag. Feel free to tend to any discovered bugs in the meantime.

gregersrygg · 2025-04-29T10:31:59Z

Do we need to wait for a new define from a library to be able to fix an existing bug? @SeppoTakalo proposed to use sec_tag_t which already exist, and if we need a value for an invalid sectag, can't we just use0xFFFFFFFF in a local define until it's available from libmodem?

maxd-nordic requested review from a team as code owners April 11, 2025 14:19

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Apr 11, 2025

maxd-nordic force-pushed the allow-debug-sectags branch from 7661466 to ab6606b Compare April 11, 2025 14:26

pascal-nordic reviewed Apr 11, 2025

View reviewed changes

MarkusLassila approved these changes Apr 14, 2025

View reviewed changes

tokangas requested changes Apr 14, 2025

View reviewed changes

maxd-nordic force-pushed the allow-debug-sectags branch from ab6606b to 6697b98 Compare April 14, 2025 08:03

pascal-nordic reviewed Apr 14, 2025

View reviewed changes

SeppoTakalo requested changes Apr 14, 2025

View reviewed changes

MirkoCovizzi requested changes Apr 25, 2025

View reviewed changes

maxd-nordic closed this Apr 29, 2025

treewide: cellular: add support for debug sectags #21727

treewide: cellular: add support for debug sectags #21727

Uh oh!

Conversation

maxd-nordic commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NordicBuilder commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

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!

github-actions bot commented Apr 11, 2025

Uh oh!

MarkusLassila left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregersrygg commented Apr 14, 2025

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!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pascal-nordic Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

maxd-nordic commented Apr 25, 2025

maxd-nordic commented Apr 11, 2025 •

edited

Loading

NordicBuilder commented Apr 11, 2025 •

edited

Loading

pascal-nordic Apr 14, 2025 •

edited

Loading

SeppoTakalo Apr 14, 2025 •

edited

Loading