[Asterisk-code-review] STIR/SHAKEN: Option split and response codes. (asterisk[16])

[Kevin Harwell]

Attention is currently required from: Benjamin Keith Ford.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16497 )

Change subject: STIR/SHAKEN: Option split and response codes.
......................................................................


Patch Set 3: Code-Review-1

(6 comments)

File include/asterisk/res_stir_shaken.h:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/05904bd7_8678e614 
PS3, Line 94: int ast_stir_shaken_verify(const char *header, const char *payload, const char *signature, const char *algorithm,
The altered function definition breaks ABI


File res/res_pjsip_session.c:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/a28e0dcb_1e261ab2 
PS3, Line 4070: ast_sip_rdata_get_header_value
I was curious what this function returned. It returns a pointer to the underlying pjsip "string" data. However pjsip strings are not null terminated, and not sure if they are even initialized to NULL/empty.

This could potentially reference some random place in memory and cause unexpected results. Also, anyone place that calls this function and attempts to dereference the pointer will either cause a crash, or at the very least point to a non-NULL terminated string.

So I believe the the 'ast_sip_rdata_get_header_value' function needs "fixing".


File res/res_pjsip_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/78729996_bb405b98 
PS3, Line 126: 	char timezone[80];
Initialize this to NULL just in case, i.e. timezone[80] = {0}.

If sscanf fails to fill it below then a subsequent reference to it could result in indeterminate a value.


https://gerrit.asterisk.org/c/asterisk/+/16497/comment/b0031bd4_59d04ee8 
PS3, Line 129: 	date_hdr_val = ast_sip_rdata_get_header_value(rdata, date_hdr_str);
The return value of 'ast_sip_rdata_get_header_value' needs to be fixed somehow.

'ast_sip_rdata_get_header_value' returns a non-null terminated string, and potentially an unitialized pointer. Any use/reference to data_hdr_val below could cause unexpected behavior.


https://gerrit.asterisk.org/c/asterisk/+/16497/comment/71e3daec_cf7d7caa 
PS3, Line 140: 	sscanf(remainder, "%79s", timezone);
Is remainder always guaranteed to be the expected timezone?

strptime returns a pointer to the first character not processed, or NULL. You catch the NULL case above, but could remainder be an empty string, or some other value besides timezone?

At the very least I'd say you want to check the return value of sscanf to ensure the item was filled.


https://gerrit.asterisk.org/c/asterisk/+/16497/comment/e17c597d_4be1a432 
PS3, Line 199: 	RAII_VAR(struct ast_json *, json, NULL, ast_json_unref);
             : 	RAII_VAR(char *, combined_str, NULL, ast_free);
Maybe I am missing something, but these to variables appear to be unused. If that's the case they can be removed.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16497
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I4ac1ecf652cd0e336006b0ca638dc826b5b1ebf7
Gerrit-Change-Number: 16497
Gerrit-PatchSet: 3
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Benjamin Keith Ford <bford at digium.com>
Gerrit-Comment-Date: Wed, 22 Sep 2021 20:00:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210922/6ee54ec7/attachment.html>