Change in osmo-msc[master]: improve handling of BSC-chosen algo in CIPHER MODE COMPLETE

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Fri Dec 21 02:52:10 UTC 2018


Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12349 )

Change subject: improve handling of BSC-chosen algo in CIPHER MODE COMPLETE
......................................................................


Patch Set 3: Code-Review-1

(11 comments)

https://gerrit.osmocom.org/#/c/12349/2/include/osmocom/msc/gsm_04_08.h
File include/osmocom/msc/gsm_04_08.h:

https://gerrit.osmocom.org/#/c/12349/2/include/osmocom/msc/gsm_04_08.h@80
PS2, Line 80: int gsm48_conn_sendmsg(struct msgb *msg, struct ran_conn *conn, struct gsm_trans *trans);
> Yes that works, too. Done in next patch set.
Actually in .h files, in general, please rather define opaque structs.
Headers grow less dependency spaghetti and side effects if we just define opaque structs.
#include ran_conn.h would rather belong in the .c file.

It's not really a problem, no need to change it back again.
But for next time: @Max, an opaque struct is good, not bad; and @stsp, next time refuse to #include lots of stuff if all you need is a single struct pointer.


https://gerrit.osmocom.org/#/c/12349/2/src/libmsc/a_iface_bssap.c
File src/libmsc/a_iface_bssap.c:

https://gerrit.osmocom.org/#/c/12349/2/src/libmsc/a_iface_bssap.c@410
PS2, Line 410: 		int supported;
> This was copied from existing code in gsm_04_08.c. I'll fix it there, too.
it's because there is A5/0 .. A5/7. That's eight. This is a fact throughout the encryption code. BTW, A5/4...7 don't even exist, only as placeholders / defined bits in the specs.


https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c
File src/libmsc/a_iface_bssap.c:

https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@409
PS3, Line 409: 	for (i = 0; i < ENCRY_INFO_PERM_ALGO_MAXLEN; i++) {
This loop wants to iterate the a5_encryption_mask, so it should actually be exactly i < 8.

This constant instead defines how many perm_algo[] items are allowed. Those are theoretically two different pairs of shoes, but incidentally they are the same. Consider, even though 8 encryption types exist, the amount of permitted algos passed in that bssmap ie could have been chosen as three, or five, or two...

Right. Both are eight. So it comes down to the same thing. At least now you know what things mean.


https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@419
PS3, Line 419: 			ei.perm_algo[j++] = vlr_ciph_to_gsm0808_alg_id(i);
this here could instead make sure that j < ENCRY_INFO_PERM_ALGO_MAXLEN


https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@426
PS3, Line 426: 			vlr_ciph = ei.perm_algo[ei.perm_algo_len - 1] - 1;
do we have a reverse function for vlr_ciph_to_gsm0808_alg_id()? The -1 might be correct, but it's a magic number here.


https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@429
PS3, Line 429: 		LOGPCONN(conn, LOGL_NOTICE, "BSC didn't specify algorithm in CIHPER MODE COMPLETE; falling back to %s\n",
You are parsing the "Ciphering is complete" message. Ciphering is already in place between MS and BTS, it's not like you could fall back or anything. Either you know what cipher is being used, or you don't. But definitely ciphering is being used now.


https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@430
PS3, Line 430: 			 get_value_string(vlr_ciph_names, vlr_ciph));
just use vlr_ciph_name(vlr_ciph)


https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@440
PS3, Line 440: 			LOGPCONN(conn, LOGL_ERROR, "Unsupported encryption algorithm in CIHPER MODE COMPLETE: %s\n",
"Non-permitted"?


https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@456
PS3, Line 456: 	ran_conn_cipher_mode_compl(conn, msg, vlr_ciph);
maybe this function needs to be taught to not pass a cipher algo to the MSC when the BTS didn't send an IE with the chosen cipher. This entire "which cipher is it" is merely informational, also towards the MSC. All this is in the end after all ciphering negotiation, and nothing can be changed about it at this point.


https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/gsm_04_08.c
File src/libmsc/gsm_04_08.c:

https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/gsm_04_08.c@125
PS3, Line 125: int gsm48_classmark_supports_a5(const struct gsm_classmark *cm, uint8_t a5)
gsm48 is a prefix commonly used by old libosmocore API. Would prefer to not use that here as well.
Since there is only one kind of classmark, 'classmark_..' is fine even for a non-static function, it is still used only in the msc.


https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/gsm_04_08.c@1613
PS3, Line 1613: 	for (i = 0; i < ENCRY_INFO_PERM_ALGO_MAXLEN; i++) {
again, 8 is accurate here, because of (1 << i), and this is not a gsm0808 array of perm algo bytes.



-- 
To view, visit https://gerrit.osmocom.org/12349
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3260bee43cfe135ebfc33c13aee3c4ba43466c81
Gerrit-Change-Number: 12349
Gerrit-PatchSet: 3
Gerrit-Owner: Stefan Sperling <stsp at stsp.name>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <stsp at stsp.name>
Gerrit-Comment-Date: Fri, 21 Dec 2018 02:52:10 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181221/0a12dbd4/attachment.htm>


More information about the gerrit-log mailing list